[RNMobile] Scroll to selected block that goes off the screen #26596

jd-alexander · 2020-10-30T12:55:25Z

Fixes wordpress-mobile/gutenberg-mobile#735

Gutenberg Mobile PR wordpress-mobile/gutenberg-mobile#2771

Description

This solution seeks to address the issue where a block may get cut off due to it being moved off the edge of the screen. In a nutshell, whenever the selected BlockListItem component (which is a parent component for rendered blocks) is updated then we check to see if this list item is within the current viewport and if not we are utilizing the underlying scroll behavior for the respective Lists on each platform to reposition that item on the screen.

How has this been tested?

To test follow the instructions below.

Open a post.
Select a block and press the move down button until the block reaches the edge of the screen.
Observe that once the Selected block that is being moved is positioned outside the bounds of the screen then the editor automatically scrolls to that position.
If you select a block and press the move up button, then the same behavior as expounded on above will take place. The editor will scroll instantaneously so that the block is in the viewport.

Screenshots

Below are two gifs of the functionality in action on Android and iOS

Moving a block off screen will auto-scroll the editor on Android

Moving a block off screen will auto-scroll the editor on iOS

Types of changes

The main tenets of the solution are as follows :

The measure method of the View component is being utilized to determine the width, height, and position of the view within the current page/screen.
The measureLayout method of the View component detects where within the FlatList the current BlockListItem exists so that we can utilize it's y position for scrolling behavior.
For scrolls, the implementation differs per platform because the FlatList's scroll functionality on iOS is managed by the KeyboardAwareFlatList so the ref to it's underlying ScrollView was utilized while on Android, scrolling was accomplished using the FlatList component itself.
To detect if a BlockListItem is within the window/screen we check to see if the pageY value is below zero ( which means we would need to scroll up) or above the window height ( which means we would need to scroll down)
You will notice within the code that we are utilizing scrollAdjustmentY in the scroll algorithm. This is to give the BlockListItem the ability to initiate scrolling without it's bottom or top being at the edge of the screen. This is also used within the calculation of the offset to scroll to so that we don't have an issue where we scroll to the bottom or top of a view and there's no space above or below the block.

Concerns

On Android, I have noticed that scrolling a large block will sometimes cause a bit of the block to be cut off. I will need to have another look at the algorithm in place to ensure that it incorporates the height of the block in the right way so this never happens.
I realized that scrolling a block pass a really large block that takes up the entire screen messes with its scrolling. It's like it stops scrolling on that very page because the block was moved a page away.

Below you will observe that the list scrolls but it stops on the screen with the block that has taken up the entire viewport.

You will also observe the same behavior when trying to scroll up with the block up movement action. The block is not scrolled to. The list stops the scroll action on the part of the page with the larger block.

I am wondering if this is being caused because a FlatList is Virtualized so the scroll is faster than the fill rate of the content being rendered asynchronously off-screen. If so, then probably a delay could be introduced to ensure the list has rendered or we would have to somehow extend the off-screen limit in some way. Another approach would be to have some form of timeout/flag mechanism that runs a check to see if the block is within the viewport after the scroll has completed and if not then it does another scroll to the position it was supposed to scroll to in the first place.

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
My code has proper inline documentation.
I've included developer documentation if appropriate.
I've updated all React Native files affected by any refactorings/renamings in this PR.

jd-alexander · 2020-11-03T02:31:57Z

packages/block-editor/src/components/block-list/block-list-item.native.js

+
+			// this adjustment factor is utilized to create an invisible bounds at the top and bottom of the screen where once the View enters within
+			//any of these bounds then the scrolling action is triggered.
+			const scrollAdjustmentY = isIOS ? 200 : 100;


I am always concerned about hardcoding these types of values because I am not sure if they will support every screen size. I haven't done a thorough test on multiple screen sizes as yet as I wanted to get some opinions on the proposed solution here.

I ended up going for a more dynamic solution using the window's height. Done here

guarani · 2020-11-04T16:25:51Z

packages/block-editor/src/components/block-list/index.native.js

+	setListRef( ref ) {
+		this.listRef = ref;
+	}
+


Is this being used?

It was being used before but not anymore. I will remove it 🙏

guarani · 2020-11-04T16:28:32Z

packages/components/src/mobile/keyboard-aware-flat-list/index.android.js

 * External dependencies
 */
 import { FlatList } from 'react-native';
-


Not sure if this line being removed was intentional, the codebase seems to use an empty line in these situations (it would be nice though to have a linter rule to ensure consistency, but that's another topic).

Thanks for spotting. I will remove it.

guarani · 2020-11-04T16:52:06Z

packages/components/src/mobile/keyboard-aware-flat-list/index.android.js

+export const KeyboardAwareFlatList = ( props ) => {
+	return (
+		<KeyboardAvoidingView style={ { flex: 1 } }>
+			<FlatList ref={ props.listRef } { ...props } />
+		</KeyboardAvoidingView>
+	);
+};


I'm curious, what was the reason for changing the lines surrounding <FlatList ref={ props.listRef } { ...props } />? It seems the other lines changed, but are not directly relevant to the PR. Was it due to a formatting change that the editor did on save?

Yes, it seems the editor probably formatted this. I will revert this and I will be mindful of it in other PRs 👍

Makes sense, I've had some formatting issues with the editor too.

guarani · 2020-11-04T17:00:56Z

packages/block-editor/src/components/block-list/block-list-item.native.js

+						( _x, _y, blockWidth, blockHeight, px, py ) => {
+							const window = Dimensions.get( 'window' );
+
+							if ( isSelected ) {


It looks like this can be moved up to the beginning of scrollToSelectedBlockIfItsNotVisible to a) avoid execution reaching here if there's no selected block and b) reduce the number of levels of indentation.

scrollToSelectedBlockIfItsNotVisible() { const { isSelected } = this.props; if (!isSelected) return; ...

guarani · 2020-11-04T17:02:24Z

packages/block-editor/src/components/block-list/block-list-item.native.js

+	scrollToSelectedBlockIfItsNotVisible() {
+		const { isSelected } = this.props;
+
+		if ( this.blockRef.current ) {


Just a nitpick: this could be changed to if ( !this.blockRef.current ) return; to reduce the number of levels of indentation.

Thanks for this. I did it this way because that's the way how I wrote code in Kotlin in WPAndroid and the linter would throw an error if I did this without the braces 😅

guarani

This looks really good! I think the approach is very solid too.

I noticed this strange glitch on iOS, it's hard to see what's happening in the gif below, so here's the steps to reproduce:

Scroll down to the bottom of the gallery block in the demo content
Tap the bottom of the gallery block
Notice the editor jumps up to near the top of the content

I noticed you talked about issues with really large blocks, so maybe this is related. I couldn't reproduce with the current WPiOS build, so it looks to be related to these changes. Can you reproduce it?

jd-alexander · 2020-11-04T18:11:32Z

This looks really good! I think the approach is very solid too.

Thanks for the review @guarani 🙇

I noticed you talked about issues with really large blocks, so maybe this is related. I couldn't reproduce with the current WPiOS build, so it looks to be related to these changes. Can you reproduce it?

Yes, this issue the same as the one I described in the PR. I am going to see what I can do to resolve it.

Do you have any thoughts about this comment :

I am wondering if this is being caused because a FlatList is Virtualized so the scroll is faster than the fill rate of the content being rendered asynchronously off-screen. If so, then probably a delay could be introduced to ensure the list has rendered or we would have to somehow extend the off-screen limit in some way. Another approach would be to have some form of timeout/flag mechanism that runs a check to see if the block is within the viewport after the scroll has completed and if not then it does another scroll to the position it was supposed to scroll to in the first place.

I am wondering how we can determine how the block list FlatList gets loaded ahead of time i.e how many blocks exist beyond the edge of the screen because it's probably not able to scroll to it because it's not fully rendered yet. I am really not sure. I will try to run some experiments to figure this out.

guarani · 2020-11-05T13:35:24Z

One option could be to create a separate RN project and try to reproduce this behavior with a scrollable list of "blocks", that could help with determining if the issue is related to the virtualization of lists or something else.

jd-alexander · 2020-11-05T18:39:20Z

One option could be to create a separate RN project and try to reproduce this behavior with a scrollable list of "blocks", that could help with determining if the issue is related to the virtualization of lists or something else.

Thanks, @guarani that's a really good idea! Locally, I came up with a fix that would start the scroll again if the block isn't actually on the screen after a delay. So it would scroll to the next page, realize the block isn't there and then scroll to the actual block position. ( This is necessary because it seems the Flat List only loads items for one screen ahead - I haven't investigated this as yet but these assumptions are what led to the solution I'm mentioning now.)

So far, I am thinking this might not be an optimal solution but in the interest of HACK week, I was wondering if it would be a sensible improvement because currently the block just goes off the edge which isn't good. Also, most users will not have blocks taking up the entire screen so it might be a justified compromise if no other solution is found. Wdyt?

jd-alexander · 2020-11-05T18:41:10Z

The only I found with my local solution/fix for this large black problem is that sometimes when the scroll is completely the block would be close to the end of that screen and a third scroll would be started so I need to modify the scroll logic a bit to ensure that the yOffset that's generated does not go close to the edge of the screen else the block will think it's outside the bounds of the visible area which it isn't and initiate an unneeded scroll operation.

guarani · 2020-11-11T20:00:13Z

Locally, I came up with a fix that would start the scroll again if the block isn't actually on the screen after a delay. So it would scroll to the next page, realize the block isn't there and then scroll to the actual block position.

I could take a look if you'd like, feel free to push the commit up.

So far, I am thinking this might not be an optimal solution but in the interest of HACK week, I was wondering if it would be a sensible improvement because currently the block just goes off the edge which isn't good. Also, most users will not have blocks taking up the entire screen so it might be a justified compromise if no other solution is found. Wdyt?

Yeah, I agree that most users won't have really long blocks. Is your preference to go forward with the latest code that you have on this PR (the code that I looked at already), or would you like to add the changes from your local test and I could take a look at that? I also agree that even if the solution is not perfect, if it's a good improvement we can ship it.

jd-alexander · 2020-11-11T20:09:45Z

I could take a look if you'd like, feel free to push the commit up.

Yeah, I agree that most users won't have really long blocks. Is your preference to go forward with the latest code that you have on this PR (the code that I looked at already), or would you like to add the changes from your local test and I could take a look at that? I also agree that even if the solution is not perfect, if it's a good improvement we can ship it.

Thanks for the feedback Paul! The solution I have isn't finished as yet but I am going to see if I can add in a final piece soon once I wrap up some other tasks on my list so we can ship this because it's a good improvement so far. It's buggy so I prefer to iron them out before pushing so you can be of more help 😄

This current solution involves checking if the py value of the yOffset passed will be within the bounds of any of the off screen bounds that's defined and if so then the yOffset being used to scroll needs to be readjusted to be within those bounds so another scroll isn't triggered. We are utilizing the height of the Flat List container to determine how the scroll view is divided based on the list's height.

jd-alexander · 2020-11-16T21:33:56Z

Hi @guarani just an update here. I pushed some code that I was working on for the issue but it doesn't work as well I intended it to. I am going to revisit this task at a later time and once I am able to get it to a good place that doesn't have any bugs, at that point I will ping you for a follow-up review. Thanks for your help so far. 🙇

guarani · 2020-11-17T17:49:08Z

Hi @guarani just an update here. I pushed some code that I was working on for the issue but it doesn't work as well I intended it to. I am going to revisit this task at a later time and once I am able to get it to a good place that doesn't have any bugs, at that point I will ping you for a follow-up review. Thanks for your help so far. 🙇

Sure, no worries! Anytime

jd-alexander added 4 commits October 26, 2020 20:32

added the ability to access the FlatList via a ref

6b1ea80

added functionality to detect when a block isn't in the view port

81be297

added more ref logic and ability to scroll based on focus.

e5dc593

added scroll logic to each block and now the flat list ties into that.

9c803d6

jd-alexander added [Feature] Writing Flow Block selection, navigation, splitting, merging, deletion... Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) labels Oct 30, 2020

jd-alexander added 11 commits October 31, 2020 14:15

created scroll implementation

55c91d9

Merge branch 'master' into rnmobile/issue-scroll

52ac27e

Simplified scrolling logic and removed unneeded code.

eaa1340

removed scrolling logic out of block.native.js

8935d61

sync Podfile

fb0add5

put the scrolling logic into the block list item.

4b58e62

utilize the scrollViewRef for iOS and the actual FlatList for Android

25dd5ad

added scroll y offset/adjustment for scroll behavior.

24d7132

put the scroll adjustment in a variable.

1a66ea7

named the value for the scroll offset

c9183c3

removed unneeded props.

6436632

jd-alexander requested review from geriux and guarani November 3, 2020 02:19

jd-alexander removed the Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) label Nov 3, 2020

jd-alexander marked this pull request as ready for review November 3, 2020 02:19

jd-alexander added the Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) label Nov 3, 2020

added comments to the scrolling algorithm.

7114389

jd-alexander commented Nov 3, 2020

View reviewed changes

jd-alexander changed the title ~~[RNMobile] Scroll to show block when its moved~~ [RNMobile] Scroll to selected block that goes off the screen Nov 3, 2020

jd-alexander added 4 commits November 2, 2020 21:40

added comments to the list scrolling function.

080eea4

added comments and utilized screen height for adjustment factor

29a0ed1

removed the ios / android specific factor and used one factor.

4c9ada8

simplified comment.

c82527d

jd-alexander mentioned this pull request Nov 3, 2020

Scroll to selected block that goes off the screen wordpress-mobile/gutenberg-mobile#2771

Closed

2 tasks

fixed comment about scroll view

09c762f

guarani reviewed Nov 4, 2020

View reviewed changes

jd-alexander added 2 commits November 14, 2020 20:50

fixed a typo

bd061d0

geriux removed their request for review February 12, 2021 11:22

Base automatically changed from master to trunk March 1, 2021 15:44

jd-alexander closed this Sep 9, 2022

[RNMobile] Scroll to selected block that goes off the screen #26596

[RNMobile] Scroll to selected block that goes off the screen #26596

Uh oh!

Conversation

jd-alexander commented Oct 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

How has this been tested?

Screenshots

Moving a block off screen will auto-scroll the editor on Android

Moving a block off screen will auto-scroll the editor on iOS

Types of changes

Concerns

Checklist:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jd-alexander Nov 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

guarani left a comment

Choose a reason for hiding this comment

Uh oh!

jd-alexander commented Nov 4, 2020

Uh oh!

guarani commented Nov 5, 2020

Uh oh!

jd-alexander commented Nov 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jd-alexander commented Nov 5, 2020

Uh oh!

guarani commented Nov 11, 2020

Uh oh!

jd-alexander commented Nov 11, 2020

Uh oh!

jd-alexander commented Nov 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

guarani commented Nov 17, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jd-alexander commented Oct 30, 2020 •

edited

Loading

jd-alexander Nov 3, 2020 •

edited

Loading

jd-alexander commented Nov 5, 2020 •

edited

Loading

jd-alexander commented Nov 16, 2020 •

edited

Loading