1139553 - Black overlay for doorhanger background

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Chenxia Liu [:liuche]]

The background of the doorhanger should be a transparent black.

Comment 1

[Chenxia Liu [:liuche]]

Attachment: Screenshot: Partial dark overlay

Opacity should be at 0.6 of black, and the overlay only covers the web content area.

Comment 2

[Chenxia Liu [:liuche]]

This isn't an aurora bug, but if we can ship this in 40, that would be fantastic.

Comment 3

[Chenxia Liu [:liuche]]

Keep in mind while you're designing this, there are two cases here - SiteIdentityPopup is summoned by clicking the favicon/lock in the toolbar, while DoorhangerPopup is summoned by the "Doorhanger:Add" notification from Gecko.

Comment 4

[Chenxia Liu [:liuche]]

This should track 41, along with the rest of the "Doorhanger content styling and layout" work.

Comment 5

[Allison Naaktgeboren :ally]

It has been suggested that I am overloaded, how would you feel about taking this back?

Comment 6

[Chenxia Liu [:liuche]]

Sure, I can take this back. Do you have any WIP or things you discovered while working on it that I should know about? Feel free to dump a comment.

Comment 7

[Allison Naaktgeboren :ally]

The patch isnt particularly interesting, just me coloring a background or two in layout. Notes from debugging & conversations are below

- plan A: find the ui object corresponding to viewport area, and wrap it in a framelayout (which has a z axis) and stick the overlay there. 
- snorp proposes wrapping it in a linearlayout and setting the background a specific opacity. (though I think that would gray out the doorhanger?)

- RelativeLayout android:id="@+id/gecko_layout is not the object you want, and gecko_app.xml isn't interesting here.
- the LayerView class is what contains the gecko content, is a LinearLayout object
- lives in shared_ui_components.xml
- todo: look at selection handlers in shared_ui_components.xml
- questions about messing with  LayerView set off sirens for mfinkle :)

Comment 8

[Chenxia Liu [:liuche]]

Attachment: MozReview Request: Bug 1139553 - Black overlay for doorhanger background. r=ally

Bug 1139553 - Black overlay for doorhanger background. r=ally

Comment 9

[Chenxia Liu [:liuche]]

Comment on attachment 8615673 [details]
MozReview Request: Bug 1139553 - Black overlay for doorhanger background. r=ally

Bug 1139553 - Black overlay for doorhanger background. r=ally

Comment 10

[Chenxia Liu [:liuche]]

I realized that Gingerbread doesn't handle initial alphas set in xml immediately, so there was always a flash of the overlay on cold start. Disabling this for GB for that, which will also make perf better.

Comment 11

[:Margaret Leibovic]

(In reply to Chenxia Liu [:liuche] from comment #10)
> I realized that Gingerbread doesn't handle initial alphas set in xml
> immediately, so there was always a flash of the overlay on cold start.
> Disabling this for GB for that, which will also make perf better.

+1, the doorhanger notification is perfectly functional without the overlay, so let's not struggle to get it working on GB.

Comment 12

[Allison Naaktgeboren :ally]

Comment on attachment 8615673 [details]
MozReview Request: Bug 1139553 - Black overlay for doorhanger background. r=ally

https://reviewboard.mozilla.org/r/10331/#review9147

::: mobile/android/base/BrowserApp.java:1410
(Diff revision 2)
> +        if (Versions.preHC) {

Why do we skip the showing the doorhanger in GB?
I get why animations are off the table in GB (and thus the overlay) but shouldn't they at least see the doorhanger?

Or, if this is only for the animation, as it looks like the rest of the function is, can it have a more obvious name like onDoorHangerShowAnimation() ?

::: mobile/android/base/BrowserApp.java:1423
(Diff revision 2)
> +    public void onDoorHangerHide() {

same comment here

::: mobile/android/base/BrowserApp.java:921
(Diff revision 2)
> +        // Gingerbread 2.3 doesn't handle starting alphas correctly, so disable doorhanger overlays for that, and perf.

\_delightful\_. How did you find out about starting alphas & GB?

Comment 13

[Chenxia Liu [:liuche]]

https://reviewboard.mozilla.org/r/10331/#review9207

::: mobile/android/base/BrowserApp.java:1410
(Diff revision 2)
> +        if (Versions.preHC) {

The doorhanger is still being shown on GB; I didn't remove any of the DoorhangerPopup/SiteIdentityPopup code. (If you build this code, you'll see that the doorhanger still shows up.)

This onDoorhangerShow is part of the interface OnVisibilityChangeListener that BrowserApp implements, which the doorhanger popup code calls when it shows/hides the dorhanger. (It shouldn't be called onDoorhangerAnimation because this interface is generic and gets called when the doorhanger is being shown - it just happens to implement an animation.)

http://stackoverflow.com/a/18279545/582004

Comment 14

[Allison Naaktgeboren :ally]

Comment on attachment 8615673 [details]
MozReview Request: Bug 1139553 - Black overlay for doorhanger background. r=ally

thank you.

Comment 15

[:Margaret Leibovic]

Comment on attachment 8615673 [details]
MozReview Request: Bug 1139553 - Black overlay for doorhanger background. r=ally

This looks good to me. Thanks for taking care to make sure the alpha animations perform well.

Comment 16

[Chenxia Liu [:liuche]]

https://hg.mozilla.org/integration/fx-team/rev/f7e900c02fdd

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 18

[Teodora Vermesan (:TeoVermesan)]

Attachment: Screenshot_2015-06-17-10-47-39(1).png

Tested with:
Device: Alcatel One Touch (Android 4.1.2)
Build: Firefox for Android 41.0a1 (2015-06-16)