1020534 - Old tablet toolbar icons hard to read on dark lightweight themes

Status: RESOLVED WONTFIX

(Firefox for Android Graveyard :: General, defect)

Description

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

Attachment: Hard to read icons

Perhaps we can implement a similar fix to bug 1019595.

Comment 2

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

Attachment: Part 0: Show light edit_cancel button using color filter

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.

Comment 3

[Lucas Rocha (:lucasr)]

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.

Comment 4

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

Attachment: Part 0: Show light edit_cancel button using color filter

Updated for nits.

Comment 5

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

Attachment: Part 1: (WIP) Show light tablet icons with dark theme

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.

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

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

[Lucas Rocha (:lucasr)]

(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

[Lucas Rocha (:lucasr)]

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.

Comment 9

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

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

[Lucas Rocha (:lucasr)]

(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

[Lucas Rocha (:lucasr)]

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.

Comment 12

[Michael Comella (:mcomella) [NI reported issues only: ex-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.

Comment 13

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

Attachment: Part 1: Add FilteredThemedImage*

New templates to use color filters and not break encapsulation, to please Director Finkle.

Comment 14

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

Attachment: Part 2: Use FilteredThemedImageView on edit_cancel button

Here's to APK size reduction.

Comment 15

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

Attachment: Part 3: Rename Themed* to MultiDrawableThemed*

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).

Comment 16

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

Attachment: Part 4: Add ThemedView interface

Comment 17

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

On second thought, maybe DrawableThemed* is an acceptable name too because MultiDrawableThemed* is pretty long.

Comment 18

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

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.

Comment 19

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

Attachment: Part 5: (WIP) Use color filters for the menu button on new tablet on dark themes

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.

Comment 20

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

Assuming bug 1085771 lands as expected, this is no longer relevant (but bug 1105541 will take its spiritual place).