-
Notifications
You must be signed in to change notification settings - Fork 522
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
Add Scroll Margin to Intersection Observer #511
Conversation
Thanks @tcaptan-cr... just to make this even easier to review, would you mind splitting out the Makefile into its own PR (and possibly even the xref bikeshed fixes if it's not a huge pain?) |
Also, @tcaptan-cr, you are committing to becoming co-Editor? That's ok... but just making sure you are 100% ok with taking on co-ownership. I'll need to announce that to the working group. |
7a20e51
to
3560926
Compare
I split the makefile into this PR, and the bikeshed fixes into this PR |
FYI, this change was reviewed in tcaptan's mirror: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Yes, I am 100% OK with taking on co-ownership. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, with that bit clarified. Thanks for doing this!
index.bs
Outdated
@@ -630,9 +673,11 @@ run these steps: | |||
of the {{document}}, and update |container| to be | |||
the <a>browsing context container</a> of |container|. | |||
2. Map |intersectionRect| to the coordinate space of |container|. | |||
3. If |container| has a <a>content clip</a> or a css <a>clip-path</a> property, | |||
3. If |container| is scrollable, apply the {{IntersectionObserver}}’s |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be great to have a more precise definition of scrollable
, or a link to the relevant CSS spec. Otherwise whether this applies to overflow: hidden
or not is a bit questionable, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. How about using CSS scroll-container?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, let's make sure that we test overflow: hidden then, and confirm that it does apply.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Emilio!
The tests I added in my PR for the wpt tests use overflow: hidden
. For example the scroll-margin.html test.
I confirmed these pass with my prototype.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's fix the link. Pretty sure there's a better way of doing this.
Same for the margin
property link, which I hadn't noticed.
Also, can you rebase to remove the merge commit?
5985bf2
to
ddbd4ad
Compare
Done. |
72b81b1
to
41baa1f
Compare
Thanks Stefan! |
Thanks Emilio! |
SHA: f22fbf4 Reason: push, by tcaptan-cr Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Hi there, Chromium web platform security reviewer here. I've been looking at the I2P for this change and have a few questions, mostly to clarify my understanding:
|
This feature is modelled after the existing
"Any target of an explicit root observer is also a same-origin-domain target, since the target must be in the same document as the intersection root."
"These offsets are only applied when handling same-origin-domain targets; |
Thanks for the answers!
Right, and I don't have context on those concerns, hence my questions :)
I see, and this is actually specified by step 2.2.2 of the Run the Update Intersection Observations Steps algorithm. I'm still left with my questions:
Oh, I see. I was looking at step 3.1 of the compute the intersection algorithm, but step 3.3 also applies because an iframe is a scroll container?
I see, this is what is mentioned in the Privacy and Security section. Can you help me understand the attack here? How would the embeddee learn about its embedder's geometry using That section mentions that
Right, this was what I referred to as "the plain english paragraph". IIUC this does not correspond to any step in the compute the intersection algorithm? I was expecting to find something there. |
Sorry, I don't understand your question. Why does what matter for explicit root observers? Explicit root observers can use
Here's the previous discussion.
Yes, that's right.
The general idea is that the attacker would create a bunch of IntersectionObservers, each with slightly different
Ah, that's a good point. There is an explanation of the same-origin-domain restriction on
|
On same-origin-domain vs same-origin, it only matters when using |
Thanks for the answers and pointers, this is super helpful. Using same-origin-domain makes sense for this, though it would always be better to avoid introducing a new dependency on +1 to amending the spec to extend the same-origin-domain restriction from I have one remaining clarification question: the intent of the same-origin-domain restriction is to prevent |
I don't think there is any new information being exposed that is not already in rootMargin. Do you think there may be something?
Yes, this is my understanding. |
I think I'm now convinced there isn't. Since
Thanks for confirming. |
There were no bugs filed for Gecko or WebKit. Please keep that in the issue template in the future. I've now filed: |
Closes #431
The following tasks have been completed:
Implementation commitment:
Preview | Diff