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

browser(firefox): Page.setComposition #9921

Closed
wants to merge 2 commits into from

Conversation

trueadm
Copy link

@trueadm trueadm commented Nov 1, 2021

Extracted the Firefox browser patches changes from #9535.

browser_patches/firefox-beta/BUILD_NUMBER Outdated Show resolved Hide resolved
browser_patches/firefox/BUILD_NUMBER Outdated Show resolved Hide resolved
browser_patches/firefox/juggler/protocol/Protocol.js Outdated Show resolved Hide resolved
@trueadm trueadm force-pushed the ff-setComposition branch 4 times, most recently from efa1aa1 to 6a3d326 Compare November 1, 2021 17:19
@trueadm trueadm force-pushed the ff-setComposition branch 3 times, most recently from bc0d444 to f5afd61 Compare November 1, 2021 17:24
} else {
const selection = window.getSelection();
const anchorNode = selection.anchorNode;
selection.setBaseAndExtent(anchorNode, replacementStart, anchorNode, replacementEnd);
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't look quite right. Aside from the shadow dom issues, the macOS api this is modeled after expects this range to be in terms of the current root editable node. However your code is in terms of the leaf editable node.

You can see this code in TextInputHandler.mm : IMEInputHandler::SetMarkedText, which ends up in the web process as ContentEventHandler::OnSelectionEvent. I am working on a patch to fix this but I'm not sure if I should duplicate the logic in JavaScript or add a hack for us to call the c++ code directly.

@mxschmitt mxschmitt requested review from mxschmitt and removed request for mxschmitt February 3, 2022 22:25
@mxschmitt mxschmitt removed their request for review February 14, 2022 21:52
@pavelfeldman
Copy link
Member

Closing due to no progress (on our side). I'm not sure what the action for this should be, but we aren't considering adding composition API at this point.

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