1196553 - Add to Reading list icon in menu doesn't look disabled when disabled on phone

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

Probably another bug 1179479 regression, like bug 1194659.

Comment 1

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

We call GeckoMenuItem.setIcon for the reading list icon [1] whereas our other calls go through:
 MenuItemActionBar.setIcon
  MenuItemActionView.setIcon
   PromptListAdapter.updateActionView

MenuItemActionBar.setIcon
 MenuItemActionBar.initialize

etc.

[1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/BrowserApp.java?rev=2b213c98f8a8#3234

Comment 2

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

Tracking 42 to match bug 1179479, where this first landed.

Comment 3

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

Attachment: MozReview Request: Bug 1196553 - Set secondary action bar colors on phone. r=margaret

Bug 1196553 - Set secondary action bar colors on phone. r=margaret

I previously set this for tablet but didn't (for an unknown reason) set this
for phones as well.

Comment 4

[:Margaret Leibovic]

Comment on attachment 8652652 [details]
MozReview Request: Bug 1196553 - Set secondary action bar colors on phone. r=margaret

https://reviewboard.mozilla.org/r/17241/#review15441

I'm not familiar with this tinting business, so this is a rubber stamp. Please ask Chenxia to review if you think this could benefit from a more knowledgable set of eyes!

Comment 5

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

Comment on attachment 8652652 [details]
MozReview Request: Bug 1196553 - Set secondary action bar colors on phone. r=margaret

(In reply to :Margaret Leibovic (PTO Aug 27 - Sept 14) from comment #4)
> I'm not familiar with this tinting business, so this is a rubber stamp.
> Please ask Chenxia to review if you think this could benefit from a more
> knowledgable set of eyes!

To be on the safe side.

Comment 6

[Chenxia Liu [:liuche]]

Comment on attachment 8652652 [details]
MozReview Request: Bug 1196553 - Set secondary action bar colors on phone. r=margaret

https://reviewboard.mozilla.org/r/17241/#review15453

I wonder if it's time to get rid of our custom attrs and use DrawableCompat? Sonuds like 22.1 support library will work.

http://stackoverflow.com/a/29890717/582004

Comment 7

[Chenxia Liu [:liuche]]

Comment on attachment 8652652 [details]
MozReview Request: Bug 1196553 - Set secondary action bar colors on phone. r=margaret

https://reviewboard.mozilla.org/r/17241/#review15485

Ship It!

Comment 8

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

https://reviewboard.mozilla.org/r/17241/#review15453

> I wonder if it's time to get rid of our custom attrs and use DrawableCompat? Sonuds like 22.1 support library will work.

By our custom attrs, are you referring to `gecko:drawableTintList`? afaik, `DrawableCompat` doesn't add XML attrs for us to use so we'd have to set the tint dynamically. I added `drawableTintList` so that we can define the `ColorStateList`s via XML. Note that `drawableTintList` is actually [backed by DrawableCompat] (and probably why we haven't done something like this sooner).

[backed by DrawableCompat]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/widget/ThemedImageView.java?rev=86782173d9b8#187

Comment 9

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

url:        https://hg.mozilla.org/integration/fx-team/rev/d3d4393b7b7d88d801a342b3a865eb87f52faded
changeset:  d3d4393b7b7d88d801a342b3a865eb87f52faded
user:       Michael Comella <[e-mail]>
date:       Tue Aug 25 17:22:11 2015 -0700
description:
Bug 1196553 - Set secondary action bar colors on phone. r=margaret

I previously set this for tablet but didn't (for an unknown reason) set this
for phones as well.

Comment 10

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

Comment on attachment 8652652 [details]
MozReview Request: Bug 1196553 - Set secondary action bar colors on phone. r=margaret

Approval Request Comment
[Feature/regressing bug #]: bug 1179479
[User impact if declined]:
  Users on phones will see the disabled reading list and bookmark buttons as enabled (though pressing them does not do anything) – i.e. the style is always the enabled style

[Describe test coverage new/current, TreeHerder]: Tested locally
[Risks and why]: 
  Low, we're applying an already-used style from tablet onto all devices (including phones). Worst case, some obscure device configuration doesn't like the new style.

[String/UUID change made/needed]: None

Comment 11

[Ryan VanderMeulen [:RyanVM]]

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

Comment 12

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8652652 [details]
MozReview Request: Bug 1196553 - Set secondary action bar colors on phone. r=margaret

Visual regression, taking it.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

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