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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@youknowriad youknowriad added the [Type] Bug An existing feature is broken. label Nov 15, 2018
@youknowriad youknowriad added this to the 4.4 milestone Nov 15, 2018
@torounit
Copy link
Member

@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

@torounit Thanks, I'll investigate further!

@yoavf
Copy link
Contributor

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

@iseulde

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

output

@ellatrix
Copy link
Member Author

@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

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

@yoavf
Copy link
Contributor

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

@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

@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
Copy link
Member

on Firefox, the deletion problem not happen.

@ellatrix
Copy link
Member Author

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

@torounit
Copy link
Member

@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

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

@ellatrix
Copy link
Member Author

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

@torounit
Copy link
Member

torounit commented Nov 15, 2018

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

@atachibana
Copy link
Contributor

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

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

@torounit
Copy link
Member

@iseulde 9110d08 dosen't work. :(

@ellatrix
Copy link
Member Author

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

@torounit
Copy link
Member

@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

@torounit Last try: 4e060d1.

@torounit
Copy link
Member

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

@ellatrix
Copy link
Member Author

@torounit Oh, great!!

@youknowriad
Copy link
Contributor

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

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

@ellatrix
Copy link
Member Author

Cool! Thanks for all the help!

@ellatrix ellatrix merged commit 2610238 into master Nov 15, 2018
@ellatrix ellatrix deleted the try/composing-input branch November 15, 2018 15:49
@waviaei
Copy link
Contributor

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

@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 commented Nov 16, 2018

Nice fix!

ghost pushed a commit to OnTheGoSystems/gutenberg that referenced this pull request Nov 22, 2018
* RichText: Don't trigger a change on composition

* Use nativeEvent

* Comment on IME

* Handle onCompositionEnd
ghost pushed a commit to OnTheGoSystems/gutenberg that referenced this pull request 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
Labels
[Feature] Rich Text [Type] Bug An existing feature is broken.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants