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

Support iframe viewport tracking #465

Merged
merged 10 commits into from
Dec 10, 2020
Merged

Support iframe viewport tracking #465

merged 10 commits into from
Dec 10, 2020

Conversation

samouri
Copy link
Contributor

@samouri samouri commented Nov 13, 2020 •
edited
Loading

summary
Implements iframe viewport tracking for the polyfill, as described in #372 (comment).

The following tasks have been completed:

  • Modified Web platform tests (link to pull request)

Implementation already completed:

cc @dvoytenko

@samouri samouri marked this pull request as ready for review November 13, 2020 17:17
@samouri
Copy link
Contributor Author

samouri commented Nov 13, 2020 •
edited
Loading

open question: do we want to do feature detection for iframe viewport tracking and replace native with the polyfill if support is missing?

I'm leaning towards no and allowing consumers to detect it themselves. If they prefer the polyfill they can run delete InOb from window before running the polyfill.

dvoytenko
dvoytenko suggested changes Nov 13, 2020
View reviewed changes
polyfill/intersection-observer-test.js Show resolved Hide resolved
polyfill/intersection-observer-test.js Show resolved Hide resolved
polyfill/intersection-observer-test.js Outdated Show resolved Hide resolved
polyfill/intersection-observer.js Outdated Show resolved Hide resolved
polyfill/intersection-observer.js Outdated Show resolved Hide resolved
polyfill/intersection-observer.js Outdated Show resolved Hide resolved
polyfill/intersection-observer.js Show resolved Hide resolved
polyfill/intersection-observer.js Outdated Show resolved Hide resolved
dvoytenko
dvoytenko reviewed Nov 13, 2020
View reviewed changes
polyfill/intersection-observer.js Outdated
rootRect = {
top: 0,
left: 0,
right: html.clientWidth || body.clientWidth,
width: html.clientWidth || body.clientWidth,
bottom: html.clientHeight || body.clientHeight,
height: html.clientHeight || body.clientHeight
height: html.clientHeight || body.clientHeight,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't do this in IE :( No trailing commas. You might have to disable the prettier.

polyfill/intersection-observer.js Outdated Show resolved Hide resolved
@dvoytenko
Copy link
Contributor

I think this is really close now.

@samouri samouri mentioned this pull request Nov 23, 2020
Closed
@samouri
Copy link
Contributor Author

samouri commented Nov 23, 2020 •
edited
Loading

Darn. I believe I found a webkit bug.

Essentially, Chrome and FF ensure that rootBounds when root:document are relative to the specified document (top is always 0). Safari gives the top based on current scroll position within the iframe.

See: https://output.jsbin.com/jutegazuxe

@samouri
Copy link
Contributor Author

samouri commented Dec 4, 2020 •
edited
Loading

Current status:

@dvoytenko: PTAL

dvoytenko
dvoytenko suggested changes Dec 4, 2020
View reviewed changes
Copy link
Contributor

@dvoytenko dvoytenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks very good!

polyfill/intersection-observer-test.js Outdated Show resolved Hide resolved
polyfill/intersection-observer-test.js Show resolved Hide resolved
polyfill/intersection-observer.js Outdated Show resolved Hide resolved
polyfill/intersection-observer.js Outdated Show resolved Hide resolved
polyfill/intersection-observer.js Show resolved Hide resolved
@dvoytenko
Copy link
Contributor

  • Chrome: A different newly introduced test fails. Chrome is allowing for cross-document tracking which is explicitly against spec right now.

Can we link the crbug issue here as well?

@samouri samouri mentioned this pull request Dec 7, 2020
Closed
@philipwalton philipwalton merged commit 76ba976 into w3c:master Dec 10, 2020
@samouri samouri mentioned this pull request Dec 14, 2020
Closed
@samouri samouri deleted the support-iframe-viewport-tracking branch February 28, 2022 02:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@philipwalton philipwalton philipwalton approved these changes

@dvoytenko dvoytenko dvoytenko approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@samouri @dvoytenko @philipwalton