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

RichText: Don't update live DOM on composition #11908

Merged
merged 4 commits into from Nov 15, 2018
Merged

Conversation

@ellatrix
Copy link
Member

@ellatrix ellatrix commented Nov 15, 2018

Description

See #11813 and #11795. This is just an attempt at fixing the issue.

How has this been tested?

Screenshots

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
Copy link
Contributor

@youknowriad youknowriad left a comment

Approving because it seems to fix the issue https://wordpress.slack.com/archives/C02QB2JS7/p1542286893546800

@youknowriad youknowriad added this to the 4.4 milestone Nov 15, 2018
@torounit
Copy link
Member

@torounit torounit commented Nov 15, 2018

@iseulde Tested this branch.
It is no longer decided automatically.

But another problem.
Input Japanese in Paragraph Block, after input delete, all characters deleted.

@ellatrix
Copy link
Member Author

@ellatrix ellatrix commented Nov 15, 2018

@torounit Thanks, I'll investigate further!

@yoavf
Copy link
Contributor

@yoavf yoavf commented Nov 15, 2018

Tested this by installing Japanese on my mac and setting it up like described here. If not using qwerty, make sure to also change the Romanji layout option.

Test:

  • In a post add a list block
  • Switch to Hiranga in the typing menu
  • start tying the characters nihonn
  • You should see it converted to にほん

Before:
screen capture on 2018-11-15 at 15-29-13

After:
screen capture on 2018-11-15 at 15-30-44

@torounit
Copy link
Member

@torounit torounit commented Nov 15, 2018

@iseulde

One character should be deleted, but all characters are deleted.

output

@ellatrix
Copy link
Member Author

@ellatrix ellatrix commented Nov 15, 2018

@torounit Does the deletion problem also happen before in 4.2, or was it okay back then? @yoavf Do you see the same problem?

@youknowriad
Copy link
Contributor

@youknowriad youknowriad commented Nov 15, 2018

From the code change, I don't think the deletion bug is an issue with the current PR

@ellatrix ellatrix force-pushed the try/composing-input branch from 689bee7 to 28b7f01 Nov 15, 2018
@yoavf
Copy link
Contributor

@yoavf yoavf commented Nov 15, 2018

@yoavf Do you see the same problem?

Works fine for me:

screen capture on 2018-11-15 at 15-45-54

I'm on a mac, using Hiragana. @torounit what are you using?

@torounit
Copy link
Member

@torounit torounit commented Nov 15, 2018

@iseulde
the deletion problem not happen in 4.2, 4.3 and master. Only This Branch.

@yoavf
I'm on a Mojave. using Hiragana.

This problem does not occur when space or alphabet is mixed.

@ellatrix
Copy link
Member Author

@ellatrix ellatrix commented Nov 15, 2018

@torounit It would be good to have the sequence of events, e.g. using the tool shared by @ocean90: https://input-inspector.now.sh/.

@torounit
Copy link
Member

@torounit torounit commented Nov 15, 2018

@torounit
Copy link
Member

@torounit torounit commented Nov 15, 2018

on Firefox, the deletion problem not happen.

@ellatrix
Copy link
Member Author

@ellatrix ellatrix commented Nov 15, 2018

@torounit To be sure, this is on Mojave and Chrome? Do you use the native IME or something else?

@torounit
Copy link
Member

@torounit torounit commented Nov 15, 2018

@iseulde 
Result of input 『わたしは』after type delete 4times.

Chrome: https://input-inspector.now.sh/profiles/JlVBqRhcZWtZxWvLeKFc
Firefox: https://input-inspector.now.sh/profiles/FyxPjxZJRDEeNKBN3D9g

This is on Mojave and Chrome 70. This problem occur native IME(Hiragana)and Other IME (ATOK).
I will try Google Japanese input (mozc).

@ellatrix
Copy link
Member Author

@ellatrix ellatrix commented Nov 15, 2018

@torounit Could you try with 166ebd8? Thanks for you patience! :)

@ellatrix
Copy link
Member Author

@ellatrix ellatrix commented Nov 15, 2018

If that doesn't work I'll try 2 more things. :)

@torounit
Copy link
Member

@torounit torounit commented Nov 15, 2018

@iseulde 166ebd8 dosen't work. All characters deleted.

@ellatrix ellatrix force-pushed the try/composing-input branch from 166ebd8 to 9110d08 Nov 15, 2018
@atachibana
Copy link
Contributor

@atachibana atachibana commented Nov 15, 2018

FYI.
Test on Windows 10 Japanese without 166ebd8.
The same error of clearing entire block by one hit of BackSpace key occurred on Windows with both Google IME and MS IME.
Edge was OK for both IMEs.

Environment
Windows 10 Pro Japanese
Google IME and/or MS-IME
Chrome 70.0.3538.77
Edge 42.17134.1.0

@ellatrix
Copy link
Member Author

@ellatrix ellatrix commented Nov 15, 2018

Thanks! @torounit @atachibana Could you test again with 9110d08? :)

@torounit
Copy link
Member

@torounit torounit commented Nov 15, 2018

@iseulde 9110d08 dosen't work. :(

@ellatrix
Copy link
Member Author

@ellatrix ellatrix commented Nov 15, 2018

@torounit Thanks! Let me try one more thing. :) Also, why is "Enter" pressed in Chrome but not in Firefox?

@ellatrix ellatrix force-pushed the try/composing-input branch from 9110d08 to 4e060d1 Nov 15, 2018
@torounit
Copy link
Member

@torounit torounit commented Nov 15, 2018

@iseulde
"Enter" pressed both browser. but It is not recorded on Firefox.

safari: https://input-inspector.now.sh/profiles/2DDzBa29IEDb7IxmszZ9

@ellatrix
Copy link
Member Author

@ellatrix ellatrix commented Nov 15, 2018

@torounit Last try: 4e060d1.

@torounit
Copy link
Member

@torounit torounit commented Nov 15, 2018

@iseulde it works. in Chrome 70 on Mojave. 👍
I will try other browser.

@ellatrix
Copy link
Member Author

@ellatrix ellatrix commented Nov 15, 2018

@torounit Oh, great!!

@youknowriad
Copy link
Contributor

@youknowriad youknowriad commented Nov 15, 2018

Awesome collaboration work on this PR 👏

@ellatrix ellatrix changed the title FOR TESTING: RichText: Don't trigger a change on composition RichText: Don't update live DOM on composition Nov 15, 2018
@torounit
Copy link
Member

@torounit torounit commented Nov 15, 2018

@iseulde Thanks !
Works properly on Chome 70 / Firefox 63 / Safari 12 on Mojave.

@ellatrix
Copy link
Member Author

@ellatrix ellatrix commented Nov 15, 2018

Cool! Thanks for all the help!

@ellatrix ellatrix merged commit 2610238 into master Nov 15, 2018
1 check passed
@ellatrix ellatrix deleted the try/composing-input branch Nov 15, 2018
@waviaei
Copy link
Contributor

@waviaei waviaei commented Nov 15, 2018

Finally catching up (got caught up with messed up local env on my home MBA...)

  • Mojave 10.14.1
  • ATOK IME (note: its a 3rd party IME software, not the one default with Mac)
  • Chrome 70.0.3538.102
  • Safari 12.0.1
  • Firefox 63.0.1

4e060d1 - as @torounit mentioned, it seem to have fixed it!
I did 1) type some Japanese, 2) then delete few letters, 3) then typed some more.
With following blocks. IME functioned normal as expected.
This is with Chrome.

  • paragraph
  • list
  • quote
  • heading
  • preformatted
  • code (mixed typing of code and comment out with Japanese text)
  • pullquote
  • table
  • verse
  • media and text
  • button

For your info, I did test with 9110d08 and it was like this (I had not done through testing on 4.2, so I cannot be sure if worked correctly before, except for paragraph, list and quote).

  • paragraph => delete button deleted everything (as per @torounit reported)
  • list => seems fixed
  • quote => seems fixed
  • verse => no issue with input, but unable to delete anything at all
  • pull quote => seems ok
  • preformatted text => no issue with input, but when you move on to a next block, placeholder text reappears underneath the typed text
  • code => seems ok
  • button => placeholder text did not disappear when you start typing. Only disappear after you confirm the Japanese character.
  • Media and text => text part was same as paragraph.

I think through testing with different blocks might still be needed, but looks good!

Many many thanks @iseulde @torounit !

@ellatrix
Copy link
Member Author

@ellatrix ellatrix commented Nov 15, 2018

@waviaei Thanks for the additional testing. It seems the major issues have been fixed. Could you report any remaining issues separately? Thanks!

@mcsf
Copy link
Contributor

@mcsf mcsf commented Nov 16, 2018

Nice fix!

grey-rsi pushed a commit to OnTheGoSystems/gutenberg that referenced this issue Nov 22, 2018
* RichText: Don't trigger a change on composition

* Use nativeEvent

* Comment on IME

* Handle onCompositionEnd
grey-rsi pushed a commit to OnTheGoSystems/gutenberg that referenced this issue Jan 4, 2019
* RichText: Don't trigger a change on composition

* Use nativeEvent

* Comment on IME

* Handle onCompositionEnd
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants