To fit with Firefox on the desktop and give more room to the lightweight theme images, we should display the theme background image on the horizontal tabstrip of the new tablet UI.

Probably not v1 material, but we can triage it later.

If I remember correctly, option C is ideal but may be underperformant, option D is plan B, and option A is *absolutely* THE. WORST.

The graphical glitches I spoke about in the meeting were caused by the API not being clear about the need to specify alpha values in setColor(int) (so I filed bug 1104928 to make it more clear).

Classes like BrowserToolbar get and set their own references to the current theme even though they are a Themed*, which has the theme as a private member. This accessor method removes the need for the excess variables.

To avoid my mistake again (see comment 3).

One remaining question as a TODO. Alpha values still need to be antlam approved.

Anthony, how do you feel about this look? The alpha value we use on the toolbar is 221. I filed bug 1105541 to clean up an color clashing issues. Note that unlike desktop, we don't draw a border between the toolbar/selected tab, and the tab strip.

Note that I did not notice any performance regressions when playing around on my N7.

Comment on attachment 8528679 [details] [diff] [review] Part 1: Add getTheme() method to Themed* views Review of attachment 8528679 [details] [diff] [review]: ----------------------------------------------------------------- Ok.

Comment on attachment 8528681 [details] [diff] [review] Part 2: Update LightweightThemeDrawable documentation Review of attachment 8528681 [details] [diff] [review]: ----------------------------------------------------------------- Good.

Comment on attachment 8529337 [details] [diff] [review] Part 3: Update LWT UI for new tablet browser toolbar Review of attachment 8529337 [details] [diff] [review]: ----------------------------------------------------------------- Looking great, just needs some tweaks. ::: mobile/android/base/tabs/TabStrip.java @@ +135,5 @@ > + return; > + } > + > + final StateListDrawable stateList = new StateListDrawable(); > + stateList.addState(PRIVATE_STATE_SET, getColorDrawable(R.color.background_tabs)); I assume this is to avoid setting LWT on strip when showing private tabs? ::: mobile/android/base/tabs/TabStripItemView.java @@ +196,5 @@ > updateTitle(tab); > updateFavicon(tab.getFavicon()); > setPrivateMode(tab.isPrivate()); > + // TODO: This should probably use tab as an arg. > + updateStyleFromLWT(); If the opacity update should only be triggered when either the checked state or the lwt changes, you shouldn't really be calling this here. You should remove this call from updateFromTab() and move it to setChecked() instead. @@ +232,5 @@ > } > > + private void updateStyleFromLWT() { > + if (getTheme().isEnabled() && isChecked()) { > + setAlpha(BrowserToolbar.LIGHTWEIGHT_THEME_ALPHA_FLOAT); This should probably be getBackground().setAlpha(...) instead. You only want to set opacity on the tab shape, not the text, right? ::: mobile/android/base/toolbar/BrowserToolbar.java @@ +156,5 @@ > protected abstract void triggerStartEditingTransition(PropertyAnimator animator); > protected abstract void triggerStopEditingTransition(); > public abstract void triggerTabsPanelTransition(PropertyAnimator animator, boolean areTabsShown); > > + protected abstract Drawable getLWTEmptyStateSetDrawable(); Maybe provide a default implementation for this method that simply returns getTheme().getDrawable(this)? This method name is slightly confusing. Maybe getLWTDefaultStateSetDrawable()? Naming is hard :-) ::: mobile/android/base/toolbar/NavButton.java @@ +72,5 @@ > + mTheme.getColorDrawable(this, toolbarBackgroundColor); > + if (lwtDrawable != null) { > + lwtDrawable.setAlpha(BrowserToolbar.LIGHTWEIGHT_THEME_INVERT_ALPHA, > + BrowserToolbar.LIGHTWEIGHT_THEME_INVERT_ALPHA); > + } Maybe consolidate this duplicate code into a separate utility static method in BrowserToolbar? Something like: static Drawable getLWTDrawable(Resources res, LightweightTheme theme, int colorResId) { final int bgColor = getResources().getColor(colorResId); final LightweightThemeDrawable d = theme.getColorDrawable(this, bgColor); if (d != null) { d.setAlpha(BrowserToolbar.LIGHTWEIGHT_THEME_INVERT_ALPHA, BrowserToolbar.LIGHTWEIGHT_THEME_INVERT_ALPHA); } return d; }

(In reply to Michael Comella (:mcomella) from comment #10) > Note that I did not notice any performance regressions when playing around > on my N7. Good to know. Please enable GPU profiling and overdraw debugging dev tools in your device and confirm that everything looks good.

(In reply to Michael Comella (:mcomella) from comment #8) > Created attachment 8529338 [details] > Light theme applied This is looking really cool btw :-)

Comment on attachment 8529343 [details] Dark theme applied Second what Lucas said! ^ this is looking nice! exactly the idea we talked about in London. :) You mentioned that we "don't draw a border between the toolbar/selected tab, and the tab strip."... I was just going to suggest this! :P but wanted to know the implications with something like that. Particularly if I just wanted it there when there's a theme active. I feel like we could use some separation, especially on lighter themes. Also, what is the alpha value out of if it's 221?

(In reply to Anthony Lam (:antlam) from comment #16) > Also, what is the alpha value out of if it's 221? 255, so 0.86666 (repeating) if out of 1.

(In reply to Anthony Lam (:antlam) from comment #16) > You mentioned that we "don't draw a border between the toolbar/selected tab, > and the tab strip."... > > I was just going to suggest this! :P but wanted to know the implications > with something like that. Particularly if I just wanted it there when > there's a theme active. I feel like we could use some separation, especially > on lighter themes. I don't think it fits into Android design convention to include a border, but since themes probably aren't an Android design convention either, this might be okay. I think the best thing to do is try it out! I'll investigate how difficult this is.

Awesome! thanks Michael. We could do this in a follow up I think.

https://hg.mozilla.org/integration/fx-team/rev/fd1ce8cc5c02 https://hg.mozilla.org/integration/fx-team/rev/e052ef75c224

re comment 20: parts 1 and 2.

https://hg.mozilla.org/mozilla-central/rev/fd1ce8cc5c02 https://hg.mozilla.org/mozilla-central/rev/e052ef75c224

Comment on attachment 8532755 [details] [diff] [review] Part 4: Move color state selector to colors/ Review of attachment 8532755 [details] [diff] [review]: ----------------------------------------------------------------- Ok.

(In reply to Lucas Rocha (:lucasr) from comment #13) > > + stateList.addState(PRIVATE_STATE_SET, getColorDrawable(R.color.background_tabs)); > > I assume this is to avoid setting LWT on strip when showing private tabs? Yep. > ::: mobile/android/base/tabs/TabStripItemView.java > > + updateStyleFromLWT(); > > If the opacity update should only be triggered when either the checked state > or the lwt changes, you shouldn't really be calling this here. You should > remove this call from updateFromTab() and move it to setChecked() instead. Removed this code in favor of the set color in resources including alpha approach. > @@ +232,5 @@ > > + private void updateStyleFromLWT() { > > + if (getTheme().isEnabled() && isChecked()) { > > + setAlpha(BrowserToolbar.LIGHTWEIGHT_THEME_ALPHA_FLOAT); > > This should probably be getBackground().setAlpha(...) instead. You only want > to set opacity on the tab shape, not the text, right? Same. This change only changes the color of the background tab. > > + protected abstract Drawable getLWTEmptyStateSetDrawable(); > > Maybe provide a default implementation for this method that simply returns > getTheme().getDrawable(this)? Done. > This method name is slightly confusing. Maybe > getLWTDefaultStateSetDrawable()? Naming is hard :-) Done. > Maybe consolidate this duplicate code into a separate utility static method > in BrowserToolbar? Something like: Done: BrowserToolbar.getLightweightThemeDrawable.

May not have time to investigate drawing a border around the LWT (comment 16, comment 18) so I filed bug 1109357.

NI to double-check performance implications while animating the alpha - I see no noticeable difference on my N7 when testing without dev tools.

Comment on attachment 8534025 [details] [diff] [review] Part 3: Update LWT UI for new tablet browser toolbar Review of attachment 8534025 [details] [diff] [review]: ----------------------------------------------------------------- Yep.

https://hg.mozilla.org/integration/fx-team/rev/d18d4ecda9c3 https://hg.mozilla.org/integration/fx-team/rev/017d0ce7f782

(In reply to Michael Comella (:mcomella) from comment #27) > NI to double-check performance implications while animating the alpha - I > see no noticeable difference on my N7 when testing without dev tools. Filed bug 1109739.

NI to uplift to 36.

https://hg.mozilla.org/mozilla-central/rev/d18d4ecda9c3 https://hg.mozilla.org/mozilla-central/rev/017d0ce7f782

Uplift?

Comment on attachment 8528679 [details] [diff] [review] Part 1: Add getTheme() method to Themed* views Applies to parts 1-4, uplift dependency on bug 1107698. Approval Request Comment [Feature/regressing bug #]: New tablet release [User impact if declined]: Lightweight themes will be unpolished in the new tablet release [Describe test coverage new/current, TBPL]: None [Risks and why]: We only touch lightweight theme code so somewhere along the line we could damage the way lightweight themes works. [String/UUID change made/needed]: None

Verified as fixed in Firefox for Android 37.0a1 (2014-12-15); Device: Asus Transformer Pad TF300T (Android 4.2.1).

https://hg.mozilla.org/releases/mozilla-aurora/rev/e76c871c8103 https://hg.mozilla.org/releases/mozilla-aurora/rev/6cc2ce65661e https://hg.mozilla.org/releases/mozilla-aurora/rev/464111748279 https://hg.mozilla.org/releases/mozilla-aurora/rev/5a92d53c1f10

Verified as fixed in Firefox for Android 36.0a2 (2014-12-18); Device: Asus Transformer Pad TF300T (Android 4.2.1).