Bug 817828 (blacktab)

Black area near tabs button after the URL bar is animated

VERIFIED FIXED in Firefox 20

Status

()

Firefox for Android
Theme and Visual Design
VERIFIED FIXED
4 years ago
9 months ago

People

(Reporter: bnicholson, Assigned: lucasr)

Tracking

({regression, reproducible})

20 Branch
Firefox 22
ARM
Android
regression, reproducible
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox19 unaffected, firefox20+ verified, firefox21 verified, firefox22 verified, relnote-firefox 20+, fennec20+)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

4 years ago
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
Summary: Black area near tabs button after the URL bar is animated with horizontal orientation → Black area near tabs button after the URL bar is animated with landscape orientation

Comment 1

4 years ago
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)
status-firefox20: --- → affected
(Assignee)

Comment 2

4 years ago
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.
(Assignee)

Comment 3

4 years ago
Brian, can you reproduce this bug on latest Nightly?
Flags: needinfo?(bnicholson)
(Reporter)

Comment 4

4 years ago
(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.
Flags: needinfo?(bnicholson)
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.
tracking-fennec: --- → ?

Updated

4 years ago
Assignee: nobody → lucasr.at.mozilla
Keywords: regression, reproducible
Version: unspecified → Trunk
tracking-fennec: ? → 20+
(Assignee)

Comment 6

4 years ago
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).

Updated

4 years ago
Duplicate of this bug: 825323
(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.
(Reporter)

Updated

4 years ago
tracking-firefox20: --- → ?
Summary: Black area near tabs button after the URL bar is animated with landscape orientation → Black area near tabs button after the URL bar is animated
Duplicate of this bug: 826367

Updated

4 years ago
Duplicate of this bug: 830101

Updated

4 years ago
tracking-firefox20: ? → +

Updated

4 years ago
Duplicate of this bug: 832682
This is still broken on Nightly builds.
A regression window might help here I guess.
Keywords: regressionwindow-wanted

Comment 14

4 years ago
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.
Keywords: regressionwindow-wanted
(Reporter)

Comment 15

4 years ago
It's a regression from bug 709433 (see blocks field).
Still an issue on firefox-20/firefox-21; I see this frequently more-so on my Galaxy SII opposed to any other device.
status-firefox21: --- → affected
(Assignee)

Comment 17

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

Updated

4 years ago
Duplicate of this bug: 839283

Updated

4 years ago
status-firefox19: --- → unaffected
Component: General → Theme and Visual Design
Version: Trunk → Firefox 20
This seems to be an issue on phones with h/w menu button. Could we see if that's a problem?
SII seems to be the easiest device to reproduce this on. I haven't encountered this on any Nexus device.
I can reproduce this 100% on my HTC Sensation 4G running Android 4.0.3.

Updated

4 years ago
Alias: blacktab
Status: NEW → ASSIGNED
status-firefox22: --- → affected
(Assignee)

Comment 22

4 years ago
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.

Updated

4 years ago
Duplicate of this bug: 843209

Comment 24

4 years ago
(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

4 years ago
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

4 years ago
Ah I spoke too soon. Yeah I reproduced it again by opening up a second tab.
Duplicate of this bug: 844212
Release team this affects Firefox beta and is very user visible.
relnote-firefox: --- → ?
Also see this on the old Samsung S1 line, using a Captivate here.
QA Contact: aaron.train
Also reproducible on the on the Samsung GT-S5660

Updated

4 years ago
Duplicate of this bug: 845031

Comment 32

4 years ago
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 :/

Updated

4 years ago
Duplicate of this bug: 845810
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.
Flags: needinfo?(lucasr.at.mozilla)
(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"?
Yes.
(Like a child raising his hand in the class)
I have a fix!
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).
Attachment #719189 - Flags: review?(lucasr.at.mozilla)
Added: http://www.mozilla.org/en-US/mobile/20.0beta/releasenotes/
relnote-firefox: ? → 20+
(Assignee)

Comment 40

4 years ago
(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.
Flags: needinfo?(lucasr.at.mozilla)
(Assignee)

Comment 41

4 years ago
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).
Attachment #719959 - Flags: review?(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?
Attachment #719959 - Flags: review?(sriram) → review+
Might be best to file a followup bug.
(Assignee)

Comment 44

4 years ago
(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.
Blocks: 846949
Filed a new bug 846949
(Assignee)

Comment 46

4 years ago
Pushed: https://hg.mozilla.org/integration/mozilla-inbound/rev/c30e00ab4c24
https://hg.mozilla.org/mozilla-central/rev/c30e00ab4c24
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 22

Comment 48

4 years ago
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.
Attachment #719189 - Attachment is obsolete: true
Attachment #719189 - Flags: review?(lucasr.at.mozilla)

Updated

4 years ago
status-firefox22: affected → fixed
(Assignee)

Comment 49

4 years ago
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
Attachment #719959 - Flags: approval-mozilla-beta?
Attachment #719959 - Flags: approval-mozilla-aurora?
Attachment #719959 - Flags: approval-mozilla-beta?
Attachment #719959 - Flags: approval-mozilla-beta+
Attachment #719959 - Flags: approval-mozilla-aurora?
Attachment #719959 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/6f7104a85618
https://hg.mozilla.org/releases/mozilla-beta/rev/544463716c0a
status-firefox20: affected → fixed
status-firefox21: affected → fixed

Comment 51

4 years ago
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.
We wont re-spin for styling issues. The fix will be in Beta 4.

Updated

4 years ago
Status: RESOLVED → VERIFIED
status-firefox20: fixed → verified
status-firefox21: fixed → verified
status-firefox22: fixed → verified
You need to log in before you can comment on or make changes to this bug.