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

Change getComposedRanges's shadowRoots parameter from rest parameter to a dictionary #176

Closed
siliu1 opened this issue May 17, 2024 · 10 comments · Fixed by #331
Closed

Change getComposedRanges's shadowRoots parameter from rest parameter to a dictionary #176

siliu1 opened this issue May 17, 2024 · 10 comments · Fixed by #331
Labels
Agenda+ Queue this item for discussion at the next WG meeting

Comments

@siliu1
Copy link

siliu1 commented May 17, 2024

Current spec of getComposedRanges API has single shadowRoots parameter which is a rest parameter. We should change it to a dictionary that contains an array of shadow roots. Similar to https://html.spec.whatwg.org/#gethtmloptions. It'd be good to be consistent.

Proposed change:

sequence<StaticRange> getComposedRanges(optional GetComposedRangesOptions options = {});

dictionary GetComposedRangesOptions {
  sequence<ShadowRoot> shadowRoots = [];
};
@siliu1
Copy link
Author

siliu1 commented May 17, 2024

@mfreed7 FYI.

@mfreed7
Copy link

mfreed7 commented May 17, 2024

@mfreed7 FYI.

LGTM! Thanks.

@smaug----
Copy link

The API has shipped already in webkit, right?

@sefeng211

@mfreed7
Copy link

mfreed7 commented May 20, 2024

I think that's a prototype implementation, though, right? The spec hasn't been reviewed by another implementer that I know of. Or perhaps Mozilla did?

Anyway, this seems reasonable. We will now have this pattern three places, getHTML(), caretPositionFromPoint(), and here with getComposedRanges() - it would be good if they all followed the same pattern.

@sefeng211
Copy link

Yeah it's shipped in Webkit. cc @rniwa

@dizhang168
Copy link
Member

Even though Webkit has shipped getComposedRanges, this question is still unanswered.
@rniwa @sefeng211 What do you think of this proposal?

@sefeng211
Copy link

Looks reasonable to me

@sanketj
Copy link
Member

sanketj commented Aug 7, 2024

@johanneswilm Can we add this to the agenda for the next Editing WG meeting? I don't have permissions to add the "Agenda+" label.

chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Aug 8, 2024
…ictionary

Per w3c/selection-api#176, we should update
selection.getComposedRanges() from using a rest `shadowRoots` parameter
to using a dictionary that contains the array `shadowRoots`.

Change-Id: I6b5b3fed786cf6d75fa4a20c0b2a583635cd2aca
Bug: 40286116
Fixed: 355577223
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Aug 8, 2024
…ictionary

Per w3c/selection-api#176, we should update
selection.getComposedRanges() from using a rest `shadowRoots` parameter
to using a dictionary that contains the array `shadowRoots`.

Change-Id: I6b5b3fed786cf6d75fa4a20c0b2a583635cd2aca
Bug: 40286116
Fixed: 355577223
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5770701
Commit-Queue: Di Zhang <dizhangg@chromium.org>
Reviewed-by: Siye Liu <siliu@microsoft.com>
Cr-Commit-Position: refs/heads/main@{#1338832}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Aug 8, 2024
…ictionary

Per w3c/selection-api#176, we should update
selection.getComposedRanges() from using a rest `shadowRoots` parameter
to using a dictionary that contains the array `shadowRoots`.

Change-Id: I6b5b3fed786cf6d75fa4a20c0b2a583635cd2aca
Bug: 40286116
Fixed: 355577223
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5770701
Commit-Queue: Di Zhang <dizhangg@chromium.org>
Reviewed-by: Siye Liu <siliu@microsoft.com>
Cr-Commit-Position: refs/heads/main@{#1338832}
@johanneswilm johanneswilm added the Agenda+ Queue this item for discussion at the next WG meeting label Aug 8, 2024
@whsieh
Copy link

whsieh commented Aug 8, 2024

This seems like a reasonable approach (it's similar to how focus() takes a parameter dictionary, as well as other APIs). Tagging a few more folks from Apple:

@rniwa @megangardner @annevk @marcoscaceres, any thoughts?

moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Aug 9, 2024
…() from rest parameter to a dictionary, a=testonly

Automatic update from web-platform-tests
[Selection API] Change getComposedRanges() from rest parameter to a dictionary

Per w3c/selection-api#176, we should update
selection.getComposedRanges() from using a rest `shadowRoots` parameter
to using a dictionary that contains the array `shadowRoots`.

Change-Id: I6b5b3fed786cf6d75fa4a20c0b2a583635cd2aca
Bug: 40286116
Fixed: 355577223
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5770701
Commit-Queue: Di Zhang <dizhangg@chromium.org>
Reviewed-by: Siye Liu <siliu@microsoft.com>
Cr-Commit-Position: refs/heads/main@{#1338832}

--

wpt-commits: 57e20404dde0cc210bbedfaf1955bad34decb16b
wpt-pr: 47523
i3roly pushed a commit to i3roly/firefox-dynasty that referenced this issue Aug 12, 2024
…() from rest parameter to a dictionary, a=testonly

Automatic update from web-platform-tests
[Selection API] Change getComposedRanges() from rest parameter to a dictionary

Per w3c/selection-api#176, we should update
selection.getComposedRanges() from using a rest `shadowRoots` parameter
to using a dictionary that contains the array `shadowRoots`.

Change-Id: I6b5b3fed786cf6d75fa4a20c0b2a583635cd2aca
Bug: 40286116
Fixed: 355577223
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5770701
Commit-Queue: Di Zhang <dizhangg@chromium.org>
Reviewed-by: Siye Liu <siliu@microsoft.com>
Cr-Commit-Position: refs/heads/main@{#1338832}

--

wpt-commits: 57e20404dde0cc210bbedfaf1955bad34decb16b
wpt-pr: 47523
ErichDonGubler pushed a commit to erichdongubler-mozilla/firefox that referenced this issue Aug 19, 2024
…() from rest parameter to a dictionary, a=testonly

Automatic update from web-platform-tests
[Selection API] Change getComposedRanges() from rest parameter to a dictionary

Per w3c/selection-api#176, we should update
selection.getComposedRanges() from using a rest `shadowRoots` parameter
to using a dictionary that contains the array `shadowRoots`.

Change-Id: I6b5b3fed786cf6d75fa4a20c0b2a583635cd2aca
Bug: 40286116
Fixed: 355577223
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5770701
Commit-Queue: Di Zhang <dizhangg@chromium.org>
Reviewed-by: Siye Liu <siliu@microsoft.com>
Cr-Commit-Position: refs/heads/main@{#1338832}

--

wpt-commits: 57e20404dde0cc210bbedfaf1955bad34decb16b
wpt-pr: 47523
@rniwa
Copy link
Contributor

rniwa commented Sep 19, 2024

That seems reasonable although it's possible we'd hit some web compatibility issues given we've already shipped this API.

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

Successfully merging a pull request may close this issue.

9 participants