Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[iOS] Fix autoscroll behaviour. #1939

Merged
merged 19 commits into from
Mar 13, 2020
Merged

Conversation

etoledom
Copy link
Contributor

@etoledom etoledom commented Feb 20, 2020

This PR tries to fix wrong autoscroll behaviour on iOS.

Fixes wordpress-mobile/WordPress-iOS#13224
Fixes #1274
Fixes #1276
Fixes #1275
Fixes #891

Note: Aztec changes on Podfile are unrelated. It might be a missing yarn preios on a previous PR.

gutenberg PR: WordPress/gutenberg#20334
rn-keyboard-aware-scroll-view PR: wordpress-mobile/react-native-keyboard-aware-scroll-view#9

Enhancements:

  • The position to scroll to is now properly calculated (to show the inner toolbar).
  • The is caret under keyboard calculation used to kick the autoscroll on typing now considers the extra height (the bars). In this way, if the bottom part of the inner toolbar goes behind the global toolbar, it will autoscroll.
  • Autoscrolling on lines that are not the last one, now will show a bit more space below the line. This is just to give more "air" to it. (cc @iamthomasbishop to know your opinion about it).

All these changes are demonstrated on the following gifs:

autoscroll-iphone
autoscroll-ipad

To test:

  • Follow the examples on the provided Gifs.
  • Test with longer texts too.
  • Test on other RichText powered blocks.

NOTE: For the external keyboard test is necessary to test with a real device.

@pinarol - Any other edge case that you consider we should test?

PR submission checklist:

  • I have considered adding unit tests where possible.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@etoledom etoledom added [Type] Enhancement Improves a current area of the editor [OS] iOS bugfix labels Feb 20, 2020
@etoledom etoledom self-assigned this Feb 20, 2020
@SergioEstevao
Copy link
Contributor

@etoledom The link for the rn-keyboard-aware-scroll-view PR is incorrect.

@etoledom
Copy link
Contributor Author

@etoledom The link for the rn-keyboard-aware-scroll-view PR is incorrect.

Fixed, thank you!

@pinarol
Copy link
Contributor

pinarol commented Feb 20, 2020

@pinarol - Any other edge case that you consider we should test?

On the top of my head:

  • In a line other than the last line: typing till the caret is in the next line and checking if caret gets under the keyboard
  • devices with/without safe area
  • switching between landscape&portrait
  • text inputs other than rich text(but keep in mind there’s no scroll to caret for them)
  • merging/splitting blocks
  • rich text inside group block

Copy link
Contributor

@iamthomasbishop iamthomasbishop left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@etoledom The scroll refinements look great, nice work! 👏

Copy link
Contributor

@iamthomasbishop iamthomasbishop left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@etoledom The scroll refinements look great, nice work! 👏

@pinarol
Copy link
Contributor

pinarol commented Feb 24, 2020

The is caret under keyboard calculation used to kick the autoscroll on typing now considers the extra height (the bars). In this way, if the bottom part of the inner toolbar goes behind the global toolbar, it will autoscroll.

This can be related to removed parameters during the first set of inner blocks feature:

Screen Shot 2020-02-21 at 20 33 43

659b327

There are already some props we can use for this, like: inputAccessoryViewHeight, extraBottomInset. So before introducing new fixes we could benefit from trying these.

@pinarol
Copy link
Contributor

pinarol commented Mar 11, 2020

Thanks! it is so much better! I tested this with following:

  • In a line other than the last line: typing till the caret is in the next line and checking if caret gets under the keyboard
  • switching between landscape&portrait
  • text inputs other than rich text(but keep in mind there’s no scroll to caret for them)
  • merging/splitting blocks
  • rich text inside group block
  • Devices with/without safe area > Failed ❗️

It looks like the current mechanism acts differently on devices without safearea, and the extra caret space isn't working on iPhone 8. It manifests itself when the caret is somewhere different than the last line:

non-iphonex

I think this is not too big of a deal on user's end but it is worth investigating because we made it so far and it'd be good not to leave a mystery bug for the future 🤔

@etoledom
Copy link
Contributor Author

Thank you for testing @pinarol !

This last issue should be fixed by wordpress-mobile/react-native-keyboard-aware-scroll-view#10

I updated this PR to include that change, so it can be tested after pulling and yarn install

Copy link
Contributor

@pinarol pinarol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Devices with/without safe area

Tested again and it is working awesome! Great job 🎉

This is also fixing the below issues, let's close them after merging this.
#1276
#1275

@etoledom
Copy link
Contributor Author

Thanks for testing @pinarol 🎉
Added the extra issues this PR fixes to the description.
5 issues going away :yay:

@etoledom etoledom added this to the 1.24 milestone Mar 13, 2020
@etoledom etoledom merged commit 7500e0e into develop Mar 13, 2020
@etoledom etoledom deleted the issue/fix-caret-scroll-calculation branch March 13, 2020 18:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment