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

RESOLVED FIXED in Firefox 42

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: mcomella, Assigned: mcomella)

Tracking

unspecified
Firefox 43
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 fixed, firefox43 fixed, fennec42+)

Details

Attachments

(2 attachments)

I think I regressed something in bug 1179479.
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...
Summary: Some action bar icons (e.g. reload) wrong shade of grey on tablet → Some action bar icons (e.g. reload) are the wrong shade of grey on tablet
Summary: Some action bar icons (e.g. reload) are the wrong shade of grey on tablet → Some action bar icons (e.g. reload) are the wrong shade of grey on pre-Lollipop tablet
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.
(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.
Curiously, I found the opposite problem (i.e. opposite API levels) on Google code:
 https://code.google.com/p/android/issues/detail?id=172067
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.
Attachment #8647207 - Flags: review?(mhaigh)
Tracking w/ bug 1179479.
tracking-fennec: --- → 42+
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.
Attachment #8647207 - Flags: review?(mhaigh) → review+
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 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.
Attachment #8647207 - Flags: approval-mozilla-aurora?
url:        https://hg.mozilla.org/integration/fx-team/rev/87ece6852f50e9671258498a25f003a09434bcb4
changeset:  87ece6852f50e9671258498a25f003a09434bcb4
user:       Michael Comella <michael.l.comella@gmail.com>
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.
https://hg.mozilla.org/mozilla-central/rev/87ece6852f50
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
Attachment #8647207 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Needs rebasing for Aurora uplift.
Flags: needinfo?(michael.l.comella)
Nevermind, this is just waiting on bug 1179479 (which is in turn waiting on bug 1194659).
Flags: needinfo?(michael.l.comella)
You need to log in before you can comment on or make changes to this bug.