guardian / scribe

DEPRECATED: A rich text editor framework for the web platform
http://guardian.github.io/scribe/
Apache License 2.0
3.51k stars 245 forks source link

Typing backspace terminates the composition of IME for Chinese/Japanese #422

Open NdYAG opened 9 years ago

NdYAG commented 9 years ago

When I was typing Chinese and Japanese in scribe, I found that code in patches/events terminates the IME's composition in Chrome.

Typing CJK(Chinese/Japanese/Korean) characters need the help of IME, which buffers the user's keystrokes until a character or word has been made.The first line in the following example shows how IME works:

scribe

In the above example, I am trying to type sekai in Japanese. If I happen to type segai (in second line), I will press backspace to leave se there, then type kai to correct myself. Problem comes. The time I pressed backspace, the buffered keystrokes were lost(though they might still visually remain on the screen), leaving me no chance to continue from the last se I've already typed.

せ(se) か(ka) い(i)
せ(se) が(ga) い(i)

During the composition, keyup/keydown are fired in Chrome(reference: https://w3c.github.io/uievents/#events-composition-event-key-events). I guess placeMarkers and selectMarkers, which mutate the DOM, make the browser discard the buffers. By commenting these two lines in patches/events, composition is not influenced.

Removing these two lines may not be good solution. I'm thinking of a isComposing polyfill. If is composing, then we do not need to check whether the user is pressing backspace or not.


After:

scribe_correct

Now in the second line, after deleting gai, I could type kai right after se.

NdYAG commented 9 years ago

The solution is simple...

if (scribe.allowsBlockElements()) {
    var isComposing = false
    scribe.el.addEventListener('compositionstart', function(event) {
        isComposing = true
    })
    scribe.el.addEventListener('compositionend', function(event) {
        isComposing = false
    })
    scribe.el.addEventListener('keyup', function (event) {
        if (isComposing) { return }
        // code continues...

I could make a pull request if you are willing to accept... composition related events are not fired if you type in English/French/... which do not need composition.

1shiharat commented 9 years ago

+1 for these changes.

rrees commented 9 years ago

We could look at a pull request and related tests, I'd like to accept it but without an update to the test suite I think we'd be liable to regressions.

danburzo commented 8 years ago

(Possibly related: https://github.com/guardian/scribe/issues/345)

danburzo commented 8 years ago

I've tried working on a general solution for this, here are my conclusions:

My first attempt, with the idea of bypassing regressions altogether, was to create a plugin with which you could handle composition events. Something like:

  return function () {
    return function (scribe) {

      var composing = false;

      scribe.el.addEventListener('compositionstart', function() {
        composing = true;
      });

      scribe.el.addEventListener('compositionend', function() {
        composing = false;
      });

      function allowComposition(event) {
        if (composing) {
          event.stopImmediatePropagation();
        }
      };

      scribe.el.addEventListener('keyup', allowComposition, true);
      scribe.el.addEventListener('keydown', allowComposition, true);
      scribe.el.addEventListener('input', allowComposition, true);

    };
  };

});

This does the trick of preventing the problematic keyup handle from firing when we're composing. However, there are a few problems with this approach:

  1. It effectively disables any other potential listener on keyup/keydown/input events while you're composing; this might break the behavior in the wild if not used carefully.
  2. It's not able to stop the input handler:
this.el.addEventListener('input', function () {
      console.log('input');
      /**
       * This event triggers when either the user types something or a native
       * command is executed which causes the content to change (i.e.
       * `document.execCommand('bold')`). We can't wrap a transaction around
       * these actions, so instead we run the transaction in this event.
       */
      this.transactionManager.run();
    }.bind(this), false);

...since this event listener is attached before we have a chance of initializing the plugin. And I intuit the transaction manager should also be disabled while composing?

The other approach, which needs to be tested more thoroughly for regressions, would be to have the composition state as a property in the scribe object, so that any plugin (including the core ones) can take their own decisions based on this flag. That would entail changes on all the keyboard/input handlers throughout Scribe.

Any other leads?

parkhw00 commented 6 years ago

Any other progress on this issue??