Closed Bug 1098096 Opened 7 years ago Closed 7 years ago

Overscroll glow not working on Android L

Categories

(Firefox for Android Graveyard :: General, defect)

All
Android
defect
Not set
normal

Tracking

(firefox34 wontfix, firefox35+ verified, firefox36 verified, fennec35+)

VERIFIED FIXED
Firefox 36
Tracking Status
firefox34 --- wontfix
firefox35 + verified
firefox36 --- verified
fennec 35+ ---

People

(Reporter: kbrosnan, Assigned: snorp)

References

Details

Attachments

(1 file, 1 obsolete file)

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
Assignee: nobody → snorp
tracking-fennec: ? → 35+
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?
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.
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.
Added a comment to explain why we need this
Attachment #8526076 - Attachment is obsolete: true
Attachment #8526076 - Flags: review?(lucasr.at.mozilla)
Attachment #8526079 - Flags: review?(lucasr.at.mozilla)
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?
Attachment #8526076 - Attachment is obsolete: false
(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.
Attachment #8526079 - Flags: review?(lucasr.at.mozilla) → review+
Attachment #8526076 - Attachment is obsolete: true
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.
Attachment #8526079 - Flags: approval-mozilla-beta?
Attachment #8526079 - Flags: approval-mozilla-aurora?
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.
Attachment #8526079 - Flags: approval-mozilla-beta? → approval-mozilla-beta-
https://hg.mozilla.org/mozilla-central/rev/54f2421af0dc
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
Attachment #8526079 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Verified as fixed on latest Aurora and Nightly and Firefox 35 Beta 1 build 2 on Nexus 7(Android 5.0).
Status: RESOLVED → VERIFIED
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.