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

Schedule to request focus instead of asking immediately #1874

Merged

Conversation

hypest
Copy link
Contributor

@hypest hypest commented Feb 7, 2020

Fixes #1870

My current theory is that the new View is not yet in the proper position in the layout and requesting focus to it causes a scroll to the top of the ScrollView, since the coordinates of the view are not yet updated in respect to its parent. I can be wrong, haven't fully reached the bottom of this issue.

To test A:

  1. Open the demo app and add an image block at the top (to make the effect more noticeable)
  2. Start adding paragraph block. Add more than a few so the view will need to scroll to show the whole content
  3. Type "Enter" on the last paragraph block to create a new one
  4. There shouldn't be a "whole list" jump to the top and back down again

To test B:

There's a prior known issue with scrolling to new block added doesn't work so, this PR is not introducing that bug: #1881

PR submission checklist:

  • I have considered adding unit tests where possible. I haven't added any because we don't currently have a way to detect this scrolling jump in automated tests.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@hypest hypest added this to the 1.23 milestone Feb 7, 2020
@cameronvoell
Copy link
Contributor

I confirmed that this fixed the list scrolling to the top and back to the bottom when hitting enter to create a new block (testing steps) when I tested on Android in the example app.

I still saw some jumpy looking animations when I opened the block inserter and am not sure if that is already a known issue. Will confirm, and post a video start of day tomorrow if helpful.

@mchowning
Copy link
Contributor

This fix also appears to address the "scroll to the top when hitting enter to create a new paragraph block" issue in my testing.

@hypest hypest changed the base branch from release/1.22.0 to develop February 11, 2020 15:30
My current theory is that the View is not yet in the layout and
requesting focus to it causes a scroll to the top of the ScrollView,
since the coordinates of the view are not yet updated in respect to its
parent. I can be wrong, haven't fully reached the bottom of this issue.
@hypest hypest force-pushed the issue/1870-jump-to-the-top-and-back-when-new-paragraph branch from 0fa1e5d to 2647409 Compare February 11, 2020 16:16
@hypest hypest changed the base branch from develop to hotix/1.22.1 February 11, 2020 16:19
@hypest hypest added the bugfix label Feb 12, 2020
@hypest hypest requested a review from marecar3 February 12, 2020 08:24
@hypest hypest marked this pull request as ready for review February 12, 2020 08:24
@hypest
Copy link
Contributor Author

hypest commented Feb 12, 2020

Cameron and Matt have already given a first look at this PR and seems to fix the issue but, added you @marecar3 as a reviewer too to have a look too. Let me know if you can think of some scrolling cases that might be broken by this PR. Thanks!

@marecar3
Copy link
Contributor

marecar3 commented Feb 12, 2020

Hey @hypest, nice work here!
Good investigation!

It's working much better now, but it seems that scroll still has some jumpy behavior.

Device: Pixel 3
OS: Android 10

ezgif com-video-to-gif (14)

@hypest
Copy link
Contributor Author

hypest commented Feb 12, 2020

Right, I think that the jumping remaining is an older "issue", where the virtual keyboard actually gets hidden momentarily while the focus is first lost from the current block and then refocusing on the new block. The fix in this PR doesn't try to address that part :(

If it's not a new issue (would appreciate if you try v1.21.0 to check if it's there too), what do you think about addressing it in a different PR @marecar3?

Copy link
Contributor

@marecar3 marecar3 left a comment

Choose a reason for hiding this comment

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

LGTM! 👍

@hypest hypest merged commit a9aa736 into hotix/1.22.1 Feb 12, 2020
@hypest hypest deleted the issue/1870-jump-to-the-top-and-back-when-new-paragraph branch February 12, 2020 15:58
@Tug
Copy link
Contributor

Tug commented Feb 12, 2020

Tested on gb-mobile develop and gb master by cherry picking this commit and it indeed fixes the worst scroll issues (scroll loops and crashes) but as noticed by others it still jumps a couple times and end up not at the optimal place in the end in particular for group blocks it seems

@hypest
Copy link
Contributor Author

hypest commented Feb 13, 2020

but as noticed by others it still jumps a couple times and end up not at the optimal place in the end in particular for group blocks it seems

👋 @Tug , let me ask, is that behavior happening on an emulator and/or physical device?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants