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.

Sign up for GitHub

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

Jump to bottom

Spec doesn't reflect reality wrt explicit roots. #457

Closed
emilio opened this issue Oct 11, 2020 · 3 comments · Fixed by #461
Closed

Spec doesn't reflect reality wrt explicit roots. #457

emilio opened this issue Oct 11, 2020 · 3 comments · Fixed by #461

Comments

@emilio
Copy link
Collaborator

emilio commented Oct 11, 2020

This test-case:

<!doctype html>
<style>
  div {
    width: 100px;
    height: 100px;
    background: blue;
    margin: 10px
  }
</style>
<div id="target"></div>
<div id="root"></div>
<script>
  let div = document.querySelector("#target");
  new IntersectionObserver(
    function() {
      console.log(arguments);
    },
    { root: document.querySelector("#root") }
  ).observe(div);
</script>

In Gecko it doesn't queue any entry. I believe this is correct per https://w3c.github.io/IntersectionObserver/#update-intersection-observations-algo:

If the intersection root is an Element, and target is not a descendant of the intersection root in the containing block chain, skip further processing for target.

I don't think that matches Blink or WebKit though, which just report a non-intersecting entry. I think it's also a bug in the spec, because per that reasoning display: none elements etc wouldn't get reported even if they're dom descendants of the root.

cc @szager-chromium
@emilio
Copy link
Collaborator Author

emilio commented Oct 11, 2020

Similar issue with step 2.2:

If the intersection root is not the implicit root, and target is not in the same document as the intersection root, skip further processing for target.

<!doctype html>
<style>
  div {
    width: 100px;
    height: 100px;
    background: blue;
    margin: 10px
  }
</style>
<div id="root"></div>
<script>
  let doc = document.implementation.createHTMLDocument("");
  let target = doc.createElement("div");
  doc.body.appendChild(target);
  new IntersectionObserver(
    function() {
      console.log(arguments);
    },
    { root: document.querySelector("#root") }
  ).observe(target);
</script>

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Oct 12, 2020
@emilio
Bug 1670327 - Fix more IntersectionObserver issues. r=hiro
6259946 
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
@emilio
Bug 1670327 - Fix more IntersectionObserver issues. r=hiro
4936f05 
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
@emilio
Bug 1670327 - Fix more IntersectionObserver issues. r=hiro
e0225c5 
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
@emilio
Bug 1670327 - Fix more IntersectionObserver issues. r=hiro
6a14480 
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

My preference would be to make the blink/webkit behavior official. I think it's useful to send the "not intersecting" notification when a target is removed from the containing block chain.

@emilio What do you think?

@emilio
Copy link
Collaborator Author

emilio commented Oct 14, 2020

Yes, I tend to agree. I aligned Gecko with WebKit/Blink in https://bugzilla.mozilla.org/show_bug.cgi?id=1670327.

szager-chromium added a commit that referenced this issue Oct 15, 2020
@szager-chromium
Send not-intersecting notifications for detached targets
7dae9e4 
Closes #457
@szager-chromium szager-chromium mentioned this issue Oct 15, 2020
Merged
@moz-wptsync-bot moz-wptsync-bot mentioned this issue Oct 22, 2020
Merged
moz-wptsync-bot pushed a commit to web-platform-tests/wpt that referenced this issue Oct 22, 2020
@emilio @moz-wptsync-bot
Fix more IntersectionObserver issues.
da71b6a 
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
@emilio @moz-wptsync-bot
Fix more IntersectionObserver issues.
6860d7a 
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
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Send not-intersecting notifications for detached targets w3c/IntersectionObserver
2 participants
@emilio @szager-chromium