Bug 189007 – [IntersectionObserver] Schedule intersection observation updates

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Ali Juma]

Intersection observations need to be updated whenever layout is updated or scroll positions change.

[Ali Juma]

Created attachment 348204 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 348204 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=348204&action=review

> Source/WebCore/ChangeLog:11
> +        2) FrameView::viewportContentsChanged -- this covers changes to layout and
> +           to scroll positions.

What about scrolls in ancestor frames of the frames containing observers, or observers in the root document on elements nested in subframes, and scrolls of intermediate frames?

> Source/WebCore/dom/Document.cpp:7575
> +    m_intersectionObservationUpdateTimer.startOneShot(0_s);

Maybe add a comment that this timer should really be something that fits into HTMLEventLoop (once we have that).

> Source/WebCore/dom/Document.h:1782
> +    bool m_needsIntersectionObservationUpdate { false };

Can you pack that bool with other bools please?

[EWS Watchlist]

Comment on attachment 348204 [details]
Patch

Attachment 348204 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/9001575

New failing tests:
svg/in-html/by-reference.html
fast/events/beforeload-input-time-crash.html

[EWS Watchlist]

Created attachment 348236 [details]
Archive of layout-test-results from ews112 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 348204 [details]
Patch

Attachment 348204 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/9002304

New failing tests:
fast/events/beforeload-input-time-crash.html

[EWS Watchlist]

Created attachment 348237 [details]
Archive of layout-test-results from ews103 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Ali Juma]

Created attachment 348290 [details]
Patch for landing

[Ali Juma]

Comment on attachment 348204 [details]
Patch

Thanks for the review!

View in context: https://bugs.webkit.org/attachment.cgi?id=348204&action=review

>> Source/WebCore/ChangeLog:11
>> +           to scroll positions.
> 
> What about scrolls in ancestor frames of the frames containing observers, or observers in the root document on elements nested in subframes, and scrolls of intermediate frames?

Updated this to clarify that it covers same-document observation only (scrolls in other frames don't affect the intersection of a same-document root and target). I'll handle these other cases as part of implementing cross-document observation.

>> Source/WebCore/dom/Document.cpp:7575
>> +    m_intersectionObservationUpdateTimer.startOneShot(0_s);
> 
> Maybe add a comment that this timer should really be something that fits into HTMLEventLoop (once we have that).

Added.

>> Source/WebCore/dom/Document.h:1782
>> +    bool m_needsIntersectionObservationUpdate { false };
> 
> Can you pack that bool with other bools please?

Done.

Also, null-checked the call to frame().document() in FrameView::viewportContentsChanged to fix layout test crashes.

[WebKit Commit Bot]

Comment on attachment 348290 [details]
Patch for landing

Clearing flags on attachment: 348290

Committed r235424: <https://trac.webkit.org/changeset/235424>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/43799359>

[Truitt Savell]

After the rebaseline in https://trac.webkit.org/changeset/235424/webkit

These two tests are flakey failures:
imported/w3c/web-platform-tests/intersection-observer/bounding-box.html imported/w3c/web-platform-tests/intersection-observer/containing-block.html

Test History:
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=imported%2Fw3c%2Fweb-platform-tests%2Fintersection-observer%2Fbounding-box.html%20imported%2Fw3c%2Fweb-platform-tests%2Fintersection-observer%2Fcontaining-block.html

Diff:
--- /Volumes/Data/slave/sierra-debug-tests-wk1/build/layout-test-results/imported/w3c/web-platform-tests/intersection-observer/bounding-box-expected.txt
+++ /Volumes/Data/slave/sierra-debug-tests-wk1/build/layout-test-results/imported/w3c/web-platform-tests/intersection-observer/bounding-box-actual.txt
@@ -1,5 +1,5 @@
 
 PASS Test that the target's border bounding box is used to calculate intersection. 
 PASS First rAF. 
-PASS target.style.transform = 'translateY(195px)' 
+FAIL target.style.transform = 'translateY(195px)' assert_equals: entries.length expected 2 but got 1


--- /Volumes/Data/slave/sierra-debug-tests-wk1/build/layout-test-results/imported/w3c/web-platform-tests/intersection-observer/containing-block-expected.txt
+++ /Volumes/Data/slave/sierra-debug-tests-wk1/build/layout-test-results/imported/w3c/web-platform-tests/intersection-observer/containing-block-actual.txt
@@ -1,7 +1,7 @@
 
 PASS IntersectionObserver should only report intersections if root is a containing block ancestor of target. 
-PASS In containing block and intersecting. 
-PASS In containing block and not intersecting. 
-PASS Not in containing block and intersecting. 
-PASS Not in containing block and not intersecting. 
+FAIL In containing block and intersecting. assert_equals: entries.length expected 1 but got 0
+FAIL In containing block and not intersecting. assert_equals: entries.length expected 2 but got 1
+FAIL Not in containing block and intersecting. assert_equals: entries.length expected 2 but got 1
+FAIL Not in containing block and not intersecting. assert_equals: entries.length expected 2 but got 1

[Ali Juma]

(In reply to Truitt Savell from comment #12)
> After the rebaseline in https://trac.webkit.org/changeset/235424/webkit
> 
> These two tests are flakey failures:
> imported/w3c/web-platform-tests/intersection-observer/bounding-box.html
> imported/w3c/web-platform-tests/intersection-observer/containing-block.html
> 
> Test History:
> https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.
> html#showAllRuns=true&tests=imported%2Fw3c%2Fweb-platform-
> tests%2Fintersection-observer%2Fbounding-box.html%20imported%2Fw3c%2Fweb-
> platform-tests%2Fintersection-observer%2Fcontaining-block.html
> 
> Diff:
> ---
> /Volumes/Data/slave/sierra-debug-tests-wk1/build/layout-test-results/
> imported/w3c/web-platform-tests/intersection-observer/bounding-box-expected.
> txt
> +++
> /Volumes/Data/slave/sierra-debug-tests-wk1/build/layout-test-results/
> imported/w3c/web-platform-tests/intersection-observer/bounding-box-actual.txt
> @@ -1,5 +1,5 @@
>  
>  PASS Test that the target's border bounding box is used to calculate
> intersection. 
>  PASS First rAF. 
> -PASS target.style.transform = 'translateY(195px)' 
> +FAIL target.style.transform = 'translateY(195px)' assert_equals:
> entries.length expected 2 but got 1
> 
> 
> ---
> /Volumes/Data/slave/sierra-debug-tests-wk1/build/layout-test-results/
> imported/w3c/web-platform-tests/intersection-observer/containing-block-
> expected.txt
> +++
> /Volumes/Data/slave/sierra-debug-tests-wk1/build/layout-test-results/
> imported/w3c/web-platform-tests/intersection-observer/containing-block-
> actual.txt
> @@ -1,7 +1,7 @@
>  
>  PASS IntersectionObserver should only report intersections if root is a
> containing block ancestor of target. 
> -PASS In containing block and intersecting. 
> -PASS In containing block and not intersecting. 
> -PASS Not in containing block and intersecting. 
> -PASS Not in containing block and not intersecting. 
> +FAIL In containing block and intersecting. assert_equals: entries.length
> expected 1 but got 0
> +FAIL In containing block and not intersecting. assert_equals:
> entries.length expected 2 but got 1
> +FAIL Not in containing block and intersecting. assert_equals:
> entries.length expected 2 but got 1
> +FAIL Not in containing block and not intersecting. assert_equals:
> entries.length expected 2 but got 1

Suppressed these flakes in https://trac.webkit.org/changeset/235471 for now. My suspicion is this has to do with WPTs expecting updates to be scheduled as part of the HTMLEventLoop, but we're using Timers instead.

[Truitt Savell]

Reopening

[Truitt Savell]

Closing bug, flakey tests are being tracked in https://bugs.webkit.org/show_bug.cgi?id=189091