Closed
Bug 1020534
Opened 10 years ago
Closed 10 years ago
Old tablet toolbar icons hard to read on dark lightweight themes
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(firefox33 wontfix, firefox34 wontfix, firefox35 wontfix, firefox36 affected)
People
(Reporter: mcomella, Assigned: mcomella)
References
Details
(Whiteboard: [lang=java])
Attachments
(6 files, 3 obsolete files)
(deleted),
image/png
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
lucasr
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review |
Perhaps we can implement a similar fix to bug 1019595.
Assignee | ||
Updated•10 years ago
|
Assignee | ||
Updated•10 years ago
|
Whiteboard: [mentor=mcomella][lang=java]
Updated•10 years ago
|
Mentor: michael.l.comella
Whiteboard: [mentor=mcomella][lang=java] → [lang=java]
Assignee | ||
Updated•10 years ago
|
Mentor: michael.l.comella
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → michael.l.comella
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•10 years ago
|
||
Lucas, what do you think of this approach for the other tablet icons? APK wins,
woo!
Note that doing this using drawables is also difficult, given the drawables are
wrapped in GeckoMenuItems, so this approach makes the code significantly
cleaner.
Attachment #8484679 -
Flags: review?(lucasr.at.mozilla)
Comment 3•10 years ago
|
||
Comment on attachment 8484679 [details] [diff] [review]
Part 0: Show light edit_cancel button using color filter
Review of attachment 8484679 [details] [diff] [review]:
-----------------------------------------------------------------
Looks good with the suggested changes. Please show antlam a screenshot if this patch causes any noticeable color changes when using light themes.
::: mobile/android/base/toolbar/BrowserToolbar.java
@@ +188,5 @@
>
> theme = ((GeckoApplication) context.getApplicationContext()).getLightweightTheme();
>
> + final Resources res = getResources();
> + colorWhite = res.getColor(android.R.color.white);
You can just use Color.WHITE instead. But see comment below.
@@ +1535,5 @@
> stateList.addState(EMPTY_STATE_SET, drawable);
>
> setBackgroundDrawable(stateList);
>
> + final int themeColorFilter = theme.isLightTheme() ? null : colorWhite;
This is a bit confusing. themeColorFilter is an int but you're using null here.
@@ +1536,5 @@
>
> setBackgroundDrawable(stateList);
>
> + final int themeColorFilter = theme.isLightTheme() ? null : colorWhite;
> + editCancel.setColorFilter(themeColorFilter);
This will cause a new instance of PorterDuffColorFilter to be created on each setColorFilter() call. You better just create it yourself and store it in a class member like:
editCancelColorFilter =
new PorterDuffColorFilter(Color.WHITE, PorterDuff.Mode.SRC_ATOP);
...then use it here:
editCancel.setColorFilter(theme.isLightTheme() ? null : editCancelColorFilter);
@@ +1542,5 @@
>
> @Override
> public void onLightweightThemeReset() {
> setBackgroundResource(R.drawable.url_bar_bg);
> + editCancel.setColorFilter(null);
Use clearColorFilter() intead.
Attachment #8484679 -
Flags: review?(lucasr.at.mozilla) → review+
Assignee | ||
Comment 4•10 years ago
|
||
Updated for nits.
Assignee | ||
Updated•10 years ago
|
Attachment #8484679 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8485157 -
Flags: review+
Assignee | ||
Comment 5•10 years ago
|
||
Lucas, the bookmark button on large tablets does not consistently use the color
filter (i.e. the icon appears white and gray, with only a few correlations) and
I can't figure out why. I think it's because the color filter only applies to
the current drawable, however, when I update the color filter when
the drawable is updated, the issue still occurs (though the symptoms are less
prominent). Do you have any idea what's going on here?
Note: this patch fixes a bug in the previous part where the filter should
not be used in private mode.
Attachment #8485279 -
Flags: feedback?(lucasr.at.mozilla)
Comment 6•10 years ago
|
||
I'm a little sad that we seem to be leaking lightweight theme stuff into the UI code a bit more than we were. We tried to keep the lightweight theme logic in ThemeXxx helpers and LightweightThemeDrawable. Are we breaking encapsulation for a good reason?
Comment 7•10 years ago
|
||
(In reply to Mark Finkle (:mfinkle) from comment #6)
> I'm a little sad that we seem to be leaking lightweight theme stuff into the
> UI code a bit more than we were. We tried to keep the lightweight theme
> logic in ThemeXxx helpers and LightweightThemeDrawable. Are we breaking
> encapsulation for a good reason?
Fair point. I guess it's tradeoff: we're leaking a bit of lightweight theme logic for some savings on the APK size. I don't feel strongly about this. Up to you mcomella.
Comment 8•10 years ago
|
||
Probably worth mentioning that we already leak some Lightweight theme logic in several parts of the toolbar to update drawables that are specific to each UI component. It's not *that* self-contained.
Assignee | ||
Comment 9•10 years ago
|
||
I can encapsulate the color filter code in a new class similar to ThemedImageView, however, we have to work around the refresh and bookmark buttons which are inflated in the GeckoMenu as MenuItemActionBar. wesj recommended adding a tint to the Widget.MenuItemActionBar style and using a color state list for the color. This can work but needs to be worked around (see [1]).
[1]: https://stackoverflow.com/questions/11095222/android-imageview-change-tint-to-simulate-button-click
Comment 10•10 years ago
|
||
(In reply to Michael Comella (:mcomella) from comment #9)
> I can encapsulate the color filter code in a new class similar to
> ThemedImageView, however, we have to work around the refresh and bookmark
> buttons which are inflated in the GeckoMenu as MenuItemActionBar. wesj
> recommended adding a tint to the Widget.MenuItemActionBar style and using a
> color state list for the color. This can work but needs to be worked around
> (see [1]).
>
> [1]:
> https://stackoverflow.com/questions/11095222/android-imageview-change-tint-
> to-simulate-button-click
Overriding drawableStateChanged() sounds like a sane workaround to me.
Comment 11•10 years ago
|
||
Comment on attachment 8485279 [details] [diff] [review]
Part 1: (WIP) Show light tablet icons with dark theme
It seems we're changing direction here. Cancelling feedback request for now.
Attachment #8485279 -
Flags: feedback?(lucasr.at.mozilla)
Updated•10 years ago
|
Blocks: lwt-improvements
Assignee | ||
Comment 12•10 years ago
|
||
(In reply to Michael Comella (:mcomella) from comment #9)
> I can encapsulate the color filter code in a new class similar to
> ThemedImageView, however, we have to work around the refresh and bookmark
> buttons which are inflated in the GeckoMenu as MenuItemActionBar.
As of bug 1077755, MenuItemActionBar extends Themed*, so we don't need to workaround this fact.
Assignee | ||
Updated•10 years ago
|
Attachment #8485157 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8485279 -
Attachment is obsolete: true
Assignee | ||
Comment 13•10 years ago
|
||
New templates to use color filters and not break encapsulation, to please Director Finkle.
Attachment #8511371 -
Flags: review?(lucasr.at.mozilla)
Assignee | ||
Comment 14•10 years ago
|
||
Here's to APK size reduction.
Attachment #8511373 -
Flags: review?(lucasr.at.mozilla)
Updated•10 years ago
|
Attachment #8511373 -
Flags: review?(lucasr.at.mozilla) → review+
Assignee | ||
Comment 15•10 years ago
|
||
To make way for a ThemedView interface which is necessary when a view may be a Filtered view on one device type and MultiDrawable on another (e.g. menu button in Private Browsing mode on old tablet).
Attachment #8512298 -
Flags: review?(lucasr.at.mozilla)
Assignee | ||
Comment 16•10 years ago
|
||
Attachment #8512299 -
Flags: review?(lucasr.at.mozilla)
Assignee | ||
Comment 17•10 years ago
|
||
On second thought, maybe DrawableThemed* is an acceptable name too because MultiDrawableThemed* is pretty long.
Assignee | ||
Comment 18•10 years ago
|
||
An alternate design pattern is to have FilteredThemed* extend MultiDrawableThemed* (or vice versa) and just override the methods that would ordinarily refresh the drawable statee, but this is not an intuitive inheritance pattern.
Assignee | ||
Comment 19•10 years ago
|
||
I need to run so I'm posting this so you can see a probably broken example of how the ThemedView interface can be used.
Attachment #8512305 -
Flags: feedback?(lucasr.at.mozilla)
Assignee | ||
Updated•10 years ago
|
Blocks: new-tablet-v1
Assignee | ||
Comment 20•10 years ago
|
||
Assuming bug 1085771 lands as expected, this is no longer relevant (but bug 1105541 will take its spiritual place).
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
status-firefox33:
--- → wontfix
status-firefox34:
--- → wontfix
status-firefox35:
--- → wontfix
status-firefox36:
--- → affected
Resolution: --- → WONTFIX
Assignee | ||
Updated•10 years ago
|
Summary: Tablet toolbar icons hard to read on dark lightweight themes → Old tablet toolbar icons hard to read on dark lightweight themes
Assignee | ||
Updated•10 years ago
|
Attachment #8511371 -
Attachment is obsolete: true
Attachment #8511371 -
Flags: review?(lucasr.at.mozilla)
Assignee | ||
Updated•10 years ago
|
Attachment #8512298 -
Attachment is obsolete: true
Attachment #8512298 -
Flags: review?(lucasr.at.mozilla)
Assignee | ||
Updated•10 years ago
|
Attachment #8511371 -
Attachment is obsolete: false
Assignee | ||
Updated•10 years ago
|
Attachment #8512298 -
Attachment is obsolete: false
Assignee | ||
Updated•10 years ago
|
Attachment #8512299 -
Flags: review?(lucasr.at.mozilla)
Assignee | ||
Updated•10 years ago
|
Attachment #8512305 -
Flags: feedback?(lucasr.at.mozilla)
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
•