Defer the toolbar handle position calculation to an animation frame - #2433
Defer the toolbar handle position calculation to an animation frame#2433novucs wants to merge 3 commits into
Conversation
|
Thank you for the PR @novucs. Would you be able to reproduce this in a tool like selenium? |
ensureHandleVisibility() reads handle.offsetWidth during init(), directly after $$.show(handle) takes the handle from display:none. Layout is dirty at that point, so the read forces a synchronous document-wide style and layout flush after the page has already painted. On projects with a large stylesheet this is slow enough to be visible as a flash of unstyled content in Firefox. It only affects a collapsed toolbar, since showToolbar() does not call ensureHandleVisibility(). Deferring the body to requestAnimationFrame moves the read past the paint. Behaviour is otherwise unchanged, and the resize listener benefits too, as repeated events now coalesce to at most one reflow per frame.
@tim-schilling great idea. Here's a minimal reproducible script, how-to for running, and attached are video comparisons of the results: https://gist.github.com/novucs/9f8c31fa5661aecb3b8a940408914865?permalink_comment_id=6299585#gistcomment-6299585 |
abbc581 to
56d3929
Compare
|
Re-pushed to resolve lint errors in the changelog |
|
Thanks for sharing that! Though I should have been clear that it was a yes/no question 😅. We have selenium tests for now so we'll need to translate your gist. I'd like to switch to playwright eventually. If you have the time, do you mind pulling that example into a selenium test in the project? No worries if you're at your limit. |
|
Oops... 😂 I'll see what I can do when I next get a minute! Thanks for letting me know |
|
@tim-schilling I've now added & pushed a small regression test for this. While there, I've found a number of failing tests due to the shadow DOM change in #2266. The Selenium tests still find elements on the document, which can't reach inside the open shadow root. |
Coverage report
This PR does not seem to contain any modification to coverable code. |
The fixture serves its stylesheet from a deliberately slow view and records, from an animation frame registered ahead of the link, whether that sheet had applied. Firefox withholds animation frames while a stylesheet is pending, so a first frame that sees unstyled content means something forced layout and lifted rendering suppression early. The toolbar has to be collapsed for hideToolbar() to reach ensureHandleVisibility(), and localStorage is cleared around the test because djdt.show takes precedence over SHOW_COLLAPSED and persists across tests in the shared driver. Reverting the requestAnimationFrame wrapper fails the test 10 times out of 10, and restoring it passes 10 out of 10. The check is specific to Firefox: Chrome does not lift render blocking on a forced layout, so the unpatched code does not flash there.
3481200 to
6378f61
Compare
tim-schilling
left a comment
There was a problem hiding this comment.
Looks good. I'd like to give this one a little time to let someone else chime in though.
Description
ensureHandleVisibility()readshandle.offsetWidthduringinit(), directly after$$.show(handle)takes the handle fromdisplay:none. Layout is dirty at that point, so the read forces a synchronous document-wide style and layout flush after the page has already painted. On projects with a large stylesheet this is slow enough to be visible as a flash of unstyled content in Firefox.It only affects a collapsed toolbar, since
showToolbar()does not callensureHandleVisibility().Deferring the body to
requestAnimationFramemoves the read past the paint. Behaviour is otherwise unchanged, and the resize listener benefits too, as repeated events now coalesce to at most one reflow per frame.No test. The only observable change is timing, andNow has a selenium test for this.ensureHandleVisibility()isn't reachable without scaffolding the whole toolbar DOM. Happy to add one if you'd like.Checklist:
docs/changes.rst.AI/LLM Usage