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

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Regression from bug 1148550.

Comment 1

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

43 to match bug 1148550.

Comment 2

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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

Comment 3

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

This does not appear to affect tablets.

Comment 4

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: Bug 1201346 - Make menu button have LWT on phones. r=liuche

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.

Comment 5

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Abandoned WIP to separate the shared code into a separate class

The WIP for comparison's sake.

Comment 6

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: Bug 1201346 - Add class comments to ShapedButton*. r=liuche

Bug 1201346 - Add class comments to ShapedButton*. r=liuche

Comment 7

[Chenxia Liu [: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.

Comment 8

[Chenxia Liu [:liuche]]

Comment on attachment 8660117 [details]
MozReview Request: Bug 1201346 - Add class comments to ShapedButton*. r=liuche

https://reviewboard.mozilla.org/r/19079/#review17639

Comment 9

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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.

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/e47dbdf4b67f
https://hg.mozilla.org/integration/fx-team/rev/1c78fb059965

Comment 11

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/e47dbdf4b67f
https://hg.mozilla.org/mozilla-central/rev/1c78fb059965