1193950 - Some action bar icons (e.g. reload) are the wrong shade of grey on pre-Lollipop tablet

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

Attachment: Screenshot of the issue

I think I regressed something in bug 1179479.

Comment 1

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

I don't remember seeing this problem on my Lollipop N9 but I see it on this Galaxy Note (presumably < Android 5) and this N7 (< 5). Maybe it's an Android fragmentation issue...

Comment 2

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

The tinting appears to work correctly when switching to private browsing mode so the DrawableTintList should be working. I changed the colors and noticed the default is state_private=false & state_enabled=false so perhaps the default "enabled" state is getting set to false, rather than true, on pre-Lollipop devices.

Comment 3

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

(In reply to Michael Comella (:mcomella) from comment #2)
> The tinting appears to work correctly when switching to private browsing
> mode so the DrawableTintList should be working.

Turns out the color used in PB mode just happened to be the color it was defaulting to in non-private mode so the DrawableTintList is not working. :( This sounds like a fragmentation issue.

Comment 4

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

Curiously, I found the opposite problem (i.e. opposite API levels) on Google code:
 https://code.google.com/p/android/issues/detail?id=172067

Comment 5

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

Attachment: MozReview Request: Bug 1193950 - Tint menu bar icons on pre-L devices. r=mhaigh

Bug 1193950 - Tint menu bar icons on pre-L devices. r=mhaigh

I wasn't using the wrapped drawable when setting the icon, which
curiously works on L+ devices, but not pre-L.

Comment 6

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

Tracking w/ bug 1179479.

Comment 7

[Martyn Haigh (:mhaigh)]

Comment on attachment 8647207 [details]
MozReview Request: Bug 1193950 - Tint menu bar icons on pre-L devices. r=mhaigh

https://reviewboard.mozilla.org/r/15925/#review14247

Ship It!

::: mobile/android/base/util/DrawableUtil.java:24
(Diff revision 1)
> +    @CheckResult

I don't think we need this annotation here.  I'd argue that it was obvious that passing in a resId won't end up with all future references to that resId being tinted.  On the otherhand the annotation on the tintDrawableWithStateList does make sense.

Comment 8

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

https://reviewboard.mozilla.org/r/15925/#review14247

> I don't think we need this annotation here.  I'd argue that it was obvious that passing in a resId won't end up with all future references to that resId being tinted.  On the otherhand the annotation on the tintDrawableWithStateList does make sense.

Ah, interesting! While it's true that the annotation is not necessary, it can't hurt. If the developer doesn't use the resultant Drawable (which they're unlikely to do), they'll be gently prompted to use the result which is fine because they should use it anyway. I'm going to leave it in.

Comment 9

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

Comment on attachment 8647207 [details]
MozReview Request: Bug 1193950 - Tint menu bar icons on pre-L devices. r=mhaigh

Approval Request Comment - see bug 1179479 comment 57.

Comment 10

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

url:        https://hg.mozilla.org/integration/fx-team/rev/87ece6852f50e9671258498a25f003a09434bcb4
changeset:  87ece6852f50e9671258498a25f003a09434bcb4
user:       Michael Comella <[e-mail]>
date:       Wed Aug 12 15:54:30 2015 -0700
description:
Bug 1193950 - Tint menu bar icons on pre-L devices. r=mhaigh

I wasn't using the wrapped drawable when setting the icon, which
curiously works on L+ devices, but not pre-L.

Comment 11

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

https://hg.mozilla.org/mozilla-central/rev/87ece6852f50

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing for Aurora uplift.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Nevermind, this is just waiting on bug 1179479 (which is in turn waiting on bug 1194659).

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/71d74dcbafd7