Closed
Bug 1201346
Opened 10 years ago
Closed 10 years ago
Toolbar menu button background does not work with lightweight theme on phone
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(firefox43 fixed, fennec43+)
RESOLVED
FIXED
Firefox 43
People
(Reporter: mcomella, Assigned: mcomella)
References
Details
Attachments
(3 files)
Regression from bug 1148550.
Assignee | ||
Comment 2•10 years ago
|
||
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
Assignee | ||
Comment 3•10 years ago
|
||
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
Assignee | ||
Comment 4•10 years ago
|
||
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)
Assignee | ||
Comment 5•10 years ago
|
||
The WIP for comparison's sake.
Assignee | ||
Comment 6•10 years ago
|
||
Bug 1201346 - Add class comments to ShapedButton*. r=liuche
Attachment #8660117 -
Flags: review?(liuche)
Comment 7•10 years ago
|
||
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 8•10 years ago
|
||
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+
Assignee | ||
Comment 9•10 years ago
|
||
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•10 years ago
|
||
Comment 11•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/e47dbdf4b67f
https://hg.mozilla.org/mozilla-central/rev/1c78fb059965
Status: NEW → RESOLVED
Closed: 10 years ago
status-firefox43:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
Updated•5 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•