The menu_panel_bg drawable is gone

RESOLVED FIXED in Firefox 46

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: sunhaitao, Assigned: sunhaitao)

Tracking

46 Branch
Firefox 46
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox46 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

3 years ago
Open Web Apps on Android still have menus spawn from hardware button. Without the menu_panel_bg drawable, their background become transparent.
(Assignee)

Comment 1

3 years ago
Posted patch menu_panel_bg.patch (obsolete) — Splinter Review
Assignee: nobody → sunhaitao
Status: NEW → ASSIGNED
Attachment #8701051 - Flags: review?(s.kaspari)
Comment on attachment 8701051 [details] [diff] [review]
menu_panel_bg.patch

mcomella is probably the best one for reviewing this. I don't remember the details but didn't we have a problem with some devices using dark backgrounds and some others using light backgrounds?
Attachment #8701051 - Flags: review?(s.kaspari) → review?(michael.l.comella)
(In reply to SUN Haitao from comment #0)
> Open Web Apps on Android still have menus spawn from hardware button.
> Without the menu_panel_bg drawable, their background become transparent.

I think the "most correct" solution is to backout the bug that changed this, bug 1229958. Thanks for the report, Sun!

(In reply to Sebastian Kaspari (:sebastian) from comment #2)
> I don't remember the
> details but didn't we have a problem with some devices using dark
> backgrounds and some others using light backgrounds?

This was a side effect of using a color as the panelBackground, rather than a drawable. iirc, AppCompat necessitated this but if the web apps don't use AppCompat, maybe the menu just works.
Status: ASSIGNED → NEW
Actually, I see we performed some funny business on GB in bug 1229958, where it was defined as:
 <item name="android:panelBackground">@color/text_and_tabs_tray_grey</item>

Note that the issue Sebastian mentioned in comment 2 was specific to GB devices, where we can't always override the menu color.

Sun, did you test this fix on GB devices?
Depends on: 1229958
Flags: needinfo?(sunhaitao)
Comment on attachment 8701051 [details] [diff] [review]
menu_panel_bg.patch

Review of attachment 8701051 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm, provided this works on GB.
Attachment #8701051 - Flags: review?(michael.l.comella) → review+
Looking at alternative solutions, what does the hardware menu button spawn on web apps, Sun? Perhaps you can post a screenshot? Can software-only menu button users also get these menus? Do we need to change the mechanisms (as we did on the full browser) to make the hardware menu button work like the software menu button and avoid the transparency issue altogether?
(Assignee)

Comment 7

3 years ago
Posted image Menu on Nightly
Flags: needinfo?(sunhaitao)
(Assignee)

Comment 8

3 years ago
Posted image Menu on Patched
(Assignee)

Comment 9

3 years ago
I haven't test this on GB yet. It is a little bit hard to find such devices around here now.
(In reply to SUN Haitao from comment #9)
> I haven't test this on GB yet. It is a little bit hard to find such devices
> around here now.

Can you give me an STR? I have a few devices I can test on.
Flags: needinfo?(sunhaitao)
(Assignee)

Comment 11

3 years ago
Steps to Reproduce:

0. Install and launch an Open Web App with Fennec.
1. Press the hardware 'menu' button.
2. A menu with transparent background will appear.
Flags: needinfo?(sunhaitao)
Where do I get an Open Web App from? Are those the web apps from marketplace.firefox.com?
Flags: needinfo?(sunhaitao)
(In reply to Michael Comella (:mcomella) from comment #12)
> Where do I get an Open Web App from? Are those the web apps from
> marketplace.firefox.com?

We are dropping support for those web apps, so I don't think we should spend time worrying about this:
https://bugzilla.mozilla.org/show_bug.cgi?id=1235869

What menu is even expected to be shown? Web apps shouldn't rely on hardware menu buttons being present.
(Assignee)

Comment 14

3 years ago
(In reply to Michael Comella (:mcomella) from comment #12)
> Where do I get an Open Web App from? Are those the web apps from
> marketplace.firefox.com?

Yes, they are.

> We are dropping support for those web apps, so I don't think we should spend
> time worrying about this:
> https://bugzilla.mozilla.org/show_bug.cgi?id=1235869

I am building a Gecko-based Crosswalk-like solution with some friends. We chose to reuse the WebRT for GeckoView seems not quite mature. Currently we have got a working prototype with some minor patches, and planning a real-world trail in next Spring. Dropping WebRT may cause us a lot of trouble. Maybe we could discuss this matter more on Bug 1235869/Mailing List?

> What menu is even expected to be shown? Web apps shouldn't rely on hardware
> menu buttons being present.

The current implementation provides a non-customizable menu to 'Quit' the OWA.
Flags: needinfo?(sunhaitao)
(In reply to SUN Haitao from comment #14)
> I am building a Gecko-based Crosswalk-like solution with some friends. We
> chose to reuse the WebRT for GeckoView seems not quite mature. Currently we
> have got a working prototype with some minor patches, and planning a
> real-world trail in next Spring. Dropping WebRT may cause us a lot of
> trouble. Maybe we could discuss this matter more on Bug 1235869/Mailing List?

Yes, we can discuss this matter in the discussion forum, and that's the best place to discuss it, since that's where we're having the conversation about disabling the web runtimes for desktop and Android.
I tested on GB and we need the panelBackground definition there too.

Since we don't want to leave open web apps in a broken state before we remove it, let's land this with the GB change.
The hardware-button-triggered menu of Open Web App is still using it.
Assignee: sunhaitao → michael.l.comella
Status: NEW → ASSIGNED
Assignee: michael.l.comella → sunhaitao
Comment on attachment 8703173 [details] [diff] [review]
Add the menu_panel_bg drawable back

Review of attachment 8703173 [details] [diff] [review]:
-----------------------------------------------------------------

I already r+'d this.
Attachment #8703173 - Flags: review+
I filed bug 1236071 to remove panelBackground if open web apps are removed.

Comment 21

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/163980e9e464
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
You need to log in before you can comment on or make changes to this bug.