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 IME bugs #738

Merged
merged 2 commits into from Jul 15, 2020
Merged

Fix IME bugs #738

merged 2 commits into from Jul 15, 2020

Conversation

ruiokada
Copy link
Contributor

@ruiokada ruiokada commented Jul 5, 2020

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 control keydowns (Arrows, Enter, Delete, 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 (specifically a space) 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. I initially thought using the null control character "\0" may be good in order to hide what is going on from the user, but this prepending of \0 gets recorded in mobiledoc editor's edit history. If the user then performs an undo, the null character will be in the text again and invisible to the user. I feel a space character is not too distracting and is noticeable in case the user does perform an undo.

The other solution for the second bug 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 modifies the DOM when receiving updates to the input of an IME. Chrome doesn't modify the DOM too much while other browsers add some extra nodes (see addendum). 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.

Tests

I thought maybe I can add a test for the Mac IME for special characters (see #696) via event triggers but it didn't work; Triggering the Option-E keypress/keydown event doesn't do anything since the desired input is a OS-level keyboard command. Instead I added some tests that roughly mimic how the IME modifies the DOM.

Addenda (Solving the root problem)

Each browser modifies the DOM differently when receiving updates from an IME. I will document the browser behaviors that I have observed that may be worth noting in the event that the renderer gets reworked someday. I also give a possible solution on dealing with the behaviors so that the Chrome solution can be used.

Safari

IME use on Safari has a strange behavior in that it tries to copy the style of some wrapping element. If I try to input "か" (ka) in Japanese with the mutation observer disabled I get:

  1. <p>[cursor]</p> (Blank line)
  2. <p>k</p> (Type "k")
  3. <p>か</p> (Type "a")
  4. <br> (Type "Enter", triggers input event "deleteCompositionText" for some reason)
  5. <font style="color:#fff"><span>か</span></font> (Occurs immediately after 4. Note that <p> is replaced by <font>)

#fff is the font color of <p>. If I remove this CSS style then at step 5 I just get か with no wrapping element. In either case, this messes up the renderer because of the mismatching section nodes. Safari does not follow this behavior if there is an initial character in <p>.

Firefox

The Chrome fix actually works in some sense for Firefox, but unfortunately it also has a strange behavior. When inputting "か" with the mutation observer disabled I get:

  1. <p>[cursor]</p> (Blank line)
  2. <p><br></p> (Type "k")
  3. <p><br></p> (Type "a")
  4. <p>か</p> (Type "Enter")

The problem with this is that you can't see what you're typing between IME initialization and termination. This behavior also ceases when there's an initial character in <p>.

How this can be dealt with

I don't know why Firefox/Safari tries to modify the DOM in this way, but this behavior can be dealt with if the <br> element introduced by cursorElement is removed. Applying the following changes gets rid of the IME bug:

  1. Have section nodes (p, ul, etc.) be contenteditable rather than giving the editor div contenteditable.
  2. Remove the cursorElement.
  3. Apply the Chrome solution in turning off the mutation observer when composing.

I'm hesitant about applying this solution because I'm not too familiar with the code base nor with the UX intentions. But I would be more than happy to give it a try if I can get some background on some of the code (mostly the purpose of cursorElement)

ruiokada added 2 commits Jul 5, 2020
* Suppress mutations when using an IME on a blank line
* Ignore control keydowns when IME is active
* Handle carriage return keypresses
@ZeeJab
Copy link
Member

@ZeeJab ZeeJab commented Jul 8, 2020

Great! Thanks. I'll take a closer look next week to see if we can get it in the next release.

@ZeeJab
Copy link
Member

@ZeeJab ZeeJab commented Jul 15, 2020

Thanks again! I'll get this merged in and when I'm done with the typescript conversion we'll be cutting a new release. We can sync up after on the more invasive refactors you proposed in the addenda.

@ZeeJab ZeeJab merged commit 5211b9a into bustle:master Jul 15, 2020
1 check passed
This was referenced Jul 15, 2020
@ruiokada
Copy link
Contributor Author

@ruiokada ruiokada commented Jul 15, 2020

Great! Thanks.

@a180285
Copy link

@a180285 a180285 commented Aug 22, 2020

I'm new to Ghost, I found the IME issue and find here.

I think it's really a bad experience when typing Chinese in default Ghost editor.

So if @ZeeJab you can release this fix earlier I will be very appreciate. Even a bug fix patch release will help a lot. Thanks.

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.

3 participants