From de1d8809de3bd40bb0c3178da726beeaf63f19dc Mon Sep 17 00:00:00 2001 From: David Calhoun <438664+dcalhoun@users.noreply.github.com> Date: Thu, 13 Oct 2022 17:20:25 -0500 Subject: [PATCH 1/4] fix: Avoid iOS block appender focus loop The focus callback triggered by Aztec-based programmatic focus events can result in focus loops between rich text elements. 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 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 Programmatic swapping focus from element to another often leads to focus loops, only delegate the programmatic focus if there are no elements focused. See: https://github.com/wordpress-mobile/WordPress-iOS/issues/18783 --- packages/react-native-aztec/src/AztecView.js | 45 +++++++++++++++----- 1 file changed, 35 insertions(+), 10 deletions(-) diff --git a/packages/react-native-aztec/src/AztecView.js b/packages/react-native-aztec/src/AztecView.js index dda3633c4c2e7..7528aade008eb 100644 --- a/packages/react-native-aztec/src/AztecView.js +++ b/packages/react-native-aztec/src/AztecView.js @@ -231,13 +231,40 @@ 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' && + // Programmatic swaping focus from element to another often leads to focus + // loops, only delegate the programmatic focus if there are no elements + // focused. + // + // See: https://github.com/wordpress-mobile/WordPress-iOS/issues/18783 + ! AztecInputState.getCurrentFocusedElement() + ) { + this._onPress(); // Call to move the focus in RN way (TextInputState) } } @@ -269,9 +296,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 } From 11a04e237f10e06f51afeafacdd9e5adb6e2bbd0 Mon Sep 17 00:00:00 2001 From: David Calhoun <438664+dcalhoun@users.noreply.github.com> Date: Fri, 14 Oct 2022 15:25:16 -0500 Subject: [PATCH 2/4] docs: Add changelog entry --- packages/react-native-editor/CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/react-native-editor/CHANGELOG.md b/packages/react-native-editor/CHANGELOG.md index 1b784aa821d48..1be9c5807e115 100644 --- a/packages/react-native-editor/CHANGELOG.md +++ b/packages/react-native-editor/CHANGELOG.md @@ -11,6 +11,7 @@ For each user feature we should also add a importance categorization label to i ## Unreleased - [*] Upgrade compile and target sdk version to Android API 31 [#44610] +- [*] [iOS] Fix focus loop when quickly tapping the block appender [#44988] ## 1.83.0 * No User facing changes * From 7a01503ddb6db156c71ec242be4994be24aac82a Mon Sep 17 00:00:00 2001 From: David Calhoun <438664+dcalhoun@users.noreply.github.com> Date: Mon, 31 Oct 2022 16:15:55 -0500 Subject: [PATCH 3/4] fix: Programmatic Aztec input focus only updates internal ref 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 Aztec now updates the focused element ref, but does not call the native focus methods. See: https://github.com/wordpress-mobile/WordPress-iOS/issues/18783 --- .../react-native-aztec/src/AztecInputState.js | 9 +++++++++ packages/react-native-aztec/src/AztecView.js | 15 ++++++++------- 2 files changed, 17 insertions(+), 7 deletions(-) diff --git a/packages/react-native-aztec/src/AztecInputState.js b/packages/react-native-aztec/src/AztecInputState.js index 973b8179ea5ec..94b612be50e7e 100644 --- a/packages/react-native-aztec/src/AztecInputState.js +++ b/packages/react-native-aztec/src/AztecInputState.js @@ -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. * diff --git a/packages/react-native-aztec/src/AztecView.js b/packages/react-native-aztec/src/AztecView.js index 7528aade008eb..9a7006f7f5a3c 100644 --- a/packages/react-native-aztec/src/AztecView.js +++ b/packages/react-native-aztec/src/AztecView.js @@ -256,15 +256,16 @@ class AztecView extends Component { // - 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' && - // Programmatic swaping focus from element to another often leads to focus - // loops, only delegate the programmatic focus if there are no elements - // focused. + 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.getCurrentFocusedElement() - ) { - this._onPress(); // Call to move the focus in RN way (TextInputState) + AztecInputState.focusInput( this.aztecViewRef.current ); } } From 2ea6b0ecddca2f893594d84462e6716e1af15871 Mon Sep 17 00:00:00 2001 From: David Calhoun <438664+dcalhoun@users.noreply.github.com> Date: Tue, 1 Nov 2022 16:35:43 -0500 Subject: [PATCH 4/4] fix: Hide keyboard button forcibly dismisses the virtual keyboard Due to race conditions between Aztec focus effects and React Native gesture handlers, it is possible to select one block while placing focus in a different block's text input. If the actively select block is not a rich text field, tapping the dismiss keyboard button will not dismiss the keyboard via clearing the selected block, as a non-rich-text block does not trigger a blur event. So, this change explicitly dismisses the keyboard rather than relying upon the selected block clear. This is a hack. Ideally, the Aztec call loop is improved so that it is not possible to select/focus more than one block. --- .../src/components/header/header-toolbar/index.native.js | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/packages/edit-post/src/components/header/header-toolbar/index.native.js b/packages/edit-post/src/components/header/header-toolbar/index.native.js index 26ab309d2e7a0..540bad7e8a518 100644 --- a/packages/edit-post/src/components/header/header-toolbar/index.native.js +++ b/packages/edit-post/src/components/header/header-toolbar/index.native.js @@ -1,7 +1,7 @@ /** * External dependencies */ -import { Platform, ScrollView, View } from 'react-native'; +import { Platform, ScrollView, View, Keyboard } from 'react-native'; /** * WordPress dependencies @@ -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(); }, }; } ),