Bug 219495

Summary:

IntersectionObserver {root:document}: Getting a different rootBounds.top than both FF/Chrome for a scrolled window

Product:

WebKit

Reporter:

Jake Fried <friedj>

Component:

Layout and Rendering

Assignee:

cathiechen <cathiechen>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

ajuma, bfulgham, cathiechen, cdumez, darin, esprehn+autocc, ews-watchlist, kangil.han, simon.fraser, smoley, webkit-bug-importer, zalan

Priority:

Keywords:

InRadar

Version:

Safari 14

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

220403

Bug Blocks:

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none

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	Details Formatted Diff Diff
Patch (3.22 KB, patch) 2021-01-08 08:50 PST, cathiechen	no flags	Details Formatted Diff Diff
Patch (4.51 KB, patch) 2021-01-12 07:59 PST, cathiechen	no flags	Details Formatted Diff Diff
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

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.