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

Clarify root rect with overflow and padding. #504

Closed
emilio opened this issue Oct 20, 2022 · 3 comments · Fixed by #517
Closed

Clarify root rect with overflow and padding. #504

emilio opened this issue Oct 20, 2022 · 3 comments · Fixed by #517
Labels

Comments

@emilio
Copy link
Collaborator

emilio commented Oct 20, 2022

https://w3c.github.io/IntersectionObserver/#intersectionobserver-root-intersection-rectangle says to use the "content area" when the element has scrollable overflow, however this seems wrong when there's padding.

Consider this test-case: https://codepen.io/jelmerdemaat/pen/wvjLMRN

  • WebKit and Blink implement the spec to the letter.
  • Firefox implements what I think is better behavior: The intersection root rect is the scrollport rect, not the content rect.

Unless I'm missing something, the intent of the spec is to not include the scrollbar and borders, but I believe the correct behavior is Firefox's.

See https://bugzilla.mozilla.org/show_bug.cgi?id=1796268

@miketaylr @szager-chromium do you agree?

@emilio emilio added the bug label Oct 20, 2022
@miketaylr
Copy link
Member

With my webdev hat on, I find Firefox's implementation to be less surprising. @szager-chromium WDYT?

@emilio
Copy link
Collaborator Author

emilio commented Oct 15, 2023

Cc @tcaptan-cr, wonder if there's any opinion here. I think this is an obvious spec bug / oversight.

@tcaptan-cr
Copy link
Collaborator

Cc @tcaptan-cr, wonder if there's any opinion here. I think this is an obvious spec bug / oversight.

I agree that the scrollport rect would be better.

emilio added a commit that referenced this issue Oct 18, 2023
Fixes #504. The overflow property clips to padding edges, so
IntersectionObserver should match that clip.
@emilio emilio mentioned this issue Oct 18, 2023
4 tasks
moz-wptsync-bot pushed a commit to web-platform-tests/wpt that referenced this issue Oct 18, 2023
This is a spec issue. See the test. The element is completely visible,
so the intersection ratio should be 1.

Spec issue: w3c/IntersectionObserver#504
Spec PR: w3c/IntersectionObserver#517

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

bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1796268
gecko-commit: 75132d2a2bac7bc70e1dc808f197967750004238
gecko-reviewers: smaug
emilio added a commit that referenced this issue Oct 18, 2023
Fixes #504. The overflow property clips to padding edges, so
IntersectionObserver should match that clip.
cfallin pushed a commit to cfallin/gecko-dev that referenced this issue Oct 18, 2023
…padding. r=smaug

This is a spec issue. See the test. The element is completely visible,
so the intersection ratio should be 1.

Spec issue: w3c/IntersectionObserver#504
Spec PR: w3c/IntersectionObserver#517

Differential Revision: https://phabricator.services.mozilla.com/D191277
moz-wptsync-bot pushed a commit to web-platform-tests/wpt that referenced this issue Oct 19, 2023
This is a spec issue. See the test. The element is completely visible,
so the intersection ratio should be 1.

Spec issue: w3c/IntersectionObserver#504
Spec PR: w3c/IntersectionObserver#517

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

bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1796268
gecko-commit: 75132d2a2bac7bc70e1dc808f197967750004238
gecko-reviewers: smaug
ErichDonGubler pushed a commit to erichdongubler-mozilla/firefox that referenced this issue Oct 20, 2023
…padding. r=smaug

This is a spec issue. See the test. The element is completely visible,
so the intersection ratio should be 1.

Spec issue: w3c/IntersectionObserver#504
Spec PR: w3c/IntersectionObserver#517

Differential Revision: https://phabricator.services.mozilla.com/D191277
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants