1055598 - Regression: door-hangers and other images used in UI are transparent

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Aaron Train [:aaronmt]]

Attachment: screenshot.png

Currently, door-hangers are transparent.

Last good revision: 0aaa2d3d15cc (2014-08-18)
First bad revision: 111a1da2a95d (2014-08-19)

Pushlog:

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=0aaa2d3d15cc&tochange=111a1da2a95d

--
LG Nexus 5 (Android 4.4.4)
Nightly (08/14)

Comment 1

[Aaron Train [:aaronmt]]

See fxteam pushes at bottom, if it's not new-toolbar, it's one of those https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=5fb5adcc3835&tochange=6030bd4e48f8

Comment 2

[Lucas Rocha (:lucasr)]

Bug 1048683 looks like the likely reason here. It changed the background image used in the popup. ckitching, could you have a look at this?

Comment 3

[Chris Kitching [:ckitching]]

Attachment: solution.patch

Testing shows this is definitely the fault of my patch.

The problematic nine-patch is arrow_popup_bg. Here it is before and after my patch:

Before:
https://www.dropbox.com/s/snd2f7wu2tpoh5l/arrow_popup_bg-before.9.png?dl=0

After:
https://www.dropbox.com/s/2ckhptm30c1be14/arrow_popup_bg-after.9.png?dl=0


The script has collapsed the scalable Y-region, which consisted of identical rows. This is ostensibly a safe transformation.

For comedic effect, I tried replacing the single row of pixels that now constitute the Y-scalable region with a pair of identical rows. The result was a DoorHanger which had about double the opacity of the one pictured in comment zero:
https://www.dropbox.com/s/daejuktofddsnjl/TwoPixels.png?dl=0

And this is what you get if you revert the changes to that png entirely:
https://www.dropbox.com/s/ubsnxgw538t1qsc/LotsOfPixels.png?dl=0

Compare these two to the image from Comment 0.

DaFuq?
Do nine-patches somehow work in a completely different way to what I think they do that causes this to all make sense?

One of the following seems to be the answer:

1) I'm just wrong about the safety of this transformation on nine-patches (but the documentation (such as it is) seems to support my claim...)

2) There's some weird bug in the way Android handles nine-patches.

3) Something complicated is happening in the code that uses this nine-patch that's producing this funky behaviour.


The other nine-patches in the patch don't appear to be exhibiting similar insanities, so I'm leaning towards option 3.

With that in mind, here's a patch that reverts the changes to the various arrow_popup_bg drawables (and which definitely fixes the reported problem). Bonus points to anyone who figures out the source of this insanity.

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Chris Kitching [:ckitching] from comment #3)

> The other nine-patches in the patch don't appear to be exhibiting similar
> insanities, so I'm leaning towards option 3.
> 
> With that in mind, here's a patch that reverts the changes to the various
> arrow_popup_bg drawables (and which definitely fixes the reported problem).
> Bonus points to anyone who figures out the source of this insanity.

What can we do to do some manual testing around the other 9 patch pngs, so we don't get surprised later.

Comment 5

[Lucas Rocha (:lucasr)]

Attachment: Autocompletion background

Noticed that the web autocompletion popup is affected too. ckitching, I'm starting to think we should simply backout the patch until we figure out what's causing this.

Comment 6

[Lucas Rocha (:lucasr)]

Comment on attachment 8475350 [details] [diff] [review]
solution.patch

This is not the only image with side-effects (see screenshot I've just posted).

Comment 7

[Chris Kitching [:ckitching]]

(In reply to Lucas Rocha (:lucasr) from comment #5)
> Created attachment 8475817 [details]
> Autocompletion background
> 
> Noticed that the web autocompletion popup is affected too. ckitching, I'm
> starting to think we should simply backout the patch until we figure out
> what's causing this.

Sounds like a plan. Pushing a backout now, then. Instead of manually verifying all the images, the way forward seems to be to figure out why this is happening. The script performed the intended modification to the images, so the issue lies in that not being as safe as I thought it was.

This is quite interesting, but not that useful to spend a lot of time on. Nine-patches aren't supposed to behave like this... :/

Comment 8

[Aaron Train [:aaronmt]]

Fixed via backout of bug 1048683