Closed Bug 1085771 Opened 10 years ago Closed 9 years ago

New tablet UI: LWTheme background should be displayed on tabstrip

Categories

(Firefox for Android Graveyard :: General, defect, P1)

ARM
Android
defect

Tracking

(firefox36 verified, firefox37 verified, fennec36+)

VERIFIED FIXED
Firefox 37
Tracking Status
firefox36 --- verified
firefox37 --- verified
fennec 36+ ---

People

(Reporter: kairo, Assigned: mcomella)

References

Details

Attachments

(7 files, 2 obsolete files)

3.44 MB, image/jpeg
Details
5.33 KB, patch
lucasr
: review+
Details | Diff | Splinter Review
2.37 KB, patch
lucasr
: review+
Details | Diff | Splinter Review
512.60 KB, image/png
Details
429.34 KB, image/png
antlam
: feedback+
Details
1.68 KB, patch
lucasr
: review+
Details | Diff | Splinter Review
12.89 KB, patch
lucasr
: review+
Details | Diff | Splinter Review
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.
Attached image (whiteboarding) options
If I remember correctly, option C is ideal but may be underperformant, option D is plan B, and option A is *absolutely* THE. WORST.
Assignee: nobody → lucasr.at.mozilla
Assignee: lucasr.at.mozilla → michael.l.comella
Priority: -- → P1
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.
Attachment #8528679 - Flags: review?(lucasr.at.mozilla)
To avoid my mistake again (see comment 3).
Attachment #8528680 - Flags: review?(lucasr.at.mozilla)
Attachment #8528680 - Attachment is obsolete: true
Attachment #8528680 - Flags: review?(lucasr.at.mozilla)
One remaining question as a TODO. Alpha values still need to be antlam approved.
Attachment #8529337 - Flags: review?(lucasr.at.mozilla)
Attached image Dark theme applied
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.
Attachment #8529343 - Flags: feedback?(alam)
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.
Attachment #8528679 - Flags: review?(lucasr.at.mozilla) → review+
Comment on attachment 8528681 [details] [diff] [review]
Part 2: Update LightweightThemeDrawable documentation

Review of attachment 8528681 [details] [diff] [review]:
-----------------------------------------------------------------

Good.
Attachment #8528681 - Flags: review?(lucasr.at.mozilla) → review+
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;
}
Attachment #8529337 - Flags: review?(lucasr.at.mozilla) → feedback+
(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?
Flags: needinfo?(michael.l.comella)
Attachment #8529343 - Flags: feedback?(alam) → feedback+
(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.
Flags: needinfo?(michael.l.comella)
Awesome! thanks Michael. We could do this in a follow up I think.
re comment 20: parts 1 and 2.
Whiteboard: [leave-open]
Comment on attachment 8532755 [details] [diff] [review]
Part 4: Move color state selector to colors/

Review of attachment 8532755 [details] [diff] [review]:
-----------------------------------------------------------------

Ok.
Attachment #8532755 - Flags: review?(lucasr.at.mozilla) → review+
tracking-fennec: --- → ?
(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.
Attachment #8534025 - Flags: review?(lucasr.at.mozilla)
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.
Flags: needinfo?(michael.l.comella)
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.
Attachment #8534025 - Flags: review?(lucasr.at.mozilla) → review+
Status: NEW → ASSIGNED
Flags: needinfo?(michael.l.comella)
Whiteboard: [leave-open]
(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.
Flags: needinfo?(michael.l.comella)
https://hg.mozilla.org/mozilla-central/rev/d18d4ecda9c3
https://hg.mozilla.org/mozilla-central/rev/017d0ce7f782
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37
Blocks: 1110157
Uplift?
tracking-fennec: ? → 36+
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
Flags: needinfo?(michael.l.comella)
Attachment #8528679 - Flags: approval-mozilla-aurora?
Attachment #8528679 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Verified as fixed in Firefox for Android 37.0a1 (2014-12-15);
Device: Asus Transformer Pad TF300T (Android 4.2.1).
Verified as fixed in Firefox for Android 36.0a2 (2014-12-18);
Device: Asus Transformer Pad TF300T (Android 4.2.1).
Status: RESOLVED → VERIFIED
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.