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

Define the implicit root better in presence of adoption to a different browsing-context tree. #456

Open
emilio opened this issue Oct 11, 2020 · 3 comments

Comments

@emilio
Copy link
Collaborator

emilio commented Oct 11, 2020

https://w3c.github.io/IntersectionObserver/#intersectionobserver-intersection-root says that the implicit root is the document node of the "top-level browsing context".

That makes sense, but if you're observing a node and adopt it into a new window, there are two potential candidates:

  • The browsing context in which the intersection observer was created.
  • The browsing context in which the target lives.

Both of those can have different viewports, as https://bugzilla.mozilla.org/attachment.cgi?id=9180905 shows. Which one should be chosen? If we choose the first, the target will presumably never intersect. If we choose the second, then we report intersections against the newly-opened window.

I think both of them would be reasonable. Gecko does something non-sensical in this case. WebKit seems to do the first, and Blink seems to do the second:

We should decide what is the right thing to do here and clarify the spec.

cc @szager-chromium

@emilio
Copy link
Collaborator Author

emilio commented Oct 11, 2020

cc @smfr

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Oct 12, 2020
Two spec issues here.

w3c/IntersectionObserver#457: I think this is
just a spec bug and I've made us match other browsers, but since the
tests don't match the spec for now I've added them as .tentative.html

w3c/IntersectionObserver#456: I've aligned
with WebKit here. There was a (disabled) test for this which tests
chrome behavior and which after this patch shouldn't be flaky. This is
what was causing the assertion.

Differential Revision: https://phabricator.services.mozilla.com/D93166
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Oct 12, 2020
Two spec issues here.

w3c/IntersectionObserver#457: I think this is
just a spec bug and I've made us match other browsers, but since the
tests don't match the spec for now I've added them as .tentative.html

w3c/IntersectionObserver#456: I've aligned
with WebKit here. There was a (disabled) test for this which tests
chrome behavior and which after this patch shouldn't be flaky. This is
what was causing the assertion.

Differential Revision: https://phabricator.services.mozilla.com/D93166
sidvishnoi pushed a commit to sidvishnoi/gecko-webmonetization that referenced this issue Oct 13, 2020
Two spec issues here.

w3c/IntersectionObserver#457: I think this is
just a spec bug and I've made us match other browsers, but since the
tests don't match the spec for now I've added them as .tentative.html

w3c/IntersectionObserver#456: I've aligned
with WebKit here. There was a (disabled) test for this which tests
chrome behavior and which after this patch shouldn't be flaky. This is
what was causing the assertion.

Differential Revision: https://phabricator.services.mozilla.com/D93166
sidvishnoi pushed a commit to sidvishnoi/gecko-webmonetization that referenced this issue Oct 13, 2020
Two spec issues here.

w3c/IntersectionObserver#457: I think this is
just a spec bug and I've made us match other browsers, but since the
tests don't match the spec for now I've added them as .tentative.html

w3c/IntersectionObserver#456: I've aligned
with WebKit here. There was a (disabled) test for this which tests
chrome behavior and which after this patch shouldn't be flaky. This is
what was causing the assertion.

Differential Revision: https://phabricator.services.mozilla.com/D93166
@szager-chromium
Copy link
Collaborator

The comment in the WebKit code suggests that they intend to match chromium's behavior, which would be fine with me.

I think a case could be made for either behavior; but if I put myself in the shoes of the developer, I could imagine being surprised at not receiving an "intersecting" notification when I insert a target element into another window.

@emilio
Copy link
Collaborator Author

emilio commented Oct 15, 2020

I think a case could be made for either behavior; but if I put myself in the shoes of the developer, I could imagine being surprised at not receiving an "intersecting" notification when I insert a target element into another window.

Yeah, on the other hand it seems weird/sketchy that only for the implicit case one IntersectionObserver can refer to multiple roots, depending on the observation target...

For now I aligned with Gecko with WebKit in https://bugzilla.mozilla.org/show_bug.cgi?id=1670327, because it was more on the lines of what the Gecko code was trying to do before that patch.

moz-wptsync-bot pushed a commit to web-platform-tests/wpt that referenced this issue Oct 22, 2020
Two spec issues here.

w3c/IntersectionObserver#457: I think this is
just a spec bug and I've made us match other browsers, but since the
tests don't match the spec for now I've added them as .tentative.html

w3c/IntersectionObserver#456: I've aligned
with WebKit here. There was a (disabled) test for this which tests
chrome behavior and which after this patch shouldn't be flaky. This is
what was causing the assertion.

Differential Revision: https://phabricator.services.mozilla.com/D93166

bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1670327
gecko-commit: 2906a77771b3abcc15c2859052c0d170b263133d
gecko-reviewers: hiro
moz-wptsync-bot pushed a commit to web-platform-tests/wpt that referenced this issue Oct 22, 2020
Two spec issues here.

w3c/IntersectionObserver#457: I think this is
just a spec bug and I've made us match other browsers, but since the
tests don't match the spec for now I've added them as .tentative.html

w3c/IntersectionObserver#456: I've aligned
with WebKit here. There was a (disabled) test for this which tests
chrome behavior and which after this patch shouldn't be flaky. This is
what was causing the assertion.

Differential Revision: https://phabricator.services.mozilla.com/D93166

bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1670327
gecko-commit: 2906a77771b3abcc15c2859052c0d170b263133d
gecko-reviewers: hiro
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants