Closed
Bug 1085771
Opened 10 years ago
Closed 10 years ago
New tablet UI: LWTheme background should be displayed on tabstrip
Categories
(Firefox for Android Graveyard :: General, defect, P1)
Tracking
(firefox36 verified, firefox37 verified, fennec36+)
VERIFIED
FIXED
Firefox 37
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+
Sylvestre
:
approval-mozilla-aurora+
|
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.
Updated•10 years ago
|
Blocks: lwt-improvements
Assignee | ||
Comment 1•10 years ago
|
||
Probably not v1 material, but we can triage it later.
Blocks: new-tablet-v1
Assignee | ||
Comment 2•10 years ago
|
||
If I remember correctly, option C is ideal but may be underperformant, option D is plan B, and option A is *absolutely* THE. WORST.
Updated•10 years ago
|
Assignee: nobody → lucasr.at.mozilla
Updated•10 years ago
|
Assignee: lucasr.at.mozilla → michael.l.comella
Updated•10 years ago
|
Priority: -- → P1
Assignee | ||
Comment 3•10 years ago
|
||
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).
Assignee | ||
Comment 4•10 years ago
|
||
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)
Assignee | ||
Comment 5•10 years ago
|
||
To avoid my mistake again (see comment 3).
Attachment #8528680 -
Flags: review?(lucasr.at.mozilla)
Assignee | ||
Comment 6•10 years ago
|
||
Attachment #8528681 -
Flags: review?(lucasr.at.mozilla)
Assignee | ||
Updated•10 years ago
|
Attachment #8528680 -
Attachment is obsolete: true
Attachment #8528680 -
Flags: review?(lucasr.at.mozilla)
Assignee | ||
Comment 7•10 years ago
|
||
One remaining question as a TODO. Alpha values still need to be antlam approved.
Attachment #8529337 -
Flags: review?(lucasr.at.mozilla)
Assignee | ||
Comment 8•10 years ago
|
||
Assignee | ||
Comment 9•10 years ago
|
||
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)
Assignee | ||
Comment 10•10 years ago
|
||
Note that I did not notice any performance regressions when playing around on my N7.
Comment 11•10 years ago
|
||
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 12•10 years ago
|
||
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 13•10 years ago
|
||
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+
Comment 14•10 years ago
|
||
(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.
Comment 15•10 years ago
|
||
(In reply to Michael Comella (:mcomella) from comment #8)
> Created attachment 8529338 [details]
> Light theme applied
This is looking really cool btw :-)
Comment 16•10 years ago
|
||
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+
Assignee | ||
Comment 17•10 years ago
|
||
(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.
Assignee | ||
Comment 18•10 years ago
|
||
(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)
Comment 19•10 years ago
|
||
Awesome! thanks Michael. We could do this in a follow up I think.
Assignee | ||
Comment 20•10 years ago
|
||
Comment 22•10 years ago
|
||
Assignee | ||
Comment 23•10 years ago
|
||
Attachment #8532755 -
Flags: review?(lucasr.at.mozilla)
Comment 24•10 years ago
|
||
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+
Updated•10 years ago
|
tracking-fennec: --- → ?
Assignee | ||
Updated•10 years ago
|
Assignee | ||
Comment 25•10 years ago
|
||
(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)
Assignee | ||
Updated•10 years ago
|
Attachment #8529337 -
Attachment is obsolete: true
Assignee | ||
Comment 26•10 years ago
|
||
May not have time to investigate drawing a border around the LWT (comment 16, comment 18) so I filed bug 1109357.
Assignee | ||
Comment 27•10 years ago
|
||
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 28•10 years ago
|
||
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+
Assignee | ||
Comment 29•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Status: NEW → ASSIGNED
Flags: needinfo?(michael.l.comella)
Whiteboard: [leave-open]
Assignee | ||
Comment 30•10 years ago
|
||
(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.
Comment 32•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/d18d4ecda9c3
https://hg.mozilla.org/mozilla-central/rev/017d0ce7f782
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37
Comment 33•10 years ago
|
||
Uplift?
Assignee | ||
Comment 34•10 years ago
|
||
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?
Updated•10 years ago
|
Attachment #8528679 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 35•10 years ago
|
||
Verified as fixed in Firefox for Android 37.0a1 (2014-12-15);
Device: Asus Transformer Pad TF300T (Android 4.2.1).
Comment 36•10 years ago
|
||
Comment 37•10 years ago
|
||
Verified as fixed in Firefox for Android 36.0a2 (2014-12-18);
Device: Asus Transformer Pad TF300T (Android 4.2.1).
Status: RESOLVED → VERIFIED
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•