Closed Bug 1139553 Opened 5 years ago Closed 5 years ago

Black overlay for doorhanger background

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 41
Tracking Status
firefox41 --- verified
fennec 41+ ---

People

(Reporter: liuche, Assigned: liuche)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

The background of the doorhanger should be a transparent black.
Opacity should be at 0.6 of black, and the overlay only covers the web content area.
Assignee: nobody → ally
This isn't an aurora bug, but if we can ship this in 40, that would be fantastic.
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.
tracking-fennec: --- → ?
This should track 41, along with the rest of the "Doorhanger content styling and layout" work.
It has been suggested that I am overloaded, how would you feel about taking this back?
Flags: needinfo?(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.
Assignee: ally → liuche
Flags: needinfo?(liuche) → needinfo?(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 :)
Flags: needinfo?(ally)
tracking-fennec: ? → 41+
Bug 1139553 - Black overlay for doorhanger background. r=ally
Attachment #8615673 - Flags: review?(ally)
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
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.
Blocks: 1171784
(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 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?
Attachment #8615673 - Flags: review?(ally)
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
Attachment #8615673 - Flags: review?(ally)
Attachment #8615673 - Flags: feedback?(margaret.leibovic)
Comment on attachment 8615673 [details]
MozReview Request: Bug 1139553 - Black overlay for doorhanger background. r=ally

thank you.
Attachment #8615673 - Flags: review?(ally) → review+
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.
Attachment #8615673 - Flags: feedback?(margaret.leibovic) → feedback+
https://hg.mozilla.org/mozilla-central/rev/f7e900c02fdd
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41
Tested with:
Device: Alcatel One Touch (Android 4.1.2)
Build: Firefox for Android 41.0a1 (2015-06-16)
You need to log in before you can comment on or make changes to this bug.