RESOLVED FIXED Bug 219495
IntersectionObserver {root:document}: Getting a different rootBounds.top than both FF/Chrome for a scrolled window 
https://bugs.webkit.org/show_bug.cgi?id=219495
Summary IntersectionObserver {root:document}: Getting a different rootBounds.top than...
Jake Fried
Reported 2020-12-03 09:56:04 PST
Hi webkit folks! While working on a PR to update the w3c/IntersectionObserver polyfill with `root:document` support, I believe I may have found an issue with the WebKit implementation. A `root:document` IntersectionObserver seems to be firing entries with a `rootBounds` that is relative to the scrollingElement.scrollTop. Both Chrome and FF always use `top:0` in this case. According to the spec, I think the rootBounds should be equal to the document viewport (spec: https://www.w3.org/TR/intersection-observer/#intersectionobserver-root-intersection-rectangle). Example: ```html <!DOCTYPE html> <html> <head><title>IntersectionObserver RootBounds Test.</title></head> <body> <script> function init() { window.document.body.style = 'height: 2000px'; window.scrollTo(0, 110); const io = new window.IntersectionObserver((e) => { const p = document.createElement('p'); p.innerText = `rootBounds: ${JSON.stringify(e[0].rootBounds)}`; document.body.appendChild(p); }, {root: document}); io.observe(document.querySelector('body')); } window.addEventListener('DOMContentLoaded', init); </script> </body> </html> ```
Attachments
Patch (3.09 KB, patch)
2021-01-07 07:40 PST, cathiechen 		no flags
DetailsFormatted DiffDiff
Patch (3.22 KB, patch)
2021-01-08 08:50 PST, cathiechen 		no flags
DetailsFormatted DiffDiff
Patch (4.51 KB, patch)
2021-01-12 07:59 PST, cathiechen 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Jake Fried
Comment 1 2020-12-03 10:14:00 PST
Here is the example page running on jsbin: https://output.jsbin.com/vawazepuqe. To run: 1. Load the jsbin link 2. Scroll up to see the inserted <p> tag with rootBounds info
Jake Fried
Comment 2 2020-12-03 11:54:25 PST
(In reply to Jake Fried from comment #1) > Here is the example page running on jsbin: > https://output.jsbin.com/vawazepuqe. > > To run: > 1. Load the jsbin link > 2. Scroll up to see the inserted <p> tag with rootBounds info I've since learned that I needed to make an account for the jsbin link not to expire. Same instructions, but new link: https://jsbin.com/rugolaw/1
Smoley
Comment 3 2020-12-07 13:39:20 PST
Thanks for filing, I can reproduce this on Safari 14.0.2 and STP116.
Radar WebKit Bug Importer
Comment 4 2020-12-07 13:39:35 PST
<rdar://problem/72061982>
cathiechen
Comment 5 2021-01-07 07:40:18 PST
Created attachment 417177 [details] Patch
Darin Adler
Comment 6 2021-01-07 09:12:03 PST
Comment on attachment 417177 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=417177&action=review > Source/WebCore/dom/Document.cpp:7675 > + else if (observer.root() == &target.document()) > + localRootBounds = frameView.layoutViewportRect(); Is this exactly the right place for the check check? Should it be before the hasOverflowClip or before the isContainingBlockAncestorFor check? Is the single test case in WPT sufficient coverage?
Simon Fraser (smfr)
Comment 7 2021-01-07 10:46:21 PST
Comment on attachment 417177 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=417177&action=review >> Source/WebCore/dom/Document.cpp:7675 >> + localRootBounds = frameView.layoutViewportRect(); > > Is this exactly the right place for the check check? Should it be before the hasOverflowClip or before the isContainingBlockAncestorFor check? > > Is the single test case in WPT sufficient coverage? And is frameView.layoutViewportRect() correct? What's the right behavior for zoomed pages? Is this tested?
cathiechen
Comment 8 2021-01-08 08:39:06 PST
Comment on attachment 417177 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=417177&action=review Hi, Thanks for the review:) >>> Source/WebCore/dom/Document.cpp:7675 >>> + localRootBounds = frameView.layoutViewportRect(); >> >> Is this exactly the right place for the check check? Should it be before the hasOverflowClip or before the isContainingBlockAncestorFor check? >> >> Is the single test case in WPT sufficient coverage? > > And is frameView.layoutViewportRect() correct? What's the right behavior for zoomed pages? Is this tested? Hi Darin, Yeah, I think it makes sense to check document before hasOverflowClip. And yes, there's only one test for it. I landed the test early this week:) Hi Simon, It seems the rects are not right for zoomed pages. Sorry, I didn't test it before. We use layout rect here and in `Document::updateIntersectionObservations`, it uses `absoluteToClientRect`. They seem not match, for `absoluteToClientRect` move offset to `visibleContentRect` which aligns to `visualViewportRect`. I think maybe in `Document::updateIntersectionObservations` we should use `absoluteToLayoutViewportRect` instead. Do you think we can fix the zoom issue in a new bug, or do you prefer to fix it here?
cathiechen
Comment 9 2021-01-08 08:50:00 PST
Created attachment 417270 [details] Patch
Simon Fraser (smfr)
Comment 10 2021-01-08 09:39:07 PST
From what I recall, Intersection Observer v1. was specified to use the layout viewport for root intersections. I think the v2 spec added an option for visual viewport?
cathiechen
Comment 11 2021-01-08 09:57:15 PST
(In reply to Simon Fraser (smfr) from comment #10) > From what I recall, Intersection Observer v1. was specified to use the > layout viewport for root intersections. I think the v2 spec added an option > for visual viewport? Ah, I found this in github. https://github.com/w3c/IntersectionObserver/issues/95 Will take a close look tomorrow:)
cathiechen
Comment 12 2021-01-12 07:59:07 PST
Created attachment 417457 [details] Patch
cathiechen
Comment 13 2021-01-12 08:00:01 PST
(In reply to Simon Fraser (smfr) from comment #10) > From what I recall, Intersection Observer v1. was specified to use the > layout viewport for root intersections. I think the v2 spec added an option > for visual viewport? Hi Simon, It seems the issue hasn't reached to a conclusion.(https://github.com/w3c/IntersectionObserver/issues/95). The latest Editor's Draft has added a note about this. (https://w3c.github.io/IntersectionObserver/#intersectionobserver-root-intersection-rectangle). > Note: Root intersection rectangle is not affected by pinch zoom and will report the unadjusted viewport, consistent with the intent of pinch zooming (to act like a magnifying glass and NOT change layout.) I think it means the coordinator of rootBounds should be layoutViewportRect, for "it is not affected by pinch zoom". In the code, `computeIntersectionState()` returns absolute rootBounds rect, then in `Document::updateIntersectionObservations` it transform the rect by `absoluteToClientRect` which first transform the absolute rect to a document rect (unscales the zoom factor), then transform the document rect to a client rect which move the location to unscaled `visibleContentRect().location()`. The `visibleContentRect().location()` is the scroll position of ScrollView which will be effected by pinch zoom. For layoutViewportRect, I think it should align to m_layoutViewportOrigin. So I think we should use `absoluteToLayoutViewportRect` instead of `absoluteToClientRect`, to return a rect that is based on layoutViewportRect. WDYT? Are you agreed with this change?
Simon Fraser (smfr)
Comment 14 2021-01-12 09:44:28 PST
(In reply to cathiechen from comment #13) > So I think we should use `absoluteToLayoutViewportRect` instead of > `absoluteToClientRect`, to return a rect that is based on layoutViewportRect. > > WDYT? Are you agreed with this change? That sounds right.
cathiechen
Comment 15 2021-01-12 23:18:45 PST
(In reply to Simon Fraser (smfr) from comment #14) > (In reply to cathiechen from comment #13) > > So I think we should use `absoluteToLayoutViewportRect` instead of > > `absoluteToClientRect`, to return a rect that is based on layoutViewportRect. > > > > WDYT? Are you agreed with this change? > > That sounds right. Thanks:)
EWS
Comment 16 2021-01-12 23:24:12 PST
Committed r271433: <https://trac.webkit.org/changeset/271433> All reviewed patches have been landed. Closing bug and clearing flags on attachment 417457 [details].
Note You need to log in before you can comment on or make changes to this bug.