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

fix(itext): fix bug that IME can not be inputted #2820

Merged
merged 2 commits into from
Mar 29, 2016

Conversation

abruzzihraig
Copy link
Contributor

Fix #2473

Fabric.js does not support IME input, it is looks weird especially in Chinese input.

The reason is fabric just listen the event oninput, and calculate a diff between the last two text, then inserts those new chars. The key is It doesn't have a reducing logic here.

For example, when you type Chinese text with any IME, the previous result may looks like xxxnanrenwangxxx, but the next you will got xxx男人王xxx after entered a return button. Note the length of the chars reduce from 16 to 9, but there is no reducing logic in onInput handler.

So I use the events compositionstart and compositionend to check if users are using IME and prevent any insertion before they ended IME typing.

@kangax
Copy link
Member

kangax commented Mar 1, 2016

LGTM

@asturur
Copy link
Member

asturur commented Mar 1, 2016

Looks good also to me. It could avoid to switch all the text input as planned. Can we pull after 1.6.0 release? I would be scared of another 0 day bug after release.

@asturur
Copy link
Member

asturur commented Mar 1, 2016

@abruzzihraig can you fix the lint errors? ( variable e declared but not used in the events you added ).

@asturur asturur mentioned this pull request Mar 28, 2016
@asturur
Copy link
Member

asturur commented Mar 29, 2016

Proven to be usefull also for managing accents on some configurations.

@asturur
Copy link
Member

asturur commented Mar 29, 2016

I leave a note: check if thos compositioncheck is also usefull when using arrows on some japanes input methods

@asturur asturur merged commit a28651e into fabricjs:master Mar 29, 2016
@astw
Copy link

astw commented Jun 16, 2016

Is this fix included 1.6.0 release? I still can see this issue in http://fabricjs.com/kitchensink
Or the demo is not updated?

@asturur
Copy link
Member

asturur commented Jun 16, 2016

something brougth the issue back. i hope on the weekend i can clear something out

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.

4 participants