817828 - (blacktab) Black area near tabs button after the URL bar is animated

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Theme and Visual Design, defect)

Description

[Brian Nicholson (:bnicholson)]

Attachment: 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]

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)]

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)]

Brian, can you reproduce this bug on latest Nightly?

Comment 4

[Brian Nicholson (:bnicholson)]

(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)]

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)]

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 8

[Scott Johnson (:jwir3)]

(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 12

[Aaron Train [:aaronmt]]

This is still broken on Nightly builds.

Comment 13

[Mounir Lamouri (:mounir)]

A regression window might help here I guess.

Comment 14

[Paul Feher]

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)]

It's a regression from bug 709433 (see blocks field).

Comment 16

[Aaron Train [:aaronmt]]

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)]

FYI: I'll be trying a couple of tentative fixes this week on the SII.

Comment 19

[Sriram Ramasubramanian [:sriram]]

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]]

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)]

I can reproduce this 100% on my HTC Sensation 4G running Android 4.0.3.

Comment 22

[Lucas Rocha (:lucasr)]

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 24

[bugecho]

(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 [:sideshowbarker]]

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 [:sideshowbarker]]

Ah I spoke too soon. Yeah I reproduced it again by opening up a second tab.

Comment 28

[Kevin Brosnan [Ex-Mozilla]]

Release team this affects Firefox beta and is very user visible.

Comment 29

[Aaron Train [:aaronmt]]

Also see this on the old Samsung S1 line, using a Captivate here.

Comment 30

[Aaron Train [:aaronmt]]

Also reproducible on the on the Samsung GT-S5660

Comment 32

[schoewilliam]

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 34

[Ed Morley [:emorley]]

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]

(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 [Ex-Mozilla]]

Yes.

Comment 37

[Sriram Ramasubramanian [:sriram]]

(Like a child raising his hand in the class)
I have a fix!

Comment 38

[Sriram Ramasubramanian [:sriram]]

Attachment: 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]

Added: http://www.mozilla.org/en-US/mobile/20.0beta/releasenotes/

Comment 40

[Lucas Rocha (:lucasr)]

(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)]

Attachment: 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]]

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 [Ex-Mozilla]]

Might be best to file a followup bug.

Comment 44

[Lucas Rocha (:lucasr)]

(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 [Ex-Mozilla]]

Filed a new bug 846949

Comment 46

[Lucas Rocha (:lucasr)]

Pushed: https://hg.mozilla.org/integration/mozilla-inbound/rev/c30e00ab4c24

Comment 47

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c30e00ab4c24

Comment 48

[Markus Popp]

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)]

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 50

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6f7104a85618
https://hg.mozilla.org/releases/mozilla-beta/rev/544463716c0a

Comment 51

[Markus Popp]

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]]

We wont re-spin for styling issues. The fix will be in Beta 4.