layout: Allow same ScriptThread <iframe>s to be resized synchronously#34656
Merged
mrobinson merged 1 commit intoservo:mainfrom Dec 17, 2024
Merged
layout: Allow same ScriptThread <iframe>s to be resized synchronously#34656mrobinson merged 1 commit intoservo:mainfrom
ScriptThread <iframe>s to be resized synchronously#34656mrobinson merged 1 commit intoservo:mainfrom
Conversation
|
🔨 Triggering try run (#12362435585) for Linux WPT |
|
Test results for linux-wpt-layout-2020 from try job (#12362435585): Flaky unexpected result (23)
Stable unexpected results that are known to be intermittent (13)
|
|
✨ Try run (#12362435585) succeeded. |
gterzian
approved these changes
Dec 17, 2024
components/script/dom/window.rs
Outdated
| return; | ||
| } | ||
|
|
||
| // Send resize message to any local `Pipeline`s synchronously, so that the value |
Member
There was a problem hiding this comment.
"synchronously" makes it sound like you are blocking on the sending of messages, so I think it's more precise to write that you are batching those messages locally for them to be handled at the next update the rendering(which is better than writing "layout", because the concept is more clearly defined across layout and script boundaries thanks to the html spec).
Member
Author
There was a problem hiding this comment.
Okay. I've reworded this a bit to be clearer.
| ); | ||
| } | ||
| }); | ||
| // Send asynchronous updates to `Constellation.` |
Member
There was a problem hiding this comment.
I think all of these will now result in receiving resize messages back from the constellation, in which case we need to write down somewhere what the proposed re-design is, which probably should include making a distinction between resizes that have already been batched locally at the script-thread(for same-origin iframes), and those that need to be propagated to other script-threads via the constellation. Describing this in an issue for a follow-up is a good idea I think.
Member
There was a problem hiding this comment.
Also one last note on the topic of "According to the
specification these frames should all be synchrnously laid out", I think the way to do that in Servo would be to still first batch the resizes locally, but somehow only process them as part of an rendering update when the cross-origin resizes have been taken into account as well. So in other words, instead of batching them for the next udpate, they'd have to be batched for "an update in the future" conditioned on somehow haven taken into account the resizing of the cross-origin iframes, and this "transaction" across script-thread would have be to brokered by the constellation. I think it's also a good idea to start discussing in more details in an issue.
Member
Author
There was a problem hiding this comment.
It's a great idea to document the new design. This change fixes the issue in the majority of cases, but there are still some tricky cases where many resize events flooding the Constellation could lead to flashing layout. I hope to address those in a followup change, but the solution will affect the final design. Maybe I can have a section of the book ready with the design once I've worked through all of the issues.
Regarding whether or not we need synchronous layout of the entire frame tree (I'm not convinced we don't need it yet), I agree it's good to discuss more in #34655.
Member
Author
|
Thanks for the review! |
70ab7e7 to
0b269bd
Compare
…usly Post layout, when a `Window` has all of the new `<iframe>` sizes, size any `Window`s for `Pipeline`s in the same `ScriptThread` synchronously. This ensures that when laying out from the outermost frame to the innermost frames, the frames sizes are set properly. There is still an issue where a non-same-`ScriptThread` `<iframe>` sits in between two `<iframe>`s of the same origin. According to the specification these frames should all be synchrnously laid out -- something quite difficult in Servo. This is issue servo#34655. This is the first change in a series of changes to improve the consistency of `<iframe>` loading and sizing. Fixes servo#14719. Fixes servo#24569. Fixes servo#24571. Fixes servo#25269. Fixes servo#25275. Fixes servo#25285. Fixes servo#30571. Signed-off-by: Martin Robinson <mrobinson@igalia.com>
0b269bd to
6b67a37
Compare
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Post layout, when a
Windowhas all of the new<iframe>sizes, sizeany
Windows forPipelines in the sameScriptThreadsynchronously.This ensures that when laying out from the outermost frame to the
innermost frames, the frames sizes are set properly.
There is still an issue where a non-same-
ScriptThread<iframe>sitsin between two
<iframe>s of the same origin. According to thespecification these frames should all be synchrnously laid out --
something quite difficult in Servo. This is issue #34655.
This is the first change in a series of changes to improve the
consistency of
<iframe>loading and sizing.Fixes #14719.
Fixes #24569.
Fixes #24571.
Fixes #25269.
Fixes #25275.
Fixes #25285.
Fixes #30571.
Signed-off-by: Martin Robinson mrobinson@igalia.com
./mach build -ddoes not report any errors./mach test-tidydoes not report any errors