Bug 243573

Summary: IntersectionObserver intersectionRatio < 1 observed
Product: WebKit Reporter: Ahmad Saleem <ahmad.saleem792>
Component: Layout and RenderingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, bfulgham, karlcow, rniwa, simon.fraser, vitor.roriz, webkit-bug-importer, zalan
Priority: P2 Keywords: BrowserCompat, GoodFirstBug, InRadar
Version: Safari 15   
Hardware: Unspecified   
OS: Unspecified   

Description Ahmad Saleem 2022-08-04 18:08:20 PDT
Hi Team,

Thanks for working continuously to improve Webkit better, I came across a test case while observing Chrome bug related to Intersection Observer and noted that Safari 15.6 and Safari Technical Preview 150 show IntersectionRatio below 1.

Test Case - https://jsfiddle.net/b9we0kh3/show

Chrome Bug - https://bugs.chromium.org/p/chromium/issues/detail?id=1020466

Past Safari Bug similar to this - https://bugs.webkit.org/show_bug.cgi?id=200776

*** STEPS TO REPRODUCE ***

1) Open Test case and note Intersection Ratio

2) Resize and note that it does not show "1" and always stay below "1"

<< EXPECTED RESULT >>

Like other browsers irrespective of window size, "Intersection Ratio remains 1" and do fluctuate when scrolling but it remains at "1"

<< ACTUAL RESULT >>

Safari keep Intersection Ration below 1 while all other browsers keep it at "1"

______

In case, if Safari is right and following spec then please ignore and mark this as "RESOLVED INVALID" and if it is Web-Spec (WHATWG) issue, please raise accordingly.

Thanks
Comment 1 Radar WebKit Bug Importer 2022-08-11 18:09:16 PDT
<rdar://problem/98541023>
Comment 2 Simon Fraser (smfr) 2022-08-18 10:23:28 PDT
This seems to happen when the element being observed has a fractional width.
Comment 3 Simon Fraser (smfr) 2022-08-18 10:30:54 PDT
(lldb) fr va intersectionState
(WebCore::IntersectionObservationState) intersectionState = {
  absoluteTargetRect = { x = 222.59375, y = 0.0, width = 958.4375, height = 167.8125 }
  absoluteRootBounds = { x = 0.0, y = 0.0, width = 1389.0, height = 898.0 }
  absoluteIntersectionRect = { x = 223.0, y = 0.0, width = 958.0, height = 168.0 }
  isIntersecting = true
}

Note the x position of absoluteTargetRect and absoluteIntersectionRect are slightly different.

It's getting converted to an integral rect via:
    LayoutRect rectInFrameViewSpace(renderer->view().frameView().contentsToView(snappedIntRect(*rectInFrameAbsoluteSpace)));
Comment 4 Simon Fraser (smfr) 2022-08-18 10:46:04 PDT
See also https://github.com/w3c/IntersectionObserver/issues/477
Comment 5 Simon Fraser (smfr) 2022-08-18 10:48:14 PDT
This can be fixed by using the FloatRect version of `contentsToView()` but the Chromium bug suggests there are other issues too.
Comment 6 Simon Fraser (smfr) 2022-08-18 10:51:13 PDT
diff --git a/Source/WebCore/dom/Document.cpp b/Source/WebCore/dom/Document.cpp
index 5d3d20c820a0e9f03642a412ad9aff493bb25ff3..5cd70b074858012e752a5f35cafaa84b97e600d0 100644
--- a/Source/WebCore/dom/Document.cpp
+++ b/Source/WebCore/dom/Document.cpp
@@ -8043,7 +8043,7 @@ static std::optional<LayoutRect> computeClippedRectInRootContentsSpace(const Lay
     if (!intersects)
         return std::nullopt;
 
-    LayoutRect rectInFrameViewSpace(renderer->view().frameView().contentsToView(snappedIntRect(*rectInFrameAbsoluteSpace)));
+    LayoutRect rectInFrameViewSpace = LayoutRect { renderer->view().frameView().contentsToView(*rectInFrameAbsoluteSpace) };
     auto* ownerRenderer = renderer->frame().ownerRenderer();
     if (!ownerRenderer)
         return std::nullopt;
Comment 7 Ahmad Saleem 2022-08-21 08:15:37 PDT
(In reply to Simon Fraser (smfr) from comment #6)
> diff --git a/Source/WebCore/dom/Document.cpp
> b/Source/WebCore/dom/Document.cpp
> index
> 5d3d20c820a0e9f03642a412ad9aff493bb25ff3..
> 5cd70b074858012e752a5f35cafaa84b97e600d0 100644
> --- a/Source/WebCore/dom/Document.cpp
> +++ b/Source/WebCore/dom/Document.cpp
> @@ -8043,7 +8043,7 @@ static std::optional<LayoutRect>
> computeClippedRectInRootContentsSpace(const Lay
>      if (!intersects)
>          return std::nullopt;
>  
> -    LayoutRect
> rectInFrameViewSpace(renderer->view().frameView().
> contentsToView(snappedIntRect(*rectInFrameAbsoluteSpace)));
> +    LayoutRect rectInFrameViewSpace = LayoutRect {
> renderer->view().frameView().contentsToView(*rectInFrameAbsoluteSpace) };
>      auto* ownerRenderer = renderer->frame().ownerRenderer();
>      if (!ownerRenderer)
>          return std::nullopt;

To fix this - I need to make pull request of this change? I am not super skilled in C++ and still learning GitHub mojo in Webkit. I am happy take it offline on Webkit Slack for any guidance on how to fix this. Thanks!
Comment 8 Vitor Roriz 2022-08-22 15:17:36 PDT
Pull request: https://github.com/WebKit/WebKit/pull/3553
Comment 9 Vitor Roriz 2022-08-23 05:45:47 PDT
I've just noticed we have already imported tests for it and we are succeeding on both without a patch:

1. /LayoutTests/imported/w3c/web-platform-tests/intersection-observer/intersection-ratio-with-fractional-bounds.html
2. /LayoutTests/imported/w3c/web-platform-tests/intersection-observer/intersection-ratio-with-fractional-bounds-2.html


Reading the chromium thread https://bugs.chromium.org/p/chromium/issues/detail?id=1020466, for them this would happen when [a] running the test in a process-isolated cross-origin iframe, like when hosting it in fiddle, [b] regular case, out of iframe.

Their related fix is to address [b], which doesn't happen for us. While [a] happens for gecko, blink and webkit and it seems to be related to the propagation window resizing to the iframe process.
Comment 10 Simon Fraser (smfr) 2022-08-23 09:33:53 PDT
I reproduced this locally in a test case with no frames (the JS fiddle).
Comment 11 EWS 2022-09-07 12:34:59 PDT
Committed 254244@main (b5701774e49f): <https://commits.webkit.org/254244@main>

Reviewed commits have been landed. Closing PR #3553 and removing active labels.