[RNMobile] Long-press on inserter to show options for "add above" and "add below"

[ceyhun]

Description

Fixes: wordpress-mobile/gutenberg-mobile#1433

How has this been tested?

I've tested it using iPhone X Simulator with iOS 13.2 and Pixel 3 Android emulator with API 28.

Elements

1. Inserter button (+) behavior
   (The current behavior on normal press should not change)
2. "ADD BLOCK HERE" indicator behavior
3. Bottom/ActionSheet behavior

Steps

1. Long press on the inserter button (+)
2. Bottom sheet should be presented
3. Options should should be different depending on the test case
4. Select an option
5. "ADD BLOCK HERE" indicator should be shown in the correct place depending on the option you picked
6. Menu to pick a block should be shown
7. Select a block
8. Block should be inserted in the correct place

Cases

1. When no block is selected
2. When title is selected
   • Bottom sheet options should be:
     Add to beginning
Add to end
3. When unmodified default block (Paragraph) is selected
   • Unmodified default block (Paragraph) should be kept as it is and insertion should happen according to the option that's picked.
4. When a block other than Paragraph is selected
5. When a block in a Media & Text block is selected
6. When a block in a Group is selected
   Note: Appending a block in an empty group currently doesn’t work. There’s a PR to fix that: [RNMobile] fix group appender behaviour #18564. A group block with some inner blocks can be added locally to initial-html for testing.
   • Bottom sheet options should be:
     Add Block Before
Add Block After

Screenshots

Nothing selected

iOS	Android

Unmodified Paragraph selected

iOS	Android

Image selected

iOS	Android

Inside Media & Text

iOS	Android

Inside Group

iOS	Android

Types of changes

New feature (non-breaking change which adds functionality)

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. .

[ceyhun]

Implemented all the required functionality. Now it also shows the Bottom/ActionSheet on long press and you can choose to insert before or after.

[SergioEstevao]

I've tested this on iOS and is working as intended.

The only little thing that I noticed is that when we add the new block to out of the screen the app doesn't scroll to that location. This may have been an already existing bug, but if possible to arrange on this PR will be great.

Code wise is looking solid!

[iamthomasbishop]

@ceyhun I've got a few bits of feedback as I'm working through a design review.

• The actions list on the Bottom Sheet on Android should be left-aligned to the 16px keyline
• You can remove the Cancel button from the Bottom Sheet on Android
• On the last example (when long-pressing on the Inserter from an empty new block), I noticed something we might want to consider – adding a Replace Current Block option in addition to the before/after options when showing the Action/Bottom Sheet. If we add this, I would add it in the in the middle of the list (Add to Beginning, Replace Current Block, Add to End)

[ceyhun]

The only little thing that I noticed is that when we add the new block to out of the screen the app doesn't scroll to that location. This may have been an already existing bug, but if possible to arrange on this PR will be great.

Sure! I'll check what’s the underlying issue and if it makes sense to include it in this PR.

[ceyhun]

@iamthomasbishop thanks for the feedback!

• The actions list on the Bottom Sheet on Android should be left-aligned to the 16px keyline

How about also displaying the icons for Android? It will look like this in the end:

• You can remove the Cancel button from the Bottom Sheet on Android

👍

• On the last example (when long-pressing on the Inserter from an empty new block), I noticed something we might want to consider – adding a Replace Current Block option in addition to the before/after options when showing the Action/Bottom Sheet. If we add this, I would add it in the in the middle of the list (Add to Beginning, Replace Current Block, Add to End)

In this case should we show the Replace Current Block option only for when an unmodified/empty paragraph block is selected or also for other types of blocks as well when they are not yet modified?
Also I think this option doesn’t exist on web editor menus and if we decide to show icons on Android what can we use for this option?

[iamthomasbishop]

How about also displaying the icons for Android? It will look like this in the end:

Yes! Absolutely 👍 😄

In this case should we show the Replace Current Block option only for when an unmodified/empty paragraph block is selected or also for other types of blocks as well when they are not yet modified?

Ah, good question. I was mainly thinking empty Paragraph blocks, but other RichText blocks such as Heading should probably behave the same way. For example, if I add a heading block, leave it empty and come back to select it, it'd seem intuitive to me if the same logic applied to a Heading.

Looking at the full list of available blocks, we could probably apply the same to other RichText blocks like Lists, Quote, Code, and Pre blocks, but I think those would be less intuitive than simple blocks like Paragraph and Heading. There are some that wouldn't make sense, like media blocks (Image, Video), More, Page Break, Separator, Spacer, Media & Text, etc.

[ceyhun]

@iamthomasbishop Other blocks than Paragraph could be a little tricky as we may need to keep an up-to-date list of “replaceable” blocks and add new blocks there as they become available. How about only making paragraph replaceable for now? It would also be consistent with the behavior of normal-press to the inserter (+) because only empty paragraph blocks are replaced on normal-press right now.

For the Replace Current Block option's icon I tried with this one from the web:

It looks like this on Android. What do you think?

[ceyhun]

@SergioEstevao about scrolling to a new block that’s out of the screen:

At first I looked if it was working with blocks that open the keyboard.

Both iOS and Android use KeyboardAwareFlatList component to render the BlockList.
But inside KeyboardAwareFlatList:

• iOS uses react-native-keyboard-aware-scroll-view
• Android uses KeyboardAvoidingView from "react-native" but a behavior prop is not passed in so it acts as a normal View

For both iOS and Android

• With blocks that open the keyboard:

  • If the new block is at the top of the current render window in the list it doesn’t work.
  • If the new block is at the bottom of the current render window in the list it works.

• With blocks that don't open keyboard it doesn’t work at all as you noticed.

So there were 2 cases where it wasn’t working:

1. New block that does not open keyboard
2. New block that does open keyboard but is at the top of the current render window

Then I looked into FlatList.scrollToIndex method to solve this. The documentation says:

Cannot scroll to locations outside the render window without specifying the getItemLayout prop.

The getItemLayout needs every item’s height calculated in advance. It might be tricky to calculate every block’s height beforehand. So I didn’t go with the FlatList solution.

Finally I looked into the react-native-keyboard-aware-scroll-view a bit. I realised that it has a scrollIntoView method. It takes a ref as a parameter and scrolls it into the view. So I used this method. Also used the same component for Android to be able to call this method. Now this works for both cases and both platforms.

iOS	Android

[hypest]

Nice to see react-native-keyboard-aware-scroll-view working for Android @ceyhun , thanks! I wonder though, perhaps there were some issues we encountered in the past that prevented us from using it for Android. @pinarol , do you perhaps recall why we ended up only using the library for iOS and not for Android?

[iamthomasbishop]

Other blocks than Paragraph could be a little tricky as we may need to keep an up-to-date list of “replaceable” blocks and add new blocks there as they become available

@ceyhun This is a good point regarding scalability. Let's just start w/ paragraph as this is the most common part.

For the Replace Current Block option's icon I tried with this one from the web:

I would avoid using the same icon that is already used for re-usable blocks. Let's use the simple (+) icon.

Also, the scrolling behavior you shared above looks great. I know we've had some technical issues with scrolling behavior but improving that aspect of the editor has been a design-wish of mine so hopefully this helps 😄

[pinarol]

I wonder though, perhaps there were some issues we encountered in the past that prevented us from using it for Android. @pinarol , do you perhaps recall why we ended up only using the library for iOS and not for Android?

We simply didn't need that for Android because Android was by default able to scroll to the focused text field when the keyboard opens. So, KeyboardAvoidingView was just enough. Also, it was written in the README that Android support of react-native-keyboard-aware-scroll-view wasn't good. From what I have seen this PR uses it with enableOnAndroid={ false } so it will continue using Android's default autoscroll capabilities. The thing that worries me more is dropping KeyboardAvoidingView and not putting something that replicates its functionality. I am a bit confused how it can be possible because then this means KeyboardAvoidingView was not necessary at all 🤔OR we are missing something.

What I can suggest is trying to avoid dropping KeyboardAvoidingView just for being able to call scrollIntoView. We can consider adding scrollIntoView function in our code base and call it by passing scrollView's ref to it. This could minimize possible future regressions. It seems like a pretty standalone function to me that doesn't use a lot from that lib.

[ceyhun]

I would avoid using the same icon that is already used for re-usable blocks. Let's use the simple (+) icon.

@iamthomasbishop You mean this one? It's used for the inserter button as well.

[ceyhun]

I am a bit confused how it can be possible because then this means KeyboardAvoidingView was not necessary at all 🤔OR we are missing something.

@pinarol on Android a behavior prop is not passed into the KeyboardAvoidingView
https://github.com/WordPress/gutenberg/blob/master/packages/components/src/mobile/keyboard-aware-flat-list/index.android.js#L12

I think it acts as a normal View when a behavior prop is not passed in. But I'm not 100% sure 😄
https://github.com/jtreanor/react-native/blob/master/Libraries/Components/Keyboard/KeyboardAvoidingView.js#L218

What I can suggest is trying to avoid dropping KeyboardAvoidingView just for being able to call scrollIntoView. We can consider adding scrollIntoView function in our code base and call it by passing scrollView's ref to it. This could minimize possible future regressions. It seems like a pretty standalone function to me that doesn't use a lot from that lib.

On Android we can still keep the KeyboardAvoidingView to avoid causing regressions. But in this case the only scrollable element inside KeyboardAwareFlatList remains the FlatList. I can try to extract scrollIntoView function into KeyboardAwareFlatList and pass the FlatList's ref to it. It might still work since FlatList also inherits ScrollView props.

[pinarol]

I think it acts as a normal View when a behavior prop is not passed in. But I'm not 100% sure 😄
https://github.com/jtreanor/react-native/blob/master/Libraries/Components/Keyboard/KeyboardAvoidingView.js#L218

In that case I wouldn't worry. One thing to make sure though, we are not using react-native's KeyboardAvoidingView directly from this file, we have our own wrapper on top of it, so let's check if we don't use behavior there either.

It might still work since FlatList also inherits ScrollView props.

Makes sense but I am also OK with the current state if we make sure KeyboardAvoidingView acts no different than a normal View in this case.

[iamthomasbishop]

You mean this one?

@ceyhun Yep that one. That's okay, it makes sense to use the same icon as it essentially performs the default inserter action.

[SergioEstevao]

@SergioEstevao about scrolling to a new block that’s out of the screen:

At first I looked if it was working with blocks that open the keyboard.

Both iOS and Android use KeyboardAwareFlatList component to render the BlockList.
But inside KeyboardAwareFlatList:

• iOS uses react-native-keyboard-aware-scroll-view
• Android uses KeyboardAvoidingView from "react-native" but a behavior prop is not passed in so it acts as a normal View

For both iOS and Android

• With blocks that open the keyboard:

  • If the new block is at the top of the current render window in the list it doesn’t work.
  • If the new block is at the bottom of the current render window in the list it works.

• With blocks that don't open keyboard it doesn’t work at all as you noticed.

So there were 2 cases where it wasn’t working:

1. New block that does not open keyboard
2. New block that does open keyboard but is at the top of the current render window

Then I looked into FlatList.scrollToIndex method to solve this. The documentation says:

Cannot scroll to locations outside the render window without specifying the getItemLayout prop.

The getItemLayout needs every item’s height calculated in advance. It might be tricky to calculate every block’s height beforehand. So I didn’t go with the FlatList solution.

Finally I looked into the react-native-keyboard-aware-scroll-view a bit. I realised that it has a scrollIntoView method. It takes a ref as a parameter and scrolls it into the view. So I used this method. Also used the same component for Android to be able to call this method. Now this works for both cases and both platforms.

iOS Android

@ceyhun tested this on iOS and now it's working perfectly.

[Tug]

Nice fix for the autoscroll to block, I tested it and it works well 👍
Would have been best to do it in another PR imo as I feel this PR is quite complex already. At least it looks complex to me because of the changes of state we introduce, I find it a bit hard to follow across the different components. Anyway it's ok to have it here since Sergio asked for it ;)

I'd like us to think of an alternative approach that does not involve modifying the web store. Let's try to keep the insertionPoint reducer "UI only" if possible.

[Tug]

So it seems that by adding this new property we stray away from the insertion point being a UI state only (where every property is just used to render the BlockInsertionPoint component) to a more complex one where we store user preference in it. So this store now tells us where and how a new block will be inserted. It's hard to see how the web could use that and I'm wondering if it wouldn't be best to just store this state into the Inserter instead. @ceyhun did you give it a try?

Minor thing: canReplaceBlock doesn't sound like the most appropriate name since it's actually a "directive" for the inserter. Maybe willReplaceBlock or shouldReplaceBlock instead?

[ceyhun]

It is already derived from Inserter’s local state via getShouldReplaceBlock function. But we need to pass this state to the BlockList so it hides the blocks where the BlockInsertionPoint will replace. Inserter and BlockList don't share a very close parent so instead of lifting the state all the way up to their common parent and drilling down, I chose to store this state it in the store :) It might be used in web for a similar behavior with "Add Block Before/After" for where a block in the list could be hidden when it will be replaced and BlockInsertionPoint is shown in it's place.

But I agree this state is now not about only showing/hiding BlockInsertionPoint but it's even about showing/hiding a block in the list.

But maybe there is another way without changing the web store. Instead of hiding a block we can try to remove the block completely from BlockList when BlockInsertionPoint is shown and the block is going to be replaced. If the block insertion is cancelled (via closing the menu) we can re-insert the block again. It should be easy to re-insert the block again because the replace only happens for unmodified default blocks (empty paragraph) right now. If a new block is picked from the menu instead of cancelling we can just call insert instead of replace to add the new block. This way we might even remove show/hide block logic completely from BlockList.

What do you think?

[Tug]

What do you think?

I think it sounds quite hacky, but that's actually a clever way to do it without compromising on the functionalities, I like it 👍

I say it's hacky because this approach won't work if isSelectedBlockReplaceable becomes more complex than just isSelectedUnmodifiedDefaultBlock. Maybe we will then be able to "store" the type of block that was removed in the Inserter state, but then it feels even more hacky 🤔

I think it's fine for now to have more custom code in the native Inserter since our version does more than the web. Adding new attributes to the store that are not presently needed is hardly justifiable at this point in the project.

[ceyhun]

I just pushed the mentioned changes (and also reverted scrollview related changes), feel free to check it out again :)

[ceyhun]

Would have been best to do it in another PR imo as I feel this PR is quite complex already. At least it looks complex to me because of the changes of state we introduce, I find it a bit hard to follow across the different components. Anyway it's ok to have it here since Sergio asked for it ;)

Group	Media&Text

I just found this bug with group blocks on Android now after merging develop into guteberg-mobile and rebasing my changes here onto guteberg-mobile/develop’s gutenberg ref. Sometimes the groups are not being rendered correctly. It seems to be caused by using react-native-keyboard-aware-scroll-view on Android because without that it just works fine.

We might need a more complex solution that doesn’t involve react-native-keyboard-aware-scroll-view for Android. @SergioEstevao For this I think it makes more sense to do it in another PR too.

[pinarol]

Sometimes the groups are not being rendered correctly. It seems to be caused by using react-native-keyboard-aware-scroll-view on Android because without that it just works fine.

We might need a more complex solution that doesn’t involve react-native-keyboard-aware-scroll-view for Android. @SergioEstevao For this I think it makes more sense to do it in another PR too.

InnerBlock enhancements are merged recently and it was a big change, it could have effected this PR as well. I didn't quite understand the buggy behavior from that screenshot but I'd avoid merging this if it is introducing some problems on Group block. I'd also suggest checking Media-Text because it uses InnerBlocks like Group, and it is already on prod. We want to ship Column and Group very soon so we wouldn't want to introduce blockers for them.

Sometimes the groups are not being rendered correctly

Could you elaborate on this a bit? What kind of problem are you observing?

Also, there was this one suggestion that you made @ceyhun, would it help solving the issue?

On Android we can still keep the KeyboardAvoidingView to avoid causing regressions. But in this case the only scrollable element inside KeyboardAwareFlatList remains the FlatList. I can try to extract scrollIntoView function into KeyboardAwareFlatList and pass the FlatList's ref to it. It might still work since FlatList also inherits ScrollView props.

react-native-keyboard-aware-scroll-view can be buggy on Android, the developers of the original library made it clear in the README that Android support wasn't perfect.