[ui] Change tag collection size-to-fit timing to avoid jank #26834
Conversation
|
Deploy preview for dagit-core-storybook ready! ✅ Preview Built with commit ec7b548. |
There was a problem hiding this comment.
It's a shame that React doesn't give us a way to schedule when it mutates the DOM :(. Batching DOM queries/mutations app-wide is key to avoiding jank and reducing the number of reflows. Though I guess in this case useVirtualizer itself probably schedules its own RAF so we'd need to coordinate with it too and make sure we happen after it.
|
Hmm might be worth trying: |
|
Will try Re the setTimeout one, I think I want to perform the layout sooner than it is currently happening - ideally there are no animation frames at all in which the incorrect height is rendered. I think that one might be going in the other direction. |
|
Confirmed that using useLayoutEffect + window.requestAnimationFrame is also better (5 renders => 3), going to go with that. |
bengotow
changed the title
bengotow
force-pushed
the
bengotow-2025-01/fix-jank
branch
from
88d575b to
830d208
Compare
We discussed this previously in #26122 — using a `useEffect + requestAnimationFrame` is technically the ideal time to perform the DOM sizing code in the asset tag collections, but it performs really badly on the runs page. This is because the tag collection appears within each row of a virtualized table, and the `useEffect + requestAnimationFrame` combo causes our layout to happen _after_ the rows have been measured, and the changes cause them to be re-measured.
bengotow
force-pushed
the
bengotow-2025-01/fix-jank
branch
from
830d208 to
ec7b548
Compare
## Summary & Motivation This is a small follow-up to #26834 I've noticed that sometimes with MiddleTruncate, there is a very slight flicker when the text is first rendered. There's a single frame where the rendered text is `null`, and you can see it before the text appears. I'm not sure if my computer is just running slow today (looking for excuses to get an M4 now...), or if it's the volume of MiddleTruncates on this page (there are at least 50 rendering all at the same time), but here's an example if you watch the tag on the right: https://github.com/user-attachments/assets/4a05049d-9e70-4aaf-883a-eb448a905b01 Looking at the code for this, I don't think there's a reason for the sizing to update React state and set the text through a React render. The text is just used to set the span content, and the component doesn't re-render unless the text changes (in which case `calculateTargetStyle` changes and the effect runs again.) Setting the textContent directly removes the flicker shown in the video. ## How I Tested These Changes Tested in FF + Chrome manually --------- Co-authored-by: bengotow <[email protected]>
|
Build errors here seem unrelated, going to merge this! |
Summary & Motivation
We discussed this previously in #26122 — using a
useEffect + requestAnimationFrameis technically the ideal time to perform the DOM sizing code in the asset tag collections, but it performs badly on the runs page due to interactions with react-virtual.The tag collection appears within each row of a virtualized table, and I think that the
useEffect + requestAnimationFramecombo causes our layout to happen after the rows have been measured, and the height changes cause them all to be re-measured after a visible re-paint.How I Tested These Changes
The bad traces look like this:
After:
(note this is a key-up event causing a big scroll to lots of new rows all at once)