Flickering in the address bar tab count when navigating to/from a page

RESOLVED FIXED in Firefox 50

Status

()

Firefox for Android
General
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: kbrosnan, Assigned: ahunt)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 50
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox45 unaffected, firefox46 unaffected, firefox47 unaffected, firefox48 unaffected, fennec48+, firefox50 unaffected)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Reporter)

Description

2 years ago
Created attachment 8741501 [details]
Video of the flickering near the tabs button

There is flickering in the address bar under the tabs count button when navigating to/from a page.

Comment 1

2 years ago
This also may be an Android bug, but we should at least try to understand what's going on here.
Assignee: nobody → s.kaspari
tracking-fennec: ? → 47+
(Assignee)

Comment 2

2 years ago
It seems Android N has issues with transparency, but setting the ToolbarProgressView to use software rendering seems to fix this (I don't really understand all of this yet, but I have a patch which fixes the issues).

- We draw a 9-patch drawable (the progress bar) that includes ~10px of transparent empty space at the top - this is painted over the toolbar / app.
- The tabs button foreground disappears wherever transparency from the progress-bar drawable is painted. (The button foreground is a dark closed path, i.e. the curve. The button background is the light grey that we see.)
Assignee: s.kaspari → ahunt
Status: NEW → ASSIGNED
(Assignee)

Comment 3

2 years ago
Created attachment 8754546 [details]
Bug 1264783 - Use Hardware layers for tabs curve (except on pre-lollipop)

We have transparency in some of the underlying toolbar elements (primarily the tab
curve). On Android N overlayed transparent areas result in any underlying transparent
areas being ignored. I.e. the transparency at the top of the progress drawable prevents
the bottom of the tab-button from being rendered, resulting in what looks like
a gap at the bottom of the tab button (since the tab button is actually a grey background
with a closed path, containing some transparency, being drawn over the top of it).

Review commit: https://reviewboard.mozilla.org/r/54030/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/54030/
(Assignee)

Comment 4

2 years ago
(In reply to Andrzej Hunt :ahunt from comment #2)
> It seems Android N has issues with transparency, but setting the
> ToolbarProgressView to use software rendering seems to fix this (I don't
> really understand all of this yet, but I have a patch which fixes the
> issues).
> 
> - We draw a 9-patch drawable (the progress bar) that includes ~10px of
> transparent empty space at the top - this is painted over the toolbar / app.
> - The tabs button foreground disappears wherever transparency from the
> progress-bar drawable is painted. (The button foreground is a dark closed
> path, i.e. the curve. The button background is the light grey that we see.)

On further investigation this doesn't seem to be transparency related, but instead possibly layout related: the tab button is clipped when we draw() the progress bar in ToolbarProgressView: if I shift the bounds for the drawable (e.g. up by 10px), the same portion of the tab button is clipped/greyed even though the drawable is now higher on screen (and if we shift it even higher, then the same area flickers even though the drawable isn't drawn any more).

I've not managed to create a simpler reproduction scenario outside of fennec, and I'm not sure what aspect of our setup is actually provoking this clipping. (The way we draw the tab button is already quite complicated, recreating that and adding a simple ImageView with the same progress bar 9-patch over the top doesn't result in the same issues.)
(Assignee)

Comment 5

2 years ago
Interestingly using setLayerType(LAYER_TYPE_HARDWARE) on the tabs button cures the tab count flickering, setting SOFTWARE for that same button results in the entire button disappearing. LAYER_TYPE_NONE seems to be the default, setting that explicitly results in both flickering behaviours occuring.

Comment 6

2 years ago
This isn't going to make 47.

ahunt, what's the status of your investigation here?
tracking-fennec: 47+ → 48+
Flags: needinfo?(ahunt)
(Assignee)

Comment 7

2 years ago
(In reply to :Margaret Leibovic from comment #6)
> This isn't going to make 47.
> 
> ahunt, what's the status of your investigation here?

I think we should go for the simple (but not particularly satisfying) fix of using hardware layers for this button - that will need testing across various older devices to make sure we don't suddenly break there.

I spent some time trying to build a reproducer outside of fennec to help with filing an Android bug, but I've not been able to get anywhere with that. Given that the flickering is highly specific to our setup (the layouting and drawing is quite complicated, and I don't think I understand all the steps involved - which is why it's difficult to build an independent reproducer), I doubt there's much chance of getting a useful Android bug filed (never mind getting it fixed).
Flags: needinfo?(ahunt)
(Assignee)

Comment 8

2 years ago
Comment on attachment 8754546 [details]
Bug 1264783 - Use Hardware layers for tabs curve (except on pre-lollipop)

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54030/diff/1-2/
Attachment #8754546 - Attachment description: MozReview Request: Bug 1264783 - Use software rendering for progress bar to avoid flickering → Bug 1264783 - Use Hardware layers for tabs curve (except on pre-lollipop)
Attachment #8754546 - Flags: review?(s.kaspari)
Comment on attachment 8754546 [details]
Bug 1264783 - Use Hardware layers for tabs curve (except on pre-lollipop)

https://reviewboard.mozilla.org/r/54030/#review54708

It would be interesting to see if this changes with new preview builds.
Attachment #8754546 - Flags: review?(s.kaspari) → review+
(Assignee)

Comment 10

2 years ago
(In reply to Sebastian Kaspari (:sebastian) from comment #9)
> Comment on attachment 8754546 [details]
> Bug 1264783 - Use Hardware layers for tabs curve (except on pre-lollipop)
> 
> https://reviewboard.mozilla.org/r/54030/#review54708
> 
> It would be interesting to see if this changes with new preview builds.

I feel like just to be safe, we should land this ~everywhere, and keep re-testing every time there's a new preview to see if we can back it out. That way we're prepared for this remaining a bug (and also there's no huge harm if we don't have time to re-test).
(Assignee)

Comment 11

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/62b97fa3c6ab25bfe0f30b7230ec8f7aadc0ebd4
Bug 1264783 - Use Hardware layers for tabs curve (except on pre-lollipop) r=sebastian
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1274430

Comment 13

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/62b97fa3c6ab
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Is this something we can pull into Firefox 48? We have a partner requirement and we'd like to get Android N stuff into that release.
(Assignee)

Comment 16

2 years ago
(In reply to Mike Kaply [:mkaply] from comment #14)
> Is this something we can pull into Firefox 48? We have a partner requirement
> and we'd like to get Android N stuff into that release.

This seems to apply cleanly, so we are able to uplift.

How soon would you want this in 48? I'd prefer to keep this on nightly for a bit for more testing, OTOH uplifting to Beta might be equally useful to test more widely (as long as we're prepared to do any device-/release- specific tweaking immediately if issues do arise).
I think if it makes it to 48 by the end of June, we should be OK.
(Assignee)

Comment 18

2 years ago
It looks like google fixed this in the latest N preview (Build number NPD56N, downloaded between 12th and 18th June) - as tested using release, beta, and aurora (and no change with nightly, which has the patch).

I'm therefore planning to back this workaround out again.
(Assignee)

Updated

2 years ago
status-firefox45: affected → unaffected
status-firefox46: affected → unaffected
status-firefox47: affected → unaffected
status-firefox48: affected → unaffected
status-firefox50: fixed → unaffected
(Assignee)

Updated

2 years ago
Blocks: 1282372
You need to log in before you can comment on or make changes to this bug.