[RNMobile] keyboard focus issues #15999
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Some blocks have multiple RichText or a RichText among other children. Example: Quote blocks has 2 RichTexts and Image block has a RichText for the caption. We want to control when and which of those RichTexts will request focus. On the web side, the DOM is used to search for a inputbox to focus but on RN, we don't have a DOM to search. Instead, this commit makes the assumption that a RichText always request focus if its parent has passed `true` in `isSelected` and only if the parent hasn't inhibited that behavior by using the `noFocusOnMount` prop.
hypest
added
the
Mobile App - i.e. Android or iOS
1 task
That can happen when the RichText based block gets tapped somewhere and the caret position is not updated yet from Aztec.
mkevins
approved these changes
Jun 7, 2019
There was a problem hiding this comment.
I have tested this via demo app and the crash has been fixed. 🎉
Also, I tested the scenarios described here: wordpress-mobile/gutenberg-mobile#1076 (comment) and everything is working as expected.
koke
added a commit
that referenced
this pull request
Jun 13, 2019
In #15999 this behavior was changed to prevent the keyboard from disappearing when you press Enter to create a new paragraph. This worked in that case, but had the side effect of TextInputState still thinking a dead view had focus, and causing a crash in some scenarios.
5 tasks
hypest
added a commit
that referenced
this pull request
Jun 13, 2019
* Remove focus from RichText when unmounting In #15999 this behavior was changed to prevent the keyboard from disappearing when you press Enter to create a new paragraph. This worked in that case, but had the side effect of TextInputState still thinking a dead view had focus, and causing a crash in some scenarios. * Only blur a RichText in replaceable block That RichText won't have the chance to blur earlier so, we need to do it on unmount. But, non replaceable blocks wont need to blur their RichText on unmount because that will happen normally in componentDidUpdate(). * Rename to an unstable, very goal-named name * Compute shouldBlurOnUnmount inside RichText
hypest
added a commit
that referenced
this pull request
Jun 14, 2019
* [Mobile] Disable Video block on Android for v1.7.0 release (#16149) * Disable Video block on Android for v1.7.0 release * Add import statement to check platform * [RNMobile] Narrow down blur on unmount to replaceable only (#16151) * Remove focus from RichText when unmounting In #15999 this behavior was changed to prevent the keyboard from disappearing when you press Enter to create a new paragraph. This worked in that case, but had the side effect of TextInputState still thinking a dead view had focus, and causing a crash in some scenarios. * Only blur a RichText in replaceable block That RichText won't have the chance to blur earlier so, we need to do it on unmount. But, non replaceable blocks wont need to blur their RichText on unmount because that will happen normally in componentDidUpdate(). * Rename to an unstable, very goal-named name * Compute shouldBlurOnUnmount inside RichText * Fix no-duplicate-imports lint issue * Fix comma-dangle lint error * Remove trailing space from comment
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR tries to align closer to the web's logic of focus and selection.
How has this been tested?
Using the gutenberg-mobile PR: wordpress-mobile/gutenberg-mobile#1076
Types of changes
When RichText gets focused, update the selection state too https://github.com/WordPress/gutenberg/pull/15999/files#diff-4828a21853e899e5a36faecfa96d83e8R591
Lift the iOS only condition of blurring the RichText when
isSelectedtransitions tofalse. Same logic will now apply to Android too. This is now the primary way of blurring the text input field when the blocks gets unselected, replacing the logic in https://github.com/wordpress-mobile/gutenberg-mobile/blob/0533baf76be6fa9c9b0d3c6fa18855f5ed4d0083/src/block-management/block-holder.js#L76-L88.Drop
onFocusfrom props and useunstableOnFocuslike the web side. Also, no need to check forownProps.onFocusanymore sinceunstableOnFocuswill be picked up directly and the onFocus from the edit context is no longer used (apparently).No need to explicitly set
isSelected,onFocusandonBluron the Heading or Paragraph blocks just to pass its props. Those functionalities are handled internally by the RichText component now.Checklist: