Skip to content

Commit

Permalink
Fix jittery scrolling at bottom of some tables in Chrome (#1409)
Browse files Browse the repository at this point in the history
Summary: In some cases, `react-window` would get itself stuck in a
computation loop relating to overscroll.
This turned out to be a consequence of an interaction between
`react-window` and [scroll
anchoring](https://developer.mozilla.org/en-US/docs/Web/CSS/overflow-anchor/Guide_to_scroll_anchoring).
By disabling scroll anchoring in this virtualized list, and fixing a
performance bug in the table summary's render, the issue goes away
completely.

Type of change: /kind bugfix

Test Plan: Run `px/cluster` in Chrome, or `px/http_data` in Firefox (in
the latter case, resize the table to be one "grid unit" shorter and run
again).
Then, scroll to the bottom of the result tables.
Before, they would jitter nonstop in Chrome, or only for a moment in
Firefox before it blocks scroll anchoring with a message in the console.
Now, they should not jitter at all.
Trying a streaming script should still work, even if you scroll all the
way down and let it lock there.

Signed-off-by: Nick Lanam <[email protected]>
  • Loading branch information
NickLanam authored Jun 1, 2023
1 parent 9dceacc commit 026841f
Show file tree
Hide file tree
Showing 2 changed files with 30 additions and 14 deletions.
5 changes: 5 additions & 0 deletions src/ui/src/components/data-table/data-table.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,11 @@ const useDataTableStyles = makeStyles((theme: Theme) => createStyles({
// The body is pushed down by the height of the (sticky-positioned) header. react-window already accounts for this.
tableBody: {
position: 'absolute',
// If we don't do this, react-window tends to recompute overflow back-and-forth between two values every frame
// when scrolling to the bottom. Firefox blocks that from happening after 10 loops, Chrome lets it keep going.
// Firefox blocks it by disabling scroll anchoring, after which the behavior is returned to something acceptable.
// If we disable it from the start, the bug never triggers.
'& > div:first-child': { overflowAnchor: 'none' },
},
bodyRow: {
borderTop: `1px solid ${theme.palette.background.six}`,
Expand Down
39 changes: 25 additions & 14 deletions src/ui/src/containers/live-widgets/table/query-result-viewer.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ export interface QueryResultTableProps {
}

export const QueryResultTable = React.memo<QueryResultTableProps>(({
display, table, propagatedArgs, customGutters = [], setExternalControls,
display, table, propagatedArgs, customGutters, setExternalControls,
}) => {
const classes = useStyles();
const { streaming } = React.useContext(ResultsContext);
Expand Down Expand Up @@ -163,28 +163,39 @@ export const QueryResultTable = React.memo<QueryResultTableProps>(({
classes.tableSummaryExtern, classes.externalControls,
]);

return (
// To reduce how many components update when scrolling
const defaultSummary = React.useMemo(() => {
if (setExternalControls) return null;
return (
<div className={classes.tableSummary}>
<TableSummary
visibleStart={visibleStart}
visibleStop={visibleStop}
numRows={numRows}
isOverload={isOverload}
/>
</div>
);
}, [setExternalControls, classes.tableSummary, visibleStart, visibleStop, numRows, isOverload]);

// This gets memoized too, since scrolling should _only_ be updating the innermost <List /> and the summary.
// Without this, the entire context stack in between the two wastes as much as 8ms per scroll event changing nothing.
return React.useMemo(() => (
<div className={classes.root}>
<div className={classes.table}>
<LiveDataTable
table={table}
gutterColumns={[display.gutterColumn, ...customGutters].filter(g => g)}
gutterColumns={[display.gutterColumn, ...(customGutters ?? [])].filter(g => g)}
propagatedArgs={propagatedArgs}
onRowsRendered={onRowsRendered}
setExternalControls={setExternalControls ? globalControlsRef : null}
/>
</div>
{!setExternalControls && (
<div className={classes.tableSummary}>
<TableSummary
visibleStart={visibleStart}
visibleStop={visibleStop}
numRows={numRows}
isOverload={isOverload}
/>
</div>
)}
{defaultSummary}
</div>
);
), [
classes.root, classes.table, defaultSummary, customGutters, display.gutterColumn, globalControlsRef,
onRowsRendered, propagatedArgs, setExternalControls, table,
]);
});
QueryResultTable.displayName = 'QueryResultTable';

0 comments on commit 026841f

Please sign in to comment.