bustle / mobiledoc-kit

A toolkit for building WYSIWYG editors with Mobiledoc
https://bustle.github.io/mobiledoc-kit/demo/
MIT License
1.55k stars 150 forks source link

Fix IME bugs #736

Closed ruiokada closed 4 years ago

ruiokada commented 4 years ago

I think this pull request should fix #548 and #696. I have tested on macOS Catalina with Brave, Chrome, Firefox, Opera, Safari, and Edge and on iOS with Safari. I would appreciate tests on other devices. I will explain my observations on why these bugs occur and how my solution addresses them.

Problem

There are two separate bugs. One is that IME inputs are duplicated when when pressing Enter (to terminate IME input) and the other is that the IME's input gets modified when inputting on a blank line. The first bug occurs because the Mobiledoc Editor interprets an Enter when using an IME as the same as inputing Enter. As the IME input is buffered, the Mobiledoc Editor cursor is at the position where the IME input begins and when Enter is inputted handleNewline is called (which duplicates the text on the new line). The second bug occurs because of the absence of a markup node in the editor on a blank line. Because of the absence of a markup node and the IME buffering, this causes the Mutation handler to modify the editor DOM which in turn modifies the initial IME input.

Solution

The first bug can be fixed by ignoring the uses of Enter/Tab when an IME is being used. The second bug can be fixed by getting rid of the offending mutations. One way to do this is to prepend a "dummy" character (I used the null control character) to blank lines when composestart is fired and deleting this character when composeend is fired. This is treated as a the user typing an input (creating a markup node) which prevents the mutation. Another is to stop listening for mutations when composestart is fired and restart listening when composeend is fired. For most browsers, the first solution works and for Chrome-based browsers the second solutions works. This is mostly due to how each browsers manages the order of the events composingend and keydown. Chrome triggers composingend after a keypress event (like delete, enter) to terminate the IME, but most browsers trigger the keypress after composingend which causes problems. Thus the solution I came up with is to use both depending on the browser being used.

Asides

When inputting accented characters on Safari via the IME, the Enter keypress event is interpreted as a carriage return "\r" and so I have made it so these keypresses are handled as editor newlines instead.

Other Possible Fixes

I tested #704, but the fix seems to work only for Chrome-based browsers. The code modifications seem simple but I don't quite understand what it does to fix the issue. It may be good to see if this fix can be integrated into mine to fix Chrome at least. #661 seems to be a fix for the first bug.

ZeeJab commented 4 years ago

Thank you @ruiokada for the PR and the thorough writeup of the root causes. This made the problem very clear and easy to understand.

This looks great and we'd like to get it in the next release. Is it possible to include some tests to ensure we don't accidentally regress this behavior in the future? It should be possible to write browser-specific tests if needed (by guarding on Browser.isChrome etc.).

Thanks again for the awesome work! 😃

ruiokada commented 4 years ago

@ZeeJab Thanks for the feedback. I will see if I can add some automated testing for IME input, but I think this might be quite challenging since the IME is an OS-level function.

Also, I just realized that the solution for "other browsers" messes up the edit history of the editor (the input/deletion of \0 gets recorded). Do you know if there's a way to edit content without having the action recorded? If not, I will see if there's a way to the modify the Chrome solution to fit other browsers.

ZeeJab commented 4 years ago

@ruiokada I'm not aware of anything that would allow edit content to "not be recorded". May be worth an investigation. Or yea, if you can see how to modify the Chrome solution to fit other browsers.

ruiokada commented 4 years ago

@ZeeJab Unfortunately I can't resolve some of the issues brought up:

I probably should have made a new branch for this PR to begin with so I will be closing this PR and opening one on a new branch to make merging less of a hassle. I'll repost everything there with all the updated information.