Dont show overscroll for webapps

VERIFIED FIXED in Firefox 18

Status

()

P1
normal
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: wesj, Assigned: wesj)

Tracking

unspecified
Firefox 18
ARM
Android
Points:
---

Firefox Tracking Flags

(firefox17 wontfix, firefox18 verified)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 years ago
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)?
(Assignee)

Updated

6 years ago
Component: General → Web Apps
QA Contact: aaron.train
Whiteboard: [blocking-webrtandroid1?]

Updated

6 years ago
Priority: -- → P1
Whiteboard: [blocking-webrtandroid1?]
Wes and I discussed implementing this by twiddling the pan/zoom prefs for webapps. Wes, did you end up trying that?
(Assignee)

Comment 2

6 years ago
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.
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).
(Assignee)

Comment 4

6 years ago
Created attachment 656538 [details] [diff] [review]
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?
Attachment #656538 - Flags: review?(bugmail.mozilla)
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
Attachment #656538 - Flags: review?(bugmail.mozilla) → review-
(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().
(Assignee)

Comment 7

6 years ago
Created attachment 657089 [details] [diff] [review]
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;
Assignee: nobody → wjohnston
Attachment #656538 - Attachment is obsolete: true
Attachment #657089 - Flags: review?(bugmail.mozilla)
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!
Attachment #657089 - Flags: review?(bugmail.mozilla) → review+

Updated

6 years ago
OS: All → Android
Hardware: All → ARM
https://hg.mozilla.org/mozilla-central/rev/872e823bd5fd
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 18

Updated

6 years ago
Status: RESOLVED → VERIFIED
status-firefox18: --- → verified

Updated

6 years ago
status-firefox17: --- → affected
(Assignee)

Comment 11

6 years ago
I intentionally didn't uplift this to 17 because it feels more like polish to me than something we need to uplift.
(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.
status-firefox17: affected → wontfix
You need to log in before you can comment on or make changes to this bug.