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

[RNMobile]: Fix search block focus issues #29500

Merged
merged 44 commits into from
Mar 16, 2021

Conversation

AmandaRiu
Copy link
Contributor

@AmandaRiu AmandaRiu commented Mar 3, 2021

NOTE: This PR must be merged before this one can be taken out of draft

Gutenberg Mobile PR: wordpress-mobile/gutenberg-mobile#3210

Description

Fixes an couples issues found in a previous PR review:

  • iOS: When adding a new search block when there are other text-based blocks already on the page would cause a bizarre UI issue where the search block kept stealing the focus causing a strange blinking of focusing between the search block and the other text blocks.
  • Also on iOS when adding the search block, the app would appear to hang for a while before finally adding the block.
  • Clicking on the placeholder text input when the search block was not selected would not select the block.

How has this been tested?

Tested on both Android emulator and iOS simulator

  1. Add a text block to the page.
  2. Add a search block. Notice the placeholder text input has the focus.
  3. Click the text in the text block added from step 1. Notice the focus remains on the text block as expected.
  4. Click the placeholder text. Notice the search block is now selected.
  5. Click the button text of the search block and modify it. Verify successful.
  6. Click the search label text and modify it. Verify successful.

Screenshots

iOS before - search block stealing focus from text-based blocks

Screen.Recording.2021-03-02.at.10.27.29.PM.mov

iOS After:

Screen.Recording.2021-03-02.at.10.15.04.PM.mov

Android before - selecting placeholder text field when block not selected:

Screen.Recording.2021-03-02.at.10.26.38.PM.mov

Android after - placeholder text is focused when block first added:

Screen.Recording.2021-03-02.at.10.21.38.PM.mov

Types of changes

  • Started tracking when different components of the Search Block are focused and passing that state via the isSelected property of those components.
  • Passed the Search Block's onFocus prop to it's child components so when the placeholder text was directly clicked the block would become selected.
  • Swapped out the TextInput component with the PlainText component.
  • Added a blur() method to the PlainText component and then added a useEffect to be called when theisSelected property changes to force the PlainText to blur when the Search Block is no longer selected. Similar things are done in other blocks (Image, MediaText, Video) except they implement a static getDerivedStateFromProps method to do the same thing. It works in those blocks because they are React classes. My Search Block is a Function Component so I'm using useEffect to accomplish the same thing.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • I've tested my changes with keyboard and screen readers.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

AmandaRiu and others added 30 commits February 12, 2021 20:15
Only adds a shell of a block.
This alert will be removed once styling is added in a later PR.
This will keep the code cleaner and easier to follow.
This toolbar action is now visible in the UI so the temporary alert message
is no longer needed.
Was causing issues with layout in iOS. Styling will be addressed fully
 in a different PR.
Moved the CSS_UNIT code to a shared Utils file in preparation for using the same code inside the native implementation of the search block.
Also moved the default values from the web version to the shared utils file.
# Conflicts:
#	packages/block-library/src/search/edit.native.js
#	packages/block-library/src/search/rich-text.android.scss
#	packages/block-library/src/search/rich-text.ios.scss
#	packages/block-library/src/search/style.native.scss
# Conflicts:
#	packages/block-library/src/search/edit.native.js
- Set the onFocus() method to ensure if the placeholder text is clicked on when the block is NOT selected then the block will be selected.
- Set isSelected in RichText elements to false for initial draw to prevent odd behavior on iOS where the Search block will attempt to steal focus from other text blocks.
- Set the placeholder text to focused when the block is first added.
Base automatically changed from rnmobile/3073-search-block-settings to trunk March 9, 2021 16:37
@AmandaRiu AmandaRiu marked this pull request as ready for review March 9, 2021 17:00
@AmandaRiu AmandaRiu requested a review from cameronvoell March 9, 2021 17:00
@cameronvoell
Copy link
Member

cameronvoell commented Mar 10, 2021

The focus behavior is looking much better @AmandaRiu 🙌

One thing I noticed is that when you unselect the search block while one of the text fields was selected (with the blinking cursor active) - the blinking cursor stays active on that text field even though another block is now selected, and the user can still type in that field even though a new block is selected. I think the expected behavior would be that when you select a new block, the cursor should not remain active in the previously selected text field of the search block. My hunch is that it would be best to address that in this PR, but let me know if you think a separate PR would be better 🙏

@AmandaRiu
Copy link
Contributor Author

Thanks for the review @cameronvoell !

My hunch is that it would be best to address that in this PR, but let me know if you think a separate PR would be better

I agree it should be handled in this PR. I'll take a look at it today 🙇‍♀️

Fixes an issue where if the label or button text was selected in the search block and then a different block was selected, the cursor would still be active in the search block text field.
@AmandaRiu
Copy link
Contributor Author

@cameronvoell Issue with text not losing focus when block loses focus has been fixed in 452356a. Ready for another review! 👍

@cameronvoell
Copy link
Member

@cameronvoell Issue with text not losing focus when block loses focus has been fixed in 452356a. Ready for another review! 👍

@AmandaRiu In my tests it looks like it is fixed for focus on the Button text, and the label text, but not the placeholder text.

Steps:

  1. Add an image block and add an image to it
  2. Add a search block
  3. select the placeholder text so the cursor is active there and the keyboard is opened.
  4. Select the Image block
  5. Notice the cursor is still active in the placeholder and the keyboard did not close. Compare this to label and button text where the cursor would disappear and the keyboard would close when you select the image block.

This isn't really a concern for mobile, but on web there is an option for `button-only` so this
change ensures that we don't try to manipulate a reference that would be null.
@AmandaRiu
Copy link
Contributor Author

@cameronvoell Issue with text not losing focus when block loses focus has been fixed in 452356a. Ready for another review! 👍

@AmandaRiu In my tests it looks like it is fixed for focus on the Button text, and the label text, but not the placeholder text.

Steps:

1. Add an image block and add an image to it

2. Add a search block

3. select the placeholder text so the cursor is active there and the keyboard is opened.

4. Select the Image block

5. Notice the cursor is still active in the placeholder and the keyboard did not close. Compare this to label and button text where the cursor would disappear and the keyboard would close when you select the image block.

Fixed! Ready for another round!

@AmandaRiu
Copy link
Contributor Author

Hey @guarani 👋 I’ve made the changes we discussed. Ready for another review!

@guarani guarani self-requested a review March 16, 2021 19:57
Copy link
Contributor

@guarani guarani left a comment

Choose a reason for hiding this comment

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

Tested on iOS and Android, code looks good! Great work, @AmandaRiu! 💯

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Search Affects the Search Block - used to display a search field Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants