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: Avoid iOS block appender focus loop #44988

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/**
* External dependencies
*/
import { Platform, ScrollView, View } from 'react-native';
import { Platform, ScrollView, View, Keyboard } from 'react-native';

/**
* WordPress dependencies
Expand Down Expand Up @@ -185,6 +185,12 @@ export default compose( [
onHideKeyboard() {
clearSelectedBlock();
togglePostTitleSelection( false );
// Explicitly dismiss the keyboard for circumstances where Aztec race
// conditions lead to one block's rich text focused, but a different
// non-rich-text block selected. In this context, merely clearing block
// selection does not trigger a blur event and therefore will not
// dismiss the keyboard.
Keyboard.dismiss();
Comment on lines +188 to +193
Copy link
Member Author

Choose a reason for hiding this comment

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

This change reintroduces an explicit keyboard dismissal that was removed in 54323e9 as it was deemed unnecessary. It should be unnecessary, but there are race conditions between Aztec and React Native event handlers (e.g. block/rich text tap) that makes it possible to focus one block and select another when tapping the two near simultaneously.

Explicitly dismissing the keyboard ensures that the user is able to close the keyboard when they find themselves in a state where a non-rich-text block is selected with the keyboard visible (the second image below).

Rich Text Non Rich Text
differing-focus-and-rich-text-block-select differing-focus-and-non-rich-text-block-select

This issue is not new in this PR, it is just more accessible now that focus loops are avoided on iOS. The change on this line fixes the focus + selection conflict on iOS. Unfortunately, it does not fix the issue on Android for an unknown reason currently.

},
};
} ),
Expand Down
9 changes: 9 additions & 0 deletions packages/react-native-aztec/src/AztecInputState.js
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,15 @@ export const notifyInputChange = () => {
}
};

/**
* Sets the current focused element ref held within TextInputState.
*
* @param {RefObject} element Element to be set as the focused element.
*/
export const focusInput = ( element ) => {
TextInputState.focusInput( element );
};

/**
* Focuses the specified element.
*
Expand Down
46 changes: 36 additions & 10 deletions packages/react-native-aztec/src/AztecView.js
Original file line number Diff line number Diff line change
Expand Up @@ -231,13 +231,41 @@ class AztecView extends Component {
}
}

_onAztecFocus( event ) {
// IMPORTANT: the onFocus events from Aztec are thrown away on Android as these are handled by onPress() in the upper level.
// It's necessary to do this otherwise onFocus may be set by `{...otherProps}` and thus the onPress + onFocus
// combination generate an infinite loop as described in https://github.com/wordpress-mobile/gutenberg-mobile/issues/302
// For iOS, this is necessary to let the system know when Aztec was focused programatically.
if ( Platform.OS === 'ios' ) {
this._onPress( event );
_onAztecFocus() {
// IMPORTANT: This function serves two purposes:
//
// Android: This intentional no-op function prevents focus loops originating
// when the native Aztec module programmatically focuses the instance. The
// no-op is explicitly passed as an `onFocus` prop to avoid future prop
// spreading from inadvertently introducing focus loops. The user-facing
// focus of the element is handled by `onPress` instead.
//
// See: https://github.com/wordpress-mobile/gutenberg-mobile/issues/302
//
// iOS: Programmatic focus from the native Aztec module is required to
// ensure the React-based `TextStateInput` ref is properly set when focus
// is *returned* to an instance, e.g. dismissing a bottom sheet. If the ref
// is not updated, attempts to dismiss the keyboard via the `ToolbarButton`
// will fail.
//
// See: https://github.com/wordpress-mobile/gutenberg-mobile/issues/702
if (
// The Android keyboard is, likely erroneously, already dismissed in the
// contexts where programmatic focus may be required on iOS.
//
// - https://github.com/WordPress/gutenberg/issues/28748
// - https://github.com/WordPress/gutenberg/issues/29048
// - https://github.com/wordpress-mobile/WordPress-Android/issues/16167
Platform.OS === 'ios'
) {
// Programmatically swapping input focus creates an infinite loop if the
// user taps a different input in between the programmatic focus and
// the resulting update to the React Native TextInputState focused element
// ref. To mitigate this, the below updates the focused element ref, but
// does not call the native focus methods.
//
// See: https://github.com/wordpress-mobile/WordPress-iOS/issues/18783
AztecInputState.focusInput( this.aztecViewRef.current );
}
}

Expand Down Expand Up @@ -269,9 +297,7 @@ class AztecView extends Component {
onBackspace={ this.props.onKeyDown && this._onBackspace }
onKeyDown={ this.props.onKeyDown && this._onKeyDown }
deleteEnter={ this.props.deleteEnter }
// IMPORTANT: the onFocus events are thrown away as these are handled by onPress() in the upper level.
// It's necessary to do this otherwise onFocus may be set by `{...otherProps}` and thus the onPress + onFocus
// combination generate an infinite loop as described in https://github.com/wordpress-mobile/gutenberg-mobile/issues/302
// IMPORTANT: Do not remove the `onFocus` prop, see `_onAztecFocus`
onFocus={ this._onAztecFocus }
onBlur={ this._onBlur }
ref={ this.aztecViewRef }
Expand Down
1 change: 1 addition & 0 deletions packages/react-native-editor/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ For each user feature we should also add a importance categorization label to i
-->

## Unreleased
- [*] [iOS] Fix focus loop when quickly tapping the block appender [#44988]

## 1.85.0
- [*] [iOS] Fixed iOS Voice Control support within Image block captions. [#44850]
Expand Down