Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cannot track intersection with an iframe's viewport #372

Closed
szager-chromium opened this issue Jul 2, 2019 · 16 comments
Closed

Cannot track intersection with an iframe's viewport #372

szager-chromium opened this issue Jul 2, 2019 · 16 comments
Labels

Comments

@szager-chromium
Copy link
Collaborator

The implicit root refers to the top window's scrolling viewport, but there is currently no way to track the intersection of an element inside an iframe with the iframe window's scrolling viewport. We might try something like this (running in the iframe's context):

new IntersectionObserver(callback, {root: document.scrollingElement})

... but that doesn't work, because document.scrollingElement.getBoundingClientRect gives you the full size of the document's content, not the window's scrolling viewport.

I think document.scrollingElement ought to be treated specially when it's used as the root of an IntersectionObserver: it should be treated as measuring intersection with the document window's scrolling viewport.

@chrishtr
Copy link

AMP documents would benefit from this. In many cases, content within AMP is rendered within an iframe. The lack of support for this use-case means AMP can't use IntersectionObserver for such situations.

@chrishtr
Copy link

The proposal as stated would be a change of semantics without any IDL change, meaning it's technically not backwards-compatible. However, @szager-chromium and I agree that there seems to be no good use-case for supplying an explicit root that is a scrolling element without such a clip, so we expect usage of the API in this mode to be almost non-existent.

@chrishtr
Copy link

Also, there is an easy workaround if the other behavior is desired: make sure the element is the scrolling element, and observe relative to .

@emilio
Copy link
Collaborator

emilio commented Aug 21, 2019

Copying from an email thread:

Off-hand the idea seems fine, I agree this is a really unfortunate limitation. It seems unfortunate to change the semantics of the root option though... Maybe we should extend to allow root take a window or a document instead, and use that to represent the viewport root?

That'd make {root: window} or {root: document} on a top level document behave as no-root, and on an iframe behave as you want with respect to the viewport.

I haven't thought it through all that much though, but given the scrollingElement can change over time as a result of style changes, at least in quirks mode documents, if you define it as you propose you need to decide whether the "is scrolling element" check happens at the moment you create the observer (which would mean that creating an observer would need to update layout, which is bad), or at the moment the notifications are sent, or at some other point (when?).

It seems it'd cause confusion / subtle bugs either way though.

@emilio
Copy link
Collaborator

emilio commented Aug 21, 2019

cc @mstange / @dholbert in case they have other opinions.

@szager-chromium
Copy link
Collaborator Author

FYI, we landed this change behind a flag in chromium:

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

In this implementation, the (root == document.scrollingElement) check happens every time we run the intersection algorithm.

We can still change this if we decide that { root: document } or { root: window } makes more sense. My objection to that proposal is that it's simplifying if root can be an Element rather than a Node, but I don't feel especially strongly about it. I would be surprised if using scrollingElement broke any sites, but I suppose we can run a experiment to measure it.

@emilio
Copy link
Collaborator

emilio commented Oct 18, 2019

My objection to that proposal is that it's simplifying if root can be an Element rather than a Node, but I don't feel especially strongly about it.

Can you elaborate? If you mean on the IDL level, you can use "unions" on IDL (so Element or Document or such, I think, which limit all the stuff you can pass.

I still think scrollingelement is not great due to the quirks mode stuff, and I think it's a bug that you call scrollingElement() directly from blink in quirks mode (will file), but I guess the intersection of pages using intersection observer and pages using quirks mode is close to zero.

cc @bzbarsky in case he has opinions.

@bzbarsky
Copy link

I think relying on scrollingElement, given that it's not time-invariant, is just not a great idea. Furthermore, the whole "have APIs behave weirdly on this one element" thing we have in CSSOM for the scrollingElement is not really how we'd design an API if we were designing it today; it all just exists for backwards compat. If we want to observe something about the entire document, it's probably better to just accept a Document as an argument to indicate that rather than adding more scrollingElement bits.

@szager-chromium
Copy link
Collaborator Author

What about documentElement?

The IDL does get kinda messy if we have to use a union type.

@bzbarsky
Copy link

bzbarsky commented Oct 22, 2019

documentElement has the same "we're going to make this one element work weirdly unlike everything else and represent the document" issue. We can do it; lots of other CSSOM-view APIs do that sort of thing for historical reasons. But it seems like it would be good to start moving away from that in new APIs.

Prettiness of IDL is dead last in the priority of constituencies. I'd be a lot more interested in what developers think about the options than in how pretty the IDL is..

@szager-chromium
Copy link
Collaborator Author

OK, I will put together a patch based on accepting a Document as root and see how gnarly it gets.

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jan 16, 2020
This implements a behavioral change as described in:

w3c/IntersectionObserver#372

If the 'root' argument to the IntersectionObserver constructor is a
Document, then the observer will clip to the Document's root
scroller. For the top Document, this is the same behavior as the
implicit root. For an iframe Document, this is new behavior which is
unobtainable by other means.

BUG=1015183

Change-Id: I234bfce28ab954bb15b8d157bc22ee2d2469d5e4
@szager-chromium
Copy link
Collaborator Author

Off-hand the idea seems fine, I agree this is a really unfortunate limitation. It seems unfortunate to change the semantics of the root option though... Maybe we should extend to allow root take a window or a document instead, and use that to represent the viewport root?

That'd make {root: window} or {root: document} on a top level document behave as no-root, and on an iframe behave as you want with respect to the viewport.

This issue has been getting a good deal of developer attention, so I'd like to move towards finalizing a spec change. It sounds like the behavior of accepting {root: document} is the most popular and palatable option.

Any objections or other comments?

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jan 18, 2020
This implements a behavioral change as described in:

w3c/IntersectionObserver#372

If the 'root' argument to the IntersectionObserver constructor is a
Document, then the observer will clip to the Document's root
scroller. For the top Document, this is the same behavior as the
implicit root. For an iframe Document, this is new behavior which is
unobtainable by other means.

BUG=1015183

Change-Id: I234bfce28ab954bb15b8d157bc22ee2d2469d5e4
szager-chromium added a commit to szager-chromium/IntersectionObserver that referenced this issue Jan 21, 2020
@szager-chromium
Copy link
Collaborator Author

@emilio @bzbarsky -- please take a look at the pull request and let me know if it looks good to you.

@dreamofabear
Copy link

Any thoughts how to feature detect this new functionality? E.g. by UA?

With document, I suppose callers could try/catch the constructor TypeError:

TypeError: Failed to construct 'IntersectionObserver': member root is not of type Element.

This error appears to fire consistently across browsers but not sure if it's technically spec behavior.

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jan 22, 2020
This implements a behavioral change as described in:

w3c/IntersectionObserver#372

If the 'root' argument to the IntersectionObserver constructor is a
Document, then the observer will clip to the Document's root
scroller. For the top Document, this is the same behavior as the
implicit root. For an iframe Document, this is new behavior which is
unobtainable by other means.

BUG=1015183

Change-Id: I234bfce28ab954bb15b8d157bc22ee2d2469d5e4
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jan 22, 2020
This implements a behavioral change as described in:

w3c/IntersectionObserver#372

If the 'root' argument to the IntersectionObserver constructor is a
Document, then the observer will clip to the Document's root
scroller. For the top Document, this is the same behavior as the
implicit root. For an iframe Document, this is new behavior which is
unobtainable by other means.

BUG=1015183

Change-Id: I234bfce28ab954bb15b8d157bc22ee2d2469d5e4
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jan 23, 2020
This implements a behavioral change as described in:

w3c/IntersectionObserver#372

If the 'root' argument to the IntersectionObserver constructor is a
Document, then the observer will clip to the Document's root
scroller. For the top Document, this is the same behavior as the
implicit root. For an iframe Document, this is new behavior which is
unobtainable by other means.

BUG=1015183

Change-Id: I234bfce28ab954bb15b8d157bc22ee2d2469d5e4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2003750
Reviewed-by: Chris Harrelson <chrishtr@chromium.org>
Reviewed-by: vmpstr <vmpstr@chromium.org>
Commit-Queue: Stefan Zager <szager@chromium.org>
Cr-Commit-Position: refs/heads/master@{#734268}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Jan 23, 2020
This implements a behavioral change as described in:

w3c/IntersectionObserver#372

If the 'root' argument to the IntersectionObserver constructor is a
Document, then the observer will clip to the Document's root
scroller. For the top Document, this is the same behavior as the
implicit root. For an iframe Document, this is new behavior which is
unobtainable by other means.

BUG=1015183

Change-Id: I234bfce28ab954bb15b8d157bc22ee2d2469d5e4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2003750
Reviewed-by: Chris Harrelson <chrishtr@chromium.org>
Reviewed-by: vmpstr <vmpstr@chromium.org>
Commit-Queue: Stefan Zager <szager@chromium.org>
Cr-Commit-Position: refs/heads/master@{#734268}
pull bot pushed a commit to FreddyZeng/chromium that referenced this issue Jan 23, 2020
This implements a behavioral change as described in:

w3c/IntersectionObserver#372

If the 'root' argument to the IntersectionObserver constructor is a
Document, then the observer will clip to the Document's root
scroller. For the top Document, this is the same behavior as the
implicit root. For an iframe Document, this is new behavior which is
unobtainable by other means.

BUG=1015183

Change-Id: I234bfce28ab954bb15b8d157bc22ee2d2469d5e4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2003750
Reviewed-by: Chris Harrelson <chrishtr@chromium.org>
Reviewed-by: vmpstr <vmpstr@chromium.org>
Commit-Queue: Stefan Zager <szager@chromium.org>
Cr-Commit-Position: refs/heads/master@{#734268}
@szager-chromium
Copy link
Collaborator Author

@choumx I think it's pretty safe to rely on TypeError, especially if you use a completely simple and inert IntersectionObserver construction:

var document_root_supported = true;
try {
new IntersectionObserver(() => {}, {root: document});
catch (e if e instanceof TypeError) {
document_root_supported = false;
}

Because the IDL for the 'root' argument changes from "Element?" to "(Element or Document)?", every browser should catch it in their bindings layer.

xeonchen pushed a commit to xeonchen/gecko that referenced this issue Jan 27, 2020
…gument to constructor, a=testonly

Automatic update from web-platform-tests
[IntersectionObserver] Allow Document argument to constructor

This implements a behavioral change as described in:

w3c/IntersectionObserver#372

If the 'root' argument to the IntersectionObserver constructor is a
Document, then the observer will clip to the Document's root
scroller. For the top Document, this is the same behavior as the
implicit root. For an iframe Document, this is new behavior which is
unobtainable by other means.

BUG=1015183

Change-Id: I234bfce28ab954bb15b8d157bc22ee2d2469d5e4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2003750
Reviewed-by: Chris Harrelson <chrishtr@chromium.org>
Reviewed-by: vmpstr <vmpstr@chromium.org>
Commit-Queue: Stefan Zager <szager@chromium.org>
Cr-Commit-Position: refs/heads/master@{#734268}

--

wpt-commits: aebf5e72a429c448637c388512bcadd4d4e3fa65
wpt-pr: 21222
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Jan 28, 2020
…gument to constructor, a=testonly

Automatic update from web-platform-tests
[IntersectionObserver] Allow Document argument to constructor

This implements a behavioral change as described in:

w3c/IntersectionObserver#372

If the 'root' argument to the IntersectionObserver constructor is a
Document, then the observer will clip to the Document's root
scroller. For the top Document, this is the same behavior as the
implicit root. For an iframe Document, this is new behavior which is
unobtainable by other means.

BUG=1015183

Change-Id: I234bfce28ab954bb15b8d157bc22ee2d2469d5e4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2003750
Reviewed-by: Chris Harrelson <chrishtr@chromium.org>
Reviewed-by: vmpstr <vmpstr@chromium.org>
Commit-Queue: Stefan Zager <szager@chromium.org>
Cr-Commit-Position: refs/heads/master@{#734268}

--

wpt-commits: aebf5e72a429c448637c388512bcadd4d4e3fa65
wpt-pr: 21222
gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this issue Jan 28, 2020
…gument to constructor, a=testonly

Automatic update from web-platform-tests
[IntersectionObserver] Allow Document argument to constructor

This implements a behavioral change as described in:

w3c/IntersectionObserver#372

If the 'root' argument to the IntersectionObserver constructor is a
Document, then the observer will clip to the Document's root
scroller. For the top Document, this is the same behavior as the
implicit root. For an iframe Document, this is new behavior which is
unobtainable by other means.

BUG=1015183

Change-Id: I234bfce28ab954bb15b8d157bc22ee2d2469d5e4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2003750
Reviewed-by: Chris Harrelson <chrishtrchromium.org>
Reviewed-by: vmpstr <vmpstrchromium.org>
Commit-Queue: Stefan Zager <szagerchromium.org>
Cr-Commit-Position: refs/heads/master{#734268}

--

wpt-commits: aebf5e72a429c448637c388512bcadd4d4e3fa65
wpt-pr: 21222

UltraBlame original commit: ec3f916509c9a7ac7e2d170db1a1d1e6ac027d55
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Jan 28, 2020
…gument to constructor, a=testonly

Automatic update from web-platform-tests
[IntersectionObserver] Allow Document argument to constructor

This implements a behavioral change as described in:

w3c/IntersectionObserver#372

If the 'root' argument to the IntersectionObserver constructor is a
Document, then the observer will clip to the Document's root
scroller. For the top Document, this is the same behavior as the
implicit root. For an iframe Document, this is new behavior which is
unobtainable by other means.

BUG=1015183

Change-Id: I234bfce28ab954bb15b8d157bc22ee2d2469d5e4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2003750
Reviewed-by: Chris Harrelson <chrishtrchromium.org>
Reviewed-by: vmpstr <vmpstrchromium.org>
Commit-Queue: Stefan Zager <szagerchromium.org>
Cr-Commit-Position: refs/heads/master{#734268}

--

wpt-commits: aebf5e72a429c448637c388512bcadd4d4e3fa65
wpt-pr: 21222

UltraBlame original commit: ec3f916509c9a7ac7e2d170db1a1d1e6ac027d55
ryanhaddad pushed a commit to WebKit/WebKit that referenced this issue Dec 22, 2020
https://bugs.webkit.org/show_bug.cgi?id=208047

Patch by Frederic Wang <fwang@igalia.com> on 2020-03-06
Reviewed by Simon Fraser.

LayoutTests/imported/w3c:

* web-platform-tests/intersection-observer/document-scrolling-element-root-expected.txt:
Update expectation now that the test passes.

Source/WebCore:

This patch introduces a recent enhancement to the Intersection Observer specification: the
root initialization parameter can be explicitly be set to a Document. The typical use case
is when document is an iframe. See w3c/IntersectionObserver#372

This patch also updates the way Element's intersection observer data is handled so that it is
more consistent with the explicit Document root case introduced here.

Test: imported/w3c/web-platform-tests/intersection-observer/document-scrolling-element-root.html

* dom/Document.cpp:
(WebCore::Document::~Document): Notify observers about our desctruction.
(WebCore::Document::updateIntersectionObservations): Use new method name. This does not
require null-check because ensureIntersectionObserverData() has been called in
IntersectionObserver::observe().
(WebCore::Document::ensureIntersectionObserverData): Return reference to intersection
observer data for this document, creating one if it does not exist.
* dom/Document.h: Add new intersection observer data, used for documents that are explicit
intersection observer roots.
(WebCore::Document::intersectionObserverDataIfExists): Return pointer to intersection
observer data or null if it does not exist.
* dom/Element.cpp:
(WebCore::Element::didMoveToNewDocument): Use new method name.
(WebCore::Element::disconnectFromIntersectionObservers): Ditto and null-check weak refs.
(WebCore::Element::intersectionObserverDataIfExists): Rename method to match Document's one
and be more explicit that it will be null if it does not exist.
(WebCore::Element::intersectionObserverData): Renamed.
* dom/Element.h: Renamed.
* html/LazyLoadImageObserver.cpp:
(WebCore::LazyLoadImageObserver::intersectionObserver): Initialize with a WTF::Optional
after API change.
* page/IntersectionObserver.cpp:
(WebCore::IntersectionObserver::create): Pass a Node* root, which can be null (implicit
root), Document* or Element* (explicit roots). This is determined from init.root.
(WebCore::IntersectionObserver::IntersectionObserver): Handle the case of explicit Document
root.
(WebCore::IntersectionObserver::~IntersectionObserver): Ditto and update method name for
the explicit Element case. Note that in both explicit root cases the corresponding
ensureIntersectionObserverData() method had been called in the constructor so they can
be safely deferenced.
(WebCore::IntersectionObserver::removeTargetRegistration): Use new method name.
* page/IntersectionObserver.h: Update comment and code now that explicit root is a Node* and
IntersectionObserver::Init::root is either an Element or a Document or null.
(WebCore::IntersectionObserver::root const): Ditto.
(): Deleted.
* page/IntersectionObserver.idl: Update IDL to match the spec IntersectionObserver::root
is a nullable Node and IntersectionObserverInit::root a nullable Element or Document.

Canonical link: https://commits.webkit.org/221607@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@257976 268f45cc-cd09-0410-ab3c-d52691b4dbfc
@johannesodland
Copy link

@choumx I think it's pretty safe to rely on TypeError, especially if you use a completely simple and inert IntersectionObserver construction:

var document_root_supported = true;
try {
new IntersectionObserver(() => {}, {root: document});
catch (e if e instanceof TypeError) {
document_root_supported = false;
}

Because the IDL for the 'root' argument changes from "Element?" to "(Element or Document)?", every browser should catch it in their bindings layer.

Some Safari versions now pass this feature-detection but have a bug in the implementation. Callbacks are not invoked when the target intersects with the viewport: https://bugs.webkit.org/show_bug.cgi?id=224324

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants