Android: Keep ScrollView content visible after edits #11000

rigdern · 2016-11-17T21:46:42Z

Suppose that the user is scrolled to the bottom of a ScrollView. Next, the ScrollView's content is edited such that the height of the content changes and the current scroll position is larger than the new height of the content. Consequently, the user sees a blank ScrollView. As soon as the user interacts with the ScrollView, the ScrollView will jump to its max scroll position.

This change improves this scenario by ensuring that the user is never staring at a blank ScrollView when the ScrollView has content in it. It does this by moving the ScrollView to its max scroll position when the scroll position after an edit is larger than the max scroll position of the ScrollView.

Here are some pictures to illustrate how this PR improves the scenario described above:

Test plan (required)

Verified ScrollView keeps the content visible after an edit in a test app. Also, my team uses this change in our app.

Adam Comella
Microsoft Corp.

facebook-github-bot · 2016-11-17T21:47:43Z

By analyzing the blame information on this pull request, we identified @astreet and @janicduplessis to be potential reviewers.

astreet

Thanks for fixing this! Just some small changes before landing.

Style nit: we don't prepend functions/private vars with this.

astreet · 2016-11-22T18:51:27Z

ReactAndroid/src/main/java/com/facebook/react/views/scroll/ReactScrollView.java

@@ -98,6 +101,8 @@ public ReactScrollView(ReactContext context, @Nullable FpsListener fpsListener)
    } else {
      mScroller = null;
    }
+
+    this.setOnHierarchyChangeListener(this);


nit: don't prefix with this

astreet · 2016-11-22T18:53:45Z

ReactAndroid/src/main/java/com/facebook/react/views/scroll/ReactScrollView.java

+    int currentScrollY = getScrollY();
+    int thisHeight = this.getMeasuredHeight();
+
+    int maxScrollY = Math.max(0, contentHeight - thisHeight);


We also need to factor in padding (see this from line 337):

int scrollRange = Math.max( 0, getChildAt(0).getHeight() - (getHeight() - getPaddingBottom() - getPaddingTop()));

(please extract this into a private helper)

astreet · 2016-11-22T18:54:53Z

ReactAndroid/src/main/java/com/facebook/react/views/scroll/ReactScrollView.java

+    this.onContentLayoutChange(v, left, top, right, bottom, oldLeft, oldTop, oldRight, oldBottom);
+  }
+
+  private void onContentLayoutChange(View v, int left, int top, int right, int bottom, int oldLeft, int oldTop, int oldRight, int oldBottom) {


Just inline this above? Or create a inner class to make things more obvious that onLayoutChange refers to the content view

astreet · 2016-11-22T18:55:49Z

ReactAndroid/src/main/java/com/facebook/react/views/scroll/ReactScrollView.java

+
+  }
+
+  private void setContentView(int expectedChildCount) {


nit: seems like we can just put part of this logic in onChildViewAdded and the rest in onChildViewRemoved. There are already assertions in android code that a scrollview can only have 1 child so I'm not really worried about that.

mkonicek · 2016-11-22T19:13:53Z

Thanks for fixing this! I'll let @astreet review.

rigdern · 2016-11-23T02:35:27Z

@astreet Thanks for the feedback. I pushed changes to address your comments and rebased my branch on master.

I'd like to point out one possible concern with the refactoring of the max scroll y logic into a helper. I noticed that one place that calls it used to use getHeight while the other place used getMeasuredHeight. Through reading the docs and testing, I think getHeight will work okay in the onLayoutChange method (it used to use getMeasuredHeight). However, I wanted to point this out in case you have any knowledge in this area.

astreet · 2016-11-23T12:53:51Z

I think getHeight is more correct iirc, since getMeasuredHeight just returns how big the view wants to be, not how big it actually draws.

astreet · 2016-11-23T12:54:36Z

ReactAndroid/src/main/java/com/facebook/react/views/scroll/ReactScrollView.java

-          getChildAt(0).getHeight() - (getHeight() - getPaddingBottom() - getPaddingTop()));
-        if (scrollY >= scrollRange) {
+        int scrollRange = getMaxScrollY();
+        if (scrollY >= getMaxScrollY()) {


Can you just update this to use scrollRange?

@astreet I'm not sure what you mean. Can you elaborate?

I just meant you're calling getMaxScrollY twice when you already have the result :)

@astreet Oops 😊 . I made this change, rebased, and pushed my branch.

astreet · 2016-11-23T12:55:21Z

ReactAndroid/src/main/java/com/facebook/react/views/scroll/ReactScrollView.java

+  }
+
+  /**
+   * Called when a mContentView's layout has changed.


Can you add a comment that we use this to fix scroll position if it's too large when the content resizes?

astreet

Thanks for the fixes, just two small things before landing

astreet · 2016-11-29T11:25:10Z

ReactAndroid/src/main/java/com/facebook/react/views/scroll/ReactScrollView.java

 import android.widget.OverScroller;
 import android.widget.ScrollView;

 import com.facebook.react.bridge.ReactContext;
+import com.facebook.common.logging.FLog;


You'll need to add this as a dependency in the buck file... that's why the builds are failing. Or just remove this and the FLog statement below, I don't think they're that useful ;)

@astreet Thanks for diagnosing my build failure. I removed the logging.

I verified the problematic build step is now passing locally by running these commands (got them from the circleci log):

buck fetch ReactAndroid/src/main/java/com/facebook/react/shell/ buck build ReactAndroid/src/main/java/com/facebook/react/shell/

Is there an easy way to do a full local build with buck so I can be more confident my build will succeed on the CI server? I build with gradle locally so I didn't catch this error.

If I recall correctly you can build UIExplorer with buck by running buck build uiexplorer

(might need to buck fetch uiexplorer too)

astreet · 2016-11-29T11:26:31Z

(Also, I appreciate you sticking with this, sorry it's taking so long)

Suppose that the user is scrolled to the bottom of a ScrollView. Next, the ScrollView's content is edited such that the height of the content changes and the current scroll position is larger than the new height of the content. Consequently, the user sees a blank ScrollView. As soon as the user interacts with the ScrollView, the ScrollView will jump to its max scroll position. This change improves this scenario by ensuring that the user is never staring at a blank ScrollView when the ScrollView has content in it. It does this by moving the ScrollView to its max scroll position when the scroll position after an edit is larger than the max scroll position of the ScrollView.

Android's ScrollView already asserts there's only 1 child which is why we can make this assumption.

Also, refactored this logic into a helper.

astreet · 2016-11-30T11:11:24Z

@facebook-github-bot shipit

Thanks!!

facebook-github-bot · 2016-11-30T11:13:40Z

@astreet has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Summary: Suppose that the user is scrolled to the bottom of a ScrollView. Next, the ScrollView's content is edited such that the height of the content changes and the current scroll position is larger than the new height of the content. Consequently, the user sees a blank ScrollView. As soon as the user interacts with the ScrollView, the ScrollView will jump to its max scroll position. This change improves this scenario by ensuring that the user is never staring at a blank ScrollView when the ScrollView has content in it. It does this by moving the ScrollView to its max scroll position when the scroll position after an edit is larger than the max scroll position of the ScrollView. Here are some pictures to illustrate how this PR improves the scenario described above: ![image](https://cloud.githubusercontent.com/assets/199935/20408839/0e731774-accc-11e6-9f0a-3d77198645e9.png) ![image](https://cloud.githubusercontent.com/assets/199935/20408844/12877bb6-accc-11e6-8fe2-1c1bb26569cc.png) **Test plan (require Closes facebook#11000 Differential Revision: D4250792 Pulled By: astreet fbshipit-source-id: 940fff6282ad29c796726f68b4519cbdabbfe554

facebook-github-bot added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Nov 17, 2016

astreet suggested changes Nov 22, 2016

View reviewed changes

mkonicek added the Core Team label Nov 22, 2016

rigdern force-pushed the rigdern/androidAutoScroll branch from a2c2973 to 511188d Compare November 23, 2016 02:29

astreet reviewed Nov 23, 2016

View reviewed changes

astreet suggested changes Nov 23, 2016

View reviewed changes

rigdern force-pushed the rigdern/androidAutoScroll branch from 4f5bb1e to ea5bd49 Compare November 28, 2016 20:57

astreet suggested changes Nov 29, 2016

View reviewed changes

astreet added GH Review: needs-revision and removed GH Review: review-needed labels Nov 29, 2016

Adam Comella added 8 commits November 29, 2016 08:23

Don't prefix method calls with this

f06ca88

Simplify code by assuming there's always 0 or 1 children

ce23366

Android's ScrollView already asserts there's only 1 child which is why we can make this assumption.

Inlined onContentLayoutChange

3b187a4

When calculating maxScrollY, take padding into account

95feb6e

Also, refactored this logic into a helper.

Added comment to describe purpose of onLayoutChange

c69031c

Use stored version of getMaxScrollY()

093bb10

Remove logging which wasn't too useful

36f0821

rigdern force-pushed the rigdern/androidAutoScroll branch from ea5bd49 to 36f0821 Compare November 29, 2016 16:33

facebook-github-bot added GH Review: review-needed and removed GH Review: needs-revision labels Nov 29, 2016

astreet added GH Review: accepted and removed GH Review: review-needed labels Nov 30, 2016

facebook-github-bot closed this in 528a3c7 Nov 30, 2016

rigdern deleted the rigdern/androidAutoScroll branch December 5, 2016 21:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Android: Keep ScrollView content visible after edits #11000

Android: Keep ScrollView content visible after edits #11000

rigdern commented Nov 17, 2016

facebook-github-bot commented Nov 17, 2016

astreet left a comment

astreet Nov 22, 2016

astreet Nov 22, 2016

astreet Nov 22, 2016

astreet Nov 22, 2016

astreet Nov 22, 2016

mkonicek commented Nov 22, 2016

rigdern commented Nov 23, 2016

astreet commented Nov 23, 2016

astreet Nov 23, 2016

rigdern Nov 23, 2016

astreet Nov 24, 2016

rigdern Nov 28, 2016

astreet Nov 23, 2016

astreet left a comment

astreet Nov 29, 2016

rigdern Nov 29, 2016

janicduplessis Nov 29, 2016

janicduplessis Nov 29, 2016

astreet commented Nov 29, 2016

astreet commented Nov 30, 2016

facebook-github-bot commented Nov 30, 2016

Android: Keep ScrollView content visible after edits #11000

Android: Keep ScrollView content visible after edits #11000

Conversation

rigdern commented Nov 17, 2016

facebook-github-bot commented Nov 17, 2016

astreet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkonicek commented Nov 22, 2016

rigdern commented Nov 23, 2016

astreet commented Nov 23, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astreet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astreet commented Nov 29, 2016

astreet commented Nov 30, 2016

facebook-github-bot commented Nov 30, 2016