Closed
Bug 1127154
Opened 10 years ago
Closed 10 years ago
Enabling LWT changes the pressed/focused/etc. color of the navigation buttons
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(firefox36 fixed, firefox37 fixed, firefox38 fixed)
RESOLVED
FIXED
Firefox 38
People
(Reporter: mcomella, Assigned: mcomella)
References
Details
Attachments
(2 files, 2 obsolete files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
text/x-review-board-request
|
mhaigh
:
review+
|
Details |
The states here should take their values from new_tablet_url_bar_nav_button.
https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/toolbar/NavButton.java#76
Assignee | ||
Comment 1•10 years ago
|
||
Attachment #8556225 -
Flags: review?(mhaigh)
Assignee | ||
Comment 2•10 years ago
|
||
/r/3119 - Bug 1127154 - Use new_tablet_url_bar_nav_button styles on LWT change in NavButton.
Pull down this commit:
hg pull review -r 7f64a3849a6b68fbc77a8ebd6bb80818b4e3cf71
Assignee | ||
Comment 3•10 years ago
|
||
Comment on attachment 8556228 [details]
MozReview Request: bz://1127154/mcomella
/r/3119 - Bug 1127154 - Use new_tablet_url_bar_nav_button styles on LWT change in NavButton.
Pull down this commit:
hg pull review -r 7f64a3849a6b68fbc77a8ebd6bb80818b4e3cf71
Attachment #8556228 -
Flags: review?(mhaigh)
Assignee | ||
Updated•10 years ago
|
Attachment #8556225 -
Attachment is obsolete: true
Attachment #8556225 -
Flags: review?(mhaigh)
Comment 4•10 years ago
|
||
Comment on attachment 8556228 [details]
MozReview Request: bz://1127154/mcomella
https://reviewboard.mozilla.org/r/3117/#review2491
Looks good - nit is minor and doesn't really need addressing, but it may be nice to expand if it doesn't mean having to change the rest of that file (consistency is key).
::: mobile/android/base/toolbar/NavButton.java
(Diff revision 1)
> - stateList.addState(PRIVATE_PRESSED_STATE_SET, getColorDrawable(R.color.highlight_nav_pb));
> + stateList.addState(PRIVATE_PRESSED_STATE_SET, getColorDrawable(R.color.new_tablet_highlight_pb));
nit: I'd expand "pb" to "private_browsing". Having not looked at this code before I was confused about what it was.
Attachment #8556228 -
Flags: review?(mhaigh) → review+
Assignee | ||
Comment 5•10 years ago
|
||
https://reviewboard.mozilla.org/r/3117/#review2495
> nit: I'd expand "pb" to "private_browsing". Having not looked at this code before I was confused about what it was.
It's been around for a while so I'm going to skip it for now (and easier uplift).
However, I think we can put some efforts into consolidating our colors w/ :antlam's color palette - I'll talk to him while he's in SF.
Assignee | ||
Comment 6•10 years ago
|
||
Assignee | ||
Comment 7•10 years ago
|
||
(In reply to Michael Comella (:mcomella) from comment #6)
> https://hg.mozilla.org/integration/fx-team/rev/d69f2fb5f901
r=mhaigh - hg error caused it to be excluded on the commit.
Assignee | ||
Comment 8•10 years ago
|
||
Comment on attachment 8556228 [details]
MozReview Request: bz://1127154/mcomella
Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]:
[Describe test coverage new/current, TreeHerder]:
[Risks and why]:
[String/UUID change made/needed]:
Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]:
[Describe test coverage new/current, TreeHerder]:
[Risks and why]:
[String/UUID change made/needed]:
Approval Request Comment
[Feature/regressing bug #]: New tablet UI (bug 1014156)
[User impact if declined]:
LWT theme users (and those that disable LWT but have not yet restarted FF) will see different colors when focusing or pressing the back/forward buttons.
[Describe test coverage new/current, TreeHerder]: None
[Risks and why]:
Low - we're just changing some resource names so we'll likely screw up the colors added, if anything. In the worst case, the resources are missing and we have compile time errors, or they are null, so we crash. However, unlikely.
[String/UUID change made/needed]: None
Attachment #8556228 -
Flags: approval-mozilla-beta?
Attachment #8556228 -
Flags: approval-mozilla-aurora?
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 38
Updated•10 years ago
|
Updated•10 years ago
|
Attachment #8556228 -
Flags: approval-mozilla-beta?
Attachment #8556228 -
Flags: approval-mozilla-beta+
Attachment #8556228 -
Flags: approval-mozilla-aurora?
Attachment #8556228 -
Flags: approval-mozilla-aurora+
Comment 10•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/eec6da7356b4
Needs rebasing for beta uplift.
Assignee | ||
Comment 11•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(michael.l.comella) → needinfo?(ryanvm)
Keywords: branch-patch-needed
Comment 12•10 years ago
|
||
Updated•10 years ago
|
Flags: needinfo?(ryanvm)
Assignee | ||
Comment 13•9 years ago
|
||
Attachment #8556228 -
Attachment is obsolete: true
Attachment #8619240 -
Flags: review+
Assignee | ||
Comment 14•9 years ago
|
||
Updated•4 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
•