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

Clarify association between a selection and its range #2

Open
rniwa opened this issue Sep 16, 2014 · 19 comments
Open

Clarify association between a selection and its range #2

rniwa opened this issue Sep 16, 2014 · 19 comments

Comments

@rniwa
Copy link
Contributor

rniwa commented Sep 16, 2014

"Once a selection is associated with a given range, it must continue to be associated with that same range until this specification requires otherwise."

This paragraph above is vague. It needs to be replaced by detailed conformance requirements saying exactly what to do for particular keystrokes, like we have for backspace/delete/etc.

yoichio pushed a commit to yoichio/selection-api that referenced this issue Oct 12, 2018
@yoichio
Copy link

yoichio commented Nov 12, 2018

Current spec states:
"Each selection can be associated with a single range. When there is no range associated with the selection, the selection is empty. The selection must be initially empty."
but per WICG/webcomponents#79, selection should be associated with a composition range so that
selection can represent range-over-shadow and calling SetBaseAndExtent with shadow nodes sets such selection simply w/o any adjustment.

Concerns.
Selection is associated with DOM Range, witch adjusts itself when DOM tree changes but a composition range might not or it is not speced.
getRangeAt() needs some adjustment to return DOM Range from the composition range.

@rniwa
Copy link
Contributor Author

rniwa commented Nov 12, 2018

Yeah, I think we need to change the internal model of selection to be some kind of composed range from which Range and composed range exposed to a script is computed.

@dizhang168
Copy link
Member

Hi @rniwa! With the currently specced getComposedRanges(), I believe this issue is a blocker and should be resolved.

The spec is being vague about the type of Range is associated to the Selection. In Blink (and likely all browsers), this is a Live Range and responds to DOM Mutations accordingly. However, as pointed above, if we want to support composed range, we cannot have it as a live range.

I propose we clarify the above definition to:

"Once a selection is associated with a given live range range, it must continue to be associated with that same range until this specification requires otherwise."

But also add new definitions in Selection:

  • composed focus
  • composed anchor

Which should default to focus/anchor, but can be set when selection crosses shadow trees. And these are the endpoints we use to compute getComposedRanges.

@rniwa
Copy link
Contributor Author

rniwa commented Oct 21, 2024

Like we discussed during TPAC, we likely need to introduce the concept of live "composed" range to DOM spec and use that concept here.

@dizhang168
Copy link
Member

Sorry, I know we decided to introduce the concept of "live composed range", but the spec changes needed is still unclear to me.

Currently, the DOM definition:

Objects implementing the Range interface are known as lives ranges.

Should we change to either:
A) class Range final : public ComposedRange and class ComposedRange: public AbstractRange
B) class ComposedRange final : public Range and class Range: public AbstractRange
C) class ComposedRange final : public AbstractRange

Further, currently the range associated to Selection is Range. Is the resolution to change this associated range to be ComposedRange? I am unsure how we can have only one ComposedRange range associated to the selection yet be able to return a Range when getRangeAt(0) is called.

(This is why I am thinking adding the internal concept of composed anchor/focus would be sufficient and easier. I am happy to add the concept of live composed range if there is a good way to implement it.)

@annevk
Copy link
Member

annevk commented Oct 22, 2024

I don't think we need any new IDL at this stage.

A live composed range is an internal concept, though it would require many of the things that a live range has, just none of the public API. But we would have to update it after tree mutations, for instance. A live composed range also has a pointer to a Selection object, to forward mutations.

A Selection object points to a live composed range and also forwards mutations.

The live range from getRangeAt(0) can initially be computed from the live composed range. Though once it's out there we'd also have to maintain it in parallel, requiring yet more pointers and forwarding of operations.

Having said that, what is an internal concept of composed anchor/focus?

@dizhang168
Copy link
Member

Thanks for the explanation, very helpful. Here is my thought process.

Right now, the spec says we have:

  • Selection <-> Range (live range).

If we introduce the concept of a live composed range, then the mapping would be:

  • Selection <-> Composed Range (live range) <-> Range (live range)
  • For setBaseAndExtent(), it updates this composed range.
  • We will need to revisit mutations.
  • For getRangeAt(0), we will get the live composed range and rescope it. This live range will be pointing to the live composed range and its mutation will affect it.
  • For getComposedRanges(), we can get the start/end boundary points of live composed range.

What I am proposing instead is to not add/change Range definitions: ranges are not composed. Instead, we add a new concept of a Composed Selection. The composed selection internally stores anchor and focus endpoints to be across the document. It has no direct pointer to any range.

  • Selection <-> Composed Selection
  • Selection <-> Range (live range)
  • For setBaseAndExtent(), it updates both the Composed Selection and the Live Range. Live range might collapse if across trees, but there are no such check on composed selection.
  • When mutations happen to a live range, it will call UpdateSelection(), which will UpdateComposedSelection().
  • For getRangeAt(0), no change needed.
  • For getComposedRanges, we can get the start/end by accessing the composed selection's anchor/focus.

I think this approach might be easier on the specifications, while still giving the user agents enough leeway on how they want to implement it.

@annevk
Copy link
Member

annevk commented Oct 24, 2024

But the live range and the selection are out-of-sync if the selection is across a node tree boundary, right? And the selection offsets would similarly be out-of-sync with the composed selection offsets. So you would also have to possibly update the selection when a node tree is mutated and the live range associated with the selection is not impacted. Just as you would for a live composed range.

I don't really see how this concept of a composed selection (where you still need to support the normal selection as well as you cannot directly reveal the existence of the shadow tree) is fundamentally different from a live composed range. It seems to me you have the same state to maintain and the same algorithms need to be updating that state. I guess the main difference is that these internal offsets live directly on the selection and that node tree mutations now have to know directly about a selection. Not entirely sure that that's a win.

@dizhang168
Copy link
Member

dizhang168 commented Nov 12, 2024

I see. My original proposal was to avoid having to modify Range and have everything selection scoped within the Selection. I have given it more thoughts and I am good with using a composed live range instead of introducing a new composed selection concept. The user facing changes will be the same and the implementation can be left to the user agent. Bonus, the specification should be easier because we utilize the existing Range (live range) definitions.

To simplify, I propose the new structure would be:

  • Selection <-> Composed Range
  • Selection <-> Range (so we can keep existing behavior).

From a standard point of view, the main changes needed would be:

In DOM spec

  • Define composed before, composed after, composed equal
  • Define a Range is "composed" if it is associated with a selection and its shadow-including root is the document.

In the Selection API spec

  • Change to "Each selection can be associated with a single composed range and a single range."
  • Change getRangeAt(0): If there is an associated composed range, rescope it to be within the document tree, set this's range to it and return it.
  • Change anchor/focus to refer to the composed range.
  • Change setBaseAndExtent/etc to updated both associated ranges.

Note:
Also, I don't think we need to update the mutation related spec. For example, let's say we have a composed live range and it has a start node inside a shadow tree. That shadow tree's host is removed. The spec says to rescope this start node to the host's parent. That is the right step to follow. I have added a few WPT tests of what's expected if we keep the mutation rules we have now. Please correct me if that is the wrong assumption.

Edit 11/12: Updated the above proposal to be clearer on how the associated range and the associated composed range interact with each other and more details on Selection API changes.

@annevk
Copy link
Member

annevk commented Nov 12, 2024

So in the case where your selection starts outside the shadow host and goes inside of it, the API acts as-if nothing is selected? That doesn't seem good.

@dizhang168
Copy link
Member

Not sure why that would result in nothing getting selected, but we can move the mutation conversation to issue #168.
For now, I will add Agenda+ to this issue so we can discuss at upcoming meeting.

@dizhang168 dizhang168 added the Agenda+ Queue this item for discussion at the next WG meeting label Nov 12, 2024
@dizhang168
Copy link
Member

Added some slides here to explain a potential proposal I would like to discuss.

@dizhang168
Copy link
Member

Editing WG Nov 14, 2024 IRC notes: Resolved to go with option 1

08:05 #2
08:05 Di: discussed at TPAC, we should have a new composed range concept in the spec. unclear how to track selection endpoints in different trees
08:06 https://github.com/dizhang168/shadow-dom-selection/blob/main/Selection%20API%20-%20composed%20range.pdf
08:06 Di: 1. want to define what a composed range is (defn. in slide above)
08:07 Di: could make it inherit from live range but that runs into other issues
08:07 Di: 2. define association between live range and selection
08:08 Di: (see above slides for more details on updates to getRangeAt and setBaseAndExtent)
08:09 johanneswilm: does this mean you always have live range in composed range? or only 1
08:10 Di: live range implies existence of composed range (?)
08:11 mfreed: both are implementable, yes?
08:11 Di: yes
08:15 mfreed: preference?
08:15 Di: option 1 might be more straightforward to implement
08:15 johanneswilm: impact on JS devs?
08:15 Di: in practice, JS developers shouldn't need to care
08:15 Di: more browser-internal. but it should be in the spec, since it's useful for defining other algorithms
08:15 johanneswilm: sounds like this is just an implementation detail
08:16 mfreed: also my comment earlier — seems like this can be implemented by a browser engine either way. this is essentially a way to rigorously define what happens when we say "composed range from the selection", etc.
08:16 Di: advantage is that this would allow us to define editing operations
08:17 dandclark: can you remind me of the current state of the spec?
08:17 Di: selection can have a range, range is anything that implements abstract range
08:17 Di: browsers implement live range
08:17 dandclark: gut reaction — easier to think about the version where there's the 1 range. we compute composed ranges on the fly
08:18 +1 my vote is option 1
08:18 dandclark: probably easier to have the spec that way (maps to option 1)
08:18 whsieh: same
08:19 resolution: lets' go with option 1
08:20 (would be good to confirm with Sanket et al.)

where option 1 is:
Each selection can be associated with one composed range and each composed range can be associated with one range.
Selection <-> Composed Range <-> Live Range

and option 2 is:
Each selection can be associated with one composed range and one range.
Selection <-> Live Range
Selection <-> Composed Range

@annevk @sanketj @smaug---- for reference

@dizhang168 dizhang168 removed the Agenda+ Queue this item for discussion at the next WG meeting label Nov 14, 2024
@annevk
Copy link
Member

annevk commented Nov 15, 2024

I don't understand how this is an implementation detail. Typically we don't have to define implementation details. This we have to define however as it's very much observable.

I'm also not sure how the composed range can be computed on the fly.

I also don't really understand the slides unfortunately. They suggest that getRange(0) allocates, but I think the range stays 1:1 with the selection for the lifetime of the selection. And changes to the range are reflected in the selection and vice-versa. You could do some lazy initialization perhaps, but that would in fact be an implementation detail and would not need to be specified.

Similarly the steps under what setBaseAndExtent() would do are confusing. First it updates this's composed range and then it replaces it?

@dizhang168
Copy link
Member

The slides were written quickly and don't have all the implementation details. Apologies and I will work on a spec PR soon that does include everything.

The composed range is not computed on the fly. It is tracked by the selection. Overall, my proposal is as follow:

In Selection API, we change the wording to say:
"Each selection is associated with a single composed range."

And in the DOM spec, we define what a composed range is:
A composed range is the range associated with a selection. Its start and end share the same shadow-inclusive root.

A composed range can be associated with a live range.

1 Selection <=> 1 Composed Range
1 Composed Range <=> 0 or 1 Live range

I also don't really understand the slides unfortunately. They suggest that getRange(0) allocates, but I think the range stays 1:1 with the selection for the lifetime of the selection. And changes to the range are reflected in the selection and vice-versa. You could do some lazy initialization perhaps, but that would in fact be an implementation detail and would not need to be specified.

I see your point. We can remove that implementation detail and leave it to the user agent. We can just say return the live range associated with the this's composed range.

Similarly the steps under what setBaseAndExtent() would do are confusing. First it updates this's composed range and then it replaces it?

First, it updates the composed range. Then, it updates the live range associated to this composed range.

@annevk
Copy link
Member

annevk commented Nov 20, 2024

Thanks. And to be clear, the composed range is live as well right? That's why we called it "live composed range" at TPAC, iirc.

@dizhang168
Copy link
Member

It should definitely react to mutation. However, a live range is defined as "Objects implementing the Range interface are known as live ranges." and I don't think we want to expose any of the Range interface on this composed live range. So we might need to define this differently.

@annevk
Copy link
Member

annevk commented Nov 22, 2024

I agree with all of that, but I still think "live composed range" is the correct name given that it very much behaves in many of the same ways as a live range, it's just not script-exposed, doesn't support the same API, and can cross the shadow boundary.

@dizhang168
Copy link
Member

(Sorry for taking a while to get back to this)

If so, I suggest we redefine what a Live Range means. It isn't only if it implements the Range interface, but is for any object that implements the AbstractRange interface and that listens to DOM mutations. Given this new definition, we can say a "Composed Live Range" is a live range. I have updated my explainer with what this would look like in spec land:
https://github.com/dizhang168/shadow-dom-selection/blob/main/selection-api-spec-changes.md

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

4 participants