Bug 189007 - [IntersectionObserver] Schedule intersection observation updates
Summary: [IntersectionObserver] Schedule intersection observation updates
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ali Juma
URL:
Keywords: InRadar
Depends on:
Blocks: 159475
  Show dependency treegraph
 
Reported: 2018-08-27 14:43 PDT by Ali Juma
Modified: 2018-08-29 12:45 PDT (History)
14 users (show)

See Also:


Attachments
Patch (12.54 KB, patch)
2018-08-27 14:53 PDT, Ali Juma
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews112 for mac-sierra (3.16 MB, application/zip)
2018-08-27 16:43 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews103 for mac-sierra (2.35 MB, application/zip)
2018-08-27 16:45 PDT, Build Bot
no flags Details
Patch for landing (13.12 KB, patch)
2018-08-28 07:13 PDT, Ali Juma
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ali Juma 2018-08-27 14:43:46 PDT
Intersection observations need to be updated whenever layout is updated or scroll positions change.
Comment 1 Ali Juma 2018-08-27 14:53:33 PDT
Created attachment 348204 [details]
Patch
Comment 2 Simon Fraser (smfr) 2018-08-27 14:58:23 PDT
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?
Comment 3 Build Bot 2018-08-27 16:43:02 PDT
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
Comment 4 Build Bot 2018-08-27 16:43:04 PDT
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
Comment 5 Build Bot 2018-08-27 16:45:10 PDT
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
Comment 6 Build Bot 2018-08-27 16:45:12 PDT
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
Comment 7 Ali Juma 2018-08-28 07:13:32 PDT
Created attachment 348290 [details]
Patch for landing
Comment 8 Ali Juma 2018-08-28 07:14:33 PDT
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.
Comment 9 WebKit Commit Bot 2018-08-28 08:45:39 PDT
Comment on attachment 348290 [details]
Patch for landing

Clearing flags on attachment: 348290

Committed r235424: <https://trac.webkit.org/changeset/235424>
Comment 10 WebKit Commit Bot 2018-08-28 08:45:41 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Radar WebKit Bug Importer 2018-08-28 08:48:25 PDT
<rdar://problem/43799359>
Comment 12 Truitt Savell 2018-08-29 09:05:28 PDT
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
Comment 13 Ali Juma 2018-08-29 11:48:35 PDT
(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.
Comment 14 Truitt Savell 2018-08-29 12:05:34 PDT
Reopening
Comment 15 Truitt Savell 2018-08-29 12:45:04 PDT
Closing bug, flakey tests are being tracked in https://bugs.webkit.org/show_bug.cgi?id=189091