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

getComposedRange() review #161

Open
annevk opened this issue Dec 19, 2022 · 19 comments
Open

getComposedRange() review #161

annevk opened this issue Dec 19, 2022 · 19 comments
Labels

Comments

@annevk
Copy link
Member

annevk commented Dec 19, 2022

If this is empty, return a new StaticRange whose start node and end node are null and whose start offset and end offset are 0.

Start/end node cannot be null. I think we use some kind of default instead, such as "the body element"?

While startNode is a node, startNode's root is a shadow root, and startNode is not a shadow-including inclusive ancestor of any of shadowRoots

It seems that if you pass in the ShadowRoot instance start node is part of, this wouldn't result in a match. Which if you made a selection inside that ShadowRoot instance doesn't seem to be what you want?


One thing I'm missing here still is WICG/webcomponents#79 (comment). As we can no longer expose the true range directly we need to deal with that duality somehow, also for existing APIs such as getRangeAt(). That will require changes to both DOM and Selection.

@masayuki-nakano
Copy link

If this is empty, return a new StaticRange whose start node and end node are null and whose start offset and end offset are 0.

Start/end node cannot be null. I think we use some kind of default instead, such as "the body element"?

Why is just returning null or throwing an exception like getRangeAt not properly here? I feel it's odd that returning a range when there is no range(s). Returning "dummy" range makes web apps need to consider whether it's real one or not.

@rniwa
Copy link
Contributor

rniwa commented Dec 20, 2022

Yeah, returning null is probably most straight forward behavior in this case.

@annevk
Copy link
Member Author

annevk commented Dec 20, 2022

(I was thinking of what https://dom.spec.whatwg.org/#dom-range-range does, but returning null from the method makes sense as well.)

@andreubotella
Copy link
Member

andreubotella commented Jan 3, 2023

Would there be some way to know the direction of a selection in a shadow tree if (anchor|focus)(Node|Offset) are all null? Since StaticRange doesn't have a concept of direction.

@rniwa
Copy link
Contributor

rniwa commented Feb 9, 2023

@andreubotella : yeah, maybe we want direction property on DOMSelection.

@annevk
Copy link
Member Author

annevk commented Feb 9, 2023

Other things worth tracking here:

  • Perhaps we should still allow for multiple ranges API-wise and return a list. (And add a trailing s to the API name.)
  • StaticRange has a concept of "valid" and this does not adhere to that. Perhaps we need a concept of "shadow-including valid" or at least point out that these ranges will not always be valid (but that's okay). Probably the latter until there's an actual use case.

@rniwa
Copy link
Contributor

rniwa commented Feb 9, 2023

@andreubotella : yeah, maybe we want direction property on DOMSelection.

Added direction property in cd5148f

@rniwa
Copy link
Contributor

rniwa commented Feb 13, 2023

I prototyped getComposedRanges in WebKit: https://commits.webkit.org/260121@main

rniwa added a commit that referenced this issue Feb 13, 2023
…ray of StaticRange.

This matches the prototype implementation of this API in WebKit.
See #161
@rniwa rniwa added the NewAPI label Feb 13, 2023
@rniwa
Copy link
Contributor

rniwa commented Feb 14, 2023

Renamed this function to getComposedRanges in 941fa81 to match WebKit's implementation.

@mfreed7
Copy link

mfreed7 commented Mar 2, 2023

Thanks for writing up the spec and getting a prototype started @rniwa!

I need to dedicate some time to read through it in detail, but it would help if you could comment on whether/how it differs from the explainer we discussed before?

Also, as part of the WebKit implementation, did you happen to write any WPTs?

@rniwa
Copy link
Contributor

rniwa commented Mar 2, 2023

@mfreed7 : There are a few differences with that explainer. For starters, the method is called getComposedRanges, and returns a sequence of StaticRange objects. The direction property also returns "forward", "backward", or "none" to match selectionDirection property on the input element.

@mfreed7
Copy link

mfreed7 commented Mar 3, 2023

@mfreed7 : There are a few differences with that explainer. For starters, the method is called getComposedRanges, and returns a sequence of StaticRange objects. The direction property also returns "forward", "backward", or "none" to match selectionDirection property on the input element.

Interesting! Is there an issue discussing why getComposedRanges() plural, that returns a [sequence]<StaticRange> that seems to always just contain one StaticRange? I did really like the part of the new proposal that limited the API to a single range.

Also, as part of the WebKit implementation, did you happen to write any WPTs?

Any comment on this one? I didn't see any tests, but wanted to double-check. We are interested in also implementing this API later this year, and WPTs would help to make sure we're interoperable.

@rniwa
Copy link
Contributor

rniwa commented Mar 7, 2023

Interesting! Is there an issue discussing why getComposedRanges() plural, that returns a [sequence]<StaticRange> that seems to always just contain one StaticRange? I did really like the part of the new proposal that limited the API to a single range.

So that it's consistent with the rest of selection APIs. If we ever wanted to add multi-range selection support, we don't want having to re-invent yet another API for that.

Also, as part of the WebKit implementation, did you happen to write any WPTs?

Any comment on this one? I didn't see any tests, but wanted to double-check. We are interested in also implementing this API later this year, and WPTs would help to make sure we're interoperable.

These two:
https://github.com/WebKit/WebKit/blob/main/LayoutTests/fast/shadow-dom/selection-getComposedRanges.html
https://github.com/WebKit/WebKit/blob/main/LayoutTests/fast/shadow-dom/selection-direction.html

@sefeng211
Copy link

These two:
https://github.com/WebKit/WebKit/blob/main/LayoutTests/fast/shadow-dom/selection-getComposedRanges.html
https://github.com/WebKit/WebKit/blob/main/LayoutTests/fast/shadow-dom/selection-direction.html

@rniwa Any plan to merge them into WPT? Probably as tentative?

Interesting! Is there an issue discussing why getComposedRanges() plural, that returns a [sequence] that seems to always just contain one StaticRange?

Also it simplifies Gecko's implementation since we probably need to make them able to return multiple ranges.

@mfreed7
Copy link

mfreed7 commented Mar 15, 2023

Interesting! Is there an issue discussing why getComposedRanges() plural, that returns a [sequence] that seems to always just contain one StaticRange?

Also it simplifies Gecko's implementation since we probably need to make them able to return multiple ranges.

I get that it simplifies Gecko's implementation, and it makes no implementation difference for Chromium/WebKit who only support a single selection. My concern is more about developers: a) multiple ranges are only supported on a single browser at the moment, and b) due to (a) I'm guessing there is an overwhelming volume of web code that just blindly does Selection.getRangeAt(0). I.e. it hard codes that there's only one range. Given that multiple ranges isn't therefore an interoperable use case, I'm loathe to bake it in further by returning multiple ranges from this new API. If we eventually end up standardizing multiple ranges for real, we can/should cross that bridge when we come to it. Until then, let's make sure selection works interoperably as much as possible today, and also simplify the API for developers, by just returning a single range.

@rniwa
Copy link
Contributor

rniwa commented Mar 21, 2023

Interesting! Is there an issue discussing why getComposedRanges() plural, that returns a [sequence] that seems to always just contain one StaticRange?

Also it simplifies Gecko's implementation since we probably need to make them able to return multiple ranges.

I get that it simplifies Gecko's implementation, and it makes no implementation difference for Chromium/WebKit who only support a single selection. My concern is more about developers: a) multiple ranges are only supported on a single browser at the moment, and b) due to (a) I'm guessing there is an overwhelming volume of web code that just blindly does Selection.getRangeAt(0). I.e. it hard codes that there's only one range. Given that multiple ranges isn't therefore an interoperable use case, I'm loathe to bake it in further by returning multiple ranges from this new API.

Our concern lies in having to add yet another API when that happens. There are increasingly more content that would warrant the support for multi-range selection: bidirectional text, flex box, etc... Since existing APIs are designed to work with multi-range selection, it would be only natural to parallel that in this API.

@mfreed7
Copy link

mfreed7 commented Mar 31, 2023

Our concern lies in having to add yet another API when that happens. There are increasingly more content that would warrant the support for multi-range selection: bidirectional text, flex box, etc... Since existing APIs are designed to work with multi-range selection, it would be only natural to parallel that in this API.

Is WebKit planning to add multi-range selection?

@rniwa
Copy link
Contributor

rniwa commented Apr 3, 2023

Our concern lies in having to add yet another API when that happens. There are increasingly more content that would warrant the support for multi-range selection: bidirectional text, flex box, etc... Since existing APIs are designed to work with multi-range selection, it would be only natural to parallel that in this API.

Is WebKit planning to add multi-range selection?

I cannot comment on our future plans but we're increasingly seeing the need for multi-range selection.

@keithamus
Copy link
Member

WCCG had their spring F2F in which this was discussed. Present members of WCCG identifiers that there are several issues to resolve:

  • The behavior of DOM mutations needs to be specified. For example, if the selection ends within light dom content that is slotted, and that content is removed, where is the new endpoint of the “true range”?
  • There are still open questions around support of multiple ranges

There was also a resolution to open two new issues to discuss further details; #163 and #164 have been opened.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

7 participants