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.

Sign up for GitHub

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

fix: Avoid iOS block appender focus loop #44988

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
de1d880
fix: Avoid iOS block appender focus loop
dcalhoun Oct 13, 2022
11a04e2
docs: Add changelog entry
dcalhoun Oct 14, 2022
7a01503
fix: Programmatic Aztec input focus only updates internal ref
dcalhoun Oct 31, 2022
2ea6b0e
fix: Hide keyboard button forcibly dismisses the virtual keyboard
dcalhoun Nov 1, 2022
fda398e
Merge branch 'trunk' of github.com:WordPress/gutenberg into fix/ios-b…
dcalhoun Nov 2, 2022
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
45 changes: 35 additions & 10 deletions packages/react-native-aztec/src/AztecView.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
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 is the key change. The rest is merely documentation for this complex context.

Copy link
Contributor

Choose a reason for hiding this comment

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

AztecInputState already exposes a function for checking if any Aztec view is focused via isFocused, so probably we could use it instead of getCurrentFocusedElement:

Suggested change
// 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()
// 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.isFocused()

) {
this._onPress(); // Call to move the focus in RN way (TextInputState)
}
}

Expand Down Expand Up @@ -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 }
Expand Down
1 change: 1 addition & 0 deletions packages/react-native-editor/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 *
Expand Down