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

measureIntersection: use root=document #32402

Merged
merged 6 commits into from
Feb 10, 2021
Merged

Conversation

samouri
Copy link
Member

@samouri samouri commented Feb 3, 2021

summary

  • When iframed we should likely always use root:document for InObs. Otherwise rootBounds=null.
  • question: should default for needsRootBounds be true or false?

I recommend viewing the diff without whitespace changes

@samouri samouri force-pushed the fixmeasure branch 2 times, most recently from a8a69a5 to 17ea125 Compare February 4, 2021 19:34
@samouri samouri changed the title legacy-inob-observer-host: remove fireInOb vs. measureIntersection measureIntersection: use root=document Feb 4, 2021
@samouri samouri marked this pull request as ready for review February 4, 2021 19:49
@samouri samouri requested review from dvoytenko and jridgewell and removed request for erwinmombay February 4, 2021 20:25
@samouri
Copy link
Member Author

samouri commented Feb 10, 2021

I'm struggling with the failing unit test here. Unsure what rootBounds is supposed to be equal to for the root=document case.

My impression was document window's viewport. So for the case of an iframe'd element, width should be: iframe.contentWindow.innerWidth.

What I'm actually seeing is: Math.min(iframe.ownerDocument.body.clientWidth, iframe.contentWindow.innerWidth).

@dvoytenko
Copy link
Contributor

I'm struggling with the failing unit test here. Unsure what rootBounds is supposed to be equal to for the root=document case.

My impression was document window's viewport. So for the case of an iframe'd element, width should be: iframe.contentWindow.innerWidth.

What I'm actually seeing is: Math.min(iframe.ownerDocument.body.clientWidth, iframe.contentWindow.innerWidth).

I think the spec probably spells it out. But off the top of my head, documentElement.clientWidth/Height do sound like a reasonable thing here.

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

Successfully merging this pull request may close these issues.

None yet

4 participants