Wrap toggleScrollListener calls in requestAnimationFrame callback to prevent forced style recalculations#25328
Wrap toggleScrollListener calls in requestAnimationFrame callback to prevent forced style recalculations#25328chenxinyanc wants to merge 2 commits into
Conversation
Perf Analysis (
|
| Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
|---|---|---|---|---|---|
| Avatar | mount | 1269 | 1269 | 5000 | |
| Button | mount | 923 | 925 | 5000 | |
| FluentProvider | mount | 1494 | 1489 | 5000 | |
| FluentProviderWithTheme | mount | 575 | 571 | 10 | |
| FluentProviderWithTheme | virtual-rerender | 539 | 547 | 10 | |
| FluentProviderWithTheme | virtual-rerender-with-unmount | 588 | 579 | 10 | |
| MakeStyles | mount | 1940 | 1998 | 50000 | |
| SpinButton | mount | 2318 | 2354 | 5000 |
Asset size changesSize Auditor did not detect a change in bundle size for any component! Baseline commit: a96544608a06750d4e551f13de68884e1a287fec (build) |
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 985b06b:
|
📊 Bundle size report
Unchanged fixtures
|
|
|
||
| updatePosition(); | ||
| // toggleScrollListener requires computed styles; thus use RAF to prevent forced style reevaluation. | ||
| window.requestAnimationFrame(() => { |
There was a problem hiding this comment.
We need to use a proper window instance, otherwise requestAnimationFrame may not be triggered properly.
You can get it from useFluent() (it's already used in this component):
const { targetDocument } = useFluent();
targetDocument?.defaultView?.requestAnimationFrame()|
|
||
| updatePosition(); | ||
| // toggleScrollListener requires computed styles; thus use RAF to prevent forced style reevaluation. | ||
| window.requestAnimationFrame(() => { |
There was a problem hiding this comment.
All requestAnimationFrame() calls should be cleaned up on a component to avoid reference/memory leaks or throwing exceptions. Typically it's:
const requestRef = React.useRef();
// ---
requestRef.current = requestAnimationFrame();
// ---
React.useEffect(() => {
return () => {
cancelAnimationFrame(requestRef.current);
};
}, []);
|
@chenxinyanc thanks for the PR (and the detailed issue) 👍 I left some comments about proposed implementation. However, I think that the problem has different roots: it's not expected that positioning will executed until a tooltip is visible. On the initial look it seems that it should work like that: And indeed positioning logic is never triggered until it's needed: The problem is that as you mentioned/noticed we add event listeners in Interesting that it was different before we moved to Floating UI (#24254): we had an instance of I was looking on how to solve it and came with something like (code needs some refactor! 🚨): function useContainerRef(updatePosition: () => void, enabled: boolean) {
const containerRef = React.useRef<HTMLElement | null>();
React.useEffect(() => {
if (enabled && containerRef.current) {
const scrollParent = getScrollParent(containerRef.current);
scrollParent.addEventListener('scroll', updatePosition);
updatePosition();
}
}, [enabled, updatePosition]);
return useCallbackRef<HTMLElement | null>(null, (container, prevContainer) => {
if (prevContainer) {
const prevScrollParent = getScrollParent(prevContainer);
prevScrollParent.removeEventListener('scroll', updatePosition);
}
containerRef.current = prevContainer;
if (container && enabled) {
// When the container is first resolved, set position `fixed` to avoid scroll jumps.
// Without this scroll jumps can occur when the element is rendered initially and receives focus
Object.assign(container.style, { position: 'fixed', left: 0, top: 0, margin: 0 });
const scrollParent = getScrollParent(container);
scrollParent.addEventListener('scroll', updatePosition);
updatePosition();
}
});
}
function useTargetRef(updatePosition: () => void, enabled: boolean) {
const targetRef = React.useRef<HTMLElement | PositioningVirtualElement | null>();
React.useEffect(() => {
if (enabled && targetRef.current instanceof HTMLElement) {
const scrollParent = getScrollParent(targetRef.current);
scrollParent.addEventListener('scroll', updatePosition);
updatePosition();
}
}, [enabled, updatePosition]);
return useCallbackRef<HTMLElement | PositioningVirtualElement | null>(null, (target, prevTarget) => {
if (prevTarget instanceof HTMLElement) {
const prevScrollParent = getScrollParent(prevTarget);
prevScrollParent.removeEventListener('scroll', updatePosition);
}
targetRef.current = target;
if (enabled && target instanceof HTMLElement) {
const scrollParent = getScrollParent(target);
scrollParent.addEventListener('scroll', updatePosition);
updatePosition();
}
});
}In this case we don't call Current master
The change
|
|
A different fix was applied in #25456. |
5 participants
Current Behavior
Rendering
<Tooltips>will force style recalculations duringtoggleScrollListenercalls.New Behavior
toggleScrollListenercalls are deferred until the next animation frame, when the browser has already calculated the styles and done page layout.Related Issue(s)
Fixes #25326