[RNMobile] Auto-scroll upon block insertion #57273
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
…ion of `KeyboardAwareFlatList `
fluiddot
added
[Type] Enhancement
|
Size Change: 0 B Total Size: 1.69 MB ℹ️ View Unchanged
|
…AwareFlatList` platform-specific components
This was referenced Dec 21, 2023
|
Hey @fluiddot 👋 I'm going through the test cases but I have a question for Case 8:
What do you mean by "The scroll remains in the same position."? Because I think that will depend if you paste the content from the title or if you paste it from the first Paragraph block. If you paste it from the Title, the scroll remains the same but if you paste it in the Paragraph block it scrolls down. This behavior was already like that I just want to make sure which steps should I take for this test case specifically. Thank you! |
Good observation. I'll split the test case into two:
Thanks for the call out 🙇 ! |
@geriux I've just updated the testing instructions to reflect the two mentioned test cases. |
|
Sharing my initial results so far:
AndroidLandscapeAutoscroll.movSteps to reproduce:
Are you able to reproduce? I even waited a bit for the content to be fully rendered but no luck so far. On Portrait mode, it does work as expected 🎉 I also checked with a text-based block and it doesn't auto-scroll, but It looks like it's an issue unrelated to these changes because I can also reproduce on the production app. I was expecting that at least for text-based blocks it would auto-scroll. |
geriux
reviewed
Dec 22, 2023
| [ scrollViewRef ] | ||
| ); | ||
|
|
||
| useImperativeHandle( ref, () => { |
|
Thanks @geriux for reviewing the PR 🙇 !
That's interesting, in previous tests I've made it seems to work nice. However, I've re-tested using a longer post (as the one proposed in the Preparation steps) and I encountered the issue. I'm afraid this might be related to the list virtualization, which AFAIK is only enabled on Android, because if you scroll down a bit the auto-scroll is triggered (see attached video). I'll check further but this might be a case to be handled differently. I'm thinking to use android-auto-scroll-landscape.mp4
I presume the problem I shared above affects all blocks, including text-based blocks. |
|
Following this comment and as discussed internally, due to the list virtualization on Android, the auto-scroll won't be triggered in all cases. In 87a7a12, I addressed an issue on Android related to triggering the auto-scroll multiple times for the same block, which was caused by the list virtualization. @geriux let me know if you could take a look at the recent changes, thanks 🙇 ! |
|
Flaky tests detected in 8827a4e. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7301841649
|
geriux
approved these changes
Dec 28, 2023
There was a problem hiding this comment.
LGTM! Great work!! 🤩
I've followed the test cases for both platforms using the latest builds and it worked as expected taking into account the limitations on Android.
The code changes look go to me as well, thank you for unifying the logic to re-use some of the code we had 🥇
4 tasks
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:
What?
Add a mechanism to automatically scroll to a block after being inserted.
Why?
How?
This section is divided into subsections related to the three main items tackled in this PR:
Conditions to trigger the auto-scroll
I assumed we needed two conditions for this:
wasBlockJustInserted).Based on the above, the following cases will trigger the auto-scroll:
And these won't trigger it:
The component to check the conditions
I was hesitant about whether to implement the check in the
BlockListor theBlockcomponents. I finally decided to use theBlockcomponent due to the need to retrieve the layout data for calculating the scroll offset (see more in the below section).Most of the logic related to this has been extracted in a new hook named
useScrollUponInsertion.Determine the offset for scrolling
The calculation of the offset requires four elements:
Reuse scroll logic from
KeyboardAwareFlatListA similar calculation is being performed to scroll when typing on a
RichTextcomponent (reference), so part of the implementation implied abstracting the logic performed in the componentKeyboardAwareFlatListto avoid redundancy and benefit from previous work. In this regard, the following hooks have been created to expose new scroll functions:useScrollToSectionuseScrollToElementAdditionally, as part of the abstraction, the following changes have been applied:
KeyboardAwareFlatListref usingforwardRef.useScrollToTextInputwithuseScrollToSection.KeyboardAwareFlatListto expose the new scroll functions., which will be used in the auto-scroll functionality.useScroll.Retrieve inserted block's layout
I originally thought about using the layout data provided by the block list context. But the fact, that at the moment of the insertion, it's not guaranteed that layout data is available. It made me lean toward using the native function
measureLayout. This also will allow us to get the offset when inserting inner blocks, which it's currently not provided by the block list context.Testing Instructions
Note
It's recommended to use the following installable builds for testing:
Warning
As mentioned in #57273 (comment), on Android the auto-scroll won't be triggered in all cases. We'll try to address this in the future.
Preparation:
Case 1 - Insert a non-text-based block after opening a post
Case 2 - Insert a text-based block after opening a post
Case 3 - Insert a block to the beginning
Case 4 - Insert a block after another located at the top screen
Case 5 - Insert a block after another located at the bottom screen
Case 6 - Insert a block before another located at the top screen
Case 7 - Insert a block before another located at the bottom screen
Case 8 - Paste content in the post title
Case 9 - Paste content in a text-based block
Case 10 - Copy block
...button located on the toolbar.Case 11 - Duplicate block
...button located on the toolbar.Case 12 - Landscape mode
Testing Instructions for Keyboard
N/A
Screenshots or screencast
android-auto-scroll-upon-insertion.mp4
ios-auto-scroll-upon-insertion.MP4