Implement ResizeObserver infra, support content box calculations. #31108
Conversation
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
5176c1b to
1f071d3
Compare
|
🔨 Triggering try run (#7655994547) with platforms=linux and layout=2020 |
|
Test results for linux-wpt-layout-2020 from try job (#7655994547): Flaky unexpected result (14)
Stable unexpected results that are known to be intermittent (13)
Stable unexpected results (3)
|
|
|
1805dd6 to
9bf1b6b
Compare
13 tasks
|
🔨 Triggering try run (#7664120775) with platforms=linux and layout=2020 |
|
Test results for linux-wpt-layout-2020 from try job (#7664120775): Flaky unexpected result (16)
Stable unexpected results that are known to be intermittent (14)
Stable unexpected results (1)
|
|
|
|
🔨 Triggering try run (#7664526221) with platforms=linux and layout=2020 |
|
Test results for linux-wpt-layout-2020 from try job (#7664526221): Flaky unexpected result (10)
Stable unexpected results that are known to be intermittent (11)
Stable unexpected results (2)
|
|
|
Member
Author
|
I have filed an issue for tests that are still failing: #31182 The base case appears to work, there are some failures which appears related to the delivery of rafs(used in the tests), |
|
|
Member
Author
|
@mrobinson I think the PR needs an approval from you as well. I have addressed your suggestions at the time, but I haven't added the additional layout calculations because I think it's a good follow-up(see comment above). |
Loirooriol
reviewed
Jun 10, 2024
components/script/dom/document.rs
Outdated
| let mut shallowest = ResizeObservationDepth::max(); | ||
| // Breaking potential re-borrow cycle on `resize_observers`: | ||
| // broadcasting resize observations calls into a JS callback, | ||
| // which can add new obeservers. |
| .pop() | ||
| .unwrap_or_else(|| Rect::zero()) | ||
| }, | ||
| // TODO#31182): add support for border box, and device pixel size, calculations. |
Member
Author
|
@mrobinson @Loirooriol Thanks for the additional comments.
I'm not sure if I have addressed this comment, but as per the latest, I am using the @Loirooriol can you please confirm? See the relevant part of the code: https://github.com/servo/servo/pull/31108/files#diff-2a44f61a5e5b7cf6b7a47e74a04dbec244b81507f0517bdc92484ba58e6e90c6R263 Also @mrobinson I think this needs your explicit approval for it to merge, since you have requested changes before. |
mrobinson
requested changes
Jun 13, 2024
Member
mrobinson
left a comment
There was a problem hiding this comment.
Just a couple nits, but I don't see any big issues with this change. Hopefully we can land it in the next couple days. Sorry for the big delays!
| } | ||
| let _ = self | ||
| .callback | ||
| .Call__(entries, self, ExceptionHandling::Report); |
Member
There was a problem hiding this comment.
Should this only happen when the size changes? I'm a bit unclear on the mechanism here because is_active() checks to see if the size has changed, but it seems this code both updates the size and then calls the resize observer callbacks. I don't see how the code avoids that if the size changes.
It also seems like is_active() is calculating the size but not caching it anywhere...meaning it updates here as well. It's not a cheap calculating right now, unfortunately, so that seems a little wasteful. Perhaps I misunderstand this code though...?
Member
Author
There was a problem hiding this comment.
Should this only happen when the size changes?
This only happens for active observations, so that means size has changed plus observation depth is right. The code here does not update the size.
It's not a cheap calculating right now, unfortunately, so that seems a little wasteful.
I have added a caching mechanism, see the last commit.
c59a74b to
68013c6
Compare
mrobinson
approved these changes
Jun 14, 2024
Member
|
@gterzian Looks good, though please squash your changes before landing or else the commit message will contain a lot of noise. Thanks you! |
68013c6 to
68e30d5
Compare
Member
Author
|
@Loirooriol @jdm @mrobinson thank you for your reviews. |
8b72b78 to
78f606a
Compare
78f606a to
8a04803
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 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.
Implement the basic infra for the ResizeObserver Web API, and support size calculations for content boxes.
./mach build -ddoes not report any errors./mach test-tidydoes not report any errors