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

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Kevin Brosnan [Ex-Mozilla]]

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

[:Margaret Leibovic]

This also may be an Android bug, but we should at least try to understand what's going on here.

Comment 2

[Andrzej Hunt :ahunt]

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

Comment 3

[Andrzej Hunt :ahunt]

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

Comment 4

[Andrzej Hunt :ahunt]

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

Comment 5

[Andrzej Hunt :ahunt]

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

[:Margaret Leibovic]

This isn't going to make 47.

ahunt, what's the status of your investigation here?

Comment 7

[Andrzej Hunt :ahunt]

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

Comment 8

[Andrzej Hunt :ahunt]

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/

Comment 9

[Sebastian Kaspari (:sebastian; :pocmo)]

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.

Comment 10

[Andrzej Hunt :ahunt]

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

Comment 11

[Andrzej Hunt :ahunt]

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

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/62b97fa3c6ab

Comment 14

[Mike Kaply [:mkaply]]

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.

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/62b97fa3c6ab

Comment 16

[Andrzej Hunt :ahunt]

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

Comment 17

[Mike Kaply [:mkaply]]

I think if it makes it to 48 by the end of June, we should be OK.

Comment 18

[Andrzej Hunt :ahunt]

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.