dom: Implement minimal IntersectionObserver workflow by stevennovaryo · Pull Request #35551 · servo/servo

stevennovaryo · 2025-02-20T09:38:39Z

Implement minimal IntersectionObserver processing model which includes the processing model algorithm, omitting the implementation documented in the tracking issue. Most of the major implementations would depends on other API/module.

WPT Result

We are not experiencing timeouts in the many of IntersectionObserver WPT tests that happens because no intersection is reported. Although the intersection reported is not accurate because of the aforementioned reasons. More of "first rAF" started to pass too, presumably because in the initial observations, many of the intersections cases is still not stimulated yet.

Test /intersection-observer/nested-cross-origin-iframe.sub.html produces quite flaky result, either between TIMEOUT or FAIL. Due to scrollIntoView is yet to be implemented, test is supposed to fail. I guess it might be caused by the bad timing of event loop.
Update: this test seems to be quite stable after several additional commits. Still could create a different issue for this specific test.

cc: @xiaochengh

Try

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes are part of Implement IntersectionObserver #31021
There are tests for these changes
Make sure all todo is documented and summarized accordingly.

stevennovaryo · 2025-02-25T07:01:51Z

I guess the changes is fairly complete for reviews. I am still analyzing the test and rechecking the logic and components, but it will be great to get overall review and discussions to make sure I am on the right track.

gterzian

I haven't reviewed all the logic but to the question as to whether you are on the right track the answer is: yes.

gterzian · 2025-02-25T09:46:53Z

components/script/dom/intersectionobserver.rs

+            // > Let registration be the IntersectionObserverRegistration record in target’s internal
+            // > [[RegisteredIntersectionObservers]] slot whose observer property is equal to observer.
+            // We will clone now to prevent borrow error, and set the value in the end of the loop.
+            let registration = target


I'm not sure it makes sense to separate the info from the registration, because when implementing step 14 you will need the registration.

gterzian · 2025-02-25T09:56:30Z

components/script/dom/intersectionobserver.rs


+/// IntersectionObserverRegistration but the fields other than observer is
+/// separated to pass unrooted them with ease.
+///


I'm not sure this distinction makes sense, because in update_intersection_observations_steps you will need to eventually use both the registration and the info(when implementing what is now a todo).

To make this rootable, you need to use the example of #34194

Sure! I believe making it rootable is correct in this scope. I have tried to implement it here bf85252.

After rechecking it, I am actually not sure whether it should have used cloned version or borrow in this case. I believe since observation step should avoid making changes to DOM tree (e.g. reflow), it would be better to use reference instead. a0e8794

gterzian

Looks good to me overall.

Would be good to create a meta issue with a todo list reflecting all the todos in the code.

Perhaps someone more familiar with layout can also take a look? cc @mrobinson @mukilan

gterzian · 2025-02-28T04:51:37Z

components/script/dom/intersectionobserver.rs

+
+    /// Whether the observer is connected to the owner document.
+    /// An observer is connected if it is registered in intersection_observers list inside document.
+    is_connected: Cell<bool>,


This seems redundant because I think you can deduce that value from observation_targets.

gterzian · 2025-02-28T04:56:03Z

components/script/script_thread.rs

        // as well as those for which a rendering update would be unnecessary,
        // but this isn't happening here.

+        // TODO(#31021): the filtering of docs is extended to not exclude the ones that


I don't think this is the right issue; If any it should be #31242

gterzian · 2025-02-28T05:29:41Z

components/script/dom/intersectionobserver.rs

+    /// > The root intersection rectangle for an IntersectionObserver is
+    /// > the rectangle we’ll use to check against the targets.
+    ///
+    /// NOTE: Firefox seems to only account for viewport/boundingBox area without not covered by


It would be useful to point out why FF's behavior is relevant here. I assume because we do the same?

Currently, it is using the viewport that is quite different from the spec and other implementation. It is considering the scroll offset and not considering the size of the scrollbar. I really should add this into another todo also, individually on each impacted lines.

I have checked this further, it seems that FF behavior make more sense. And, the spec was quite incomplete in this root intersection rectangle section. I believe it should consider clipping and probably several more steps similar to https://w3c.github.io/IntersectionObserver/#compute-the-intersection. I will check chromium's implementation too, as FF is not quite updated regarding several new features.

I will leave viewport addressing identical to element's client-width for now.

Ok, thanks for all the research. For clarity, what I mean is that we should write the comment from the perspective of what Servo is doing, and then perhaps explain that this matches FF's behavior and that the spec has not settled down yet. SO basically the same content, but putting the focus on what we're trying to do.

Thanks for pointing out. Sorry for the misunderstanding the intention.

gterzian · 2025-02-28T05:33:02Z

components/script/dom/intersectionobserver.rs

+                debug!("descendant of containing block chain is not implemented");
+            }
+
+            // NOTE: Firefox skips these steps too, if target_rect or root_bounds is zero.


Following Gecko's behavior and skipping these steps if ... (writing from the point of what we're doing here, not what FF is doing).

My bad it is more like an todo rather than a note as these is not implemented. It should also mention that it was for null value instead of zero (rectangle?).

I added it in ecc6743, I guess it is better to add it here since it does not make sense to assume null return from getBoundingBox as a zero rectangle.

gterzian · 2025-02-28T05:36:02Z

components/script/dom/document.rs

    /// <https://w3c.github.io/IntersectionObserver/#document-intersectionobservertaskqueued>
    intersection_observer_task_queued: Cell<bool>,
+    /// Active intersection observers that should be processed by this document.
+    intersection_observers: DomRefCell<Vec<Dom<IntersectionObserver>>>,


I think it would be good to link to https://w3c.github.io/IntersectionObserver/#run-the-update-intersection-observations-steps

and add the prose from step 1,

and also a note that this is discussed at w3c/IntersectionObserver#525

gterzian · 2025-02-28T05:37:51Z

components/script/dom/document.rs

        true
    }
+
+    /// <https://w3c.github.io/IntersectionObserver/#dom-intersectionobserver-intersectionobserver>


Here I don't think the link is useful, because it does not link to anything describing "adding an intersection observer". I think it would be better to point out that this does not follow the spec as per w3c/IntersectionObserver#525

(same below)

xiaochengh · 2025-02-28T06:12:57Z

components/script/dom/document.rs

+        // > 2. Let notify list be a list of all IntersectionObservers whose root is in the DOM tree of document.
+        // > 3. For each IntersectionObserver object observer in notify list, run these steps:
+        // Iterating a copy of observer stored in the document to prevent borrow error.
+        for intersection_observer in self.intersection_observers.clone().borrow().iter() {


Can we avoid the cloning by making IntersectionObserver.callback an Rc<RefCell<>>, so that there's internal mutability?

Or at least we can move the cloning to inside invoke_callback so that we don't clone every observer.

I am not sure, because a callback can modify the self.intersection_observers completely, by using disconnect(), observe(), and unobserve(). I should probably wrote about that in the comment too. We could filter the observer that won't invoke any callback and copy it before any iteration I guess.

I see, good point! Thanks!

xiaochengh · 2025-02-28T06:41:34Z

components/script/dom/intersectionobserver.rs

+    }
+
+    /// Step 3.1-3.5 of <https://w3c.github.io/IntersectionObserver/#notify-intersection-observers-algo>
+    pub(crate) fn invoke_callback(&self) {


We should rename this method, because it doesn't always invoke the callback.

Maybe invoke_callback_if_necessary?

stevennovaryo · 2025-02-28T08:32:33Z

Looks good to me overall.

Would be good to create a meta issue with a todo list reflecting all the todos in the code.

Thanks for the review!

Sure, most of the todo should be summarized in the description. I would like to hear from the other, especially about the todos related to layout. If it seems reasonable we could create the meta issue based of that.

gterzian · 2025-03-03T10:09:49Z

Looks good to me overall.
Would be good to create a meta issue with a todo list reflecting all the todos in the code.

Thanks for the review!

Sure, most of the todo should be summarized in the description.

Here is an example for ResizeObserver #31182

stevennovaryo · 2025-03-04T04:42:18Z

Rebased to recent changes.

@Loirooriol @mrobinson If you are not busy, could you please give a look as well? Thank you. (sorry for the ping)

jschwe · 2025-03-17T10:21:28Z

@stevennovaryo Could you rebase this PR again?

Signed-off-by: stevennovaryo <[email protected]>

I suppose these changes is better separated to other PRs. Signed-off-by: stevennovaryo <[email protected]>

Signed-off-by: stevennovaryo <[email protected]>

stevennovaryo · 2025-03-17T17:35:09Z

@stevennovaryo Could you rebase this PR again?

Rebased, could we get this changes merged soon? Try

Signed-off-by: stevennovaryo <[email protected]>

Loirooriol · 2025-03-17T21:03:37Z

components/script/dom/intersectionobserverrootmargin.rs


 //! Copy of Stylo Gecko's [`style::values::specified::gecko::IntersectionObserverRootMargin`] implementation.
-//! TODO: expose the object to servo as well in Stylo
+//! TODO(stevennovaryo): when stylo's IntersectionObserverMargin is up, make a thin wrapper and remove copied codes


If you mean https://phabricator.services.mozilla.com/D237514, it's already available

Updated, Thanks!

Loirooriol · 2025-03-17T21:14:49Z

components/script/dom/window.rs

        )
    }

+    // query content box without considering any reflow


Nit:

Suggested change

// query content box without considering any reflow

// Query content box without considering any reflow

Signed-off-by: stevennovaryo <[email protected]>

stevennovaryo force-pushed the intersection-observer-observation-step branch from 6bcc915 to f63da18 Compare February 21, 2025 06:45

stevennovaryo changed the title ~~[WIP] dom: Implement minimal IntersectionObserver workflow~~ dom: Implement minimal IntersectionObserver workflow Feb 25, 2025

stevennovaryo marked this pull request as ready for review February 25, 2025 07:02

stevennovaryo requested a review from gterzian as a code owner February 25, 2025 07:02

gterzian reviewed Feb 25, 2025

View reviewed changes

stevennovaryo force-pushed the intersection-observer-observation-step branch from 8422a42 to f118ce8 Compare February 28, 2025 04:21

gterzian approved these changes Feb 28, 2025

View reviewed changes

xiaochengh reviewed Feb 28, 2025

View reviewed changes

stevennovaryo force-pushed the intersection-observer-observation-step branch from dc66d04 to 1793841 Compare February 28, 2025 22:49

stevennovaryo mentioned this pull request Mar 3, 2025

Tracking issue: IntersectionObserver #35767

Open

16 tasks

stevennovaryo force-pushed the intersection-observer-observation-step branch from ddf5905 to 268d92d Compare March 4, 2025 04:06

wusyong mentioned this pull request Mar 17, 2025

Upcoming Servo Triage and Embedding meeting March 17 2025 servo/project#133

Closed

stevennovaryo added 13 commits March 17, 2025 20:06

Add very rough implemnentation of observation steps

93757cd

Signed-off-by: stevennovaryo <[email protected]>

Fix entry reflection and propagate can_gc

53fb01d

Signed-off-by: stevennovaryo <[email protected]>

Fix BorrowError and add fragment find descendant

7f4ca6d

Signed-off-by: stevennovaryo <[email protected]>

Implement is descendant in containing block path correctly

249dd91

Signed-off-by: stevennovaryo <[email protected]>

Fix unrooted error and tidy issues

4f4c60b

Signed-off-by: stevennovaryo <[email protected]>

Fix comments

81f96db

Signed-off-by: stevennovaryo <[email protected]>

Remove is descendant of other node query

ed62d0e

I suppose these changes is better separated to other PRs. Signed-off-by: stevennovaryo <[email protected]>

Fix intersection and refactor registration

693fe2f

Signed-off-by: stevennovaryo <[email protected]>

Use AppUnit more and propagate GlobalScope better

506b8d2

Signed-off-by: stevennovaryo <[email protected]>

Update WPT expectations

03a21ca

Signed-off-by: stevennovaryo <[email protected]>

Revert delay changes

aeef065

Signed-off-by: stevennovaryo <[email protected]>

Align compute intersection algo to other browser actual behavior

7c916fb

Signed-off-by: stevennovaryo <[email protected]>

Align processing documents and note several issues

0c6e998

Signed-off-by: stevennovaryo <[email protected]>

stevennovaryo added 16 commits March 17, 2025 20:07

Update WPT

7f5cdf1

Signed-off-by: stevennovaryo <[email protected]>

Minor lint

50f7bd2

Signed-off-by: stevennovaryo <[email protected]>

Fix top level browsing context

90b7030

Signed-off-by: stevennovaryo <[email protected]>

Make Registration rootable

bd615a1

Signed-off-by: stevennovaryo <[email protected]>

Avoid reflow inside observation step algo

7160254

Signed-off-by: stevennovaryo <[email protected]>

Using borrow for iterating registration

f850b75

Signed-off-by: stevennovaryo <[email protected]>

Fix document disconnect

64f73f9

Signed-off-by: stevennovaryo <[email protected]>

Update WPT

1a8a16b

Signed-off-by: stevennovaryo <[email protected]>

Address comments and minor quality suggestions

d24a125

Signed-off-by: stevennovaryo <[email protected]>

Root the observer before nofifying any of it

1a1b870

Signed-off-by: stevennovaryo <[email protected]>

Tidy docs

53431fb

Signed-off-by: stevennovaryo <[email protected]>

Account not found element and refactor observation step

288812e

Signed-off-by: stevennovaryo <[email protected]>

Fix documentations

223e8bf

Signed-off-by: stevennovaryo <[email protected]>

Ignore position of document viewport

5fd4d39

Signed-off-by: stevennovaryo <[email protected]>

Refactor root intersection rectangle

90e51bf

Signed-off-by: stevennovaryo <[email protected]>

Add can GC note to the callback

8a7c049

Signed-off-by: stevennovaryo <[email protected]>

stevennovaryo force-pushed the intersection-observer-observation-step branch from 268d92d to f6c5d49 Compare March 17, 2025 17:21

Fix top-level browsing context term

8fa5559

Signed-off-by: stevennovaryo <[email protected]>

stevennovaryo force-pushed the intersection-observer-observation-step branch from f6c5d49 to 8fa5559 Compare March 17, 2025 18:08

Loirooriol approved these changes Mar 17, 2025

View reviewed changes

Fix minor comments

76fc0d7

Signed-off-by: stevennovaryo <[email protected]>

gterzian added this pull request to the merge queue Mar 18, 2025

Merged via the queue into servo:main with commit 67a5f28 Mar 18, 2025
21 checks passed

	// query content box without considering any reflow
	// Query content box without considering any reflow

Uh oh!

Conversation

stevennovaryo commented Feb 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

WPT Result

Uh oh!

stevennovaryo commented Feb 25, 2025

Uh oh!

gterzian left a comment

Choose a reason for hiding this comment

Uh oh!

gterzian Feb 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gterzian left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stevennovaryo Feb 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stevennovaryo Mar 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stevennovaryo Mar 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stevennovaryo commented Feb 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gterzian commented Mar 3, 2025

Uh oh!

stevennovaryo commented Mar 4, 2025

Uh oh!

jschwe commented Mar 17, 2025

Uh oh!

stevennovaryo commented Mar 17, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

stevennovaryo commented Feb 20, 2025 •

edited

Loading

gterzian Feb 25, 2025 •

edited

Loading

stevennovaryo Feb 28, 2025 •

edited

Loading

stevennovaryo Mar 1, 2025 •

edited

Loading

stevennovaryo Mar 1, 2025 •

edited

Loading

stevennovaryo commented Feb 28, 2025 •

edited

Loading