-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Conversation
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: wordpress-mobile/gutenberg-mobile#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: wordpress-mobile/gutenberg-mobile#702 The Android keyboard is, likely erroneously, already dismissed in the contexts where programmatic focus may be required on iOS. - #28748 - #29048 - wordpress-mobile/WordPress-Android#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: wordpress-mobile/WordPress-iOS#18783
Size Change: 0 B Total Size: 1.28 MB ℹ️ View Unchanged
|
// 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() |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
:
// 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() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @dcalhoun for addressing this issue 🙇 !
I've tested the changes and found an infinite focus loop when tapping several times (usually three or more times) beneath the last block (see attached video capture). I haven't explored further but looks like we might have here a race condition when two Aztec views notify the focus and blur events at the same time.
NOTE: I'm printing a console.log
when the conditions that execute _onPress
function are fulfilled.
ios-focus-loop.mp4
// 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() |
There was a problem hiding this comment.
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
:
// 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() |
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: wordpress-mobile/WordPress-iOS#18783
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.
7262548
to
2ea6b0e
Compare
…lock-appender-focus-loop
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fluiddot thank you for reviewing and testing this PR. I modified the approach taken to better mitigate focus loops like you experienced.
It does seem like the latest approach works. Unfortunately, it does uncover an existing bug that I expand upon in a separate comment. The workaround for the uncovered bug only works for iOS.
I will not be able to continue forward on this PR in the near future. Because of the state of this PR, I am uncertain if it is worth merging in its current state. It is arguably an improvement if it prevents focus loops on iOS. However, it is not the direct solution as it merely masks the underlying issue.
If we'd like to leave this PR as-is for the time being without merging, that is understandable from my perspective. I am removing from myself as the assignee for the associated issues for the time being.
// 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(); |
There was a problem hiding this comment.
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 |
---|---|
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.
#44988 but taking into account that the input is not currently focused to avoid a focus loop and also to use AztecInputState.focus to trigger the notifyInputChange callback
Incorporated into and superseded by #53217. |
Related PRs
What?
During programmatic Aztec focus events, only update the internal focused
element ref found within
TextInputState
and do not call the native focusmethods.
Why?
Fixes wordpress-mobile/WordPress-iOS#18783. Quickly adding multiple rich text
blocks on iOS resulted in a focus loop occurring between the rich text blocks.
The app would become unresponsive and eventually crash.
How?
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 avoidfuture prop spreading from inadvertently introducing focus loops. The
user-facing focus of the element is handled by
onPress
instead.See: wordpress-mobile/gutenberg-mobile#302
iOS: Programmatic focus from the native Aztec module is required to
ensure the React-based
TextStateInput
ref is properly set when focusis 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: wordpress-mobile/gutenberg-mobile#702
The Android keyboard is, likely erroneously, already dismissed in the
contexts where programmatic focus may be required on 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: wordpress-mobile/WordPress-iOS#18783
Testing Instructions
Verify block appender loop fix
See wordpress-mobile/WordPress-iOS#18783.
Verify link picker keyboard dismissal
See wordpress-mobile/WordPress-iOS#18783.
iOS
Android
Verify focus + selection conflict resolution
Test general Writing Flow behavior
Given the foundational nature of this change, it feels appropriate to thoroughly test the writing experience for the block editor in general. Common Writing Flow tests are documented.
Screenshots or screencast
See:
Footnotes
This works on iOS, but fails on Android. ↩