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/delete all closes keyboard #5368

Merged
merged 6 commits into from
Mar 20, 2023

Conversation

edhager
Copy link
Contributor

@edhager edhager commented Mar 16, 2023

Description
There are situations in which deleting all of the text in the editor causes some phone keyboards to hide. This is easiest to reproduce using the Samsung Android keyboard.

Below are a couple test cases. I've been testing with a Samsung Galaxy S23 running Android v13.0 in Chrome.

  1. Open the plain text example page.
  2. Touch at the end of the text.
  3. Hold the delete key until all of the text is gone.

My results are the keyboard hides.

  1. Follow the steps above.
  2. Click in the editor to show the keyboard.
  3. Type "Eng" and choose a suggested word.
  4. Type any additional letter and then delete until you are into the first word.
  5. Choose a suggested word.

My results are the suggested word appears in the editor but the keyboard hides.

The problem is that some keyboards will create a selection covering 2 or more of the remaining characters and then issue a deleteContentBackward input event. When the Editable component renders the placeholder, it does so inside the selection created by the keyboard. The placeholder is in a span that can't be edited and this causes the keyboard to hide.

The solution presented in this PR is to delay the rendering of the placeholder long enough for the editor contents to settle and the selection to be reset.

Checks

  • The new code matches the existing patterns and styles.
  • The tests pass with yarn test.
  • The linter passes with yarn lint. (Fix errors with yarn fix.)
  • The relevant examples still work. (Run examples with yarn start.)
  • You've added a changeset if changing functionality. (Add one with yarn changeset add.)

@changeset-bot
Copy link

changeset-bot bot commented Mar 16, 2023

🦋 Changeset detected

Latest commit: 577ff45

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
slate-react Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@edhager edhager force-pushed the fix/delete-all-closes-keyboard branch from 8390cd1 to c83cbc2 Compare March 17, 2023 14:19
@edhager
Copy link
Contributor Author

edhager commented Mar 17, 2023

This screen capture shows the fix in action. The keyboard does not hide when deleting all text and when choosing a suggested word.

You may notice that in some cases, the cursor is positioned after the first letter of the inserted word. This is a bug that existed in the original code. The second half of the screen capture shows that behavor.

2023-03-17 09 32 38

The cursor positioning will be handled in a separated PR but here is a description of cursor positioning problem...

In some cases when inserting a suggested word first as the only content, some IMEs remove all of the exiting text with a deleteContentBackward input event and then insert the new word using an insertText input event. This PR stops the placeholder from being rendered between those two events but a <span> containing a <br> is rendered and then removed between those two events. The window selection is also being moved over one character.

When the insertText event arrives, window selection is anchored at an offset of 1 and the selection range in the event is anchored at 0. The slate code determines the current selection is inside the one defined by the insertText event and it places the selection inside the word, after the first character.

@edhager edhager marked this pull request as ready for review March 17, 2023 14:56
@edhager
Copy link
Contributor Author

edhager commented Mar 17, 2023

In commit c83cbc2, I moved the coded that gets the size of the placeholder into a layout effect. Getting the bounding rectangle for the placeholder will force the browser to run layout cycle which isn't a good thing to do during a react render.

@edhager
Copy link
Contributor Author

edhager commented Mar 17, 2023

I'll work on fixing the integration tests.

@edhager edhager force-pushed the fix/delete-all-closes-keyboard branch from 3fdc3c4 to 577ff45 Compare March 20, 2023 15:26
@dylans dylans merged commit 5a0d397 into ianstormtaylor:main Mar 20, 2023
@github-actions github-actions bot mentioned this pull request Mar 20, 2023
@edhager edhager deleted the fix/delete-all-closes-keyboard branch March 20, 2023 19:06
@codeingforcoffee
Copy link

codeingforcoffee commented Jul 11, 2023

... This commit make slate editor like a duncky, when I clear all contents, placeholder is not shown at once... I m using PC BTW

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.

None yet

3 participants