Implement ResizeObserver infra, support content box calculatons. #31108
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
gterzian
force-pushed
the
implement_resize_observer
branch
2 times, most recently
from
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)
|
|
|
gterzian
force-pushed
the
implement_resize_observer
branch
from
1805dd6
to
9bf1b6b
Compare
12 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)
|
|
|
|
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), |
|
|
gterzian
force-pushed
the
implement_resize_observer
branch
from
f9b3755
to
3546a09
Compare
|
🔨 Triggering try run (#9113882854) for Linux WPT |
|
Test results for linux-wpt-layout-2020 from try job (#9113882854): Flaky unexpected result (11)
Stable unexpected results that are known to be intermittent (12)
Stable unexpected results (3)
|
|
|
initial files stub methods and slots add internal slots to document and resize observer add internal slots to resize observation add resize observation constructor implement methods of resize observer implement resize observer entry methods implement resize observer size methods add note about leaking resize observers add stubs for algorithms, event-loop integration add doc for adding observer to a document call broadcast active resize obs in event-loop improve integration with event-loop and batched reflows add calculate depth for node remove rooting requirements on ResizeObservation event-loop: remove reflowed var, enter realm remove conditional on skip target logic implement is_active for resize observation add constructor mechanism for resize observer entry implement calculate box size fix docs fix calculate box size implement broadcast of active resize observations break potential borrow cycle on the refcell containing resize observers add and fix docs pass shallowest depth by mut ref, and check for skipped observations add delivery of resize loop error notification turn on wpt tests fix boolean logic in is_active fix observe fix last reported sizes stack remove prints fix tidy update test expectations fix active observation logic, break infinite loop fix skipped observations fix observe update observe.html test expectation update resize observer doc comment on document udpate docs in resize observer remove step numbers add note to leak no need to reflow if there are no previously reported sizes update wpt meta fix last reported sizes add pref, set to true in resources use dir file in meta to set pref to true update test expectations remove from mozilla interfaces update test expectation map dom observers to root in broadcast loop
gterzian
force-pushed
the
implement_resize_observer
branch
from
8bd51e5
to
7f94b6f
Compare
|
🔨 Triggering try run (#9197068442) for Linux WPT |
|
Test results for linux-wpt-layout-2020 from try job (#9197068442): Flaky unexpected result (16)
Stable unexpected results that are known to be intermittent (14)
Stable unexpected results (1)
|
|
|
jdm
reviewed
May 23, 2024
components/script/script_thread.rs
Outdated
| // TODO(#31006): Implement the resize observer steps. | ||
| // Run the resize observer steps. | ||
| let mut depth = Default::default(); | ||
| loop { |
There was a problem hiding this comment.
Nit: can we use a while instead if loop/if/break?
components/script/dom/document.rs
Outdated
| /// <https://drafts.csswg.org/resize-observer/#broadcast-active-resize-observations> | ||
| pub fn broadcast_active_resize_observations(&self) -> ResizeObservationDepth { | ||
| let mut shallowest = ResizeObservationDepth::max(); | ||
| // Breaking potential re-borrow cycle. |
There was a problem hiding this comment.
Could you elaborate on this? What's the risk here and how is it being addressed?
There was a problem hiding this comment.
When we call the observer callback, new observers can be registered, which would could a double-borrow(will add a more detailed comment).
| &[&*observer_size], | ||
| ); | ||
| entries.push(entry); | ||
| *observation.last_reported_sizes.borrow_mut() = vec![size_impl]; |
There was a problem hiding this comment.
Would it make sense to use clear() on the existing vector and avoid allocating a new one?
| *shallowest_target_depth = target_depth; | ||
| } | ||
| } | ||
| let _ = self |
There was a problem hiding this comment.
Why do we want to ignore the return value of this call?
There was a problem hiding this comment.
I've taken another look at all uses of Call__ in the codebase, and we ignore all return values.
| } | ||
|
|
||
| /// State machine equivalent of active and skipped observations. | ||
| #[derive(Default, JSTraceable, MallocSizeOf)] |
There was a problem hiding this comment.
If you add PartialEq, we can replace all of the matches! checks with ==/!=.
| /// <https://drafts.csswg.org/resize-observer/#dom-resizeobservation-observedbox> | ||
| observed_box: ResizeObserverBoxOptions, | ||
| /// <https://drafts.csswg.org/resize-observer/#dom-resizeobservation-lastreportedsizes> | ||
| last_reported_sizes: RefCell<Vec<ResizeObserverSizeImpl>>, |
There was a problem hiding this comment.
Do we actually need this RefCell if this struct is stored inside of a domrefcell already?
There was a problem hiding this comment.
I guess not, will look into.
| let box_size = calculate_box_size(target, &self.observed_box); | ||
| let width = box_size.width().to_f64_px(); | ||
| let height = box_size.height().to_f64_px(); | ||
| !(( |
There was a problem hiding this comment.
Could we write this with != instead?
components/script/script_thread.rs
Outdated
| let _realm = enter_realm(&*document); | ||
|
|
||
| let mut depth = Default::default(); | ||
| loop { |
| @@ -0,0 +1,4 @@ | |||
| [observe.html] | |||
| expected: ERROR | |||
There was a problem hiding this comment.
Yes, it is a missing API. See #31182
| @@ -0,0 +1,7 @@ | |||
| [notify.html] | |||
| expected: ERROR | |||
There was a problem hiding this comment.
It is the ResizeObserver loop completed with undelivered notifications. error reported by deliver_resize_loop_error_notification. It is in fact the test2 failure.
|
@jdm thank you for the review, I have addressed all points. @Loirooriol @emilio @mrobinson Thank you all for your earlier reviews and comments as well. I have reduced the scope of the current implementation to only cover calculations for "content box"(see https://github.com/servo/servo/pull/31108/files#diff-2a44f61a5e5b7cf6b7a47e74a04dbec244b81507f0517bdc92484ba58e6e90c6R268), and I would like to leave the other layout calcuations as follow-ups(device pixel, and border box, as well as apparently scrollbars). The failing tests are documented at #31182, which I think are rich material for follow-ups for either myself or the community. |
gterzian
changed the title
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.
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