iOS - Add ability to scroll to caret when typing goes under the keyboard #474

pinarol · 2019-01-15T12:44:33Z

Fixes: #464

This PR adds ability to follow caret when typing goes under the keyboard.

WPiOS PR: wordpress-mobile/WordPress-iOS#10826

Depends on PRs:
wordpress-mobile/react-native-aztec#112
WordPress/gutenberg#13323
wordpress-mobile/react-native-keyboard-aware-scroll-view#3

To Test

Prerequisites

Run yarn install <-- important yarn start yarn ios
Might need to remember the scrolling mechanism, reviewers can refer to the diagram from this PR.

Test 1 - Paragraph block - Add new text to end

Add new text at the end of the block until caret goes under the keyboard
Verify that it automatically refreshes the scroll position to show the caret and also the block toolbar

Note: it is also possible to scroll for only showing the caret without the block toolbar, I wanted to keep this consistent with what happens when we open the keyboard, however, we can still change it for typing case.

Test 2 - Paragraph block - Add new text in the middle

Add new text in the middle of the block until caret goes under the keyboard
Verify that it automatically refreshes the scroll position to show the caret

Test 3 - Repeat tests 1,2 for Heading block

Test 4 - Android

We don't expect any change on Android side.

Run yarn android
Make sure Android example app opens successfully and you can do basic operations on it

etoledom

Thank you @pinarol for taking care of this. Great job!

Code looks good but I have one concern that I pointed out as a code comment.

If I understand correctly, what is happening is:

Aztec iOS wrapper adds the caret position info to the onSelectionChange bubbling event.
Aztec JS receive the event and calls a new onCaretVerticalPositionChange function added to its props.
Any component using aztec (rich-text, heading, paragraph) implement this props by passing it through.
Block holder also passes it through in the same way.
Block manager implement block-holder's onCaretVerticalPositionChange props calling a function on our keyboard-aware-flat-list.
keyboard-aware-flat-list reuses the logic used to scroll on focus to scroll te caret back to a visible area when it's necessary.

I did found an issue testing on WPiOS. At the beginning I wasn't sure what was the problem but I think it's related to the iOS version. Testing on iOS 11 (used by default by yarn ios it works fine, but using iOS 12, this happens:

Apart of that, I noticed that the behavior is a bit different than how it works on Android.
With this implementation, when typing at the end of the block, on iOS the content will scroll until the inline toolbar is visible, as it does on focus (that's nice I think). On Android, it will scroll until the caret is visible but will keep the inline toolbar hidden (feels a bit more expected at the beginning)

It would be good to make them both behave in the same way. I'm personally OK with any.
This is probably something to keep in mind for the future, and not to solve now.
Any opinion @iamthomasbishop ?

etoledom · 2019-01-16T07:42:53Z

src/block-management/block-manager.js

+			return;
+		}
+		if ( previousCaretY ) { //if this is not the first tap
+			this.scrollViewRef.props.refreshScrollForField( targetId );


This seems to be called just when running iOS, right? (referring to onCaretVerticalPositionChange)
If that's the case, wouldn't it be a bit confusing for someone from Android trying to understand/debug scroll behavior?

In the other hand, I'm not sure how could this be done differently 🤔

yes the scrollViewRef is null for Android and it will never run this code for Android. Maybe we can make it more explicit by putting an isIOS check, I can't think of any other solution either :(

I've moved handling of this event to platform specific files, at least we can make it more obvious this way, let me know what you think

pinarol · 2019-01-16T09:10:04Z

It would be good to make them both behave in the same way. I'm personally OK with any.
This is probably something to keep in mind for the future, and not to solve now.
Any opinion @iamthomasbishop ?

We can do this easily, actually I had already put a note about this in the PR description:

Note: it is also possible to scroll for only showing the caret without the block toolbar, I wanted to keep this consistent with what happens when we open the keyboard, however, we can still change it for typing case.

So it behaves that way only if caret is at the last line. I'll go and change it and request a second try from you @etoledom I can take it back if you feel like previous one was better, it is an easy change.

Just adding this link here to remind the decision mechanism of auto scrolling upon keyboard opening.

pinarol · 2019-01-16T09:13:13Z

If I understand correctly, what is happening is:

All correct @etoledom

pinarol · 2019-01-16T09:13:33Z

I did found an issue testing on WPiOS. At the beginning I wasn't sure what was the problem but I think it's related to the iOS version. Testing on iOS 11 (used by default by yarn ios it works fine, but using iOS 12, this happens:

Looking now

pinarol · 2019-01-16T11:20:34Z

I did found an issue testing on WPiOS. At the beginning I wasn't sure what was the problem but I think it's related to the iOS version. Testing on iOS 11 (used by default by yarn ios it works fine, but using iOS 12, this happens:

Solved this one, ready for another check @etoledom

pinarol · 2019-01-16T11:28:10Z

Just a small reminder before testing: you'll need to run yarn install again @etoledom

pinarol · 2019-01-16T11:29:46Z

I've also made changes for this one #474 (comment)

# Conflicts: # src/block-management/block-holder.js # src/block-management/block-manager.js

etoledom

Hey @pinarol - The updates look good!

Personally, scrolling just to show the caret feels more natural in this particular case. But I still would go with what @iamthomasbishop says is the best. 😁

The scrolling problem on iOS 12 seems to be fixed 🎉
I did find another issue, probably related:

This also happens only on iOS 12, and when the block is using all the viewable area.

As a tip, you can run this yarn ios --simulator "iPhone XS" to easily get the example project running on iOS 12.

iamthomasbishop · 2019-01-16T20:28:27Z

when typing at the end of the block, on iOS the content will scroll until the inline toolbar is visible, as it does on focus (that's nice I think)

I agree with this – My hope is that the inline toolbar is visible in most cases, this one included. When I focus a block, it should align to the bottom of the inline toolbar, and as I proceed to type beyond that line, it should adjust to do the same with the added line.

I also agree that it should be the same between both platforms.

pinarol · 2019-01-17T14:17:57Z

@etoledom I think that issue is related with #463 I've linked your comment under that issue. I'll be looking into it when I start to work on that.

Also I have reverted back the scrolling behavior to include the inner toolbar as @iamthomasbishop suggested. @iamthomasbishop you can compare 2 behaviors by trying iOS and Android at the same time, please let me once you have a chance to try it with the new build and if you think that sth needs to be changed don't hesitate to reach out! ( My opinions usually tend to change after trying :) )

etoledom

Given that the last issue was moved to another ticket, let's move on and 🚢 this.
Tested the last change on scrolling behavior and is ✅

Great job @pinarol! Thanks for taking care of this 🎉

Let's update the references before merging :)

iamthomasbishop · 2019-01-17T19:56:06Z

Just got a chance to take a test build for a spin. While I think there are a couple of things that we should do to further improve the interaction, this particular PR seems to solve the originally stated issue. I'm fine with shipping this and making further refinements separately.

Related PR on Android: #427 , as we should make sure Android behavior is matching.

Add ability to follow cursor when typing

467a9f3

pinarol added [Status] Not Ready For Review Writing Flow [OS] iOS labels Jan 15, 2019

pinarol added this to the Beta milestone Jan 15, 2019

pinarol self-assigned this Jan 15, 2019

pinarol requested a review from etoledom January 15, 2019 12:44

pinarol removed the [Status] Not Ready For Review label Jan 15, 2019

pinarol mentioned this pull request Jan 15, 2019

Gutenberg - Scroll to caret while typing wordpress-mobile/WordPress-iOS#10826

Merged

1 task

etoledom reviewed Jan 16, 2019

View reviewed changes

pinarol added 2 commits January 16, 2019 13:11

Update keyboard-aware-scroll-view ref

2cf7364

Move handling the caret position change to platform specific files

3bb5c68

Make a small formatting update

a8ee1b5

Merge branch 'develop' into issue/464-autoscroll-to-cursor

0f96660

# Conflicts: # src/block-management/block-holder.js # src/block-management/block-manager.js

etoledom reviewed Jan 16, 2019

View reviewed changes

pinarol mentioned this pull request Jan 17, 2019

Scrolling glitch when splitting a long paragraph #463

Closed

Update react-native-keyboard-aware-scroll-view ref

050c850

etoledom approved these changes Jan 17, 2019

View reviewed changes

pinarol added 4 commits January 17, 2019 20:29

Update react-native-keyboard-aware-scroll-view to v0.8.3

227a2a3

Update subrepo refs to point to master

d3c1212

Add missing symlink 'priority-queue' to fix unittests

76b4bc6

Add missing symlink 'priority-queue' to fix unittests

2cc6351

pinarol merged commit ad6ba7e into develop Jan 17, 2019

pinarol deleted the issue/464-autoscroll-to-cursor branch January 17, 2019 18:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

iOS - Add ability to scroll to caret when typing goes under the keyboard #474

iOS - Add ability to scroll to caret when typing goes under the keyboard #474

pinarol commented Jan 15, 2019 •

edited

Loading

etoledom left a comment

etoledom Jan 16, 2019

pinarol Jan 16, 2019

pinarol Jan 16, 2019

pinarol commented Jan 16, 2019 •

edited

Loading

pinarol commented Jan 16, 2019

pinarol commented Jan 16, 2019

pinarol commented Jan 16, 2019

pinarol commented Jan 16, 2019

pinarol commented Jan 16, 2019

etoledom left a comment

iamthomasbishop commented Jan 16, 2019 •

edited

Loading

pinarol commented Jan 17, 2019

etoledom left a comment

iamthomasbishop commented Jan 17, 2019

iOS - Add ability to scroll to caret when typing goes under the keyboard #474

iOS - Add ability to scroll to caret when typing goes under the keyboard #474

Conversation

pinarol commented Jan 15, 2019 • edited Loading

etoledom left a comment

Choose a reason for hiding this comment

etoledom Jan 16, 2019

Choose a reason for hiding this comment

pinarol Jan 16, 2019

Choose a reason for hiding this comment

pinarol Jan 16, 2019

Choose a reason for hiding this comment

pinarol commented Jan 16, 2019 • edited Loading

pinarol commented Jan 16, 2019

pinarol commented Jan 16, 2019

pinarol commented Jan 16, 2019

pinarol commented Jan 16, 2019

pinarol commented Jan 16, 2019

etoledom left a comment

Choose a reason for hiding this comment

iamthomasbishop commented Jan 16, 2019 • edited Loading

pinarol commented Jan 17, 2019

etoledom left a comment

Choose a reason for hiding this comment

iamthomasbishop commented Jan 17, 2019

pinarol commented Jan 15, 2019 •

edited

Loading

pinarol commented Jan 16, 2019 •

edited

Loading

iamthomasbishop commented Jan 16, 2019 •

edited

Loading