[MWPW-204799] - Perf: debounce resize handler and cache sticky-top read in comparison… - #6502
[MWPW-204799] - Perf: debounce resize handler and cache sticky-top read in comparison…#6502DKos95 wants to merge 3 commits into
Conversation
…-table-c2 Batches resize-triggered layout work into a single rAF and avoids a getComputedStyle read on every scroll event. Uses the shared getGnavHeight() utility instead of a block-local nav-height calculation.
The sticky element is the outer header, not the nav inside it (per MWPW-201841 / PR #6347's getGnavHeight fix). The ResizeObserver was still targeting header > nav, so nav-height resync could miss size changes on the actual sticky element.
| @@ -1,5 +1,6 @@ | |||
| import { createTag, getConfig, loadStyle } from '../../../utils/utils.js'; | |||
| import { getMetadata as getSectionMetadata } from '../section-metadata/section-metadata.js'; | |||
| import { getGnavHeight } from '../../../blocks/global-navigation/utilities/utilities.js'; | |||
There was a problem hiding this comment.
We've seen the need of measuring gnavHeight in many places and will likely see more, so instead of importing it from the global-navigation block, should we move it to some higher-level util files? We also have the css variables in c2 styles sheet (stage branch, not in site-redesign-foundation branch yet), so maybe in the future we need to consolidate a bit
There was a problem hiding this comment.
I agree. I think once we merge stage into site-redesign-foundation we'll have access to that CSS token, which should help.
|
|
||
| const nav = document.querySelector('header > nav') ?? document.querySelector('header'); | ||
| if (nav) new ResizeObserver(syncTop).observe(nav); | ||
| const header = document.querySelector('header'); |
There was a problem hiding this comment.
Will the Promo-aside changes trigger a resync?
The height calc includes .feds-promo-aside-wrapper, but the ResizeObserver only watches <header>, and promo-aside height changes don't resize the header box.
There was a problem hiding this comment.
Promo is not in the header element, also it vanishes once you start scrolling so really it shouldn't be taken into account seeing as it doesn't occupy space.
| // breakpoint change) — it's a static CSS value the rest of the time, not | ||
| // something that needs a fresh getComputedStyle read on every scroll | ||
| // event. Cache it here and refresh only when syncTop() actually runs. | ||
| let stickyTopCache = getStickyTop(); |
There was a problem hiding this comment.
This is read synchronously at setup, before syncTop() has ever set --ct-nav-height, so it can resolve to 0 on a scroll event that lands early (e.g., scroll restoration, hash-jump). The old per-scroll getComputedStyle read self-healed instantly. The new cache doesn't until a resize/breakpoint event reruns syncTop().
There was a problem hiding this comment.
Very nice catch, thanks for the review!
There was a problem hiding this comment.
Updated to match feedback.
|
Not related to this PR but leaving some notes from Claude about
|
5 participants
What/why
resizelistener calledremoveCollapsed()synchronously on every event; batched into a singlerequestAnimationFrameper resize burst to avoid redundant layout work.getStickyTop()(agetComputedStylecall) ran on everyscrollevent even though the value only changes whensyncTop()runs (nav height/breakpoint change). Cached it and only refresh onsyncTop().getNavHeight()calculation for the sharedgetGnavHeight()utility fromglobal-navigation/utilities.js, so it stays consistent with the rest of the codebase instead of maintaining a duplicate nav-height calc.Resolves: MWPW-204799
Test URLs: