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

(feat): Composition support with keyboard.imeSetComposition, keyboard.imeCancelComposition and keyboard.imeCommitComposition #7487

Closed
wants to merge 9 commits into from

Conversation

t-javierc
Copy link

Implemented composition support.

Related feature thread: #5777

Chromium changes are currently in review: https://chromium-review.googlesource.com/c/chromium/src/+/3009937

@JoelEinbinder
Copy link
Contributor

Thanks for the PR!

The api here is going to take some work to get right. Aside from nits like camel casing and api names, we need to have a better sense of how user tests should look. I want to reduce the size of this api to make it easier for us to manage and for users to use correctly. For example I don't think we need delay.

The point of triggering_key seems to be to send a keypress without the associated text. Maybe we can leave this out of the api for now if its not essential? Or if it is essential, can we help the users with what key they should actually put here?

Can you send some examples of real world tests that use this api? If we can't write a test that uses selection/delay etc, lets leave that out of the api for now.

@dgozman

@JoelEinbinder
Copy link
Contributor

Let's remove the triggering_key part of this patch in favor of this patch which does options for keyboard.down. In the OS apis for IME the keyboard events and IME events are not associated, so we don't need to associate them here. Anyone using this api is doing something pretty advanced, so we can prioritize correctness over convenience.

@dgozman
Copy link
Contributor

dgozman commented Jul 21, 2021

Here is a proposed API draft. Assumes #7659.

// Compose
await page.keyboard.down('s', { eventKey: 's', text: '' });  // Optional
await page.keyboard.up('s', { eventKey: 's' });  // Optional
await page.keyboard.imeSetComposition({ text: 's', selectionStart: 1, selectionEnd: 1 });
await page.keyboard.down('u', { eventKey: 'す', text: '' });  // Optional
await page.keyboard.up('u', { eventKey: 'す' });  // Optional
await page.keyboard.imeSetComposition({ text: 'す', selectionStart: 1, selectionEnd: 1, replacementStart: 0, replacementEnd: 1 });

// Commit
await page.keyboard.insertText('す');

// Cancel
await page.keyboard.down('Escape');  // Optional
await page.keyboard.up('Escape');  // Optional
await page.keyboard.insertText('');

Overall:

  • Drop triggering_key in favor of keybpard.down options.
  • Remove delay since we don't have usecases for it.
  • Use keyboard.insertText() to commit/cancel.

@trueadm
Copy link

trueadm commented Sep 27, 2021

@JoelEinbinder has there been any more traction on this? I was wondering if this feature was still under development.

@trueadm
Copy link

trueadm commented Oct 15, 2021

Hey, @t-javierc are you still working on this PR? I'd be happy to help with it, or continue it if you have not?

@trueadm
Copy link

trueadm commented Oct 15, 2021

I followed up with a patched version of this PR, rebased with changes from main. #9535

@alexkeng
Copy link

Hi! @trueadm, thanks for working on this! t-javierc has finished his internship at MS and no longer working on this, but this work for IME testing is still in our (Edge Editing team's) backlog. please let us know if you have any feedback on the design, and we can discuss further. thanks!

@trueadm
Copy link

trueadm commented Oct 18, 2021

@alexkeng Thanks for letting me know. It would be great if we could maybe chat and talk about the design and also this as a general feature. We're building a new text editor at Facebook (we hope to open source it soon) that already powers many parts of Facebook. IME automated testing is going to play a very important role and it would be awesome if we could sync up. My email is domgan [at] fb.com if you want to talk more.

@aslushnikov
Copy link
Collaborator

Closing this in favor of #9535

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.

6 participants