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 RenderingAssignee: 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: P2 Keywords: InRadar
Version: Safari 14   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 220403    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Jake Fried 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>
```
Comment 1 Jake Fried 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
Comment 2 Jake Fried 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
Comment 3 Smoley 2020-12-07 13:39:20 PST 
Thanks for filing, I can reproduce this on Safari 14.0.2 and STP116.
Comment 4 Radar WebKit Bug Importer 2020-12-07 13:39:35 PST 
<rdar://problem/72061982>
Comment 5 cathiechen 2021-01-07 07:40:18 PST 
Created attachment 417177 [details]
Patch
Comment 6 Darin Adler 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?
Comment 7 Simon Fraser (smfr) 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?
Comment 8 cathiechen 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?
Comment 9 cathiechen 2021-01-08 08:50:00 PST 
Created attachment 417270 [details]
Patch
Comment 10 Simon Fraser (smfr) 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?
Comment 11 cathiechen 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:)
Comment 12 cathiechen 2021-01-12 07:59:07 PST 
Created attachment 417457 [details]
Patch
Comment 13 cathiechen 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?
Comment 14 Simon Fraser (smfr) 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.
Comment 15 cathiechen 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:)
Comment 16 EWS 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].