Mobile - Fix iOS Focus loop for RichText components #53217
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
geriux
added
[Type] Bug
|
Size Change: +241 B (0%) Total Size: 1.44 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 5882051. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5724785624
|
2 tasks
dcalhoun
reviewed
Aug 1, 2023
There was a problem hiding this comment.
Thank you for exploring a fix to this disruptive bug. 🙇🏻♂️
The outlined test plan succeeded for me when using an iPhone SE running iOS 16.5.1. However, I believe there may be couple of gaps in the implementation. I left an inline comment elaborating.
If you haven't already, I recommend reviewing the inline comments in #44988. The comments are obnoxiously large, but that is because I found this subject to be very complex with several nuances. I did my best to capture all of the details I found while debugging this issue.
packages/block-editor/src/components/default-block-appender/index.native.js
Show resolved
Hide resolved
derekblank
approved these changes
Aug 2, 2023
There was a problem hiding this comment.
I ran through the Writing Flow test cases and some of the test suites, and did not note any regressions. I was also not able to enter a focus loop, despite my best efforts to do so. 👍
I did explore the issue @dcalhoun mentioned regarding change of focus and the Keyboard Dismiss button (steps to replicate). Outside of the Link BottomSheet, I did not note another area where this occurs. I also think David's inline comments are an important consideration that we should address. If I'm not misunderstanding anything more serious from that change of focus issue (and I didn't run into any during my testing), I don't think that issue alone should prevent the merging of this PR. This change is a major improvement on the current behavior in trunk. In that context, I'm providing an approval for these changes, but let's continue the discussion!
Well done to you both for your long-term diligence in resolving this issue. 🚀
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
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
…oves inline functions and other minor code style changes
geriux
force-pushed
the
rnmobile/fix-rich-text-loop-issues
branch
from
e8d69b6
to
dbb4d22
Compare
|
Hey @derekblank 👋 !
Thank you for testing using the writing flow test cases!
I agree and thank you for the approval! However, I wanted to spend a bit more time reviewing the pending issues so we don't ship something that fixed something but broke another functionality 😅 I brought @dcalhoun 's changes into this branch from #44988 since those solved the pending bugs and also it included a very well-detailed explanation for this code that will be helpful for future changes. I created a new build in wordpress-mobile/WordPress-iOS#21209 (comment) like the one before, it includes all of these changes but in a Let me know if you find something else that I might have missed, thank you both! |
… having a RichText component focused while another block is selected
dcalhoun
approved these changes
Aug 3, 2023
Thanks for addressing this! I tested the writing flow test cases successfully on the new build, and did not note anything unusual. The Keyboard Dismiss button works as expected. 👍 |
geriux
pushed a commit
that referenced
this pull request
Aug 3, 2023
* Mobile - Update changelog * 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: 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 * 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: wordpress-mobile/WordPress-iOS#18783 * Mobile - AztecView - Check for isFocused before forcing the focus * Mobile - DefaultBlockAppender and BlockList Footer placeholders - Removes inline functions and other minor code style changes * Mobile - AztecView - Trigger _onFocus within _onAztecFocus to prevent having a RichText component focused while another block is selected --------- Co-authored-by: David Calhoun <438664+dcalhoun@users.noreply.github.com>
4 tasks
4 tasks
This was referenced Aug 3, 2023
geriux
pushed a commit
that referenced
this pull request
Aug 3, 2023
* Release script: Update react-native-editor version to 1.100.1 * Release script: Update with changes from 'npm run core preios' * Update `react-native-editor` changelog * Release script: Update react-native-editor version to 1.100.2 * Release script: Update with changes from 'npm run core preios' * Mobile - Fix iOS Focus loop for RichText components (#53217) * Mobile - Update changelog * 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: 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 * 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: wordpress-mobile/WordPress-iOS#18783 * Mobile - AztecView - Check for isFocused before forcing the focus * Mobile - DefaultBlockAppender and BlockList Footer placeholders - Removes inline functions and other minor code style changes * Mobile - AztecView - Trigger _onFocus within _onAztecFocus to prevent having a RichText component focused while another block is selected --------- Co-authored-by: David Calhoun <438664+dcalhoun@users.noreply.github.com> --------- Co-authored-by: Derek Blank <derekpblank@gmail.com> Co-authored-by: Carlos Garcia <fluiddot@gmail.com> Co-authored-by: David Calhoun <438664+dcalhoun@users.noreply.github.com>
2 tasks
|
So nice to see this fixed, thank you Gerardo, David, and Derek! |
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.
Related PRs:
Fixes wordpress-mobile/gutenberg-mobile#6002
Fixes wordpress-mobile/WordPress-iOS#18783
Fixes wordpress-mobile/gutenberg-mobile#1696
Fixes #30562
Fixes wordpress-mobile/gutenberg-mobile#953
What?
This PR attempts to fix the current iOS focus loop with RichText components. It's co-authored with @dcalhoun's work to address these issues from #44988.
Why?
This known issue is now more easy to reproduce after the recent design updates, so this PR addresses this bug to avoid a bad experience on the mobile editor.
How?
You can find more information in #44988 but it updates how the
onFocuscallback is handled for iOS from Aztec, by avoiding calling thethis._onPresscallback and instead calling the newly addedAztecInputState.focusInputwhich sets the current TextInput's reference without calling the native onFocus methods that were generating focus loops.It also updates the DefaultBlockAppender component code and the Footer Appender to avoid inline arrow functions and other minor code-style improvements.
Testing Instructions
Precondition: Use the following testing build from the iOS host app. You could test the demo app but it's not as easy since you'd need to build the app on your device for better testing.
Scenario 1
Scenario 2
Scenario 3
Testing Instructions for Keyboard
N/A
Screenshots or screencast
Scenario2_Before.MP4
Scenario2_After.MP4
Scenario3_Before.MP4
Scenario3_After.MP4