Closed Bug 1201346 Opened 4 years ago Closed 4 years ago

Toolbar menu button background does not work with lightweight theme on phone

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 43
Tracking Status
firefox43 --- fixed
fennec 43+ ---

People

(Reporter: mcomella, Assigned: mcomella)

References

Details

Attachments

(3 files)

43 to match bug 1148550.
tracking-fennec: --- → 43+
It seems the issue is that the menu button used to be a ShapedButton, which supplies methods for dealing with LWT changes [1] but the new inheritance from ThemedFrameLayout does not have the same methods.

Looking for a solution...

[1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/toolbar/ShapedButton.java?rev=f4b8d637f2d7#56
This does not appear to affect tablets.
Summary: Toolbar menu button background does not work with lightweight theme → Toolbar menu button background does not work with lightweight theme on phone
Bug 1201346 - Make menu button have LWT on phones. r=liuche

This was not broken on tablet.

The new ShapedButtonFrameLayout class is a duplicate of ShapedButton's LWT
code. I tried an approach that extracted this code out to an external class, to
prevent code duplication and reduce the code size but due to the access rights
on the super classes, it was really messy and, imo, not worth it.
Attachment #8660110 - Flags: review?(liuche)
Comment on attachment 8660110 [details]
MozReview Request: Bug 1201346 - Make menu button have LWT on phones. r=liuche

https://reviewboard.mozilla.org/r/19077/#review17637

Looks good, this matches the ShapedButton implementation pretty well, but there seems to be some cargo-culting throughout all of these ThemedViews. Do you think we should file a bug to audit this code? Some of it just doesn't make sense, but I guess it works well enough right now?

::: mobile/android/base/toolbar/ShapedButtonFrameLayout.java:25
(Diff revision 1)
> +    public void onLightweightThemeChanged() {

Digging through this code, we never ever call setTheme or resetTheme, which is weird because it's implemented in the super.

::: mobile/android/base/toolbar/ShapedButtonFrameLayout.java:44
(Diff revision 1)
> +    public void onLightweightThemeReset() {

Maybe we should call through to the super here too.
Attachment #8660110 - Flags: review?(liuche) → review+
Comment on attachment 8660117 [details]
MozReview Request: Bug 1201346 - Add class comments to ShapedButton*. r=liuche

https://reviewboard.mozilla.org/r/19079/#review17639
Attachment #8660117 - Flags: review?(liuche) → review+
https://reviewboard.mozilla.org/r/19077/#review17637

Filed bug 1206291.

Note that I filed bug 1104928 a while back to clean up the general LWT API, but it doesn't really include these changes.
https://hg.mozilla.org/mozilla-central/rev/e47dbdf4b67f
https://hg.mozilla.org/mozilla-central/rev/1c78fb059965
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
You need to log in before you can comment on or make changes to this bug.