script: Start rework to better match the specification HTML event loop #31505
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
wusyong
reviewed
Mar 5, 2024
mrobinson
reviewed
Mar 5, 2024
gterzian
force-pushed
the
fix_html_event_loop
branch
6 times, most recently
from
5b9dc96
to
b066ac6
Compare
|
So, "fixing" the html loop will require some heavy refactoring, and sometimes additional implementation, for each step that is done as part of the new "update the rendering" task. Currently those steps are run in an ad hoc way by handling messages from the constellation/compositor/layout immediately when they are received. In each case, we have to review what the steps are doing exactly, and perform a kind of batching when handling incoming messages, and only running the steps for all batched items inside the single "update the rendering" task. I have done this for:
I think this shows the overall structure, and I have added TODOs for the other items. Doing, and reviewing, this in a single PR is not doable. Please let me know what you think. @mrobinson @jdm @wusyong (and anyone else interested). I'd say we can see how this affects the test suite, and try to merge it as is, with follow-ups for each TODO (See https://github.com/servo/servo/pull/31505/files#diff-2c5948584719622eef023c56cdf90e8278971f2f2eda26e7fd0485a24c32be3cR1675) I'll also file more details issues for those TODOs(first one is at #31665) |
gterzian
force-pushed
the
fix_html_event_loop
branch
from
aa273aa
to
3e2a784
Compare
|
🔨 Triggering try run (#8431586341) for Linux WPT |
|
Test results for linux-wpt-layout-2020 from try job (#8431586341): Flaky unexpected result (19)
Stable unexpected results that are known to be intermittent (34)
Stable unexpected results (10)
|
|
|
|
@jdm @mrobinson + anyone else interested: I am going to debug the I think this is one open issue, for which I also opened #31871. Perhaps one way to address this at first is to do one reflow at the start of the "update the rendering task", and one at the end. Then later we can refine this and remove reflows, while maintaining test outcomes. Ideally we should have a single reflow at the end. But I think some of the steps fire events but expect the UI to already have been updated. Example is the resize steps, which reads: "If (...) had its width or height changed since the last time these steps were run, fire an event (...)" emphasis is mine, and I think this means we would need to reflow after having updated sizes, but before firing those events as part of the "update the rendering task". In other words, in general we would need to:
But 3 should happen only inside the "update the rendering task", so that it is properly ordered with respect to other stuff, like animation frame callbacks or resize observer callbacks. So if we reflow only at the end of the task, it means that those resize events should only fire at the next rendering opportunity(or we should reflow before firing the events, but then we have more than one reflow, if you see what I mean). |
|
🔨 Triggering try run (#8447959621) for Linux WPT |
|
Test results for linux-wpt-layout-2020 from try job (#8447959621): Flaky unexpected result (11)
Stable unexpected results that are known to be intermittent (21)
Stable unexpected results (5)
|
|
|
|
Looks like there are a lot of timeouts in MediaQuery tests and also another test using |
Looking at one specific test, This is a new problem, because we are consolidating all logic into a task, whereas previously the logic would run when the compositor event was received(so as part of the script-thread message handling loop, but not part of the task queue which represents HTML event-loop tasks). I guess I can try to run "update the rendering" tasks even when the pipeline is considered not fully active, at least as a start. |
gterzian
force-pushed
the
fix_html_event_loop
branch
from
a138a90
to
fee7bca
Compare
|
@mrobinson Ok thank you, let's schedule a call via zulip. I'll squash it all into one, because there was a lot of back and forth and some earlier commits do not reflect what happened eventually... |
gterzian
force-pushed
the
fix_html_event_loop
branch
from
e008939
to
eea3684
Compare
mrobinson
requested changes
May 1, 2024
components/script/dom/window.rs
Outdated
| @@ -222,7 +222,7 @@ pub struct Window { | |||
|
|
|||
| /// Pending resize event, if any. | |||
| #[no_trace] | |||
| resize_event: Cell<Option<(WindowSizeData, WindowSizeType)>>, | |||
| resize_event: DomRefCell<VecDeque<(WindowSizeData, WindowSizeType)>>, | |||
There was a problem hiding this comment.
I guess this should be called resize_events now and the rustdoc comment should be updated above. What's the reason that multiple resize events are stored now? Does it make sense to coalesce them?
There was a problem hiding this comment.
What's the reason that multiple resize events are stored now? Does it make sense to coalesce them?
To keep the same test pass rates, all of these events need to be handled one at the time.
mrobinson
reviewed
May 1, 2024
gterzian
force-pushed
the
fix_html_event_loop
branch
from
e516894
to
cc68cd7
Compare
|
@mrobinson Thanks for the review, I've addressed all comments(or commented). |
mrobinson
reviewed
May 13, 2024
add rendering task source sketch structure to update the rendering resize steps composition events fix warnings in rendering task source refactor handling of composition events: put window and doc for pipeline on top set script as user interacting in update the rendering task fmt add todos for other steps, put all compositor handling logic in one place update the rendering: evaluate media queries and report changes update the rendering: update animations and send events update the rendering: run animation frames update the rendering: order docs put rendering related info on documents map tidy update the rendering: add issue numbers to todos update the rendering: reflow as last step update the rendering: add todo for top layer removals note rendering opportunity when ticking animations for testing fix double borrow crash in css/basic-transition fix faster reversing of transitions test undo ordering of docs bypass not fully-active pipeline task throttling for rendering tasks ensure tasks are dequed from task queue prioritize update the rendering task remove servo media stuff from set activity tidy debug update the rendering: perform microtask checkpoint after task tidy-up only run evaluate media queries if resized re-add evaluation of media queries for each rendering task, re-prioritize rendering tasks, re-add microtask checkpoint for all sequential messages re-structure resize steps, and their interaction with evaluating media queries and reacting to environment changes update the rendering: remove reflow call at the end update webmessaging expectations update to FAIL /html/browsers/browsing-the-web/navigating-across-documents/initial-empty-document/load-pageshow-events-iframe-contentWindow.html update to FAIL load-pageshow-events-window-open.html add issue number for ordering of docs nits move batching of mouse move event to document info nits add doc for mouse move event index reset mouse move event index when taking pending compositor events fix replacing mouse move event nits
mrobinson
force-pushed
the
fix_html_event_loop
branch
from
92d90e5
to
688f0ac
Compare
mrobinson
changed the title
mrobinson
approved these changes
May 13, 2024
|
Apologies for the big delay here. I was on PTO. I will send this to the MQ now. |
jschwe
added a commit
to jschwe/servo
that referenced
this pull request
May 13, 2024
…vent loop (servo#31505)" This reverts commit 1d66ea2.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Bring our HTML event-loop implementation in line with the spec: add an "update the rendering" task.
./mach build -ddoes not report any errors./mach test-tidydoes not report any errors