Closed Bug 1020534 Opened 6 years ago Closed 5 years ago

Old tablet toolbar icons hard to read on dark lightweight themes

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED WONTFIX
Tracking Status
firefox33 --- wontfix
firefox34 --- wontfix
firefox35 --- wontfix
firefox36 --- affected

People

(Reporter: mcomella, Assigned: mcomella)

References

(Blocks 1 open bug)

Details

(Whiteboard: [lang=java])

Attachments

(6 files, 3 obsolete files)

348.58 KB, image/png
Details
6.34 KB, patch
Details | Diff | Splinter Review
10.45 KB, patch
lucasr
: review+
Details | Diff | Splinter Review
49.29 KB, patch
Details | Diff | Splinter Review
4.23 KB, patch
Details | Diff | Splinter Review
8.72 KB, patch
Details | Diff | Splinter Review
Attached image Hard to read icons
Perhaps we can implement a similar fix to bug 1019595.
Whiteboard: [mentor=mcomella][lang=java]
Mentor: michael.l.comella
Whiteboard: [mentor=mcomella][lang=java] → [lang=java]
Mentor: michael.l.comella
Duplicate of this bug: 1030748
Assignee: nobody → michael.l.comella
Status: NEW → ASSIGNED
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 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+
Attachment #8484679 - Attachment is obsolete: true
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)
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?
(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.
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.
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
(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 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)
(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.
Attachment #8485157 - Attachment is obsolete: true
Attachment #8485279 - Attachment is obsolete: true
New templates to use color filters and not break encapsulation, to please Director Finkle.
Attachment #8511371 - Flags: review?(lucasr.at.mozilla)
Here's to APK size reduction.
Attachment #8511373 - Flags: review?(lucasr.at.mozilla)
Attachment #8511373 - Flags: review?(lucasr.at.mozilla) → review+
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)
On second thought, maybe DrawableThemed* is an acceptable name too because MultiDrawableThemed* is pretty long.
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.
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)
Assuming bug 1085771 lands as expected, this is no longer relevant (but bug 1105541 will take its spiritual place).
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → WONTFIX
Summary: Tablet toolbar icons hard to read on dark lightweight themes → Old tablet toolbar icons hard to read on dark lightweight themes
Attachment #8511371 - Attachment is obsolete: true
Attachment #8511371 - Flags: review?(lucasr.at.mozilla)
Attachment #8512298 - Attachment is obsolete: true
Attachment #8512298 - Flags: review?(lucasr.at.mozilla)
Attachment #8511371 - Attachment is obsolete: false
Attachment #8512298 - Attachment is obsolete: false
Attachment #8512299 - Flags: review?(lucasr.at.mozilla)
Attachment #8512305 - Flags: feedback?(lucasr.at.mozilla)
You need to log in before you can comment on or make changes to this bug.