Perhaps we can implement a similar fix to bug 1019595.

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

Updated for nits.

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.

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.

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

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

Here's to APK size reduction.

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

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.

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