1127154 - Enabling LWT changes the pressed/focused/etc. color of the navigation buttons

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

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

Comment 1

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

Attachment: Patch

Comment 2

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

Attachment: 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

Comment 3

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

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

Comment 4

[Martyn Haigh (:mhaigh)]

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.

Comment 5

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

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.

Comment 6

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

https://hg.mozilla.org/integration/fx-team/rev/d69f2fb5f901

Comment 7

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

(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.

Comment 8

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

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

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d69f2fb5f901

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/eec6da7356b4

Needs rebasing for beta uplift.

Comment 11

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

Attachment: (beta) Use new_tablet_url_bar_nav_button styles on LWT change in NavButton

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/25a28c2481d2

Comment 14

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

Attachment: MozReview Request: Bug 1127154 - Use new_tablet_url_bar_nav_button styles on LWT change in NavButton.