search

WICG / scroll-to-text-fragment

Proposal to allow specifying a text snippet in a URL fragment
Other
589 stars 42 forks source link

[Spec] Restrict based on new browsing context group #70

Closed bokand closed 4 years ago

bokand commented 4 years ago

Fixes #64

bokand commented 4 years ago

@annevk - took a crack at this, would appreciate if you could take a look and let me know if this is on the right track.

As you noted, I'll still need to change target="_same" to allow noopener but I'll do that in a followup. Spec-wise, I think that's just a small change to the rules for choosing a browsing context to follow the noopener path for _self right?

annevk commented 4 years ago

It's a little trickier as we don't have infrastructure for replacing the browsing context yet. https://gist.github.com/annevk/6f2dd8c79c77123f39797f6bdac43f3e will add that.

bokand commented 4 years ago

Thanks, btw you can preview the changes here: http://bokand.github.io/spec.html (not sure if there's a well-beaten path for spec reviews is but my experience is that reviewing the markup diffs is unpleasant)

nickburris commented 4 years ago

LGTM overall but would like review from @annevk as well :)

bokand commented 4 years ago

@annevk: ping. Does the updated restriction satisfy your concerns around cross-origin activation? From the spec with changes here:

Should Allow Text Fragment:
  ...
- If incumbentNavigationOrigin is equal to the origin of document return true.
- If document’s browsingContext is a top level browsing context and its group's browsing context set has length 1 return true.
- Otherwise, return false.

Also, w.r.t. target="_self" rel="noopener", could we make a link with a text directive imply noopener semantics? I would think this is preferable to requiring links to add noopener since there are cases where that'll be difficult for the user and generally make this more difficult to use.

e.g. pasted URLs in a doc/chat-app should work but the user doesn't have any control over how those get linkified. Any reason not to have the UA implicitly add noopener in these cases?

annevk commented 4 years ago

That might be doable, yes, but it would require patching HTML.

bokand commented 4 years ago

Also, w.r.t. target="_self" rel="noopener", could we make a link with a text directive imply noopener semantics? I would think this is preferable to requiring links to add noopener since there are cases where that'll be difficult for the user and generally make this more difficult to use.

That might be doable, yes, but it would require patching HTML.

Actually, given that we're checking we're the only browsing context in the browsing context group, I think the mainstream case navigating a simple top-level browsing context (A --> B) with an anchor link is satisfied without it. A will go away as part of the navigation and B will remain the only browsing context). The only time you'd need this is if you have, e.g. a popup window that survives the navigation and keeps A alive. Is my understanding correct?

Given that, I think an implicit behavior would be more magical and would silently break window.opener. Given that either window.opener or the scroll-to-text has to be lost in this case, I think it's better to lose scroll-to-text because it won't affect the page's functionality. Silently losing window.opener when the page expects it could break the page's assumptions.

I think this case will be rare so making authors add rel=noopener is fine.

It might also make sense to have a dev tools warning if scroll-to-text is blocked explaining why.

annevk commented 4 years ago

Is my understanding correct?

The specification doesn't currently replace the browsing context group for such navigations, so if B opened a popup and then navigated back in history, that popup would have access to A, say. We should probably define things such that replacing actually happens.

bokand commented 4 years ago

Sorry for the delay - picking this back up...

so if B opened a popup and then navigated back in history, that popup would have access to A, say.

Right, but I don't see why that's an issue given our current restrictions - at this point the text fragment in B has already been invoked while A wasn't in the browsing context group and any new navigation in either the main window or popup will now block invoking the text fragment which is intended.

Is your point that if we have target="self" rel="noopener" we could allow invoking the text fragment in more cases? Or is there something else I'm missing?

bokand commented 3 years ago

@annevk Could you elaborate on your concern here? As it stands, if B's popup tries to navigate A, any text fragment invocation will be blocked.

Simply stated, a text-fragment can be invoked from a cross-origin source only if the browsing context is alone in its group (this allows the A->B without replacing the group), meaning the source cannot both control the target's URL and observe the navigation.

Replacing the browsing context group on on self navigations seems like it'd be useful more generally, but it doesn't seem specific to this (and the rule above would be compatible with replacing browsing context groups).