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.

Sign up for GitHub

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

Jump to bottom

[cssom-view] Specify document.caretPositionFromPoint in shadow DOM scenario #9932

Closed
siliu1 opened this issue Feb 9, 2024 · 8 comments
Closed

[cssom-view] Specify document.caretPositionFromPoint in shadow DOM scenario #9932

siliu1 opened this issue Feb 9, 2024 · 8 comments
Labels
Closed Accepted by CSSWG Resolution cssom-view-1

Comments

@siliu1
Copy link
Contributor

siliu1 commented Feb 9, 2024

Current spec is vague about the behavior in shadow DOM: https://www.w3.org/TR/cssom-view-1/#dom-document-caretpositionfrompoint

Gecko's current behavior in shadow DOM:
The returned CaretPosition's offset is the text offset of the hit tested text node and the offsetNode depends on the mode of the shadow tree:

  1. If the tree is created by user agent such as input element, the node is the shadow host.
  2. Otherwise, the node is the hit tested text node even if the tree is closed.

Gecko's existing behavior can address most of the need for web developers. Should we specify the behavior?

Or are there other alternatives that we should consider:

  1. caretPositionFromPoint do not pierce shadow DOM. Instead, put the API on DocumentOrShadowRoot.
  2. support closed shadow root with a new parameter: document.caretPositionFromPoint(x, y, closedShadowRoot);
@sanketj
Copy link
Member

sanketj commented Feb 21, 2024

@zcorpan @fantasai @tabatkins Curious if any of you have any thoughts on how we can better define the behavior of caretPositionFromPoint for shadow DOM scenarios?

@xiaochengh
Copy link
Contributor

Are there other APIs that pierce shadow DOM?

Document.getSelection() looks like a similar API to me, which doesn't pierce shadow DOM, and returns (container_node_of_host, host_offset_in_container) if the selection is inside shadow DOM (regardless of open or closed). It might be confusing if getSelection() and caretPositionFromPoint() return different results.

caretPositionFromPoint do not pierce shadow DOM. Instead, put the API on DocumentOrShadowRoot.

+1 to this idea.

@siliu1
Copy link
Contributor Author

siliu1 commented Mar 26, 2024

Asked opinions from web developer's point of view and got some feedback. Web developer's basic requirement is to be able to get a caret position inside shadow roots. The Firefox approach (piercing through the shadow boundary) is acceptable. In fact, sticking to the already existing behavior would allow web developer to simplify the implementation.

Given that, should we specify the already existing Firefox approach and put it in spec?

@mfreed7
Copy link

mfreed7 commented Apr 3, 2024

So in my opinion, the Gecko approach should definitely not be put in the spec. It trivially reveals shadow root contents, including for closed roots, which is very bad:

<!DOCTYPE html>
<div>
  <template shadowrootmode=closed>
    <div style="width:300px;height:300px;background:lightblue">
      Oops
    </div>
  </template>
</div>
<script>
  const str = document.caretPositionFromPoint(100,100).offsetNode.textContent;
  document.body.append(str)
</script>

I'd +1 the comments above that this should follow the behavior of getComposedRanges():

@annevk @rniwa

@siliu1
Copy link
Contributor Author

siliu1 commented Apr 3, 2024

Based on all feedback, the best approach seems to be the getComposedRanges() style which takes in a set of ShadowRoots and document.caretPositionFromPoint(ShadowRoot ...shadowRoots) can return position in the given shadow roots.

The alternative approach is to put the API on DocumentOrShadowRoot. But the con to this approach is that the usage might be cumbersome to get a position inside nested shadow roots (need to call the API on each level of shadow root).

Web developer needs to adjust the current usage of current API to get position in a shadow root. The existing behavior violates the boundary of closed shadow tree so we should consider fixing it as well.

@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed [cssom-view] Specify document.caretPositionFromPoint in shadow DOM scenario, and agreed to the following:

  • RESOLVED: add shadowRoots parameter to caretPositionFromPoint, same as getComposedRanges
The full IRC log of that discussion <fantasai> Liu: This API is defined in cssom-view, and spec doesn't have a definition for behavior in shadow DOM
<fantasai> Liu: Existing behavior is [missed]
<fantasai> Liu: Other alternatives are to not pierce shadow DOM
<fantasai> Liu: Another approach is to add a new parameter with set of shadow roots, and will return positio inside the shadow roots
<fantasai> Liu: Gathered feedback from community, and preference is to add parameter and only pierce the specified shadow roots
<fantasai> Liu: If this is acceptable, should we adopt?
<fantasai> Liu: Firefox's implementation is that the API can pierce the shadow tree, and tree can be open or closed shadow tree
<fantasai> mfreed: I'm definitely not a fan of revealing contents of shadow trees, particularly closed ones
<fantasai> mfreed: I heard proposed to use getComposedRanges behavior, where if you don't know about the shadow tree you get the shadow host
<dandclark> q+
<sanketj_> q+
<fantasai> astearns: I'm not familiar with these APIs. Is getComposedRanges same pattern as what's proposed here?
<fantasai> mfreed: Yes. parameters to say which you know about, and it will pierce those, and give you host for those that you don't
<astearns> ack dandclark
<fantasai> dandclark: getComposedRanges spec link dropped in the issue doesn't take a parameter
<fantasai> dandclark: do browsers implement something else?
<fantasai> mfreed: It's only implemented in WebKit, definitely not in Blink yet
<fantasai> astearns: Looking at WebIDL there's a parameter, just not in description
<fantasai> dandclark: OK
<astearns> ack sanketj_
<dholbert> q+
<fantasai> sanketj_: Seems that Firefox is only one that pierces closed shadow tree, so getComposedRanges addresses that, so +1
<astearns> ack dholbert
<fantasai> dholbert: With this version of the API, that takes list of shadow roots to pierce, if web developer listed every shadow root in their page they would get existing Firefox behavior?
<fantasai> mfreed: yes, but not possible if you don't have reference to the shadow root you can't provide it
<fantasai> mfreed: so e.g. closed shadow root, unless you set it up, you can't get it
<fantasai> astearns: wrt the spec descriptions, the descriptions don't mention parameters, but the algorithms do process them
<fantasai> astearns: Sounds like we're ready to resolve? Any additional comments?
<fantasai> astearns: Proposed resolution is that caretPositionFromPoint can follow the getComposedRanges behavior and have a shadowRoots parameter
<fantasai> RESOLVED: add shadowRoots parameter to caretPositionFromPoint, same as getComposedRanges

@mozilla-apprentice mozilla-apprentice mentioned this issue Apr 3, 2024
Closed
@chromium-helper chromium-helper mentioned this issue Apr 3, 2024
Closed
siliu1 added a commit to siliu1/csswg-drafts-fork that referenced this issue Apr 4, 2024
@siliu1
Update Overview.bs
eb7f6bb 
Per w3c#9932, add `shadowRoots` parameter to `caretPositionFromPoint` API.
This was referenced Apr 11, 2024
Merged
Open
Closed
@siliu1 siliu1 mentioned this issue Apr 25, 2024
Open
1 task
@siliu1
Copy link
Contributor Author

siliu1 commented Apr 29, 2024

Created PR #10200 to update the spec. @zcorpan would you mind taking a look? Thank you!

siliu1 added a commit that referenced this issue May 3, 2024
@siliu1
[cssom-view] add shadowRoots parameter to caretPositionFromPoint …
f0de230 
…API. (#10200)

* Update Overview.bs

Per #9932, add `shadowRoots` parameter to `caretPositionFromPoint` API.

* Update Overview.bs

Bubble `caret range` out of the shadow roots.

* Update Overview.bs

Address PR comments.

* Update Overview.bs

Address PR comment.

* Update Overview.bs

Address PR comment.

* Update Overview.bs

linked created issue(10230) to the spec.
@zcorpan
Copy link
Member

zcorpan commented May 13, 2024

The PR was merged, closing.

@zcorpan zcorpan closed this as completed May 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Closed Accepted by CSSWG Resolution cssom-view-1
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
7 participants
@zcorpan @astearns @sanketj @xiaochengh @css-meeting-bot @mfreed7 @siliu1