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

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

Open
rniwa opened this issue Aug 6, 2022 · 10 comments
Open

Comments

@rniwa
Copy link

rniwa commented Aug 6, 2022

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
Copy link
Collaborator

bokand commented Aug 9, 2022

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
Copy link
Author

rniwa commented Aug 9, 2022

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
Copy link
Collaborator

bokand commented Aug 9, 2022

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
Copy link

hober commented Aug 9, 2022

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
Copy link
Collaborator

bokand commented Aug 9, 2022

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
Copy link
Author

rniwa commented Aug 9, 2022

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
Copy link
Collaborator

bokand commented Aug 10, 2022

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
Copy link
Author

rniwa commented Aug 15, 2022

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
Copy link
Author

rniwa commented Aug 15, 2022

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

@bokand
Copy link
Collaborator

bokand commented Aug 15, 2022

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:

  • The ordering in the light DOM might not match the order in the flat tree which complicates context/range matching.
  • The highlight would have to get mapped from the light DOM node to the one in the flat tree

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?

aarongable pushed a commit to chromium/chromium that referenced this issue Nov 23, 2023
Data for resolving:
WICG/scroll-to-text-fragment#190

Bug: 1504647
Change-Id: I60bb383af17deaf189dd52178e0847d70593f0cb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5054468
Reviewed-by: Vladimir Levin <[email protected]>
Commit-Queue: David Bokan <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1228545}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants