New issue
Advanced search Search tips

Issue 879798 link

Starred by 1 user

Issue metadata

Status: Started
Owner:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug

Blocking:
issue 827639
issue 881666



Sign in to add a comment

IntersectionObserver: tracking document should be associated with observation, not observer

Project Member Reported by szager@chromium.org, Sep 1

Issue description

The "tracking document" concept is used to ensure that the intersection
observer algorithm always runs at a time when rendering is up-to-date.
The tracking document provides the entry-point to running the
IntersectionObserver algorithm during UpdateAllLifecyclePhases.

Currently, if the observer has an explicit root element, then the
tracking document is the document containing the root; otherwise, the
tracking document is the execution context where the observer was
constructed. However, both of those are problematic:

- For the implicit root case, it's possible to construct an observer
in one window, and use it to observe a target in a different window
(via window.open()). When there are two top-level windows in a single
renderer process, their lifecycle updates run independently; so if
the algorithm runs during the tracking document's lifecycle update,
it's possible that the target element will not be rendering-clean.

- The explicit root case mostly works as intended, because the
algorithm only computes an intersection when root and target are in the
same document. However, there is a potential for subtle problems when a
target is moved to a different document. When that happens, the
observer may need to send a "not intersecting" notification; but
because the tracking document -- which determines when the algorithm
runs -- is associated with the root element's document, it's not
guaranteed that the algorithm will run (e.g., if the root's document
is suspended or detached from the frame tree; or if the root element is
also removed from its document).

These problems have been masked over until now because for a long time,
the lifecycle code would run the IntersectionObserver algorithm
indiscriminately on every frame. More recently,
SetNeedsIntersectionObserveration was added to LocalFrameView, to avoid
running the algorithm unnecessarily; however, that mechanism is currently
over-invalidating, so the algorithm still runs more than necessary.
Given that a single IntersectionObserver can have multiple targets in
multiple windows -- each of which runs its lifecycle updates
independently -- "tracking document" should be associated with the
IntersectionObservation, *not* the IntersectionObserver.
 
Status: Started (was: Untriaged)
Description: Show this description
Description: Show this description
Blocking: 881666
Project Member

Comment 5 by bugdroid1@chromium.org, Sep 7

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/488a36e8d850ba3f2d05165b594356327d6deb4e

commit 488a36e8d850ba3f2d05165b594356327d6deb4e
Author: Stefan Zager <szager@chromium.org>
Date: Fri Sep 07 21:52:42 2018

[IntersectionObserver] Fix the concept of "tracking document"

As explained in the bug, "tracking document" should be associated with
IntersectionObservation, not IntersectionObserver.

One side effect of this change is that for a given observer, each of its
observations runs its algorithm independently, so that notifications may
not be generated in the order in which the observations were created
(i.e., the order of calls to observer.observe()). To preserve the
idiomatic web platform behavior that notifications are delivered in
the order in which the targets were observed, this patch moves the
storage of pending notifications from IntersectionObserver to
IntersectionObservation.

BUG=879798
R=chrishtr@chromium.org

Change-Id: I9b6f6ad8a26387f0c072ccdff7f18cea9a88004c
Reviewed-on: https://chromium-review.googlesource.com/1200388
Reviewed-by: Chris Harrelson <chrishtr@chromium.org>
Commit-Queue: Stefan Zager <szager@chromium.org>
Cr-Commit-Position: refs/heads/master@{#589678}
[modify] https://crrev.com/488a36e8d850ba3f2d05165b594356327d6deb4e/third_party/WebKit/LayoutTests/TestExpectations
[add] https://crrev.com/488a36e8d850ba3f2d05165b594356327d6deb4e/third_party/WebKit/LayoutTests/external/wpt/intersection-observer/target-in-different-window.html
[modify] https://crrev.com/488a36e8d850ba3f2d05165b594356327d6deb4e/third_party/WebKit/LayoutTests/external/wpt/lint.whitelist
[modify] https://crrev.com/488a36e8d850ba3f2d05165b594356327d6deb4e/third_party/WebKit/LayoutTests/http/tests/intersection-observer/resources/v2-subframe.html
[modify] https://crrev.com/488a36e8d850ba3f2d05165b594356327d6deb4e/third_party/blink/renderer/core/dom/element.cc
[modify] https://crrev.com/488a36e8d850ba3f2d05165b594356327d6deb4e/third_party/blink/renderer/core/dom/element.h
[modify] https://crrev.com/488a36e8d850ba3f2d05165b594356327d6deb4e/third_party/blink/renderer/core/execution_context/execution_context.h
[modify] https://crrev.com/488a36e8d850ba3f2d05165b594356327d6deb4e/third_party/blink/renderer/core/intersection_observer/element_intersection_observer_data.cc
[modify] https://crrev.com/488a36e8d850ba3f2d05165b594356327d6deb4e/third_party/blink/renderer/core/intersection_observer/element_intersection_observer_data.h
[modify] https://crrev.com/488a36e8d850ba3f2d05165b594356327d6deb4e/third_party/blink/renderer/core/intersection_observer/intersection_observation.cc
[modify] https://crrev.com/488a36e8d850ba3f2d05165b594356327d6deb4e/third_party/blink/renderer/core/intersection_observer/intersection_observation.h
[modify] https://crrev.com/488a36e8d850ba3f2d05165b594356327d6deb4e/third_party/blink/renderer/core/intersection_observer/intersection_observer.cc
[modify] https://crrev.com/488a36e8d850ba3f2d05165b594356327d6deb4e/third_party/blink/renderer/core/intersection_observer/intersection_observer.h
[modify] https://crrev.com/488a36e8d850ba3f2d05165b594356327d6deb4e/third_party/blink/renderer/core/intersection_observer/intersection_observer_controller.cc
[modify] https://crrev.com/488a36e8d850ba3f2d05165b594356327d6deb4e/third_party/blink/renderer/core/intersection_observer/intersection_observer_controller.h
[modify] https://crrev.com/488a36e8d850ba3f2d05165b594356327d6deb4e/third_party/blink/renderer/core/intersection_observer/intersection_observer_test.cc

Project Member

Comment 6 by bugdroid1@chromium.org, Sep 10

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7b4c707fd3ce173675649149fb7db13d539797bf

commit 7b4c707fd3ce173675649149fb7db13d539797bf
Author: Yutaka Hirano <yhirano@chromium.org>
Date: Mon Sep 10 01:31:23 2018

Revert "[IntersectionObserver] Fix the concept of "tracking document""

This reverts commit 488a36e8d850ba3f2d05165b594356327d6deb4e.

Reason for revert: Speculative revert for 
 - http/tests/intersection-observer/v2/cross-origin-effects.html
 - http/tests/intersection-observer/v2/cross-origin-occlusion.html  

Original change's description:
> [IntersectionObserver] Fix the concept of "tracking document"
> 
> As explained in the bug, "tracking document" should be associated with
> IntersectionObservation, not IntersectionObserver.
> 
> One side effect of this change is that for a given observer, each of its
> observations runs its algorithm independently, so that notifications may
> not be generated in the order in which the observations were created
> (i.e., the order of calls to observer.observe()). To preserve the
> idiomatic web platform behavior that notifications are delivered in
> the order in which the targets were observed, this patch moves the
> storage of pending notifications from IntersectionObserver to
> IntersectionObservation.
> 
> BUG=879798
> R=​chrishtr@chromium.org
> 
> Change-Id: I9b6f6ad8a26387f0c072ccdff7f18cea9a88004c
> Reviewed-on: https://chromium-review.googlesource.com/1200388
> Reviewed-by: Chris Harrelson <chrishtr@chromium.org>
> Commit-Queue: Stefan Zager <szager@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#589678}

TBR=szager@chromium.org,chrishtr@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 879798,  882280 
Change-Id: I885276c561c8d6f21afdc059d080d58fac3b899e
Reviewed-on: https://chromium-review.googlesource.com/1215423
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Commit-Queue: Yutaka Hirano <yhirano@chromium.org>
Cr-Commit-Position: refs/heads/master@{#589817}
[modify] https://crrev.com/7b4c707fd3ce173675649149fb7db13d539797bf/third_party/WebKit/LayoutTests/TestExpectations
[delete] https://crrev.com/1357ec4126fbc69acfc54cb520bbe096c1de82b6/third_party/WebKit/LayoutTests/external/wpt/intersection-observer/target-in-different-window.html
[modify] https://crrev.com/7b4c707fd3ce173675649149fb7db13d539797bf/third_party/WebKit/LayoutTests/external/wpt/lint.whitelist
[modify] https://crrev.com/7b4c707fd3ce173675649149fb7db13d539797bf/third_party/WebKit/LayoutTests/http/tests/intersection-observer/resources/v2-subframe.html
[modify] https://crrev.com/7b4c707fd3ce173675649149fb7db13d539797bf/third_party/blink/renderer/core/dom/element.cc
[modify] https://crrev.com/7b4c707fd3ce173675649149fb7db13d539797bf/third_party/blink/renderer/core/dom/element.h
[modify] https://crrev.com/7b4c707fd3ce173675649149fb7db13d539797bf/third_party/blink/renderer/core/execution_context/execution_context.h
[modify] https://crrev.com/7b4c707fd3ce173675649149fb7db13d539797bf/third_party/blink/renderer/core/intersection_observer/element_intersection_observer_data.cc
[modify] https://crrev.com/7b4c707fd3ce173675649149fb7db13d539797bf/third_party/blink/renderer/core/intersection_observer/element_intersection_observer_data.h
[modify] https://crrev.com/7b4c707fd3ce173675649149fb7db13d539797bf/third_party/blink/renderer/core/intersection_observer/intersection_observation.cc
[modify] https://crrev.com/7b4c707fd3ce173675649149fb7db13d539797bf/third_party/blink/renderer/core/intersection_observer/intersection_observation.h
[modify] https://crrev.com/7b4c707fd3ce173675649149fb7db13d539797bf/third_party/blink/renderer/core/intersection_observer/intersection_observer.cc
[modify] https://crrev.com/7b4c707fd3ce173675649149fb7db13d539797bf/third_party/blink/renderer/core/intersection_observer/intersection_observer.h
[modify] https://crrev.com/7b4c707fd3ce173675649149fb7db13d539797bf/third_party/blink/renderer/core/intersection_observer/intersection_observer_controller.cc
[modify] https://crrev.com/7b4c707fd3ce173675649149fb7db13d539797bf/third_party/blink/renderer/core/intersection_observer/intersection_observer_controller.h
[modify] https://crrev.com/7b4c707fd3ce173675649149fb7db13d539797bf/third_party/blink/renderer/core/intersection_observer/intersection_observer_test.cc

Commenting here as well about an issue with the test, since I know that review mail tends to be ignored for merged CLs:
https://chromium-review.googlesource.com/c/chromium/src/+/1200388#message-c9c332a00a808db49bc2c4356bc634d980e13533
Project Member

Comment 8 by bugdroid1@chromium.org, Sep 11

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/61b3108056747ea20d02627ec65a02c550e42c59

commit 61b3108056747ea20d02627ec65a02c550e42c59
Author: Stefan Zager <szager@chromium.org>
Date: Tue Sep 11 01:07:28 2018

[IntersectionObserver] Reland: Fix the concept of "tracking document"

Re-landing after revert:

https://chromium-review.googlesource.com/c/chromium/src/+/1200388

... but leave the flaky tests disabled.

R=chrishtr@chromium.org
BUG=879798

Change-Id: Ia588f9d82665a533b4e1bd2f77809692ded0cb41
Reviewed-on: https://chromium-review.googlesource.com/1215370
Commit-Queue: Stefan Zager <szager@chromium.org>
Reviewed-by: Chris Harrelson <chrishtr@chromium.org>
Reviewed-by: Stefan Zager <szager@chromium.org>
Cr-Commit-Position: refs/heads/master@{#590155}
[add] https://crrev.com/61b3108056747ea20d02627ec65a02c550e42c59/third_party/WebKit/LayoutTests/intersection-observer/target-in-different-window.html
[modify] https://crrev.com/61b3108056747ea20d02627ec65a02c550e42c59/third_party/blink/renderer/core/dom/element.cc
[modify] https://crrev.com/61b3108056747ea20d02627ec65a02c550e42c59/third_party/blink/renderer/core/dom/element.h
[modify] https://crrev.com/61b3108056747ea20d02627ec65a02c550e42c59/third_party/blink/renderer/core/execution_context/execution_context.h
[modify] https://crrev.com/61b3108056747ea20d02627ec65a02c550e42c59/third_party/blink/renderer/core/intersection_observer/element_intersection_observer_data.cc
[modify] https://crrev.com/61b3108056747ea20d02627ec65a02c550e42c59/third_party/blink/renderer/core/intersection_observer/element_intersection_observer_data.h
[modify] https://crrev.com/61b3108056747ea20d02627ec65a02c550e42c59/third_party/blink/renderer/core/intersection_observer/intersection_observation.cc
[modify] https://crrev.com/61b3108056747ea20d02627ec65a02c550e42c59/third_party/blink/renderer/core/intersection_observer/intersection_observation.h
[modify] https://crrev.com/61b3108056747ea20d02627ec65a02c550e42c59/third_party/blink/renderer/core/intersection_observer/intersection_observer.cc
[modify] https://crrev.com/61b3108056747ea20d02627ec65a02c550e42c59/third_party/blink/renderer/core/intersection_observer/intersection_observer.h
[modify] https://crrev.com/61b3108056747ea20d02627ec65a02c550e42c59/third_party/blink/renderer/core/intersection_observer/intersection_observer_controller.cc
[modify] https://crrev.com/61b3108056747ea20d02627ec65a02c550e42c59/third_party/blink/renderer/core/intersection_observer/intersection_observer_controller.h
[modify] https://crrev.com/61b3108056747ea20d02627ec65a02c550e42c59/third_party/blink/renderer/core/intersection_observer/intersection_observer_test.cc

Project Member

Comment 9 by bugdroid1@chromium.org, Sep 12

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/abf553f1e4cad76eac9229352a6c20488c831a18

commit abf553f1e4cad76eac9229352a6c20488c831a18
Author: Stefan Zager <szager@chromium.org>
Date: Wed Sep 12 00:24:17 2018

[IntersectionObserver] Fix over-invalidations in LocalFrameView

Prior to this patch, any call to SetNeedsIntersectionObservation would
cause all IntersectionObservers in all documents in the frame tree to
be marked as needing an update, even when they were not actually dirty.
This patch avoids propagating the dirty bit all the way up the frame
tree, so that each IntersectionObservation runs only when necessary.

BUG=879798
R=chrishtr@chromium.org

Cq-Include-Trybots: luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: I6a5e7b3b966441aca9193a97026ca89c66e039b1
Reviewed-on: https://chromium-review.googlesource.com/1212449
Commit-Queue: Stefan Zager <szager@chromium.org>
Reviewed-by: Chris Harrelson <chrishtr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#590546}
[modify] https://crrev.com/abf553f1e4cad76eac9229352a6c20488c831a18/third_party/blink/renderer/core/dom/element.cc
[modify] https://crrev.com/abf553f1e4cad76eac9229352a6c20488c831a18/third_party/blink/renderer/core/dom/element.h
[modify] https://crrev.com/abf553f1e4cad76eac9229352a6c20488c831a18/third_party/blink/renderer/core/frame/local_frame.cc
[modify] https://crrev.com/abf553f1e4cad76eac9229352a6c20488c831a18/third_party/blink/renderer/core/frame/local_frame_view.cc
[modify] https://crrev.com/abf553f1e4cad76eac9229352a6c20488c831a18/third_party/blink/renderer/core/frame/local_frame_view.h
[modify] https://crrev.com/abf553f1e4cad76eac9229352a6c20488c831a18/third_party/blink/renderer/core/intersection_observer/element_intersection_observer_data.cc
[modify] https://crrev.com/abf553f1e4cad76eac9229352a6c20488c831a18/third_party/blink/renderer/core/intersection_observer/element_intersection_observer_data.h
[modify] https://crrev.com/abf553f1e4cad76eac9229352a6c20488c831a18/third_party/blink/renderer/core/intersection_observer/intersection_observation.cc
[modify] https://crrev.com/abf553f1e4cad76eac9229352a6c20488c831a18/third_party/blink/renderer/core/intersection_observer/intersection_observation.h
[modify] https://crrev.com/abf553f1e4cad76eac9229352a6c20488c831a18/third_party/blink/renderer/core/intersection_observer/intersection_observer.cc
[modify] https://crrev.com/abf553f1e4cad76eac9229352a6c20488c831a18/third_party/blink/renderer/core/intersection_observer/intersection_observer.h
[modify] https://crrev.com/abf553f1e4cad76eac9229352a6c20488c831a18/third_party/blink/renderer/core/intersection_observer/intersection_observer_controller.cc
[modify] https://crrev.com/abf553f1e4cad76eac9229352a6c20488c831a18/third_party/blink/renderer/core/layout/layout_object.cc
[modify] https://crrev.com/abf553f1e4cad76eac9229352a6c20488c831a18/third_party/blink/renderer/core/paint/paint_property_tree_builder.cc
[modify] https://crrev.com/abf553f1e4cad76eac9229352a6c20488c831a18/third_party/blink/renderer/core/scheduler/frame_throttling_test.cc

Project Member

Comment 10 by bugdroid1@chromium.org, Sep 27

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/16f78349052a90ebe681aff9c5c4a48b7e199e7c

commit 16f78349052a90ebe681aff9c5c4a48b7e199e7c
Author: Philip Jägenstedt <foolip@chromium.org>
Date: Thu Sep 27 12:55:25 2018

Move target-in-different-window.html into wpt/

With tweaks from this comment:
https://github.com/web-platform-tests/wpt/pull/12858#issuecomment-419775438

Bug: 879798
Change-Id: I3806dffd01dcc7a1f7dd21a7fe68ce3c099df181
Reviewed-on: https://chromium-review.googlesource.com/1224010
Reviewed-by: Stefan Zager <szager@chromium.org>
Commit-Queue: Philip Jägenstedt <foolip@chromium.org>
Cr-Commit-Position: refs/heads/master@{#594690}
[rename] https://crrev.com/16f78349052a90ebe681aff9c5c4a48b7e199e7c/third_party/WebKit/LayoutTests/external/wpt/intersection-observer/target-in-different-window.html
[modify] https://crrev.com/16f78349052a90ebe681aff9c5c4a48b7e199e7c/third_party/WebKit/LayoutTests/external/wpt/lint.whitelist

Sign in to add a comment