1098096 - Overscroll glow not working on Android L

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Kevin Brosnan [Ex-Mozilla]]

Load a page and then scroll to the top or bottom and try to scroll past it.

Expected: Android L glow showing the end of the page has been reached
Actual: No indication that the end of the page has been reached

Comment 1

[James Willcox (:snorp) ([e-mail]) (he/him)]

If I hack things to use TextureView then it works again. It looks like L may have made it so you can't draw on top of SurfaceView?

Comment 2

[James Willcox (:snorp) ([e-mail]) (he/him)]

Something strange going on here. Back using SurfaceView again, I can see the overscroll effect if I first do canvas.drawColor(Color.argb(127, 255, 0, 0)) which sets the whole view to 50% red. If I use argb(1, 255, 0, 0) nothing appears.

Comment 3

[James Willcox (:snorp) ([e-mail]) (he/him)]

I think I figured this out. The new EdgeEffect in L uses a SRC_ATOP blending mode. I *think* that means that it will only draw the effect on pixels that have a non-zero alpha in the destination. That means our fully alpha LayerView shows nothing. Bummer, huh? I'm going to try to add a local copy of the EdgeEffect with the blending mode changed.

Comment 4

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Add a hack to make overscroll effect work on Android 5.0

Comment 5

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Add a hack to make overscroll effect work on Android 5.0

Added a comment to explain why we need this

Comment 6

[Lucas Rocha (:lucasr)]

Comment on attachment 8526076 [details] [diff] [review]
Add a hack to make overscroll effect work on Android 5.0

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

Evil :-)

::: mobile/android/base/gfx/OverscrollEdgeEffect.java
@@ +31,5 @@
>      private final View mView;
>  
>      public OverscrollEdgeEffect(final View v) {
> +        Field paintField = null;
> +        if (Versions.feature21Plus) {

Comment with the reason why this is necessary, please. Is this going to be break if we use TextureView?

@@ +46,5 @@
>              mEdges[i] = new EdgeEffect(context);
> +
> +            try {
> +                if (paintField != null) {
> +                    final Paint p = (Paint)paintField.get(mEdges[i]);

nit: space after (Paint)

@@ +47,5 @@
> +
> +            try {
> +                if (paintField != null) {
> +                    final Paint p = (Paint)paintField.get(mEdges[i]);
> +                    p.setXfermode(new PorterDuffXfermode(PorterDuff.Mode.SRC));

Do you really need to create one instance of PorterDuffXfermode for each paint?

Comment 7

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Lucas Rocha (:lucasr) from comment #6)
> Comment on attachment 8526076 [details] [diff] [review]
> Add a hack to make overscroll effect work on Android 5.0
> 
> Review of attachment 8526076 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Evil :-)
> 
> ::: mobile/android/base/gfx/OverscrollEdgeEffect.java
> @@ +31,5 @@
> >      private final View mView;
> >  
> >      public OverscrollEdgeEffect(final View v) {
> > +        Field paintField = null;
> > +        if (Versions.feature21Plus) {
> 
> Comment with the reason why this is necessary, please. Is this going to be
> break if we use TextureView?

Added a comment in the latest patch. It won't break with TextureView.

> 
> @@ +46,5 @@
> >              mEdges[i] = new EdgeEffect(context);
> > +
> > +            try {
> > +                if (paintField != null) {
> > +                    final Paint p = (Paint)paintField.get(mEdges[i]);
> 
> nit: space after (Paint)
> 
> @@ +47,5 @@
> > +
> > +            try {
> > +                if (paintField != null) {
> > +                    final Paint p = (Paint)paintField.get(mEdges[i]);
> > +                    p.setXfermode(new PorterDuffXfermode(PorterDuff.Mode.SRC));
> 
> Do you really need to create one instance of PorterDuffXfermode for each
> paint?

I'm not sure, but that's how it is now without this hack.

Comment 8

[James Willcox (:snorp) ([e-mail]) (he/him)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/54f2421af0dc

Comment 9

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8526079 [details] [diff] [review]
Add a hack to make overscroll effect work on Android 5.0

Approval Request Comment
[Feature/regressing bug #]: Android L
[User impact if declined]: No overscroll effect on Android L devices
[Describe test coverage new/current, TBPL]: local only
[Risks and why]: ~zero risk, code only runs on L, etc
[String/UUID change made/needed]: none

For some reason we are only tracking this for 35+, but all versions are affected. If we can get this into 34 it would probably be good.

Comment 10

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8526079 [details] [diff] [review]
Add a hack to make overscroll effect work on Android 5.0

It's really late in Beta and I don't think this is a must have fix. Let's wait until 35 where I expect we'll have additional Android L fixes as well. Beta-

After this has landed on m-c, let's get this uplifted to Aurora before the merge.

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/54f2421af0dc

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e40a4d279e30

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Backed out for bustage.
https://treeherder.mozilla.org/ui/logviewer.html#?job_id=421190&repo=mozilla-aurora

https://hg.mozilla.org/releases/mozilla-aurora/rev/a2e2a71b2228

Comment 14

[James Willcox (:snorp) ([e-mail]) (he/him)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d6cbbc905bda

Comment 15

[u421692]

Verified as fixed on latest Aurora and Nightly and Firefox 35 Beta 1 build 2 on Nexus 7(Android 5.0).