Last Comment Bug 817828 - (blacktab) Black area near tabs button after the URL bar is animated
(blacktab)
: Black area near tabs button after the URL bar is animated
Status: VERIFIED FIXED
: regression, reproducible
Product: Firefox for Android
Classification: Client Software
Component: Theme and Visual Design (show other bugs)
: 20 Branch
: ARM Android
: -- normal with 2 votes (vote)
: Firefox 22
Assigned To: Lucas Rocha (:lucasr)
: Aaron Train [:aaronmt]
Mentors:
: 825323 826367 830101 832682 839283 843209 844212 845031 845810 (view as bug list)
Depends on:
Blocks: 709433 846949
  Show dependency treegraph
 
Reported: 2012-12-03 14:52 PST by Brian Nicholson (:bnicholson)
Modified: 2013-04-05 07:22 PDT (History)
35 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected
+
verified
verified
verified
20+
20+


Attachments
screenshot of bug (139.11 KB, image/png)
2012-12-03 14:52 PST, Brian Nicholson (:bnicholson)
no flags Details
Patch (8.55 KB, patch)
2013-02-27 14:27 PST, Sriram Ramasubramanian [:sriram]
no flags Details | Diff | Review
Don't use hardware layers on toolbar animations (1.93 KB, patch)
2013-03-01 07:24 PST, Lucas Rocha (:lucasr)
sriram.mozilla: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta+
Details | Diff | Review

Description Brian Nicholson (:bnicholson) 2012-12-03 14:52:30 PST
Created attachment 687984 [details]
screenshot of bug

STR:
1) Orient phone horizontally
2) Click URL bar
3) After the AwesomeScreen opens, click back

This results in the broken black image shown in this screenshot.

Droid RAZR, Android 4.0.4
Comment 1 Paul Feher 2012-12-05 07:08:30 PST
Using latest Nightly this is also reproducible on:
Samsung Galaxy SII (4.0.1)

But not reproducible on:
Nexus (4.1.2)
Samsung Galaxy R (2.3.4)
Comment 2 Lucas Rocha (:lucasr) 2012-12-06 05:27:01 PST
Based on the screenshot, I suspect this is just one more instance of bug 817526. Let's wait for that patch to land to see if it helps with this issue.
Comment 3 Lucas Rocha (:lucasr) 2012-12-12 02:52:21 PST
Brian, can you reproduce this bug on latest Nightly?
Comment 4 Brian Nicholson (:bnicholson) 2012-12-12 10:56:53 PST
(In reply to Lucas Rocha (:lucasr) from comment #3)
> Brian, can you reproduce this bug on latest Nightly?

Yeah, I still see this happen on today's Nightly.
Comment 5 Peter Retzer (:pretzer) 2012-12-13 05:25:12 PST
For the record: This is also happening in portrait mode after coming back from landscape mode. That means the black square sticks after it was 'activated' in landscape mode until the browser is restarted.
Comment 6 Lucas Rocha (:lucasr) 2012-12-14 03:03:53 PST
Just a quick note: I suspect this is either caused by the custom drawing in ShapedButton or BrowserToolbarBackground. I've seen a similar glitch while working on the tablet toolbar (I had to replace a ShapedButton with an ImageButton to get it working properly).
Comment 7 Ed Morley [:emorley] 2013-01-02 08:45:25 PST
*** Bug 825323 has been marked as a duplicate of this bug. ***
Comment 8 Scott Johnson (:jwir3) 2013-01-02 09:26:09 PST
(In reply to Peter Retzer (:pretzer) from comment #5)
> For the record: This is also happening in portrait mode after coming back
> from landscape mode. That means the black square sticks after it was
> 'activated' in landscape mode until the browser is restarted.

This happens for me even in portrait mode, without switching to/from landscape.
Comment 9 Sriram Ramasubramanian [:sriram] 2013-01-10 10:42:11 PST
*** Bug 826367 has been marked as a duplicate of this bug. ***
Comment 10 Aaron Train [:aaronmt] 2013-01-13 07:47:28 PST
*** Bug 830101 has been marked as a duplicate of this bug. ***
Comment 11 Aaron Train [:aaronmt] 2013-01-19 17:52:47 PST
*** Bug 832682 has been marked as a duplicate of this bug. ***
Comment 12 Aaron Train [:aaronmt] 2013-01-19 17:53:29 PST
This is still broken on Nightly builds.
Comment 13 Mounir Lamouri (:mounir) 2013-01-21 08:29:35 PST
A regression window might help here I guess.
Comment 14 Paul Feher 2013-01-23 05:50:09 PST
The regression window for this issue is:

good build:
2012/12/03

bad build:
2012/12/04

possible push-log:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=253009438c5b&tochange=6fa6e55a93b2

There are no tinderbox-builds kept from this dates, to check this more accurate.
Comment 15 Brian Nicholson (:bnicholson) 2013-01-23 09:24:28 PST
It's a regression from bug 709433 (see blocks field).
Comment 16 Aaron Train [:aaronmt] 2013-02-04 10:53:05 PST
Still an issue on firefox-20/firefox-21; I see this frequently more-so on my Galaxy SII opposed to any other device.
Comment 17 Lucas Rocha (:lucasr) 2013-02-04 11:40:32 PST
FYI: I'll be trying a couple of tentative fixes this week on the SII.
Comment 18 Aaron Train [:aaronmt] 2013-02-07 20:40:05 PST
*** Bug 839283 has been marked as a duplicate of this bug. ***
Comment 19 Sriram Ramasubramanian [:sriram] 2013-02-08 09:49:56 PST
This seems to be an issue on phones with h/w menu button. Could we see if that's a problem?
Comment 20 Aaron Train [:aaronmt] 2013-02-13 10:18:41 PST
SII seems to be the easiest device to reproduce this on. I haven't encountered this on any Nexus device.
Comment 21 Jared Wein [:jaws] (please needinfo? me) 2013-02-13 11:57:31 PST
I can reproduce this 100% on my HTC Sensation 4G running Android 4.0.3.
Comment 22 Lucas Rocha (:lucasr) 2013-02-20 06:47:56 PST
Quick update: after testing different changes on mounir's device (affect by this bug), I found out that the problem is actually on the tabs button. It happens just after the animation for closing the awesomescreen finishes (when the entry "shrinks" back). A black background shows up below the left curve of the tabs button (not where the background curve is drawn as I suspected).

More news on that soon.
Comment 23 Aaron Train [:aaronmt] 2013-02-20 09:57:20 PST
*** Bug 843209 has been marked as a duplicate of this bug. ***
Comment 24 bugecho 2013-02-20 10:16:22 PST
(In reply to Sriram Ramasubramanian [:sriram] from comment #19)
> This seems to be an issue on phones with h/w menu button. Could we see if
> that's a problem?

I can confirm that this bug happens on an Sony Xperia Sola with Android 4.0.
Also Xperia Sola has a hardare menu button.
Comment 25 Michael[tm] Smith 2013-02-21 12:03:32 PST
Ran into this on Galaxy S2 with Android 4.0.2 (exact model is SC-02C). It showed up when I first launched after updating to the latest Nightly. But when I switched to the home screen and came back, it no longer showed (instead displayed as expected). Closed Nightly through the task manager and then restarted it, but couldn't reproduce (everything normal).
Comment 26 Michael[tm] Smith 2013-02-21 12:05:02 PST
Ah I spoke too soon. Yeah I reproduced it again by opening up a second tab.
Comment 27 Kevin Brosnan [:kbrosnan] 2013-02-22 11:24:15 PST
*** Bug 844212 has been marked as a duplicate of this bug. ***
Comment 28 Kevin Brosnan [:kbrosnan] 2013-02-22 11:31:26 PST
Release team this affects Firefox beta and is very user visible.
Comment 29 Aaron Train [:aaronmt] 2013-02-23 13:42:50 PST
Also see this on the old Samsung S1 line, using a Captivate here.
Comment 30 Aaron Train [:aaronmt] 2013-02-25 07:17:08 PST
Also reproducible on the on the Samsung GT-S5660
Comment 31 Aaron Train [:aaronmt] 2013-02-25 13:46:51 PST
*** Bug 845031 has been marked as a duplicate of this bug. ***
Comment 32 schoewilliam 2013-02-25 22:58:54 PST
Reproductible on LG L7 (P700).
I'm running latest nightly. I'm also seeing it in latest Aurora + beta. it's beginning to be very annoying :/
Comment 33 Aaron Train [:aaronmt] 2013-02-27 06:15:38 PST
*** Bug 845810 has been marked as a duplicate of this bug. ***
Comment 34 Ed Morley [:emorley] 2013-02-27 06:19:25 PST
Lucas, are you still working on this? (7 days since your last comment in the bug)

It is 100% reproducible on common handsets (such as the Galaxy S2), highly noticeable to end users & will be on release in 4 and a half weeks - so we'll need a patch soon to avoid a high-risk last-minute beta uplift.
Comment 35 Lukas Blakk [:lsblakk] use ?needinfo 2013-02-27 12:56:05 PST
(In reply to Kevin Brosnan [:kbrosnan] from comment #28)
> Release team this affects Firefox beta and is very user visible.

Are you recommending this be added to "Known issues"?
Comment 36 Kevin Brosnan [:kbrosnan] 2013-02-27 13:15:31 PST
Yes.
Comment 37 Sriram Ramasubramanian [:sriram] 2013-02-27 14:14:05 PST
(Like a child raising his hand in the class)
I have a fix!
Comment 38 Sriram Ramasubramanian [:sriram] 2013-02-27 14:27:28 PST
Created attachment 719189 [details] [diff] [review]
Patch

(Temporary fix based on minimal knowledge on the animations)
This fixes few things
1. invalidate() is expensive and doesn't trigger onMeasure() which is needed for calculating the path when the curve changes. Hence making it requestLayout().
2. That removes the redundant requestLayout() calls in BrowserToolbar.
3. Fix for not trying to animate mMenu when it is not even there.

But the main fix is:
I don't understand why we are enabling/disabling h/w layer on views! Basically our app is h/w enabled and so every view will be h/w enabled by default. We don't have to turn on/off the knob for animations. This messes up the views!
If there is a reason for making the layer-type to NONE, then we need to tweak the CanvasDelegate and TabsButton code to work just like it does on GB phones.

Basically PorterDuff modes (esp DST_OUT) cannot work directly in the Canvas on GB phones. Hence, we move it to offscreen bitmap and do it. But on ICS phones, they don't have to be. Hence that optimization is available in CanvasDelegate. Now the change to enable/disable h/w layer causes the PorterDuff to work on a software layer, making us do the GB phone way. That's the reason for the black spot there.

(However I don't understand why this doesn't happen on Galaxy Nexus. I could see only one draw() in case of Galaxy Nexus, while two on other phones before/during animation. We need to investigate that separately).
Comment 39 Lukas Blakk [:lsblakk] use ?needinfo 2013-02-27 15:07:08 PST
Added: http://www.mozilla.org/en-US/mobile/20.0beta/releasenotes/
Comment 40 Lucas Rocha (:lucasr) 2013-03-01 07:13:22 PST
(In reply to Sriram Ramasubramanian [:sriram] from comment #38)
> Created attachment 719189 [details] [diff] [review]
> Patch
> 
> (Temporary fix based on minimal knowledge on the animations)
> This fixes few things
> 1. invalidate() is expensive and doesn't trigger onMeasure() which is needed
> for calculating the path when the curve changes. Hence making it
> requestLayout().

Not sure what you mean with "invalidate() is expensive" here. This really depends on how complex the drawing is. For example, complex views with tons of children can be slow to paint.

Layout is generally much more expensive than repainting a view because it has to re-measure all views in a portion of the view hierarchy. For instance, this is why animations that cause a re-layout per frame are so choppy.

> 2. That removes the redundant requestLayout() calls in BrowserToolbar.

Good.

> 3. Fix for not trying to animate mMenu when it is not even there.

Nice.

> But the main fix is:
> I don't understand why we are enabling/disabling h/w layer on views!
> Basically our app is h/w enabled and so every view will be h/w enabled by
> default. We don't have to turn on/off the knob for animations. This messes
> up the views!
> If there is a reason for making the layer-type to NONE, then we need to
> tweak the CanvasDelegate and TabsButton code to work just like it does on GB
> phones.

You're mixing things here. Hardware layers is an optimization meant to be used mainly (but not exclusively) on animations. When you create a hardware layer for a view, it will generate a GL texture for it and avoid re-drawing complex views and their children. Hardware layers are about flattening drawing operations into a single texture (which is faster than drawing a tree of views, one by one). 

> Basically PorterDuff modes (esp DST_OUT) cannot work directly in the Canvas
> on GB phones. Hence, we move it to offscreen bitmap and do it. But on ICS
> phones, they don't have to be. Hence that optimization is available in
> CanvasDelegate. Now the change to enable/disable h/w layer causes the
> PorterDuff to work on a software layer, making us do the GB phone way.
> That's the reason for the black spot there.

Nice catch! I think case, I think a simpler (and less risky) fix for this would be to disable hardware layers altogether in this specific animation. I tested it and it works fine.

> (However I don't understand why this doesn't happen on Galaxy Nexus. I could
> see only one draw() in case of Galaxy Nexus, while two on other phones
> before/during animation. We need to investigate that separately).

Good point.
Comment 41 Lucas Rocha (:lucasr) 2013-03-01 07:24:24 PST
Created attachment 719959 [details] [diff] [review]
Don't use hardware layers on toolbar animations

Sriram, based on your findings, I think the safest approach here is to simply disable hardware layers on the toolbar animations. I tested this on an affected device and confirm this patch fixes the bug.

I think it's still worth pushing some of the changes from your patch though (avoid animating hidden menu button, redundant layout request, etc).
Comment 42 Sriram Ramasubramanian [:sriram] 2013-03-01 09:20:39 PST
Comment on attachment 719959 [details] [diff] [review]
Don't use hardware layers on toolbar animations

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

Looks good to me. Are we taking some parts of my patch too?
Comment 43 Kevin Brosnan [:kbrosnan] 2013-03-01 11:15:48 PST
Might be best to file a followup bug.
Comment 44 Lucas Rocha (:lucasr) 2013-03-01 13:35:08 PST
(In reply to Kevin Brosnan [:kbrosnan] from comment #43)
> Might be best to file a followup bug.

+1

Sriram, a patch containing points 1, 2 and 3 from comment #38 would be a good follow-up.
Comment 45 Kevin Brosnan [:kbrosnan] 2013-03-01 15:31:58 PST
Filed a new bug 846949
Comment 46 Lucas Rocha (:lucasr) 2013-03-03 14:51:23 PST
Pushed: https://hg.mozilla.org/integration/mozilla-inbound/rev/c30e00ab4c24
Comment 47 Ryan VanderMeulen [:RyanVM] 2013-03-04 14:20:37 PST
https://hg.mozilla.org/mozilla-central/rev/c30e00ab4c24
Comment 48 Markus Popp 2013-03-04 14:25:23 PST
Any chance to get the fix into Firefox 20 before Beta 3 Builds are created end of day tomorrow or Wednesday morning? And of course Aurora too.
Comment 49 Lucas Rocha (:lucasr) 2013-03-05 02:55:36 PST
Comment on attachment 719959 [details] [diff] [review]
Don't use hardware layers on toolbar animations

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 709433
User impact if declined: Black artifact every time the user goes to awesome screen on certain phones (e.g. Galaxy S2)
Testing completed (on m-c, etc.): Landed in m-c, no issues found, fixes the bug.
Risk to taking this patch (and alternatives if risky): Very low, simple patch only changes the specific animation that causes the bug.
String or UUID changes made by this patch: n/a
Comment 51 Markus Popp 2013-03-06 09:17:22 PST
Is doing a respin of the 20.0b3 candidate builds for Firefox Mobile an option, to get this fix in this week rather than next week? Pity that this fix missed 20.0b3 (at least the 1st builds) so narrowly.
Comment 52 Aaron Train [:aaronmt] 2013-03-06 09:34:04 PST
We wont re-spin for styling issues. The fix will be in Beta 4.

Note You need to log in before you can comment on or make changes to this bug.