1671396 - IntersectionObserverEntry.boundingClientRect may not match target.getBoundingClientRect() due to async scrolling

Status: RESOLVED INVALID

(Core :: Panning and Zooming, enhancement)

Description

[maiq.is.a.lie]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:81.0) Gecko/20100101 Firefox/81.0

Steps to reproduce:

Called entry.target.getBoundingClientRect() inside IntersectionObserverCallback and compared it with entry.boundingClientRect

Actual results:

entry.boundingClientRect and entry.target.getBoundingClientRect() results are not equal in the same event cycle. DOMRect acquired from the function call returns the correct values. I determined correctness based on calculations that are done with the Element.scrollTop value.

Expected results:

entry.boundingClientRect and entry.target.getBoundingClientRect() values should be the same.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Can you attach a test-case that reproduces the issue?

Comment 2

[maiq.is.a.lie]

Attachment: bug.html

Noticed something else while creating the test-case:
Values are correct when I drag the scroll slider with mouse, but inaccuracy happens when i use the mouse wheel to scroll.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

I believe that this is expected in presence of async scrolling. The entry's rect is a snapshot of when the intersection observer entry was queued: https://w3c.github.io/IntersectionObserver/#queue-intersection-observer-task

From the point it's queued to where it's run the client rect might be different because of async scrolling. I wonder why Chromium gets exactly the same value. I believe dragging the scrollbar works because that uses main-thread scrolling rather than async scrolling.

So it seems we're using regular event loop dispatch: https://searchfox.org/mozilla-central/rev/e0eb861a187f0bb6d994228f2e0e49b2c9ee455e/dom/base/Document.cpp#15262-15270

Which is what the spec requires: https://w3c.github.io/IntersectionObserver/#queue-intersection-observer-task

While Chromium is doing something similar, but somehow not hitting the same weirdness: https://source.chromium.org/chromium/chromium/src/+/master:third_party/blink/renderer/core/intersection_observer/intersection_observer.h;l=64-72;drc=500a75a127a7791feefbf523d6af75183f6ca60d

Botond, do you know what are the guarantees that we can have when updating main-thread scroll offsets from APZ? So far it seems like chrome could be doing something like only updating async scroll offsets to the main thread during refresh driver ticks, or something of that sort, which would achieve the right behavior there...

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Err, see above.

Comment 5

[Botond Ballo [:botond]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #3)

Botond, do you know what are the guarantees that we can have when updating main-thread scroll offsets from APZ? So far it seems like chrome could be doing something like only updating async scroll offsets to the main thread during refresh driver ticks, or something of that sort, which would achieve the right behavior there...

You're right that the current behaviour is that async scrolling updates a scroll frame's internal scroll offset as soon as the notification from APZ is processed (the notification arrives via PAPZ.RequestContentRepaint, and the scroll frame is updated here). The scroll frame's scroll offset is in turn exposed via APIs like getBoundingClientRect().

Could we try deferring the update of the scroll frame's scroll offset until some time in the next refresh driver tick (presumably, just before scroll events are fired)? Probably, yeah. It seems like a bit of a scary change because it would touch scroll offset sync codepaths that have historically been pretty regression-prone, but I don't think there's any fundamental reason we couldn't, and if there's sufficient webcompat motivation, it's something we can explore.

Comment 6

[Kartikaya Gupta (email:[e-mail])]

/cc Markus who might want to consider comment 5 as part of the scheduling work he's doing. I'm going to change this bug type to enhancement since it seems to be working as intended, but we could "enhance" it to be more in line with web author expectations.

Comment 7

[Markus Stange [:mstange]]

Attachment: testcase which shows different values in Chrome

I don't think this has to do with async scrolling - it just has to do with task ordering in the event loop. Let's call the place where entry.boundingClientRect is computed point A, and the place where the intersection observer callback function is called point B.
If the scroll position changes between point A and point B, then entry.boundingClientRect and target.getBoundingClientRect() will be different.

This testcase demonstrates that it is possible to change the scroll position between point A and point B in Chrome, too: We can run a task between point A and point B by calling postMessage from inside a requestAnimationFrame callback. This testcase does that and updates the scroll position by +2 or -2 in that task.

(requestAnimationFrame callbacks run in step 12, intersection observations are made in step 13: https://html.spec.whatwg.org/multipage/webappapis.html#event-loop-processing-model:run-the-animation-frame-callbacks )

So, the situation where entry.boundingClientRect and target.getBoundingClientRect() differ is possible by spec, and this testcase proves that it happen in Chrome.

Comment 8

[Markus Stange [:mstange]]

Resolving as INVALID. Intersection observations are asynchronous by design. The values are correct as of the time of observation, and not necessarily up to date by the time the callback is called. The values happen to match in Chrome because you're getting lucky, but it's not a property that is guaranteed by the spec or the implementation.

Also see the Introduction section of the spec, which says that IntersectionObserver is intended for use cases that "do not impose hard latency requirements; that is to say, the information can be delivered asynchronously (e.g. from another thread) without penalty" and where "the information is useful even when delivered at a slight delay".

Comment 9

[Markus Stange [:mstange]]

It is a bit strange for Firefox to be updating the scroll position right between the refresh tick and the first task that was dispatched from inside the refresh tick. I think this happens most frequently for APZ animations, such as the smooth scroll animation: In the compositor, on vsync, we sample the scroll position for the next frame and send it to the content process, at a time at which that content process is still busy with the refresh tick for that same vsync. So the scroll position for a refresh tick always comes in right after the previous refresh tick, and waits for almost a whole frame. This is probably worth addressing, but not as part of this bug.