SCAL-338563 Place the pre-rendered frame in the host's scrolling ancestor - #684
sastaachar wants to merge 12 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. |
The build job fails on stale static/typedoc/typedoc.json, and guard-version-bump rejects a version edit made in a PR — that repo uses the bumpVersion workflow. Reverting the bump lets CI go green, which is also what publishes the pkg.pr.new preview build. SCAL-338563
commit: |
Reworked from the earlier approach. Repositioning a body-level frame on every scroll meant a layout read and a style write per animation frame, to compensate for the frame being in the wrong box in the first place. Removed: the rAF coalescer, the window-capture scroll listener, the IntersectionObserver move detector, and the clip-path work. An absolutely positioned wrapper follows the page for free when it sits inside whatever scrolls. document.body is right whenever the document is the scroller, which is most pages and is what pre-render was built against. An app that scrolls an inner element has to name it with preRenderConfig.containerSelector, and there is nothing the SDK can do from document.body without per-scroll work. So the default stays, and the SDK now warns once, naming the scrolling element it found, when an embed sits inside one and no containerSelector is set. That is the difference between the two reports: one customer found the option, the other had no signal it existed. Also kept from the earlier work: hidePreRender parks the wrapper so a hidden frame stops adding scrollable area, 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
sastaachar
changed the title
The wrapper is absolutely positioned, so it follows the page for free — but only while it sits inside whatever scrolls. It defaulted to document.body, which is right only when the document itself is the scroller. An app that scrolls an inner element instead left the frame pinned to the viewport while the page moved under it, because window.scrollY never changes there and the position it was given is a viewport coordinate that is stale immediately. So resolve the default container to the host element's nearest scrolling ancestor, falling back to document.body when nothing scrolls. A scrolling ancestor is layout chrome rather than route content, so it outlives the embed; and if it does go, the host element goes with it and the embed is remounting anyway. reconcilePreRenderContainer re-runs the resolution on a detached container. preRenderConfig.containerSelector still overrides. Being inside the scroller also fixes wheel scrolling over the frame: events chain out of the iframe into the element that scrolls, which they cannot do from document.body, so a full-height liveboard used to swallow them. showPreRender now sets pointer-events rather than removing it. A container carrying pointer-events:none passes it down, and dropping the property left the inherited none in force with the frame dead to input. Also: hidePreRender parks the wrapper so a hidden frame adds no scrollable area, and the fullHeight placeholder seed ignores the 100vh createPreRenderWrapper writes before anything is measured. SCAL-338563
sastaachar
changed the title
…ual config React constructs a new LiveboardEmbed on every mount, so the previous === this check never matched and every remount of an unchanged pre-rendered liveboard routed via home and reloaded. Compare viewConfig instead, treating callbacks as equal. SCAL-336321
isSameEmbedConfig compared viewConfig objects directly, so it never returned true after the previous instance had been shown once. navigateToLiveboard assigns vizId, activeTabId and personalizedViewId unconditionally, leaving them as undefined-valued keys; a freshly constructed instance does not have those keys at all, and isEqual counts own keys. The comparison therefore failed on configs that were identical in every value — which is the exact case it exists to detect. Measured against a live cluster, three re-mounts with an unchanged config: 3 home hops on 1.52.1, 2 with the comparison as written, 0 with this. A changed runtime filter still routes via home, as it must. The existing tests could not catch this: both instances are constructed with vizId and activeTabId passed explicitly, so the asymmetry never arises. Added a test that reproduces it by assigning the keys as undefined on the previous instance, the way navigateToLiveboard does. SCAL-336321
The React re-mount fix did not work as written — now fixedVerified against a live cluster and independently reproduced in a sandbox demo. Cause. this.viewConfig.activeTabId = activeTabId; // undefined
this.viewConfig.vizId = vizId; // undefined
this.viewConfig.personalizedViewId = personalizedViewId; // undefinedAn instance that has been shown once therefore carries three Measured on a live cluster, three re-mounts with an unchanged config:
The one hop it did save was the Why the tests passed anyway. Both instances in the existing tests are constructed with The control still behaves: a changed Suite: 49 suites, 1989 passed. |
SCAL-338563
flushAnimationFrame was there for the coalescer that is gone; syncs are synchronous again. The hidden-frame test asserted syncPreRenderStyle is not called on a scroll event, which now passes whether the frame is hidden or shown because nothing listens for scroll — replaced with the contract that does exist, that hiding releases the placeholder observer. SCAL-338563
This branch has not been deployed
1 participant
A pre-rendered frame does not follow the page when the host scrolls an inner element rather than the document. Reported by two customers, reproduced on a live cluster against both their layouts.
Cause
The wrapper is absolutely positioned and defaulted into
document.body, withtopcomputed asplaceholderRect.y + window.scrollY.That is correct whenever the document is the scroller: the containing block moves with the page, the browser carries the frame, and no listener is needed. It is how pre-render was built, and it covers most pages.
It breaks the moment the host scrolls an inner element.
window.scrollYis then permanently0, sotopis a viewport coordinate that is stale as soon as anything scrolls, and the frame sits still while the page moves under it.Measured on a live cluster. Drift equals the scroll distance exactly:
This is not a regression, and "it worked before" is also true
1.47.2 drifts identically, so there is no version to roll back to — pre-render has never worked in a host that scrolls an inner element.
Both customers nevertheless report it working before, and they are right. Neither was using pre-render before. One customer's patch adds
warm-liveboard.tsas a new file with everypreRenderIdline an addition: they adopted pre-render in the same change as the SDK upgrade. Their "before" is no pre-render, which has no bug to speak of — the iframe is in normal flow, it scrolls with the page, and wheel events chain out of it.So the upgrade did not break this. Turning on pre-render did, and the version changed in the same commit.
Change
Resolve the default container to the host element's nearest scrolling ancestor, falling back to
document.bodywhen nothing scrolls.preRenderConfig.containerSelectorstill overrides.Nearest is the correct choice, including nested scrollers: the wrapper goes in the innermost one and is positioned against it, so scrolling that element moves it through that element's content, and scrolling anything further out moves the whole box with the wrapper inside it.
On the container outliving the embed — the reason the default was
document.body— a scrolling ancestor is layout chrome rather than route content, and it is an ancestor of the host element, so if it goes the host element goes with it and the embed is remounting anyway.reconcilePreRenderContainerre-runs the resolution on a detached container.Being inside the scroller fixes a second symptom for free. Wheel events chain out of an iframe into the nearest scrollable ancestor of its wrapper; from
document.bodythere is nothing to chain into, so a full-height liveboard swallowed them and the page would not scroll with the cursor over the frame. Measured: host scroll 300 → 800 with this change, unmoved without it.Three smaller fixes on the same path:
showPreRendernow setspointer-events: autoinstead of removing the property. A container carryingpointer-events: none— the usual styling for a parking root that must not swallow clicks — passes it down, and dropping the property left the inheritednonein force with the frame dead to input. One customer had already worked around this with a> * { pointer-events: auto }rule.hidePreRenderparks the wrapper, so a hidden frame stops contributing scrollable area. It previously kept its full measured size attop: 0: verified on a live cluster, a hidden frame held the container'sscrollHeightat 7312px with nothing visible in it.fullHeightplaceholder seed ignores the100vhthatcreatePreRenderWrapper()writes before anything is measured, so a first reveal no longer overshoots to a full viewport.Deliberately not done
An earlier revision repositioned the frame on scroll — window-capture
scroll, a rAF coalescer, an IntersectionObserver move detector. It worked, but spent a layout read and a style write per animation frame to compensate for the frame being in the wrong box. Removed. Position is the browser's job once the frame is placed correctly; the only thing it will not do for us is resize the wrapper when the placeholder's height changes, which is what the remainingResizeObserveris for.Clipping the frame to its container is also out of scope — escaping a container is a z-index concern for the host app, not something pre-render guaranteed.
Effect on the two reports
Neither customer needs a host-app change. Verified with one customer's layout and their workaround removed entirely: drift 0 at every scroll position.
Suite: 49 suites, 1986 passed.