sadpandajoe commented on code in PR #44102:
URL: https://github.com/apache/superset/pull/44102#discussion_r4289021989
##########
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 `DataTable` outer wrapper (`<div style={{ width, height }}>` at
`DataTable.tsx:552`) still sits between `.slice_container` and the
`role="table"` div, and it has no inline `overflow`, so `[style*="overflow"]`
never matches it and it keeps its fixed pixel height. I added that wrapper to a
copy of the regression fixture and ran the loop in headless Chromium:
`.chart-container` stays at 300px while the 50th row ends at about 1636px, so a
locator-scoped screenshot still cuts the rows off (without the wrapper it grows
to 1638px, which is all the current fixture exercises). `.slice_container`
being `height: auto` doesn't help because it only sizes to that wrapper's fixed
box. Could the reset also cover that wrapper, and the fixture include it so the
test fails when it isn't handled?
--
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]
