[Mobile] Remove Keyboard Aware Flatlist #48791
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
added 5 commits
March 6, 2023 16:06
- Remove usage of onCaretVerticalPositionChange - Add support for adding a listener when the caret Y coordinate position changes
…AutoScrollEnabled in the state of the component
…the caret position's changes for iOS
- Removes usage of the react-native-keyboard-aware-scroll-view library - Adds custom implementation to scroll to the focused TextInput element
geriux
added
[Status] In Progress
|
Size Change: 0 B Total Size: 1.37 MB ℹ️ View Unchanged
|
2 tasks
|
Flaky tests detected in a080674. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4656911993
|
4 tasks
added 6 commits
March 7, 2023 17:33
…rs and prevent scrolling if the scroll reference doesn't exist
dcalhoun
reviewed
Mar 10, 2023
There was a problem hiding this comment.
Thanks for working on this! The experience seems to be a solid improvement. 👏🏻
This is unfamiliar, complex code to me, so it will likely take me a bit to review and comprehend the logic. I plan to review again next week, but wanted to provide initial findings and questions in the meantime. Of course, if others are more familiar with this code, don't feel a need to await my specific review.
I noticed that with these changes, the calculations for larger text blocks — e.g. Heading blocks — feels a bit off. The Heading block text is partially obscured. I didn't test other blocks with larger text.
RPReplay_Final1678482187.MP4
packages/components/src/mobile/keyboard-aware-flat-list/index.ios.js
Outdated
Show resolved
Hide resolved
packages/components/src/mobile/keyboard-aware-flat-list/index.ios.js
Outdated
Show resolved
Hide resolved
packages/react-native-editor/__device-tests__/gutenberg-editor-block-insertion-@canary.test.js
Outdated
Show resolved
Hide resolved
fluiddot
reviewed
Mar 14, 2023
There was a problem hiding this comment.
I've tested using long paragraphs and noticed that the scroll sometimes doesn't go to the proper position, specifically I found the following issues:
1 - Moving the caret to the first line of a block moves the scroll to the wrong position
As you can see in the video, when I move the caret to the first line (i.e. caretY = 0) the scroll moves to a different position than where the caret is.
Kapture.2023-03-14.at.11.38.19.mp4
2- Moving the caret up doesn't move the scroll
Moving the caret down moves the scroll to keep the caret visible on the screen. However, moving the caret up doesn't have this behavior.
Kapture.2023-03-14.at.11.44.18.mp4
packages/components/src/mobile/keyboard-aware-flat-list/index.ios.js
Outdated
Show resolved
Hide resolved
Comment on lines
144
to
146
| if ( scrollEnabled ) { | ||
| RCTAztecView.InputState.addCaretChangeListener( onCaretChange ); | ||
| } |
There was a problem hiding this comment.
If scrollEnabled is set to false, the caret change listener won't be removed upon component unmount. Hence, we could consider removing it in this case:
Suggested change
| if ( scrollEnabled ) { | |
| RCTAztecView.InputState.addCaretChangeListener( onCaretChange ); | |
| } | |
| if ( scrollEnabled ) { | |
| RCTAztecView.InputState.addCaretChangeListener( onCaretChange ); | |
| } else { | |
| RCTAztecView.InputState.removeCaretChangeListener( onCaretChange ); | |
| } |
| const onCaretChange = useCallback( | ||
| ( { caretY } ) => { | ||
| const isFocused = | ||
| !! RCTAztecView.InputState.getCurrentFocusedElement(); |
There was a problem hiding this comment.
Maybe we could check the focus status by calling the isFocused function instead, WDYT?
Suggested change
| !! RCTAztecView.InputState.getCurrentFocusedElement(); | |
| !! RCTAztecView.InputState.isFocused(); |
added 3 commits
March 16, 2023 12:51
…veral hooks, it also fixes a few cases that weren't working correctly.
|
Hey @fluiddot thank you again for testing and I'm sorry I missed the most important issue which is what this PR is supposed to be fixing 😅 I've introduced a few changes to solve the issues you reported:
Test casesMoving the caret to the first line of a block moves the scroll to the wrong positionTestIssue1.mp4Moving the caret up doesn't move the scrollTestIssue2.mp4Splitting a long paragraph doesn't scroll to the correct positionTestIssue3.mp4Selecting text after opening/closing the keyboard doesn't scroll to the correct positionTestIssue4.movEditor jumps when inserting a new blockTestIssue5.mp4Adding a text block should scroll and auto-focusTestIssue6.mp4Opening the inserter with a text block focused and closing the inserter shouldn't reset the scrollTestIssue7.mp4Landscape 1TestIssue8.mp4Landscape 2TestIssue9.mp4iPad 1 (Hardware Keyboard)TestIssueiPad1.mp4iPad 2 (Hardware and external Keyboard)TestIssueiPad2.mp4Hopefully, all issues are now solved, let me know! Thanks again! |
fluiddot
approved these changes
Apr 5, 2023
There was a problem hiding this comment.
LGTM 🎊 ! Awesome job @geriux 🏅 !
I tested again with the last changes and confirm that they are addressed 🎊 :
- [RNMobile] [iOS] Editor jumps when inserting a new block #40029 ✅
- [iOS] Media & Text Block: Keyboard covers the text field after rotating from Landscape to Portrait wordpress-mobile/gutenberg-mobile#4894 ✅
Apart from the tests, I added some comments but none should block the PR. From my side the PR is ready ✅ 🥳 .
UPDATE: As a side note, it would be great to update the CHANGELOG to include this change as it's a user-facing change.
packages/components/src/mobile/keyboard-aware-flat-list/test/use-keyboard-offset.native.js
Outdated
Show resolved
Hide resolved
packages/components/src/mobile/keyboard-aware-flat-list/test/use-keyboard-offset.native.js
Show resolved
Hide resolved
packages/components/src/mobile/keyboard-aware-flat-list/test/use-keyboard-offset.native.js
Show resolved
Hide resolved
| clearTimeout( timeoutRef.current ); | ||
| timeoutRef.current = setTimeout( () => { | ||
| setKeyboardOffset( 0 ); | ||
| }, 200 ); |
There was a problem hiding this comment.
I understand that 200ms would be enough to wait for a change in the focus, but I'm wondering if it will cover the case of slow/old devices. In the future, it would be great to investigate if we can prevent the keyboard to open/close when the focus changes.
There was a problem hiding this comment.
Definitely, I think that would be a great improvement for the mobile editor!
packages/components/src/mobile/keyboard-aware-flat-list/use-keyboard-offset.native.js
Show resolved
Hide resolved
dcalhoun
reviewed
Apr 7, 2023
There was a problem hiding this comment.
Stopping by to say "thank you" to both of you for carrying this forward so well while I was unable to dedicate time for reviewing. The refactor to separate logic into individual Hooks improves comprehensibility immensely. I think this PR is a big improvement. Thank you for completing that. 🙇🏻
I left a couple of comments, but none are blockers.
packages/components/src/mobile/keyboard-aware-flat-list/test/use-keyboard-offset.native.js
Show resolved
Hide resolved
packages/components/src/mobile/keyboard-aware-flat-list/use-text-input-offset.native.js
Outdated
Show resolved
Hide resolved
added 3 commits
April 10, 2023 11:09
…nt keyboard height end coordinates to check different offset value
|
Thank you all for your valuable feedback and time for the reviews and testing! I can't wait to ship this 🚢 I'll start merging the PRs 🎉 🚀 |
geriux
force-pushed
the
rnmobile/remove-keyboard-aware-flatlist
branch
from
4f843a6
to
6e7556f
Compare
geriux
force-pushed
the
rnmobile/remove-keyboard-aware-flatlist
branch
from
00e9a18
to
a519085
Compare
dcalhoun
reviewed
Apr 10, 2023
packages/components/src/mobile/keyboard-aware-flat-list/use-text-input-offset.native.js
Outdated
Show resolved
Hide resolved
mokagio
added a commit
to wordpress-mobile/gutenberg-mobile
that referenced
this pull request
Apr 11, 2023
See Gutenberg commit 4454e829372874130c29b62ba9f04e03ea180480, WordPress/gutenberg#48791
mokagio
added a commit
to wordpress-mobile/gutenberg-mobile
that referenced
this pull request
Apr 12, 2023
See Gutenberg commit 4454e829372874130c29b62ba9f04e03ea180480, WordPress/gutenberg#48791
4 tasks
This was referenced Aug 2, 2023
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.
Fixes #40029
Fixes wordpress-mobile/gutenberg-mobile#4894
Related PRs:
What?
This PR fixes a high-priority bug on iOS where the scroll would jump to the top and then scroll back to the currently focused text input.
Why?
Because it affects the basic writing flow, and after several attempts to try to workaround that library I went to a different route by implementing it within the project and stop using our fork for react-native-keyboard-aware-scroll-view.
How?
BlockList
OnCaretVerticalPositionChangecontext and provider since it won't be used anymore.KeyboardAwareFlatListcomponent.isFloatingToolbarVisiblesince it was not returning the correct value.RichText (Block-editor)
useNativePropssince it won't be used anymore.KeyboardAwareFlatList
This component got a full refactor, it will leverage on the
InputStatefromRCTAztecViewfor different cases by using new hooks:scrollToTextInputOffsetfunction to scroll to the currently focused TextInput depending on where the caret is placed.VisualEditor
isAutoScrollEnabledstate value which is no longer needed.AztecInputState
AztecView
onCaretVerticalPositionChangeand now it will only set the value inAztecInputState.E2E Tests
getTitleElementhelper and the block insertion canary test.Testing Instructions
Test case 1
Test case 2
Testing Instructions for Keyboard
N/A
Screenshots or screencast
## Test case 1
TestCase1Before.mov
TestCase1After.mov
## Test case 2
TestCase2Before.mov
TestCase2After.mov