WICG / scroll-to-text-fragment

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

It's unclear whether scroll to text fragment works with flat tree or not. #190

Open rniwa opened 2 years ago

rniwa commented 2 years ago

The spec has a bunch of concepts relying on CSS boxes like nearest block ancestor, which typically walks up the flat tree, but uses shadow-including tree orders in most places.

It's very much unclear to me which tree / trees all these algorithms are supposed to work with.

bokand commented 2 years ago

Thanks for filing.

The intent in all places in this spec is to cross shadow boundaries to search against visible text in a shadow tree, i.e. I believe that means the flat tree. I was under the impression that using "shadow-including" operations on regular ("non-flattened"?) trees was equivalent to using regular operations on a flat tree.

Taking a closer look, it looks like my interpretation was wrong. My read of (e.g.) shadow-including tree order is that it would include both the children of a host's shadow root in addition to the light nodes attached to the host and doesn't fill in slots. A flat tree representation replaces the light nodes with the shadow contents and fills slots.

Is that correct and does that capture the whole concern in this issue? If so, I believe the solution is to replace the tree operations in the spec to make them operate on the flat tree representation and I'll make a fix shortly.

rniwa commented 2 years ago

The intent in all places in this spec is to cross shadow boundaries to search against visible text in a shadow tree, i.e. I believe that means the flat tree.

Yeah, that would be flat tree.

Taking a closer look, it looks like my interpretation was wrong. My read of (e.g.) shadow-including tree order is that it would include both the children of a host's shadow root in addition to the light nodes attached to the host and doesn't fill in slots. A flat tree representation replaces the light nodes with the shadow contents and fills slots.

That's right.

Is that correct and does that capture the whole concern in this issue? If so, I believe the solution is to replace the tree operations in the spec to make them operate on the flat tree representation and I'll make a fix shortly.

That is most or less what this issue amounts to.

However, I'd like to raise another concern with respect to this feature working on the flat tree. Regular document fragment doesn't work across shadow trees, why should this feature work across shadow trees?

bokand commented 2 years ago

However, I'd like to raise another concern with respect to this feature working on the flat tree. Regular document fragment doesn't work across shadow trees, why should this feature work across shadow trees?

That's a good point, and I'm open to counter arguments, but I'd argue for crossing shadow boundaries on the premise that this feature works on user-visible content. The fact that some content is in a shadow tree isn't something that the user can tell or cares about - they should be able to target content they can see (e.g. if I'm instructing a friend on which button to push on a page that implements a shadow DOM button control).

You could make the same argument for iframes, for which we made a different choice, but I think there were prevailing security considerations there that don't apply to shadow DOM.

hober commented 2 years ago

if javascript on the page can detect if scroll-to-text-fragment successfully found & scrolled to the text fragment, and the text fragment can span across closed shadow boundaries, isn't this leaking information / violating encapsulation?

bokand commented 2 years ago

This may be a dumb question as I don't have deep experience with shadow DOM: is it meant to be a hard security/content boundary rather than just a means of software encapsulation?

IMHO this doesn't violate encapsulation since (to me at least) encapsulation means restricting direct access of data between software components so that authors can prevent consumers from forming dependencies on private data. Given that this works only on the textual content (as opposed to say, DOM attributes or styles) this seems unlikely to a problem from that perspective. It's also a rather round about side-channel (akin to say, being able to inspect private fields of a C++ object via pointer arithmetic).

OTOH, if a closed shadow tree is intended to guarantee that an adversarial consumer has no means of accessing any information from inside the shadow tree then yes, this is problematic and does leak information from the shadow tree. However, it seems to me like it's already not hard to circumvent closed shadow trees but maybe I'm missing some context here.

rniwa commented 2 years ago

This may be a dumb question as I don't have deep experience with shadow DOM: is it meant to be a hard security/content boundary rather than just a means of software encapsulation?

It's not. But contents within shadow DOM is not supposed to affect the behavior of browser API without a deliberate exposition by the shadow DOM itself.

Here, this feature seems to want to behave as though there is no shadow boundary based on what you mentioned above. This would mean that attaching contents to shadow DOM would affect the behavior of this API whether shadow DOM wants to or not, which isn't desirable.

Also there has been a discussion about extending fragment navigation API to support targeting inside or across shadow boundaries. It would be highly confusing if this API crossed shadow boundaries by default unlike regular document fragment navigation, and we had a separate API to target things inside a shadow DOM.

bokand commented 2 years ago

contents within shadow DOM is not supposed to affect the behavior of browser API without a deliberate exposition by the shadow DOM itself.

I think this makes sense for informational/data APIs but the goal is encapsulation; scroll-to-text doesn't provide direct access to content/information, it performs a helpful action on behalf of the user. I'd liken it more to something like scrollIntoView which does bubble a scroll across shadow boundaries. It preserves encapsulation and this behavior is more likely to be what the user expects.

It would be highly confusing if this API crossed shadow boundaries by default unlike regular document fragment navigation, and we had a separate API to target things inside a shadow DOM.

It'd be a difference but I don't see what problem this would cause? (i.e. who'd be confused?)

Regular fragment navigation works on element ids which are explicitly encapsulated by shadow DOM and require an author to expose ids on navigable sections (and typically provide some way to link to it). e.g. If the author wants to let users link to #header on their page, it'd be problematic if a shadow DOM component above their header reused this id.

By contrast, scroll-to-text doesn't require the author to annotate anything, users can link to anything that's visible. It'd be confusing if the user could see a snippet of text but the text fragment didn't work (or worse, scrolled to some instance further down the page).

Another related consideration is that this is typically used in conjunction with some browser UI (i.e. select text > right click > copy link to highlight). If a user can highlight some text but cannot create a link to it this is difficult to communicate and frustrating to the user. I think considering the priority of constituencies is helpful here: IMHO there's clear user benefit here that's somewhat in tension with technical purity but it's not clear to me the technical aspects are problematic in practice.

rniwa commented 2 years ago

contents within shadow DOM is not supposed to affect the behavior of browser API without a deliberate exposition by the shadow DOM itself.

I think this makes sense for informational/data APIs but the goal is encapsulation; scroll-to-text doesn't provide direct access to content/information, it performs a helpful action on behalf of the user. I'd liken it more to something like scrollIntoView which does bubble a scroll across shadow boundaries. It preserves encapsulation and this behavior is more likely to be what the user expects.

With scrollIntoView, the caller has access to the shadow DOM content in order to bubble scrolling up the shadow boundary. Browser doesn't automatically take shadow DOM into account in other scrolling scenarios.

It would be highly confusing if this API crossed shadow boundaries by default unlike regular document fragment navigation, and we had a separate API to target things inside a shadow DOM.

It'd be a difference but I don't see what problem this would cause? (i.e. who'd be confused?)

It's confusing to an extent that we'd have two separate behaviors, one that cross shadow boundaries by default and one that doesn't.

Regular fragment navigation works on element ids which are explicitly encapsulated by shadow DOM and require an author to expose ids on navigable sections (and typically provide some way to link to it). e.g. If the author wants to let users link to #header on their page, it'd be problematic if a shadow DOM component above their header reused this id.

This also applies to scroll-to-text. We don't want text link to start matching random stuff in a shadow DOM. Contents inside a shadow DOM should be implementation details of a given custom element / component.

Another related consideration is that this is typically used in conjunction with some browser UI (i.e. select text > right click > copy link to highlight). If a user can highlight some text but cannot create a link to it this is difficult to communicate and frustrating to the user.

I don't think we need to restrict scroll-to-text feature to never match content inside a shadow DOM. The question is whether they match / cross shadow boundaries by default or not. Historically, we've always avoided such a default behavior to respect shadow DOM's encapsulation. For example, we could use a separate "path=~" value to define how to traverse through shadow boundaries, in which case, the browser can use such a URL to refer to a specific section of a document.

rniwa commented 2 years ago

At this point, I'm pretty convinced that we should not let this feature cross shadow boundaries by the default.

bokand commented 2 years ago

With scrollIntoView, the caller has access to the shadow DOM content in order to bubble scrolling up the shadow boundary. Browser doesn't automatically take shadow DOM into account in other scrolling scenarios.

scrollIntoView is also used implicitly in some user gestures in which case there is no caller, e.g. the user focuses an input box. That's more the scenario I was thinking of.

This also applies to scroll-to-text. We don't want text link to start matching random stuff in a shadow DOM. Contents inside a shadow DOM should be implementation details of a given custom element / component.

I guess the scenario I'm most concerned about is one where the light DOM is something like:

<custom-comment-box>
  <span slot="title">My First Post</span>
  Comment Text
</custom-comment-box>

and the provided content is inserted into slots. I think the content here should be searchable by default since it's provided by the component client, not an implementation detail of the component itself. Do you agree?

We could match the label text in a non-flat tree walk but:

I don't think we need to restrict scroll-to-text feature to never match content inside a shadow DOM. The question is whether they match / cross shadow boundaries by default or not.

If there's still a way to match into shadow DOM I don't feel very strongly. But I just wonder why anyone would ever use the non-shadow-crossing version? e.g. for an extension that generates links like this, how would it decide whether to use the shadow-crossing version or not? I assume it'd just always use the shadow-crossing version to be safe.

I worry authors/users would interpret the two versions as: "scroll-to-text" and "scroll-to-text-that-sometimes-doesnt-work" which makes me think the non-shadow-crossing one won't ever be used?