w3c / selection-api

Selection API
http://w3c.github.io/selection-api/
Other
47 stars 30 forks source link

getComposedRange() review #161

Open annevk opened 1 year ago

annevk commented 1 year ago

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 https://github.com/WICG/webcomponents/issues/79#issuecomment-972726532. 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 commented 1 year ago

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 commented 1 year ago

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

annevk commented 1 year ago

(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 commented 1 year ago

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 commented 1 year ago

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

annevk commented 1 year ago

Other things worth tracking here:

rniwa commented 1 year ago

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

Added direction property in https://github.com/w3c/selection-api/commit/cd5148fc9d3b4d08936880c27b9b18985c62938d

rniwa commented 1 year ago

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

rniwa commented 1 year ago

Renamed this function to getComposedRanges in https://github.com/w3c/selection-api/commit/941fa8123751d73d6924a35834466e103a2e373b to match WebKit's implementation.

mfreed7 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

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