Closed Bug 783553 Opened 10 years ago Closed 10 years ago

Dont show overscroll for webapps


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



(firefox17 wontfix, firefox18 verified)

Firefox 18
Tracking Status
firefox17 --- wontfix
firefox18 --- verified


(Reporter: wesj, Assigned: wesj)



(1 file, 1 obsolete file)

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)?
Component: General → Web Apps
QA Contact: aaron.train
Whiteboard: [blocking-webrtandroid1?]
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?
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:

which should kill the displacement here:

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:

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).
Attached patch Patch v1 (obsolete) — Splinter Review
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:

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/
@@ +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)


@@ +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/
@@ +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;


@@ +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/
@@ +9,5 @@
>  import org.mozilla.gecko.gfx.ImmutableViewportMetrics;
>  import org.mozilla.gecko.gfx.ViewportMetrics;
>  import;
> +import;

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().
Attached patch Patch v2Splinter Review
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:

>+    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+
OS: All → Android
Hardware: All → ARM
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 18
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.
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.