| Summary: | [IntersectionObserver] Schedule intersection observation updates | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Ali Juma <ajuma> | ||||||||||
| Component: | Layout and Rendering | Assignee: | Ali Juma <ajuma> | ||||||||||
| Status: | RESOLVED FIXED | ||||||||||||
| Severity: | Normal | CC: | bfulgham, cdumez, commit-queue, dbates, esprehn+autocc, ews-watchlist, kangil.han, realdawei, rniwa, ryanhaddad, simon.fraser, tsavell, webkit-bug-importer, zalan | ||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||
| Version: | WebKit Local Build | ||||||||||||
| Hardware: | Unspecified | ||||||||||||
| OS: | Unspecified | ||||||||||||
| Bug Depends on: | |||||||||||||
| Bug Blocks: | 159475 | ||||||||||||
| Attachments: |
|
||||||||||||
|
Description
Ali Juma
2018-08-27 14:43:46 PDT
Created attachment 348204 [details]
Patch
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 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 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 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 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
Created attachment 348290 [details]
Patch for landing
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 on attachment 348290 [details] Patch for landing Clearing flags on attachment: 348290 Committed r235424: <https://trac.webkit.org/changeset/235424> All reviewed patches have been landed. Closing bug. 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 (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. Reopening Closing bug, flakey tests are being tracked in https://bugs.webkit.org/show_bug.cgi?id=189091 |