783553 - Dont show overscroll for webapps

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Web Apps (PWAs), defect, P1)

Description

[Wesley Johnston (:wesj)]

The overscroll bounce effect doesn't really fit well for webapps. I think we should turn it off for them. Sites that want it back can implement something with transforms (or we could add a metaviewport tag to enable/disable it)?

Comment 1

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

Wes and I discussed implementing this by twiddling the pan/zoom prefs for webapps. Wes, did you end up trying that?

Comment 2

[Wesley Johnston (:wesj)]

Yeah. Not that simple it turns out. Setting ui.scrolling.overscroll_snap_limit to zero means that we should set the overscroll resistance to zero here:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/ui/Axis.java#224

which should kill the displacement here:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/ui/Axis.java#302

BUT I think because we are not using the "new" position to determine the resistance, we still have one frame where we overshoot. I played with trying to hack in the new position instead, but eventually decided that it felt more like a hack than a real fix. My plan now is to put in a better system for overscroll supporting EdgeEffect at least:

http://developer.android.com/reference/android/widget/EdgeEffect.html

maybe with a pref to turn on overscroll. That's what everyone I talked to seems to want. IMO, we should just bite the bullet and do it.

Comment 3

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

I think adding special code/a pref for disabling overscroll is reasonable, but I'm not sure about using EdgeEffect. It seems like trying to use that will be a much larger change, and will change our observed behaviour significantly. Also it screws with trying to reuse that code for B2G (not that that should necessarily stop us, just pointing it out).

Comment 4

[Wesley Johnston (:wesj)]

Attachment: Patch v1

This disables overscroll by adjusting the mDisplacement in Axis every time we're displaced. I allow setting it by setting the overscrollMode on our LayerView like a real Android app.

I played (too long) with getting EdgeEffect to work. I think because of the way we use Texture/SurfaceView we need to lock the Canvas and then draw on it. That causes me to hit this assertion:

http://mxr.mozilla.org/mozilla-central/source/gfx/layers/opengl/LayerManagerOGL.cpp#173

on startup. I tried wrapping my code in various different ifs to not draw the edge effect if the layer manager isn't set up yet, but none of that seemed to work. You know of a solution kats?

Comment 5

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

Comment on attachment 656538 [details] [diff] [review]
Patch v1

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

Overall I like this approach, but I have a few nits and one correctness concern. Enough that I'd like to see another patch.

::: mobile/android/base/gfx/LayerView.java
@@ +384,5 @@
>      }
> +
> +    @Override
> +    public void setOverScrollMode(int overscrollMode) {
> +        if (mLayerClient != null && mLayerClient.getPanZoomController() != null)

Don't need the null check on getPanZoomController(), that should always be non-null if mLayerClient is non-null. Also, please use braces for single-line ifs.

@@ +390,5 @@
> +    }
> +
> +    @Override
> +    public int getOverScrollMode() {
> +        if (mLayerClient != null && mLayerClient.getPanZoomController() != null)

Ditto.

@@ +392,5 @@
> +    @Override
> +    public int getOverScrollMode() {
> +        if (mLayerClient != null && mLayerClient.getPanZoomController() != null)
> +            return mLayerClient.getPanZoomController().getOverScrollMode();
> +        return View.OVER_SCROLL_ALWAYS;

return super.getOverScrollMode() here instead of hard-coding the return.

::: mobile/android/base/ui/Axis.java
@@ +105,5 @@
>      }
>  
>      private final SubdocumentScrollHelper mSubscroller;
>  
> +    private int mOverscrollMode = View.OVER_SCROLL_IF_CONTENT_SCROLLS; /* Default to only overscrolling if we're allowed to scroll in a direction */

nit: I would prefer moving the value initialization into the constructor. It's easier to follow if all the initialization is done together (also avoids accidentally duplicating initialization, and avoids having the generated bytecode contain an extra initializer function).

@@ +126,5 @@
>          mSubscroller = subscroller;
>      }
>  
> +    public void setOverScrollMode(int overscrollMode) {
> +        this.mOverscrollMode = overscrollMode;

nit: No need for "this."

@@ +130,5 @@
> +        this.mOverscrollMode = overscrollMode;
> +    }
> +
> +    public int getOverScrollMode() {
> +        return this.mOverscrollMode;

Ditto

@@ +309,1 @@
>              return;

If I'm reading this right, I think it will break axis locking when the overscroll mode is OVER_SCROLL_ALWAYS, because instead of returning we will continue into the function and apply displacement. I think the correct change for this should be to turn scrollable() into this (or some equivalent):

        if (mSubscroller.scrolling()) {
            // for subdocument scrolling allow scrolling unless axis locked
            return !mScrollingDisabled;
        }
        if (mScrollingDisabled) {
            // axis locked
            return false;
        }
        if (getViewportLength() <= getPageLength() - MIN_SCROLLABLE_DISTANCE) {
            // document fits to viewport, so only scrollable if we allow always-overscroll
            return getOverScrollMode() == View.OVER_SCROLL_ALWAYS;
        }
        // document is larger than viewport, so we can scroll
        return true;

Also, put back the braces.

::: mobile/android/base/ui/PanZoomTarget.java
@@ +9,5 @@
>  import org.mozilla.gecko.gfx.ImmutableViewportMetrics;
>  import org.mozilla.gecko.gfx.ViewportMetrics;
>  
>  import android.graphics.PointF;
> +import android.graphics.Canvas;

Unnecessary import

Comment 6

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

(In reply to Kartikaya Gupta (:kats) from comment #5)
> @@ +309,1 @@
> >              return;
> 
> If I'm reading this right, I think it will break axis locking when the
> overscroll mode is OVER_SCROLL_ALWAYS, because instead of returning we will

Gah, splinter! This comment applies to the condition change you made at the top of displace().

Comment 7

[Wesley Johnston (:wesj)]

Attachment: Patch v2

Updated scrollable like you suggested except that it would fail in cases where the viewport was smaller than the page size but overscrollmode != always. Changed it to:

// there is scrollable space, and we're not disabled, or the document fits the viewport
// but we always allow overscroll anyway
return getViewportLength() <= getPageLength() - MIN_SCROLLABLE_DISTANCE ||
       getOverScrollMode() == View.OVER_SCROLL_ALWAYS;

Comment 8

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

Comment on attachment 657089 [details] [diff] [review]
Patch v2

Logic looks good, thanks! Few nits:

>+
>+    @Override
>+    public void setOverScrollMode(int overscrollMode) {
>+        super.setOverScrollMode(overscrollMode);
>+        if (mLayerClient != null)
>+            mLayerClient.getPanZoomController().setOverScrollMode(overscrollMode);
>+    }
>+
>+    @Override
>+    public int getOverScrollMode() {
>+        if (mLayerClient != null)
>+            return mLayerClient.getPanZoomController().getOverScrollMode();
>+        return super.getOverScrollMode();
>+    }

Braces please. Even the official mozilla style guide says so: https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style#Control_Structures

> 
>+    private int mOverscrollMode; /* Default to only overscrolling if we're allowed to scroll in a direction */
>     private float mFirstTouchPos;           /* Position of the first touch event on the current drag. */

Might as well align the comment with the rest of the comments here.

>     // Performs displacement of the viewport position according to the current velocity.
>     void displace() {
>-        if (!scrollable()) {
>+        // if this isn't scrollable just return
>+        if (!scrollable())
>             return;
>-        }
> 

Needs moar braces!

Comment 9

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/872e823bd5fd

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/872e823bd5fd

Comment 11

[Wesley Johnston (:wesj)]

I intentionally didn't uplift this to 17 because it feels more like polish to me than something we need to uplift.

Comment 12

[Jason Smith [:jsmith]]

(In reply to Wesley Johnston (:wesj) from comment #11)
> I intentionally didn't uplift this to 17 because it feels more like polish
> to me than something we need to uplift.

Right, I agree with that. Fixing the flag.