feat(header, footer, tab-bar): add scrollEffect prop#31263
feat(header, footer, tab-bar): add scrollEffect prop#31263OS-susmitabhowmik wants to merge 47 commits into
Conversation
…rrect theme class usage
…orce footer priority
…llEffect in all modes
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
ShaneK
left a comment
There was a problem hiding this comment.
Hey, great work on this! I highlighted a few issues I found, some of them aren't as important as others, but still
ShaneK
left a comment
There was a problem hiding this comment.
Hey! Thanks for fixing up those issues! I found some more things that were a bit concerning with this change, let me know if I'm off the mark on anything here!
ShaneK
left a comment
There was a problem hiding this comment.
Thanks for grinding through all of this, most of the last round holds up and I verified it. Found a few more edges though, including one where the main header disappears with no large title (reproduced it inline). Nothing structural, mostly tightening up the cases we keep uncovering. Let me know if I'm off base on any of these!
| * collapse into the main header toolbar. | ||
| */ | ||
| const hasLargeTitle = this.el.querySelector('ion-title[size="large"]') !== null; | ||
| if (!hasLargeTitle) { |
There was a problem hiding this comment.
I pulled the branch and this one renders the page with no visible header on both md and ios. When a condense header has a plain (non-large) title, the header-collapse-condense class still gets applied in render(), but setupCondenseHeader bails on this large-title guard before it reaches the code that adds header-collapse-main to the primary header. So the :has() ghost-hide rule in header.common.scss pins the main header at opacity: 0 and nothing ever releases it.
Repro is /header/test/scroll-effect-condense-no-large-title: the main header computes to opacity: 0 and never gets header-collapse-main. The new e2e only screenshots the condense header, so it doesn't catch the main one vanishing.
I think we either gate the header-collapse-condense class on a large title too, or keep assigning header-collapse-main when we bail here. Worth a test that asserts the main header stays visible in the no-large-title case.
| * `"condense"` collapses the large title into the main toolbar on scroll. | ||
| * `"fade"` fades the toolbar background on scroll. | ||
| */ | ||
| @Prop() scrollEffect?: HeaderScrollEffect; |
There was a problem hiding this comment.
Since scrollEffect isn't reflected, this breaks down under framework property binding. I set the prop as a property (the way the Angular/Vue/React proxies bind it) and confirmed no scroll-effect attribute lands on the host, so el.matches('[scroll-effect="condense"]') is false and the class only appears after render.
That trips up the fade setup's querySelector('...ion-header[scroll-effect="condense"]') on line 161, which has no class fallback, so a fade header paired with a property-bound condense sibling loses the coordination and fades in immediately. The :has() flash guard has the same gap in the pre-hydration window.
Adding reflect: true here would make both selectors robust and match the comment in header.common.scss that already assumes it's reflected. Flagging in case reflection was intended and just got missed.
| React.PropsWithoutRef<StencilReactExternalProps<PropType, ElementType>> | ||
| > | ||
| ); | ||
| return React.forwardRef(forwardRef); |
There was a problem hiding this comment.
This looks unrelated to the scroll effect work, and it reverts the explicit forwardRef cast (plus the comment explaining it) that's still on next. Feels like a bad merge? It re-widens the type the cast was guarding. Could we drop this hunk unless it's intentional?
| this.el.setAttribute('aria-hidden', 'true'); | ||
| } else { | ||
| this.el.removeAttribute('inert'); | ||
| this.el.removeAttribute('aria-hidden'); |
There was a problem hiding this comment.
The commit says this unifies aria-hidden under one imperative writer, but there are still two paths that disagree. The keyboard callback ORs the keyboard and scroll conditions, while setHidden(false) here clears aria-hidden and inert unconditionally.
I reproduced it: with the keyboard open on a bottom tab bar (aria-hidden correctly true), scrolling down then back to the top fires setHidden(false), which strips both aria-hidden and inert while the keyboard is still open, so the off-screen tab bar gets exposed to AT again. Could setHidden OR in the keyboard state too, or route both writers through one function?
| const hasFade = effect === 'fade'; | ||
|
|
||
| this.destroyCollapsibleHeader(); | ||
| this.activeEffect = effect; |
There was a problem hiding this comment.
We set this.activeEffect = effect here before the setup actually runs, and the setup below is gated on contentEl (condense/fade have their own early returns too). If content mounts late (behind an *ngIf/v-if), the effect gets marked active with nothing wired up, and the effect === this.activeEffect guard above then short-circuits every later componentDidUpdate, so it never recovers.
I reproduced this with deferred content: the header never picks up content-header-hide-scroll-partner even after the content appears and the header re-renders, while the same setup with content present up front works fine. Same deal in footer.tsx. I think setting activeEffect only after setup succeeds would fix it.
| await content.evaluate((el: HTMLIonContentElement) => el.scrollToBottom(0)); | ||
| await page.locator('ion-header.header-scroll-hidden').waitFor(); | ||
|
|
||
| await expect(header).toHaveClass(/header-scroll-hidden/); |
There was a problem hiding this comment.
The hide effect is the headline of the PR but it's the one effect with no visual or computed-style coverage. These assert the class and inert but never that the bar actually moves. The condense tests have screenshots and the fade ones check --opacity-scale, so hide is the odd one out.
If a theme bundle ever dropped the transform rule, the bar would sit on screen but inert over the gap-compensated content, and every test here would still pass. Could we add one toHaveScreenshot() or a getComputedStyle(...).transform check in the hidden state? Applies to the footer and tab-bar hide specs too.
| } | ||
| } | ||
|
|
||
| private setupScrollEffectHide = async (contentEl: HTMLElement) => { |
There was a problem hiding this comment.
Nit / future maintenance: the controller cleanly owns the scroll math, but the wiring around it (the promise-identity guard, the ResizeObserver, the --internal-*-hide-height read/write, the content partner-class toggling, and the teardown) is copy-pasted almost verbatim across header, footer, and tab-bar. A fix to the hide behavior has to land in three places now.
Since all three already share the controller, could we push that wiring into it (or a small partner helper that takes the css var + class names) so it lives once? Fine to leave for a follow-up though.
| // Named differently from header/footer's destroyCollapsible* because | ||
| // tab-bar only supports "hide", while header/footer handle multiple | ||
| // effects (hide, condense, fade) through one teardown method. |
There was a problem hiding this comment.
| // Named differently from header/footer's destroyCollapsible* because | |
| // tab-bar only supports "hide", while header/footer handle multiple | |
| // effects (hide, condense, fade) through one teardown method. |
Nit: this reads more like PR context than something the code needs. The method name already says what it does, and it points at destroyCollapsible* in the other files which'll go stale if those get renamed. Up to you though!
Issue number: resolves internal
What is the current behavior?
ion-headerandion-footerhave acollapseprop for scroll-driven effects (condense,fade), but it only works on the iOS theme.ion-tab-barhas an unreleased hideOnScroll boolean prop that only works on the Ionic theme withexpand="compact".What is the new behavior?
scrollEffectstring enum prop toion-header,ion-footer, andion-tab-bar.scrollEffect="hide"slides the bar out of view when scrolling down and back in when scrolling up.scrollEffect="condense"andscrollEffect="fade"onion-header/ion-footerreplace the existingcollapsevalues and work across all themes.collapseprop onion-headerandion-footerwith a console warning directing developers to usescrollEffect.hideOnScrollprop fromion-tab-bar.ion-tab-baris nested insideion-footer, the footer owns the hide animation to prevent double-animationDoes this introduce a breaking change?
Other information
Relevant Test Pages:
Header
Deprecated
collapsepropscrollEffectpropFooter
Deprecated
collapsepropscrollEffectpropTab Bar