rusackas commented on code in PR #44102:
URL: https://github.com/apache/superset/pull/44102#discussion_r4021573275
##########
superset/utils/screenshot_utils.py:
##########
@@ -641,15 +641,33 @@ def _unready_chart_holders_js_body(*, viewport_only:
bool) -> str:
(el) => {{ el.style.height = 'auto'; }}
);
-
document.querySelectorAll('{GENERIC_SCROLLABLE_DESCENDANT_SELECTOR}').forEach(
- (el) => {{
- if (el.scrollHeight > el.clientHeight) {{
- el.style.overflow = 'visible';
- el.style.height = 'auto';
- el.style.maxHeight = 'none';
+ // A single pass can miss an ancestor whose own fixed height was
+ // computed as the sum of its (still-clipped) children -- e.g.
+ // plugin-chart-table's sticky wrapper (useSticky.tsx) is a
+ // `role="table"` div with a fixed height + `overflow: hidden` around
+ // its scrollable body. Before this loop runs, that wrapper's own
+ // scrollHeight already equals its clientHeight (nothing has grown yet),
+ // so the scrollHeight > clientHeight gate below skips it on the pass
+ // that also expands its child. Only after the child's height resolves
+ // to 'auto' does the wrapper's own overflow become visible to a
+ // re-check -- so repeat until a pass makes no further changes, capped
+ // to bound the cost of a pathologically deep DOM.
+ let changedInPass = true;
+ let passes = 0;
+ while (changedInPass && passes < 5) {{
+ changedInPass = false;
+ passes += 1;
+
document.querySelectorAll('{GENERIC_SCROLLABLE_DESCENDANT_SELECTOR}').forEach(
Review Comment:
`.slice_container` sits above `role="table"` and already gets `height: auto`
unconditionally a few lines above this loop, not gated on the overflow check.
The regression fixture includes that exact wrapper and `.chart-container` grows
to fit all 50 rows, so this one's covered already.
##########
tests/unit_tests/utils/test_screenshot_utils.py:
##########
@@ -1867,3 +1867,109 @@ def
test_expand_scrollable_content_js_unrolls_ag_grid_and_css_scroll() -> None:
# bounded by a report's remaining deadline (see webdriver_test.py).
assert "async (maxWaitMs) =>" in EXPAND_SCROLLABLE_CONTENT_JS
assert "Date.now() + maxWaitMs" in EXPAND_SCROLLABLE_CONTENT_JS
+
+ # The generic-descendant reset repeats to a fixed point (bounded), not a
+ # single pass: an ancestor whose own height was computed as the sum of
+ # its still-clipped children (e.g. plugin-chart-table's `role="table"`
+ # sticky wrapper in useSticky.tsx) has scrollHeight == clientHeight
+ # *before* its child is expanded, so a single querySelectorAll pass
+ # skips it -- see
test_expand_scrollable_content_resolves_nested_ancestor_clip
+ # below for the real-browser reproduction.
+ assert "changedInPass" in EXPAND_SCROLLABLE_CONTENT_JS
+ assert "passes < 5" in EXPAND_SCROLLABLE_CONTENT_JS
+
+
+def test_expand_scrollable_content_resolves_nested_ancestor_clip() -> None:
+ """Real-browser regression test (Playwright/Chromium) for the fixed-point
+ loop above: reproduces the exact nested clip that a single-pass reset
+ missed for `plugin-chart-table`.
+
+ `useSticky.tsx` renders a `role="table"` wrapper with a fixed pixel
+ height and `overflow: hidden`, around a `scrollBodyRef` div that has its
+ *own* fixed height + `overflow: auto`. Before any DOM mutation, the
+ wrapper's `scrollHeight` already equals its `clientHeight` (its height
+ was computed as the sum of its still-clipped children), so a single
+ querySelectorAll pass expands the inner scroll body but has already
+ evaluated -- and skipped -- the outer wrapper by the time the inner one
+ grows. This fixture reproduces that exact two-level shape and asserts
+ every row ends up inside the bounding box a locator-scoped
+ `element.screenshot()` would capture (the same clip Thread B of
+ @aminghadersohi's #43979 review flagged for the ag-Grid ancestor case).
+
+ Skips (does not fail) when Playwright's Python package or a Chromium
+ binary is unavailable -- this environment's own CI unit-test job does
+ not install either today, so this is currently a local/dev verification
+ aid rather than an enforced CI gate.
+ """
+ pytest.importorskip("playwright.sync_api")
Review Comment:
Fair, but wiring playwright + chromium into the unit-test job is a bigger
infra lift than this fix warrants, kind of its own PR. Called that out in the
description already; the changedInPass/passes<5 string asserts are what's
actually enforced for now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
