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

[cssom-view] Change document.caretPositionFromPoint's shadowRoots parameter from rest parameter to an array #10345

Open
siliu1 opened this issue May 16, 2024 · 9 comments

Comments

@siliu1
Copy link
Contributor

siliu1 commented May 16, 2024

The shadowRoots parameter is defined in the spec as a rest parameter of type ShadowRoot.

We should change it from rest parameter to a single array of shadow roots. Similar to https://html.spec.whatwg.org/#gethtmloptions. It'd be good to be consistent.

@siliu1 siliu1 changed the title [cssom-view] Change document.caretPositionFromPoint's shadowRoots parameter from rest parameter to an array. [cssom-view] Change document.caretPositionFromPoint's shadowRoots parameter from rest parameter to an array May 16, 2024
@siliu1
Copy link
Contributor Author

siliu1 commented May 16, 2024

@mfreed7 FYI I opened this issue to update the spec for document.caretPositionFromPoint to take a single array of shadow roots instead of using a rest parameter.

@siliu1
Copy link
Contributor Author

siliu1 commented May 16, 2024

@annevk @zcorpan @smaug---- FYI.

@annevk
Copy link
Member

annevk commented May 17, 2024

Doesn't seem unreasonable, but it might be even better to make it a dictionary argument so we can add more arguments there in the future.

I probably wouldn't go as far as https://w3ctag.github.io/design-principles/#prefer-dictionaries and also make the mandatory arguments part of the dictionary as the surrounding methods don't match that pattern and "x, y" is pretty established, but for the third argument it seems like a good idea.

@mfreed7
Copy link

mfreed7 commented May 17, 2024

@mfreed7 FYI I opened this issue to update the spec for document.caretPositionFromPoint to take a single array of shadow roots instead of using a rest parameter.

+1. That aligns with getHTML() and also allows additional arguments to be added later.

Doesn't seem unreasonable, but it might be even better to make it a dictionary argument so we can add more arguments there in the future.

+1 to that also. It makes the name of that argument explicit, which is nice, and allows additional arguments in an even better way.

By the way, I think we should make the same change to getComposedRanges() for all of the same reasons.

@siliu1
Copy link
Contributor Author

siliu1 commented May 17, 2024

Proposed solution: Put the array of shadow roots into a dictionary which is an argument of document.caretPositionFromPoint API.

document.caretPositionFromPoint(double x, double y, optional CaretPositionFromPointOptions options = {});

dictionary CaretPositionFromPointOptions {
  sequence<ShadowRoot> shadowRoots = [];
};

@siliu1
Copy link
Contributor Author

siliu1 commented May 17, 2024

@mfreed7 FYI I opened this issue to update the spec for document.caretPositionFromPoint to take a single array of shadow roots instead of using a rest parameter.

+1. That aligns with getHTML() and also allows additional arguments to be added later.

Doesn't seem unreasonable, but it might be even better to make it a dictionary argument so we can add more arguments there in the future.

+1 to that also. It makes the name of that argument explicit, which is nice, and allows additional arguments in an even better way.

By the way, I think we should make the same change to getComposedRanges() for all of the same reasons.

Filed w3c/selection-api#176

@siliu1
Copy link
Contributor Author

siliu1 commented May 20, 2024

Proposed solution: Put the array of shadow roots into a dictionary which is an argument of document.caretPositionFromPoint API.

document.caretPositionFromPoint(double x, double y, optional CaretPositionFromPointOptions options = {});

dictionary CaretPositionFromPointOptions {
  sequence<ShadowRoot> shadowRoots = [];
};

@astearns Can we get to resolution of this issue asynchronously with the proposed solution? Thanks!

@astearns astearns added the Async Resolution: Proposed Candidate for auto-resolve with stated time limit label May 20, 2024
@astearns
Copy link
Member

The CSSWG will automatically accept this resolution one week from now if no objections are raised here. Anyone can add an emoji to this comment to express support. If you do not support this resolution, please add a new comment.

Proposed Resolution: Add an array of shadow roots to a dictionary which is an argument of the document.caretPositionFromPoint API as described in #10345 (comment)

@astearns
Copy link
Member

RESOLVED: Add an array of shadow roots to a dictionary which is an argument of the document.caretPositionFromPoint API as described in #10345 (comment)

@astearns astearns added Needs Edits and removed Async Resolution: Call For Consensus Resolution will be called after time limit expires labels May 28, 2024
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue May 29, 2024
Per w3c/csswg-drafts#10345, we should change
`shadowRoots` parameter from a rest parameter to a dictionary which
contains an array of shadow roots.

Bug: 41487363
Change-Id: I0686d1eae75797ae07cad215a71a24ab54c7398d
aarongable pushed a commit to chromium/chromium that referenced this issue May 30, 2024
Per w3c/csswg-drafts#10345, we should change
`shadowRoots` parameter from a rest parameter to a dictionary which
contains an array of shadow roots.

Bug: 41487363
Change-Id: I0686d1eae75797ae07cad215a71a24ab54c7398d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5553027
Reviewed-by: Mason Freed <masonf@chromium.org>
Reviewed-by: Koji Ishii <kojii@chromium.org>
Reviewed-by: Sanket Joshi <sajos@microsoft.com>
Commit-Queue: Siye Liu <siliu@microsoft.com>
Cr-Commit-Position: refs/heads/main@{#1308173}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue May 30, 2024
Per w3c/csswg-drafts#10345, we should change
`shadowRoots` parameter from a rest parameter to a dictionary which
contains an array of shadow roots.

Bug: 41487363
Change-Id: I0686d1eae75797ae07cad215a71a24ab54c7398d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5553027
Reviewed-by: Mason Freed <masonf@chromium.org>
Reviewed-by: Koji Ishii <kojii@chromium.org>
Reviewed-by: Sanket Joshi <sajos@microsoft.com>
Commit-Queue: Siye Liu <siliu@microsoft.com>
Cr-Commit-Position: refs/heads/main@{#1308173}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue May 30, 2024
Per w3c/csswg-drafts#10345, we should change
`shadowRoots` parameter from a rest parameter to a dictionary which
contains an array of shadow roots.

Bug: 41487363
Change-Id: I0686d1eae75797ae07cad215a71a24ab54c7398d
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/5553027
Reviewed-by: Mason Freed <masonf@chromium.org>
Reviewed-by: Koji Ishii <kojii@chromium.org>
Reviewed-by: Sanket Joshi <sajos@microsoft.com>
Commit-Queue: Siye Liu <siliu@microsoft.com>
Cr-Commit-Position: refs/heads/main@{#1308173}
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