Fix scroll position on iOS by pinarol · Pull Request #387 · wordpress-mobile/gutenberg-mobile

pinarol · 2018-12-17T12:47:05Z

Fixes #197 partially

This change fixes inconsistent scroll behavior with input texts on iOS. Android behavior didn't change since it wasn't problematic.

iOS Before:

After:

Testing prerequisites

For WPiOS

Checkout the PRs branch to any arbitrary folder and cd .. to it
run yarn install, yarn start:reset
Open XCode WPiOS on the latest develop
Clean build folder on Xcode, and then run the app

For Android

open grade.properties at WordPress-Android folder
add wp.BUILD_GUTENBERG_FROM_SOURCE = true to grade.properties
checkout the PRs branch in the subrepo of WordPress-Android repo
cd to WordPress-Android/libs/gutenberg-mobile
run yarn install, yarn start:reset
yarn wpandroid on a separate terminal in the same directory

To Test

Test 1 - Empty post on WPiOS with return button

Open an empty post via WPiOS app
Start typing and press return button to create a new block
Repeat this until you add about 10 paragraph blocks
Verify that typed text input is viewable
Close the keyboard
Verify that scroll position goes back to normal

Test 2 - Add block with (+) button

Open some profiled post via WPiOS app
Add new text blocks with (+) button
Verify that recently added text input is viewable

Test 3 - Verify inner block buttons are working

Use up&down&trash buttons when keyboard is open or closed
Verify that they are working

Test 4 - Repeat tests for Android and make sure we didn't break anything, Android behavior shouldn't change with this PR.

pinarol · 2018-12-17T12:48:46Z

cc @daniloercoli

pinarol · 2018-12-17T12:49:56Z

I will work on travis failures but feel free to take a look @koke

pinarol · 2018-12-17T14:18:11Z

we'll need to merge WordPress/gutenberg#12946 first

pinarol · 2018-12-17T14:18:45Z

ready to review @etoledom @mzorz

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

etoledom · 2018-12-18T08:02:36Z

src/components/keyboard-aware-flat-list.ios.js

+			extraScrollHeight={ extraScrollHeight }
+			extraHeight={ 0 }
+			innerRef={ ( ref ) => {
+				( this: any ).flatList = ref;


I believe that all these ( this: any ) notation in this file are not necessary. I tried changing them to this. and it works fine and flow is still happy.

Also, this reference seems to be to the scrollView and not to the flatList? I might be a bit lost 😅

I believe that all these ( this: any ) notation in this file are not necessary. I tried changing them to this. and it works fine and flow is still happy.

flow isn't happy to me :( that was the reason I used ( this: any ) in the first place, maybe you can have error if you run travis check in your local.

Also, this reference seems to be to the scrollView and not to the flatList? I might be a bit lost

right, I was using KeyboardAwareFlatList before and then changed it to KeyboardAwareScrollList but it looks like I forgot updating the name flatList. Will push a new commit for this 👍

pushed 401d698

flow isn't happy to me :( that was the reason I used ( this: any ) in the first place, maybe you can have error if you run travis check in your local.

May that be because we are declaring variables "on the fly" in a stateless function component instead of "properly" declaring them in a class before accessing them?

But again, I'm not asking to make that change, just trying to understand the issue 👍

yes right.. but we don't have a class here, KeyboardAwareFlatList is a function and we want these variable to be shared among these sub functions of KeyboardAwareFlatList.
so by using this we just insert the variables in the current context, not the ideal solution I agree.
I tried declaring KeyboardAwareFlatList as a React.PureCompoent/React.Compoent also but it causes unnecessary re-renders because extraData prop of FlatList changes and that change makes KeyboardAwareScrollView re-render.
Another solution would be declaring these variables in BlockManager and updating them via functions declared in the KeyboardAwareFlatList props, but I think that wouldn't be good since that variables are only needed here in iOS code :(

I see now. Sounds good to me! Let's ✅

etoledom · 2018-12-18T08:05:51Z

src/components/keyboard-aware-flat-list.ios.js

@@ -0,0 +1,65 @@
+import { KeyboardAwareScrollView } from 'react-native-keyboard-aware-scroll-view';
+/**
+* @format


Small nitpicking:
Can we have the import bellow those comments?

I put this here as a solution to flow error "Cannot resolve module react-native-keyboard-aware-scroll-view"
I think that happens because I added react-native-keyboard-aware-scroll-view to flowconfig ignore list because flow was trying to validate library source code as well. if there's another solution to this I can apply that happily I just couldn't find any.

I see, that makes sense. In that case can we put a small comment explaining that? :)

@hypest offered a new solution and I've pushed a new commit for it 4254072 It is so much better this way 🎉

Great solution! 😁

etoledom

Hey @pinarol - This is looking really good! 🎉

Auto scrolling behaves nice and consistent, and I love that the inner toolbar remains visible.

I added a couple of non-blocking/non-important comments to the code but in general it looks super good.

I saw something weird with the scrolling indicator, and a very big extra bottom padding when the keyboard is showing. There's a gif showing it:

I don't think this issue is blocking this PR since I don't see it really as something annoying for the users (as it was the auto-scrolling before). I would completely recommend you to merge this PR and just create an issue for that to be handled somewhen in the future (unless is really easy/fast to fix).

Great job! ✅

cc @mzorz - I haven't tested the Android WP App. In the Example app on android, autoscroll looks good but it doesn't show the inner toolbar. If that's how it was before, it would be great to match that behavior :)

etoledom · 2018-12-18T08:27:52Z

src/components/keyboard-aware-flat-list.ios.js

+			extraScrollHeight={ extraScrollHeight }
+			extraHeight={ 0 }
+			innerRef={ ( ref ) => {
+				( this: any ).flatList = ref;


Also, this reference seems to be to the scrollView and not to the flatList? I might be a bit lost 😅

Also replaced some spaces with tabs

pinarol · 2018-12-18T12:03:40Z

@etoledom I've opened a bug for that issue, I don't think it is easily fixable, it is related with the inner mechanisms of KeyboardAwareScrollView.

mzorz · 2018-12-18T12:21:43Z

Test 4 - Repeat tests for Android and make sure we didn't break anything, Android behavior shouldn't change with this PR.

I've been testing this for a while (smoke tests mostly), it seems to be OK as per my tests.

pinarol · 2018-12-18T12:39:20Z

@etoledom @mzorz about showing the inner toolbar on Android: as far as I know KeyboardAvoidingView can't do it, I had tried KeyboardAwareScrollView on Android side also to achieve that but it behaves different, it first scrolls to the cursor and after some little time it scrolls again, this time for showing the inner toolbar. This is because Android already provides scrolling to the cursor position in the OS level(I guess) and the library just handles the extra scroll, this makes it look scrolling happen in 2 parts and doesn't look very good.

marecar3 · 2018-12-18T12:57:30Z

src/components/keyboard-aware-flat-list.android.js

+* @format
+* @flow
+*/
+import { FlatList, KeyboardAvoidingView } from 'react-native';


Hey @pinarol . Late one...
We didn't use KeyboardAvoidingView from 'rect-native' but we had our abstract presentation of it in components/keyboard-avoiding-view.ios.js and components/keyboard-avoiding-view.android.js

That was introduced to fix the problem with toolbar visibility after orientation change in WPiOS.

thanks for the heads up @marecar3 addressing that in #397

marecar3 · 2018-12-18T13:29:59Z

src/block-management/block-manager.js

 import BlockPicker from './block-picker';
 import HTMLTextInput from '../components/html-text-input';
 import BlockToolbar from './block-toolbar';
 import KeyboardAvoidingView from '../components/keyboard-avoiding-view';


Hey @pinarol , here we are importing from our file.

pinarol added 8 commits December 16, 2018 23:46

Add KeyboardAwareFlatList to handle text focus

f04ee5b

Replace KeyboardAwareFlatList with KeyboardAwareScrollView

4bf5833

Fix lint issues

1027a9f

Merge branch 'master' into fix/scroll-position

fa5e9a7

Revert android part to KeyboardAvoidingView

253dcfe

Merge branch 'master' into fix/scroll-position

5e6d209

Update gutenberg subrepo ref

a10cf88

Set keyboardShouldPersistTaps to handled

689023d

pinarol added [Status] Needs iOS Dev [Status] Needs Android Dev labels Dec 17, 2018

pinarol added this to the Alpha milestone Dec 17, 2018

pinarol self-assigned this Dec 17, 2018

pinarol requested review from etoledom and mzorz December 17, 2018 12:47

pinarol added the [Status] Not Ready For Review label Dec 17, 2018

pinarol added 2 commits December 17, 2018 16:41

Fix travis issues

8519695

Fix failing unittests

7c1864f

pinarol mentioned this pull request Dec 17, 2018

Revert "Remove the call that does blur the undelying native component WordPress/gutenberg#12946

Merged

5 tasks

pinarol removed the [Status] Not Ready For Review label Dec 17, 2018

pinarol added 3 commits December 17, 2018 17:52

Update gutenberg subrepo ref

98bd903

Update gutenberg subrepo ref

cdaa613

Merge branch 'master' into fix/scroll-position

15a6265

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

etoledom reviewed Dec 18, 2018

View reviewed changes

etoledom approved these changes Dec 18, 2018

View reviewed changes

Update flatList as scrollViewRef

401d698

Also replaced some spaces with tabs

Fix flow issue

4254072

Update gutenberg subrepo to point to mobile branch

f14772b

pinarol merged commit 6dff0c7 into master Dec 18, 2018

pinarol deleted the fix/scroll-position branch December 18, 2018 12:51

marecar3 reviewed Dec 18, 2018

View reviewed changes

This was referenced Dec 18, 2018

iOS Only - Re-arrange scroll position of text inputs on landscape mode #397

Merged

Disable code and more blocks for the alpha #398

Closed

hypest mentioned this pull request Jun 4, 2021

[RNmobile] Scroll to new-block indicator WordPress/gutenberg#31144

Closed

13 tasks

Conversation

pinarol commented Dec 17, 2018

Uh oh!

pinarol commented Dec 17, 2018

Uh oh!

pinarol commented Dec 17, 2018

Uh oh!

pinarol commented Dec 17, 2018

Uh oh!

pinarol commented Dec 17, 2018

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!

etoledom Dec 18, 2018 • 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!

etoledom left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pinarol commented Dec 18, 2018

Uh oh!

mzorz commented Dec 18, 2018

Uh oh!

pinarol commented Dec 18, 2018

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

etoledom Dec 18, 2018 •

edited

Loading