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

Modification of selection APIs to account for shadow dom #694

Closed
1 task done
mfreed7 opened this issue Dec 3, 2021 · 8 comments
Closed
1 task done

Modification of selection APIs to account for shadow dom #694

mfreed7 opened this issue Dec 3, 2021 · 8 comments
Assignees
Labels
Resolution: satisfied with concerns The TAG is satisfied with this work overall but requires changes Review type: CG early review An early review of general direction from a Community Group Venue: WHATWG Venue: WICG

Comments

@mfreed7
Copy link

mfreed7 commented Dec 3, 2021

Braw mornin' TAG!

I'm requesting a TAG review of modifying the selection APIs to account for shadow dom.

Shadow DOM (v1) is now supported by all evergreen rendering engines. However, the user selection APIs are not well supported for the case that the user selection crosses shadow boundaries. The existing Selection API specifies that there is only a single selection in a document and the selection is bound to a single Range, which means that it cannot represent a range over the composed tree. Said another way, if the selection crosses shadow boundaries, the existing API cannot represent this situation correctly. For backwards compatibility, we also can't update the Range API to support ranges that cross shadow boundaries, because that could break existing assumptions about ranges.

This proposal suggests a new API that lets web authors control and query selections in the composed tree.

  • Explainer¹: https://github.com/mfreed7/shadow-dom-selection#readme
  • User research: Discussion on the issue thread, starting roughly here. Plus I directly solicited feedback from three editor builders: [1] [2] [3]
  • Security and Privacy self-review²: None
  • Primary contacts (and their relationship to the specification):
  • Organization/project driving the design: Google
  • External status/issue trackers for this feature (publicly visible, e.g. Chrome Status): https://crbug.com/762799

Further details:

  • I have reviewed the TAG's Web Platform Design Principles
  • The group where the incubation/design work on this is being done (or is intended to be done in the future): WICG
  • The group where standardization of this work is intended to be done ("unknown" if not known): WHATWG
  • Existing major pieces of multi-stakeholder review or discussion of this design: discussion
  • Major unresolved issues with or opposition to this design: None that I know of
  • This work is being funded by: Google

We'd prefer the TAG provide feedback as (please delete all but the desired option):

💬 leave review feedback as a comment in this issue and @-notify @mfreed7

@mfreed7 mfreed7 added Progress: untriaged Review type: CG early review An early review of general direction from a Community Group labels Dec 3, 2021
@LeaVerou LeaVerou self-assigned this Dec 4, 2021
@torgo torgo added this to the 2022-02-07 milestone Feb 2, 2022
@plinss
Copy link
Member

plinss commented Feb 8, 2022

Took a look at this today with @LeaVerou and @maxpassion, we were wondering what the purpose was of adding getComposedRange vs overloading getRangeAt to accept the ComposedRangeOptions. As far as we could tell the only apparent difference is the result of a live Range vs a StaticRange, which seems like an orthogonal issue.

@plinss plinss added the Progress: pending external feedback The TAG is waiting on response to comments/questions asked by the TAG during the review label Feb 8, 2022
@w3ctag w3ctag deleted a comment from venmathiraj Mar 23, 2022
@torgo torgo modified the milestones: 2022-02-07, 2022-03-21-week Mar 23, 2022
@mfreed7
Copy link
Author

mfreed7 commented Mar 23, 2022

Thanks for the review and the question. (And sorry for the delay replying!)

So I think the main reason for the new function, vs. re-using the old getRangeAt(), is backwards compatibility. This is mentioned a few places in the explainer, but the thought is that we would likely break sites if we just change getRangeAt() to return a StaticRange (rather than a live Range) that also crosses shadow bounds. Perhaps you were implying that if the developer passed a ComposedRangeOptions to getRangeAt(), you'd get the new behavior? The problem there is that the ComposedRangeOptions is optional, so a call like getRangeAt() (with no arguments) would now get the new behavior. That's likely ok compat-wise, since the existing behavior is to throw an exception with no arguments. However, in all current rendering engines, getSelection().getRangeAt({}) currently returns the live Range. So there's the possibility of compat problems if existing sites are using getRangeAt({}) for whatever reason.

Overall, I think the motivation for a new API was that we're rather-significantly changing the selection API, and beyond the calling syntax, developers will have to understand more about shadow DOM and how this new API works. Giving it a fresh/different function name helps (in my opinion) them understand that there's a difference.

I'm open to suggestions, of course.

@LeaVerou
Copy link
Member

Hi @mfreed7,

We revisited this today during our Hybrid F2F. We understand the reasoning for defining a new function rather than overloading getRangeAt() and we agree that a new function is needed (having a function return different types depending on arguments is not a good idea).

One thing we noticed is that with the addition of this new function, we will now basically have two independent concepts:
a. "Does the range cross Shadow DOM boundaries?" and
b. "Do we need a live or a static range?"

While these are orthogonal, authors cannot specify any of the 4 combinations independently, but only two ([a. no, b. live] or [a. yes, b. static]), which is a coupling that could be confusing since these are not intrinsically related. We understand why [a. yes b. live] is not desirable, but perhaps there is value in designing the API so that [a. no, b. static] can be specified as well, which would make the new function a more general replacement for getRangeAt() as long as live ranges are not desired. What do you think?

@mfreed7
Copy link
Author

mfreed7 commented Mar 25, 2022

Thanks for the feedback!

I understand your question - you're saying it'd be nice to allow users of the new API to get/use StaticRange as a return value, while not crossing shadow bounds. If that's correct, I think the existing API already offers a few ways to do that. First, just calling selection.getComposedRange() with no arguments will result in a StaticRange that does not cross shadow bounds (because none were provided). So that might be the API you're asking for. If authors are building a component, and would just like a StaticRange that is scoped within their tree, the selectionRoot parameter (see this section of the explainer) can be used:

selection.getComposedRange({selectionRoot: componentRoot});

In both of these cases, the returned StaticRange will not cross shadow bounds. Let me know if I missed something about your question.

I appreciate (very much) that you didn't ask for the other permutation, returning a live Range that crosses shadow bounds. 😄

@LeaVerou
Copy link
Member

We were under the impression that you only need to provide shadowRoots for closed shadows. It may be a little tedious to have to loop over the elements and recursively get all their shadows to provide to shadowRoots.

@mfreed7
Copy link
Author

mfreed7 commented Mar 29, 2022

We were under the impression that you only need to provide shadowRoots for closed shadows. It may be a little tedious to have to loop over the elements and recursively get all their shadows to provide to shadowRoots.

Right, the proposal started with only needing to provide closed shadow roots (and with a parameter called something like closedShadowRoots). I felt the same way - it seems like a hassle to have to go enumerate all of the shadow roots. However, after some discussion (e.g. see this summary from TPAC WICG/webcomponents#79 (comment), point #2), it became clear that not only were other implementers against just requiring closed shadow roots, but also that perhaps for the typical use cases for this API it wasn't such a big ask. In particular, editor component vendors (the #1 use case) seem to want to get selection information within their component only, and receiving endpoints outside that tree, particularly inside other unknown shadow roots, was counterproductive. For these use cases, we added the selectionRoot argument, which makes the typical case work without needing to specify shadowRoots at all. There may be other use cases that require selection information anywhere in the document, but these haven't come to light. When they do, we could think about expanding the API with something like getComposedRange({includeAllOpenShadowRoots: true}) or something. And in the meantime, they can certainly enumerate the shadow roots themselves, today.

LMK if that answers your question.

@LeaVerou
Copy link
Member

LeaVerou commented Jun 6, 2022

Hi @mfreed7, we took another look today and we understand your reasoning and are happy with the proposal so we are going to close this.

One thing we wondered about: once this is implemented are there any remaining use cases for getRangeAt()? It seems that the new method covers all of them, and since it returns a static range it's more performant and less error-prone. If that assumption is correct, would it make sense to deprecate getRangeAt() and name the new method in a more generic way (e.g. getRange())?

@LeaVerou LeaVerou closed this as completed Jun 6, 2022
@LeaVerou LeaVerou added the Resolution: satisfied with concerns The TAG is satisfied with this work overall but requires changes label Jun 6, 2022
@LeaVerou LeaVerou removed the Progress: pending external feedback The TAG is waiting on response to comments/questions asked by the TAG during the review label Jun 6, 2022
@mfreed7
Copy link
Author

mfreed7 commented Jun 17, 2022

Thanks! Your question is a good one, and I've raised it on the issue to gather comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Resolution: satisfied with concerns The TAG is satisfied with this work overall but requires changes Review type: CG early review An early review of general direction from a Community Group Venue: WHATWG Venue: WICG
Projects
None yet
Development

No branches or pull requests

5 participants