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] add shadowRoots parameter to caretPositionFromPoint API. #10200

Merged
merged 6 commits into from
May 3, 2024

Conversation

siliu1
Copy link
Contributor

@siliu1 siliu1 commented Apr 11, 2024

Per #9932, add shadowRoots parameter to caretPositionFromPoint API.

Implementation bugs:
https://bugzilla.mozilla.org/show_bug.cgi?id=1889503
https://issues.chromium.org/issues/332774620
https://bugs.webkit.org/show_bug.cgi?id=172137

WPT test coverage will be added subsequently.

Per w3c#9932, add `shadowRoots` parameter to `caretPositionFromPoint` API.
@siliu1 siliu1 changed the title Update Overview.bs [cssom-view] add shadowRoots parameter to caretPositionFromPoint API. Apr 11, 2024
@siliu1 siliu1 marked this pull request as ready for review April 11, 2024 21:07
cssom-view-1/Overview.bs Outdated Show resolved Hide resolved
Bubble `caret range` out of the shadow roots.
@siliu1 siliu1 requested a review from dandclark April 16, 2024 21:36
cssom-view-1/Overview.bs Outdated Show resolved Hide resolved
Copy link
Contributor

@dandclark dandclark left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggested some refactoring/wording nits but functionally this LGTM.

cssom-view-1/Overview.bs Outdated Show resolved Hide resolved
cssom-view-1/Overview.bs Outdated Show resolved Hide resolved
Address PR comments.
@sanketj
Copy link
Member

sanketj commented Apr 18, 2024

nit: Could you add links to all the implementation bugs in the PR description?

cssom-view-1/Overview.bs Outdated Show resolved Hide resolved
cssom-view-1/Overview.bs Outdated Show resolved Hide resolved
cssom-view-1/Overview.bs Outdated Show resolved Hide resolved
cssom-view-1/Overview.bs Show resolved Hide resolved
Address PR comment.
@siliu1 siliu1 requested a review from sanketj April 18, 2024 21:47
@sanketj
Copy link
Member

sanketj commented Apr 18, 2024

nit: Could you add links to all the implementation bugs in the PR description?

Could you also add the link for the WebKit implementation bug? If one doesn't exist, we should file one.

cssom-view-1/Overview.bs Show resolved Hide resolved
cssom-view-1/Overview.bs Outdated Show resolved Hide resolved
@siliu1
Copy link
Contributor Author

siliu1 commented Apr 19, 2024

nit: Could you add links to all the implementation bugs in the PR description?

Could you also add the link for the WebKit implementation bug? If one doesn't exist, we should file one.

Added WebKit implementation bug for the API.

Address PR comment.
@siliu1 siliu1 requested a review from sanketj April 19, 2024 01:49
cssom-view-1/Overview.bs Show resolved Hide resolved
@siliu1 siliu1 requested a review from sanketj April 19, 2024 06:31
linked created issue(10230) to the spec.
@zcorpan
Copy link
Member

zcorpan commented May 3, 2024

As far as I can tell this works like in https://w3c.github.io/selection-api/#dom-selection-getcomposedranges as @mfreed7 suggested in #9932 (comment) and per the CSSWG resolution.

cc @emilio

@siliu1 siliu1 merged commit f0de230 into w3c:main May 3, 2024
1 check passed
Copy link
Collaborator

@emilio emilio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks generally fine.

@@ -1148,6 +1145,8 @@ aborting on the first step that returns a value:

Note: This {{DOMRect}} object is not <a spec=html>live</a>.

Issue(10230): Consider removing <a>caret range</a> concept from <dfn>caret position</dfn> interface.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This introduces a build error fwiw. caret position is defined twice.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix for that in #10285

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

Successfully merging this pull request may close these issues.

5 participants