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

Composition event doesn't trigger correctly for IME #24724

Open
garasubo opened this issue Nov 13, 2019 · 5 comments
Open

Composition event doesn't trigger correctly for IME #24724

garasubo opened this issue Nov 13, 2019 · 5 comments

Comments

@garasubo
Copy link
Contributor

@garasubo garasubo commented Nov 13, 2019

In #22346 , I found that composition start evnet doesn't trigger when I started to input something in IME. My test environment was Ubuntu 18.04 with fcitx IME.
Ref: https://developer.mozilla.org/en-US/docs/Web/API/Element/compositionstart_event

The keyboard event itself comes from winit library. I also found some of issues mentioning about IME. So, the root cause might be winit library itself.

@garasubo garasubo mentioned this issue Nov 13, 2019
3 of 5 tasks complete
@nox nox changed the title Compostion evnet doesn't trigger correctly for IME Compostion event doesn't trigger correctly for IME Nov 13, 2019
@nox nox changed the title Compostion event doesn't trigger correctly for IME Composition event doesn't trigger correctly for IME Nov 13, 2019
@garasubo
Copy link
Contributor Author

@garasubo garasubo commented Nov 15, 2019

Found this commit
b936fea

In this commit message, it clearly said "CompositionEvents currently can only be created by WebDriver and not by embedders."
So, we need to implement the logic to create CompositionEvents for embedders.

To handle composition events correctly, we need to know the current character status of IME editor, but it seems winit sends only key release event and this event doesn't contain the status of IME (at least my Linux x11 environment. I checked the event interface, but looks like there is no interface for it). I think we should implement IME event handler in winit first.

@pshaughn
Copy link
Member

@pshaughn pshaughn commented Dec 5, 2019

Relevant to this, document.createEvent currently isn't supporting the "CompositionEvent" or "TextEvent" strings. WPT dom/nodes/Document-createEvent.https.html expects it to so it can make mock CompositionEvents, and I imagine webapps might themselves use mock CompositionEvents for in-app touchscreen input methods.

@garasubo
Copy link
Contributor Author

@garasubo garasubo commented Dec 6, 2019

In rust-windowing/winit#753, they are discussing to introduce a new keyboard input model. This would be a breaking change, but could resolve those kind of keyboard events problems. However, the discussion seems to paused for a while.

@garasubo
Copy link
Contributor Author

@garasubo garasubo commented Dec 6, 2019

@pshaughn I checked that part, but it is not directly related to my problem. Actually, there are some components to handle CompositionEvent in servo itself. But the problem here is we cannot generate composition event from hardware events since abstracted hardware events which are generated by dependent library (winit) loses information which need to know to trigger composition events.

bors-servo added a commit that referenced this issue Dec 8, 2019
implement composition event creation in Document.createEvent

Solved the problem entioned in #24724 (comment)

Add logic to create composition event for Document.createEvent. This resolved some WPT test failure, so updated the metadata as well

Ref: https://developer.mozilla.org/en-US/docs/Web/API/Document/createEvent

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [ ] These changes fix #___ (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
bors-servo added a commit that referenced this issue Dec 11, 2019
implement composition event creation in Document.createEvent

Solved the problem mentioned in #24724 (comment)

Add logic to create composition event for Document.createEvent. This resolved some WPT test failure, so updated the metadata as well

Ref: https://developer.mozilla.org/en-US/docs/Web/API/Document/createEvent

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [ ] These changes fix #___ (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
bors-servo added a commit that referenced this issue Dec 12, 2019
implement composition event creation in Document.createEvent

Solved the problem mentioned in #24724 (comment)

Add logic to create composition event for Document.createEvent. This resolved some WPT test failure, so updated the metadata as well

Ref: https://developer.mozilla.org/en-US/docs/Web/API/Document/createEvent

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [ ] These changes fix #___ (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
bors-servo added a commit that referenced this issue Dec 12, 2019
implement composition event creation in Document.createEvent

Solved the problem mentioned in #24724 (comment)

Add logic to create composition event for Document.createEvent. This resolved some WPT test failure, so updated the metadata as well

Ref: https://developer.mozilla.org/en-US/docs/Web/API/Document/createEvent

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [ ] These changes fix #___ (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [ ] These changes do not require tests because ___

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
@garasubo
Copy link
Contributor Author

@garasubo garasubo commented Jan 25, 2020

I submitted a PR to winit to support composition event in X11 environment rust-windowing/winit#1404
After that is merged, we can receive preedit string from the library.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.