SCAL-338563 Reposition pre-rendered frames on any host scroll or layout shift - #684
sastaachar wants to merge 3 commits into
Conversation
The pre-render wrapper is absolutely positioned and was re-synced by only two triggers: a scroll listener on a custom containerSelector, and a ResizeObserver on the placeholder. Neither fires for the common case. With no containerSelector the wrapper sits in document.body and its top is placeholderRect.y + window.scrollY, so when the host app scrolls an inner element — window.scrollY stays 0 — that top is frozen at the placeholder's viewport position and the frame stays pinned while the page scrolls under it. trackPreRenderPosition() replaces both triggers with the four signals that cover every way a placeholder moves: scroll captured on window, which sees scrolls on any element so the scrolling element needs no configuration; resize; a ResizeObserver on the placeholder and its ancestors; and observeElementMove, an IntersectionObserver framed on the placeholder's own rect, which catches displacement with no size change, such as content growing above the frame. Syncs coalesce to one per animation frame. hidePreRender() now parks the wrapper above the container's top edge, so a hidden frame keeps its measured size without inflating scrollable overflow, and the fullHeight placeholder seed uses a height the app has actually reported rather than the 100vh createPreRenderWrapper() writes before anything is measured. SCAL-338563
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements position tracking for pre-rendered embeds to ensure the wrapper correctly follows the placeholder during scrolling or layout changes, addressing issues where the frame would remain pinned to the viewport. It introduces utility functions getPositioningAncestors and observeElementMove to monitor element displacement and updates TsEmbed to manage these tracking listeners. The reviewer provided a high-severity suggestion to throttle the refresh logic in observeElementMove to prevent layout thrashing and scroll jank.
| export const observeElementMove = ( | ||
| element: HTMLElement, | ||
| onMove: () => void, | ||
| ): (() => void) => { | ||
| if (typeof IntersectionObserver === 'undefined') { | ||
| return () => undefined; | ||
| } | ||
|
|
||
| let observer: IntersectionObserver | null = null; | ||
| let stopped = false; | ||
| // Guards against the rebuild below re-entering through the initial | ||
| // callback every observer delivers on observe(). | ||
| let isFirstCallback = true; | ||
|
|
||
| const refresh = () => { | ||
| if (stopped) { | ||
| return; | ||
| } | ||
| observer?.disconnect(); | ||
|
|
||
| const rect = element.getBoundingClientRect(); | ||
| const { innerHeight, innerWidth } = window; | ||
| // A zero-area element cannot be framed; wait for it to gain a size. | ||
| if (!rect.width || !rect.height) { | ||
| observer = null; | ||
| return; | ||
| } | ||
|
|
||
| const margins = [ | ||
| -Math.floor(rect.top), | ||
| -Math.floor(innerWidth - rect.right), | ||
| -Math.floor(innerHeight - rect.bottom), | ||
| -Math.floor(rect.left), | ||
| ]; | ||
|
|
||
| isFirstCallback = true; | ||
| observer = new IntersectionObserver( | ||
| (entries) => { | ||
| const ratio = entries[0]?.intersectionRatio ?? 0; | ||
| if (isFirstCallback) { | ||
| isFirstCallback = false; | ||
| return; | ||
| } | ||
| if (ratio !== 1) { | ||
| onMove(); | ||
| } | ||
| refresh(); | ||
| }, | ||
| { | ||
| rootMargin: margins.map((margin) => `${margin}px`).join(' '), | ||
| threshold: 1, | ||
| }, | ||
| ); | ||
| observer.observe(element); | ||
| }; | ||
|
|
||
| refresh(); | ||
|
|
||
| return () => { | ||
| stopped = true; | ||
| observer?.disconnect(); | ||
| observer = null; | ||
| }; | ||
| }; |
There was a problem hiding this comment.
Recreating the IntersectionObserver and calling getBoundingClientRect() on every single scroll/movement frame causes severe layout thrashing and scroll jank. Since scroll events are already captured on window and trigger the sync, we should throttle the refresh() call inside observeElementMove to avoid constant observer recreation during active scrolling.
export const observeElementMove = (
element: HTMLElement,
onMove: () => void,
): (() => void) => {
if (typeof IntersectionObserver === 'undefined') {
return () => undefined;
}
let observer: IntersectionObserver | null = null;
let stopped = false;
// Guards against the rebuild below re-entering through the initial
// callback every observer delivers on observe().
let isFirstCallback = true;
let timeoutId: any = null;
const refresh = () => {
if (stopped) {
return;
}
observer?.disconnect();
const rect = element.getBoundingClientRect();
const { innerHeight, innerWidth } = window;
// A zero-area element cannot be framed; wait for it to gain a size.
if (!rect.width || !rect.height) {
observer = null;
return;
}
const margins = [
-Math.floor(rect.top),
-Math.floor(innerWidth - rect.right),
-Math.floor(innerHeight - rect.bottom),
-Math.floor(rect.left),
];
isFirstCallback = true;
observer = new IntersectionObserver(
(entries) => {
const ratio = entries[0]?.intersectionRatio ?? 0;
if (isFirstCallback) {
isFirstCallback = false;
return;
}
if (ratio !== 1) {
onMove();
}
// Throttle the observer recreation to avoid layout thrashing
// and high CPU usage during continuous updates (like scrolling).
if (!timeoutId) {
timeoutId = setTimeout(() => {
timeoutId = null;
refresh();
}, 150);
}
},
{
rootMargin: margins.map((margin) => `${margin}px`).join(' '),
threshold: 1,
},
);
observer.observe(element);
};
refresh();
return () => {
stopped = true;
observer?.disconnect();
observer = null;
if (timeoutId) {
clearTimeout(timeoutId);
}
};
};…observer thrashing Two follow-ups from testing the positioning fix against the reported cases. The wrapper is not a descendant of the host's scrolling box — by default it is a document.body sibling — so that box cannot clip it. Positioned correctly it still paints over a sticky nav or a panel edge the moment its placeholder scrolls underneath one, which is what a frame "moving above the nav" looks like. syncPreRenderStyle now reproduces the clip its placeholder would get in flow, via getClipInsetForElement and the clipping ancestors the SDK already tracks. observeElementMove re-armed inside its own callback, so a continuous scroll allocated an IntersectionObserver every frame and read layout with it, to report movement the window scroll listener had already handled. The re-frame is now deferred until movement settles; because the element can move while disarmed, each re-frame compares against the rect it last framed and reports what it missed. SCAL-338563
Second defect found while testing this against the reported casesPositioning was only half of it. The wrapper is a Measured in a browser at the same deep scroll, wrapper raw top 34px in both cases:
The second row is with this PR's position tracking already applied:
One sharp edge worth knowing for anyone writing tests here: Also in this push
Suite: 49 suites, 1995 passed, 4 skipped. |
Now reproduced against the real SDK, with primary evidencePreviously this PR was reasoned from source plus a synthetic port of the positioning arithmetic. It is now backed by a capture of the actual failing customer page and a replica driven by the real SDK. From the customer's captured DOM:
Replica: their layout — fixed nav, inner Grow the placeholder to 4597px (what
The 3917px shortfall matches the production capture exactly, so the replica is faithful rather than approximate. Mechanism, confirmed rather than inferred. The only re-sync trigger is the ResizeObserver on the placeholder. It fires on size changes and incidentally corrects position too, which is why the height self-heals. A pure scroll changes no size, so nothing fires and the frame sits frozen — drift equals the scroll distance exactly. Once any sync does run, the wrapper lands at the placeholder's viewport coordinates with no clipping and then overruns the nav (measured at 403px). One correction to this PR's earlier description: the "Regression range" section naming #517 is wrong and I'll remove it. The frozen |
Verified end-to-end on a live clusterPrevious evidence was a stub whose iframe never loaded. This is a real Liveboard rendering real charts from a real cluster, laid out the way the reporting customer's page is: fixed nav, inner
On 1.52.1 the frame never moves: drift equals the scroll distance exactly, at every position. On this branch drift stays 0 and the frame is clipped to the nav's lower edge, the clip growing as it scrolls under. Both defects this PR addresses are now confirmed against a live embed rather than a simulation, and both are fixed. |
This branch has not been deployed
1 participant
Two customers, one defect: a pre-rendered liveboard frame stays pinned while the host page scrolls, so it overlaps page content and the liveboard can never be scrolled to its bottom.
Cause
The pre-render wrapper is
position: absoluteand is placed on its placeholder bysyncPreRenderStyle(). It is re-synced by exactly two triggers: ascrolllistener on a custompreRenderConfig.containerSelector, and aResizeObserveron the placeholder.Neither fires in the common case. With no
containerSelectorthe wrapper is appended todocument.bodyand itstopisplaceholderRect.y + window.scrollY. When the host app scrolls an inner element rather than the document —bodynever scrolls, sowindow.scrollYstays0— thattopis frozen at wherever the placeholder was when it was last synced. Both reported apps have that shape.Measured against a port of the current algorithm: scrolling an inner container 260px leaves the frame 260px out of place, exactly the scroll distance.
Three related gaps, each reported independently by one of the customers and each confirmed:
hidePreRender()never resets the wrapper's width/height, so a hidden frame keeps its full measured size attop: 0and inflates the container's scrollable overflow.showPreRender()seeds the placeholder frompreRenderWrapper.style.heightunderfullHeight, which on a first reveal is still the100vhthatcreatePreRenderWrapper()wrote — a full-viewport overshootframeParamscannot override.containerSelectorconfigured correctly.Change
trackPreRenderPosition()replaces the container-only scroll listener and the placeholder-onlyResizeObserverwith the four signals that cover every way a placeholder moves, coalesced to one sync per animation frame:scrollcaptured onwindow, which sees scrolls on any element in the page, so the host app's scrolling element needs no configuration;resize;ResizeObserveron the placeholder and every ancestor up to the container;observeElementMove()(new inutils.ts) — anIntersectionObserverframed on the placeholder's own rect, which fires on displacement with no size change.hidePreRender()parks the wrapper withtranslateY(-100%): it keeps the measured size, so the next show still needs no resize, while adding no scrollable overflow. ThefullHeightplaceholder seed now uses a height the embedded app has actually reported.The container scroll listener and
reconcilePreRenderContainer()'s listener migration are removed as dead code.preRenderConfig.containerSelectorkeeps working and is no longer needed for correct positioning.Regression range
syncPreRenderStyle()measured the host element directly until #517, which introduced the SDK-owned placeholder sized fromframeParamsand switched the measurement to it.createPreRenderPlaceholderis absent from published 1.48.0 and present in 1.49.0, which is the boundary the reporting customer crossed.Verification
position tracking (SCAL-338563). All 7 fail on unfixed source, all 7 pass with the change.containerSelectorset.package.jsonis on1.52.1-beta.1so the two customers can verify before release.