1276463 - docs.google.com - mobile site cannot be scrolled down

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Toolbar, defect)

Description

[Jens]

User Agent: Mozilla/5.0 (X11; Linux i686; rv:48.0) Gecko/20100101 Firefox/48.0
Build ID: 20160527004031
Firefox for Android

Steps to reproduce:

See https://github.com/webcompat/web-bugs/issues/2646

1) Navigate to: https://docs.google.com/document/d/10kTyCmGPhvZy94F7VWyS-dQ4lsBacR2dUgGTtV98C40/mobilebasic?pref=2&pli=1
2) try to scroll down with Nightly (yesterdays or todays version, probably also olders). By "scroll down" I of course mean the touching/swiping down


Actual results:

Does not scroll down, but stays fixed at top.


Expected results:

Being able to scroll down. Works in Firefox for Android 46 and also Mobile Chrome, but not in current Nightly.

Maybe it is APZ related.

Comment 1

[Vlad Bacia-Mociran [:VladB]]

I'm moving your issue to Firefox for Android - Graphics, Panning and Zooming component. Activity Streams is an add-on and has nothing to do with your issue.

Comment 2

[Jens]

Does also not happen on Firefox for Android Beta (47.0b8). Only Nightly, maybe a regression or a new feature (APZ?)

Comment 3

[Kartikaya Gupta (email:[e-mail])]

I can repro this, it does seem like an APZ regression. I can scroll the document if I fling on the fixed header, but it looks like if I'm flinging in the body area it hits some other scrollable frame which has a few pixels of give but mostly just eats the scroll.

Comment 4

[Kartikaya Gupta (email:[e-mail])]

On Aurora it happens on the first fling, it looks like there's something that shifts by a few pixels. On subsequent flings the scrolling works fine. So this is a regression in 49.

Comment 5

[Sorina Florean [:sflorean]]

Regression window:
Last good build: 2016-05-11
First bad build: 2016-05-12

Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=1579b9e2e50f3a27ad02d58cc9170c91e0973fec&tochange=c3f5e6079284a7b7053c41f05d0fe06ff031db03

Comment 6

[Kartikaya Gupta (email:[e-mail])]

I bisected inbound on that range, and (surprisingly) found bug 1229462 is the culprit.

Comment 7

[Kartikaya Gupta (email:[e-mail])]

Verified it affects the latest aurora build as well (which has the bug 1229462 uplift).

Comment 8

[Kartikaya Gupta (email:[e-mail])]

Assigning to randall since he was looking into this.

Comment 9

[Randall Barker [:rbarker]]

Attachment: 0001-Bug-1276463-Allow-Android-native-fling-animation-to-correctly-handoff-velocity-r-16060716-6b110c1.patch

Comment 10

[Botond Ballo [:botond]]

Comment on attachment 8760987 [details] [diff] [review]
0001-Bug-1276463-Allow-Android-native-fling-animation-to-correctly-handoff-velocity-r-16060716-6b110c1.patch

Review of attachment 8760987 [details] [diff] [review]:
-----------------------------------------------------------------

This generally looks fine. I don't fully understand the reason for all of the changes, though. I'm sure you have good reasons for each of them, they're just not clear to me as a reader/reviewer. For example, why do the bounds-checking functions now take a direction and limit their checking based on it?

::: gfx/layers/apz/src/AndroidAPZ.cpp
@@ +39,5 @@
>    mOverScroller = scroller;
>  }
>  
> +static int32_t
> +ClampStart(float aOrigin, float aMin, float aMax)

I don't understand what this function does. Can you add a comment explaining?

@@ +42,5 @@
> +static int32_t
> +ClampStart(float aOrigin, float aMin, float aMax)
> +{
> +  if (aOrigin <= aMin) {
> +    return (int32_t)floor(aOrigin);

In particular, if it were a conventional "clamp" function, I would expect it to return "aMin" here.

@@ +85,5 @@
> +
> +  float pageStartX = mApzc.mX.GetPageStart().value;
> +  float pageEndX = (mApzc.mX.GetPageEnd() - mApzc.mX.GetCompositionLength()).value;
> +  float pageStartY = mApzc.mY.GetPageStart().value;
> +  float pageEndY = (mApzc.mY.GetPageEnd() - mApzc.mY.GetCompositionLength().value);

These variables denote the bounds of the page's scroll range, not of the page itself, so let's called them "scrollRangeStartX" etc.

Also, given how frequently we do "GetPageEnd() - GetCompositionLength()", it may be worth introducing an Axis::GetScrollRangeEnd() function.

@@ +134,5 @@
> +  // Sometimes the OverScroller fails to update the offset for a frame.
> +  // If the frame can still scroll we just use the velocity from the previous
> +  // frame. However, if the frame can no longer scroll in the direction
> +  // of the fling, then end the animation.
> +  if (offset != mStartOffset) {

Did you mean to compare to mPreviousOffset here?

@@ +153,5 @@
>        // the stopping threshold so abort the animation.
>        mOverScroller->AbortAnimation();
>      }
> +    if (!mSentBounceX && !mSentBounceY && (speed > 0.0f)) {
> +      DeferHandleFlingOverscroll(velocity);

Should we set one or both of mSentBounce[XY] to true here?

@@ +238,5 @@
> +bool
> +AndroidFlingAnimation::AtBounds(Axis& aAxis, float aValue, float aDirection)
> +{
> +  // Only check if we hit a bounds if we are moving along the axis being checked
> +  if ((aDirection < 0.0f) && (aValue <= aAxis.GetPageStart().value)) {

Why does this comparison not use BOUNDS_EPSILON while CheckBounds() does?

Comment 11

[Randall Barker [:rbarker]]

(In reply to Botond Ballo [:botond] from comment #10)
> Comment on attachment 8760987 [details] [diff] [review]
> 0001-Bug-1276463-Allow-Android-native-fling-animation-to-correctly-handoff-
> velocity-r-16060716-6b110c1.patch
> 
> Review of attachment 8760987 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This generally looks fine. I don't fully understand the reason for all of
> the changes, though. I'm sure you have good reasons for each of them,
> they're just not clear to me as a reader/reviewer. For example, why do the
> bounds-checking functions now take a direction and limit their checking
> based on it?
> 


> @@ +134,5 @@
> > +  // Sometimes the OverScroller fails to update the offset for a frame.
> > +  // If the frame can still scroll we just use the velocity from the previous
> > +  // frame. However, if the frame can no longer scroll in the direction
> > +  // of the fling, then end the animation.
> > +  if (offset != mStartOffset) {
> 
> Did you mean to compare to mPreviousOffset here?
> 

Yes, I think originally I was checking if the page had scrolled away from its starting position and if it had not was checking to see if it was already at the max scroll position so I could hand off the fling. But in the process of re-writing this patch repeatedly I think I changed the intention with out changing the code.

> @@ +153,5 @@
> >        // the stopping threshold so abort the animation.
> >        mOverScroller->AbortAnimation();
> >      }
> > +    if (!mSentBounceX && !mSentBounceY && (speed > 0.0f)) {
> > +      DeferHandleFlingOverscroll(velocity);
> 
> Should we set one or both of mSentBounce[XY] to true here?
>

I don't think so, the animation is about to end so setting them serves no purpose.
 
> @@ +238,5 @@
> > +bool
> > +AndroidFlingAnimation::AtBounds(Axis& aAxis, float aValue, float aDirection)
> > +{
> > +  // Only check if we hit a bounds if we are moving along the axis being checked
> > +  if ((aDirection < 0.0f) && (aValue <= aAxis.GetPageStart().value)) {
> 
> Why does this comparison not use BOUNDS_EPSILON while CheckBounds() does?

By switching the bounds to floor/ceil the BOUNDS_EPSILON is no longer need. I just forgot to remove it from the check bounds function.

Comment 12

[Randall Barker [:rbarker]]

Attachment: 0001-Bug-1276463-Allow-Android-native-fling-animation-to-correctly-handoff-velocity-r-16060816-5c3b6c5.patch

Comment 13

[Randall Barker [:rbarker]]

Comment on attachment 8761427 [details] [diff] [review]
0001-Bug-1276463-Allow-Android-native-fling-animation-to-correctly-handoff-velocity-r-16060816-5c3b6c5.patch

Attempted to address review comments.

Comment 14

[Botond Ballo [:botond]]

Comment on attachment 8761427 [details] [diff] [review]
0001-Bug-1276463-Allow-Android-native-fling-animation-to-correctly-handoff-velocity-r-16060816-5c3b6c5.patch

Review of attachment 8761427 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

::: gfx/layers/apz/src/AndroidAPZ.cpp
@@ +16,5 @@
>  // #define ANDROID_APZ_LOG(...) printf_stderr("ANDROID_APZ: " __VA_ARGS__)
>  
> +#include <android/log.h>
> +#define RLOG(format, ...) __android_log_print(ANDROID_LOG_INFO, "reb", format, ##__VA_ARGS__);
> +#define RLINE RLOG("%s:%s:%d", __FILE__, __FUNCTION__, __LINE__)

Is this included on purpose?

@@ +41,5 @@
> +// to the end of the page as it will always be 0.5 pixels short. To work around this issue,
> +// the min and max scroll extents are floor/ceil to convert them to the nearest integer
> +// just outside of the actual scroll extents. This means, the starting
> +// scroll offset must be converted the same way so that if the frame has already been
> +// scrolled 1.5 pixes, it won't be snapped back when converted to an integer. This integer

s/pixes/pixels

@@ +42,5 @@
> +// the min and max scroll extents are floor/ceil to convert them to the nearest integer
> +// just outside of the actual scroll extents. This means, the starting
> +// scroll offset must be converted the same way so that if the frame has already been
> +// scrolled 1.5 pixes, it won't be snapped back when converted to an integer. This integer
> +// rounding error was on of several causes of Bug 1276463.

s/on of/one of

@@ -130,5 @@
> -    mOverScrollCount = 0;
> -  }
> -
> -  // If the offset hasn't changed in over MAX_OVERSCROLL_COUNT we have overflowed
> -  // the OverScroller and it needs to be aborted.

Why is this part no longer necessary?

Comment 15

[Randall Barker [:rbarker]]

(In reply to Botond Ballo [:botond] from comment #14)
> Comment on attachment 8761427 [details] [diff] [review]
> 0001-Bug-1276463-Allow-Android-native-fling-animation-to-correctly-handoff-
> velocity-r-16060816-5c3b6c5.patch
> 
> Review of attachment 8761427 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks!
> 
> ::: gfx/layers/apz/src/AndroidAPZ.cpp
> @@ +16,5 @@
> >  // #define ANDROID_APZ_LOG(...) printf_stderr("ANDROID_APZ: " __VA_ARGS__)
> >  
> > +#include <android/log.h>
> > +#define RLOG(format, ...) __android_log_print(ANDROID_LOG_INFO, "reb", format, ##__VA_ARGS__);
> > +#define RLINE RLOG("%s:%s:%d", __FILE__, __FUNCTION__, __LINE__)
> 
> Is this included on purpose?

Nope, just debugging code I forgot to remove :)

> @@ -130,5 @@
> > -    mOverScrollCount = 0;
> > -  }
> > -
> > -  // If the offset hasn't changed in over MAX_OVERSCROLL_COUNT we have overflowed
> > -  // the OverScroller and it needs to be aborted.
> 
> Why is this part no longer necessary?

This code:

  if (offset != mPreviousOffset) {
    if (aDelta.ToMilliseconds() > 0) {
      velocity = (mPreviousOffset - offset) / (float)aDelta.ToMilliseconds();
      mPreviousVelocity = velocity;
    }
  } else if (hitBoundX || hitBoundY) {
    // We have reached the end of the scroll in one of the directions being scrolled and the offset has not
    // changed so end animation.
    shouldContinueFling = false;
  }

now handles that case. Changing mStartOffset to mPreviousOffset performs basically the same task with out having to have counter. Adding the scroll direction to CheckBounds means it will no longer incorrectly detect a bounds when we are not scrolling towards it such as when the scroll position is the top of the page and we are flinging to the bottom. So if the scroll position stays the same for multiple frames but we haven't hit a boundary then it is okay to keep waiting for it to end on its own. I hope that makes sense.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa7e93d774c1
Allow Android native fling animation to correctly handoff velocity r=botond

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/aa7e93d774c1

Comment 18

[Sorina Florean [:sflorean]]

Verified as fixed in build 50.0a1 (2016-06-12);
Device: Asus ZenPad 8 (Android 5.0.1).

Comment 19

[Sylvestre Ledru [:Sylvestre]]

Blocking fennec as the impact is important.

Comment 20

[Milan Sreckovic [:milan] (needinfo for best results)]

Sounds like we need an uplift.

Comment 21

[Randall Barker [:rbarker]]

Comment on attachment 8761427 [details] [diff] [review]
0001-Bug-1276463-Allow-Android-native-fling-animation-to-correctly-handoff-velocity-r-16060816-5c3b6c5.patch

Approval Request Comment
[Feature/regressing bug #]: Google Docs will not scroll in Fennec with Android Native Fling animations.
[User impact if declined]: Users will not be able to use Google Docs in Fennec.
[Describe test coverage new/current, TreeHerder]: I don't know of any tests that currently cover the fling animation.
[Risks and why]: May introduce other defects not yet detected?
[String/UUID change made/needed]: none.

Comment 22

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8761427 [details] [diff] [review]
0001-Bug-1276463-Allow-Android-native-fling-animation-to-correctly-handoff-velocity-r-16060816-5c3b6c5.patch

Fix a new regression of a major website.

However, not really happy to see the size and the kind of changes in this patch ...

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/43db2c56b8ca

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/43db2c56b8ca

Comment 25

[Carsten Book [:Tomcat]]

has problems uplifting to beta:

grafting 351418:43db2c56b8ca "Bug 1276463 - Allow Android native fling animation to correctly handoff velocity r=botond, a=sylvestre"
merging gfx/layers/apz/src/AndroidAPZ.cpp
warning: conflicts while merging gfx/layers/apz/src/AndroidAPZ.cpp! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use 'hg resolve' and 'hg graft --continue')

Comment 26

[Randall Barker [:rbarker]]

Attachment: Rebased patch for beta.

I have rebased the patch on to beta and tested locally.

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/1f6535a1845b

Comment 28

[Jens]

This is still not fully fixed (which I find a bit surprising considering the number of people that have already looked at it). Instead of reopening this one here, I created a new Bug 1284044 and made this here dependent on it (hope this was correct).

Comment 29

[Kartikaya Gupta (email:[e-mail])]

(In reply to Jens from comment #28)
> Instead of reopening this
> one here, I created a new Bug 1284044 and made this here dependent on it
> (hope this was correct).

Yup, thanks!

Comment 30

[Sorina Florean [:sflorean]]

Verified as fixed in build Firefox 48 Beta 6;
Device: Asus ZenPad 8 (Android 5.0.2).