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

RESOLVED FIXED in Firefox 48

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: nachtigall, Assigned: rbarker)

Tracking

(Depends on 2 bugs, {regression})

48 Branch
Firefox 50
ARM
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox47 unaffected, firefox48blocking verified, firefox49 fixed, firefox50 verified)

Details

Attachments

(2 attachments, 1 obsolete attachment)

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.
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.
Component: Activity Streams: General → Graphics, Panning and Zooming
OS: Unspecified → Android
Product: Firefox → Firefox for Android
Hardware: Unspecified → ARM
Does also not happen on Firefox for Android Beta (47.0b8). Only Nightly, maybe a regression or a new feature (APZ?)
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
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.
I bisected inbound on that range, and (surprisingly) found bug 1229462 is the culprit.
Blocks: 1229462
Flags: needinfo?(rbarker)
Verified it affects the latest aurora build as well (which has the bug 1229462 uplift).
Version: 49 Branch → 48 Branch
Assigning to randall since he was looking into this.
Assignee: nobody → rbarker
Flags: needinfo?(rbarker)
Attachment #8760987 - Flags: review?(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?
Attachment #8760987 - Flags: review?(botond) → feedback+
(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 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.
Attachment #8761427 - Flags: review?(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?
Attachment #8761427 - Flags: review?(botond) → review+
(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.
Pushed by rbarker@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa7e93d774c1
Allow Android native fling animation to correctly handoff velocity r=botond
https://hg.mozilla.org/mozilla-central/rev/aa7e93d774c1
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Verified as fixed in build 50.0a1 (2016-06-12);
Device: Asus ZenPad 8 (Android 5.0.1).
Blocking fennec as the impact is important.
Sounds like we need an uplift.
Flags: needinfo?(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.
Flags: needinfo?(rbarker)
Attachment #8761427 - Flags: approval-mozilla-beta?
Attachment #8761427 - Flags: approval-mozilla-aurora?
Blocks: 1280656
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 ...
Attachment #8761427 - Flags: approval-mozilla-beta?
Attachment #8761427 - Flags: approval-mozilla-beta+
Attachment #8761427 - Flags: approval-mozilla-aurora?
Attachment #8761427 - Flags: approval-mozilla-aurora+
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')
Flags: needinfo?(rbarker)
I have rebased the patch on to beta and tested locally.
Flags: needinfo?(rbarker)
Blocks: 1280580
No longer depends on: 1280580
Depends on: 1284044
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).
(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!
Verified as fixed in build Firefox 48 Beta 6;
Device: Asus ZenPad 8 (Android 5.0.2).
You need to log in before you can comment on or make changes to this bug.