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

Specify the behavior when DOM mutation occurs across shadow-crossing selection #168

Open
rniwa opened this issue Sep 14, 2023 · 8 comments

Comments

@rniwa
Copy link
Contributor

rniwa commented Sep 14, 2023

We should specify the behavior when DOM trees are mutated (e.g. shadow host is removed) when selection cross shadow boundaries.

@annevk
Copy link
Member

annevk commented Sep 18, 2023

I think this is essentially what I mentioned in #161 (comment) which references WICG/webcomponents#79 (comment).

Let's think about this a bit:

  • We want a new selection live range whose boundary points can be in distinct trees so we can have text selection across the shadow boundary. (Though ultimately they still need to have some kind of shared shadow-including root.)
    • This needs to be updated correctly by the various node tree mutation algorithms.
    • This should not be exposed to script. (Instead we hand out static ranges.)
  • We want a selection live range to point back to a Selection. Currently Selection is mostly defined in terms of a single range, but this might change and it makes sense to let Selection be responsible for rendering the range when it changes and such.
    • I guess ideally after each mutation we invoke some kind of "update selection" algorithm, although currently that does not exist.
  • Selection needs to be reworked to hold onto this new selection live range and not return it directly.
    • What might be tricky is that if someone accesses getRange(0) and manipulates that, this selection live range needs to be updated as well (or perhaps thrown away). We need to be careful not to introduce loops there.

@mfreed7 @rniwa @smaug---- thoughts?

@mfreed7
Copy link

mfreed7 commented Sep 18, 2023

Similar to my reply on #79, I agree.

Your summary above sounds exactly right to me. The devil is in the details, but the broad strokes look correct.

What might be tricky is that if someone accesses getRange(0) and manipulates that, this selection live range needs to be updated as well (or perhaps thrown away). We need to be careful not to introduce loops there.

Seems like that operation (manipulation of a live range) should go through the same "update selection" algorithm which should take care of the details.

@annevk
Copy link
Member

annevk commented Sep 18, 2023

Seems like that operation (manipulation of a live range) should go through the same "update selection" algorithm which should take care of the details.

That would mean we have multiple ranges associated with the selection representing the same conceptual range within the selection.

Maybe instead the "public selection live range" (getRange(0)) should point directly to the selection live range and update that, which then updates the selection. With multiple ranges that would still work, with each selection live range having its own public selection live range.

The most difficult part here is probably defining the "selection live range".

@smaug----
Copy link

This needs to be updated correctly by the various node tree mutation algorithms.

That is the tricky bit. Not only we need to handle mutations in one tree but I believe also changes to slotting should affect selection.

@masayuki-nakano
Copy link

I wrote a test to check the behavior of a selection range which is in a shadow DOM and after the host is removed. Currently, Chrome keeps the range as a selection range without modifying the range in the shadow. Firefox removes the range from Selection without updating the range. (It seems that Safari collapse the selection range to the removed node position, but the test fails at checking the stored range behavior.)

I changed the Firefox's behavior as so for consistency with whatwg/dom#1274 and #175, but could be reasonable to make the selection range cloned (to avoid unexpected range update for the referrers of the Range object) and collapsed to the parent DOM tree (like as in the light DOM, i.e., like Safari).

@dizhang168
Copy link
Member

In your test, you are checking the effect of removing the Shadow host when the Live Range of the current selection is inside the shadow root. For the Selection's Live Range, I believe Chrome is doing the specced behavior. The range should stay within the shadow root's tree scope, even when the the host is removed. No collapsing and boundary points need to be changed.

For the Selection's Composed Range (not defined yet, but this is not the output of getRangeAt), we can compare ancestors while crossing the shadow boundaries. In this case, I would expect the behavior Safari is doing. I added more tests for the new getComposedRanges() API.

@dizhang168
Copy link
Member

Agenda+ this issue.
If we agree on adding the new concept of a "composed live range" (issue #2), we should determinate if and how the DOM spec for mutations should be changed on it.

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

dizhang168 commented Nov 14, 2024

Editing WG Nov 14, 2024 IRC notes: Specify composed range and mutation behavior in DOM spec

08:20 next issue: #168
08:21 Di: how do DOM mutations affect compose ranges? see slide 6 above
08:22 Di: if we use current definition in the DOM spec, there would not be any change in the live range
08:22 Di: however, if we were to use composed ranges, it would need to change
08:22 Di: both blink and gecko don't care about shadow inclusiveness
08:23 dandclark: seems straightforward enough to patch the spec to be shadow inclusive
08:24 whsieh what about multi-range selections?
08:25 Di: generalizes to multiple ranges
08:25 resolve: define composed range mutations in the DOM spec
08:26 Di: check with other folks how are not in the meeting (sanket + smaug)
08:27 mfreed: next step — make a spec PR?

Update DOM spec by adding details for composed range
For example:

To remove a node node, with an optional suppress observers flag, run these steps:
...
4. For each live range whose start node is an inclusive descendant of node, set its start to (parent, index).

Add: For each composed range whose start node is a shadow-inclusive descendant of node, set its start to (parent, index).

We could also refactor the DOM specification with a new “Update Selection” algorithm.
See last 3 slides for an example for node removal.

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

@dizhang168 dizhang168 removed the Agenda+ Queue this item for discussion at the next WG meeting label Nov 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants