sadpandajoe commented on code in PR #44102:
URL: https://github.com/apache/superset/pull/44102#discussion_r4020271695
##########
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:
The actual classic-table DOM has another fixed-height `<div style={{ width,
height }}>` above the `role="table"` wrapper, but it has no overflow style, so
this selector never resets it; after the inner rows expand, `.chart-container`
remains at the tile height and the screenshot still clips them. Could this
reset that wrapper too and update the regression fixture to include the full
`DataTable` chain?
##########
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:
This is the only behavioral regression test, but `pytest.importorskip` skips
it in the Python unit job because that environment does not install the Python
Playwright package or Chromium; the two substring assertions still pass if the
loop stops expanding anything. Could this run in an enforced browser CI job so
reverting the fix fails a required check?
--
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]
