Closed Bug 952953 Opened 10 years ago Closed 10 years ago

Use the Fx Australis tabs

Categories

(Thunderbird :: Theme, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 29.0

People

(Reporter: Paenglab, Assigned: Paenglab)

References

Details

(Whiteboard: [ux-feature-wanted-31])

Attachments

(3 files, 5 obsolete files)

This bug is to adapt the latest Firefox Australis tabs. This could also make the UI a little bit faster as the Fx guys put a lot of power to speed up the Fx UI also for the tabs.
Attached patch Patch (obsolete) — Splinter Review
This patch uses a shared file for tabmail.css and also a file for defining the colors (linuxShared.inc, osxShared.inc, windowsShared.inc) I copied from Fx. I also removed a lot of .contentTabToolbox rules which are interfering the other rules, especially with LWThemes.
I also included the Fx LightweightThemeListener to hide the border between selected tabs and toolbar. This is a clone, I only changed the path to the CSS file.

Fx uses a makefile to create selected-start.svg and selected-end.svg. To let this patch easier I included the computed files from Fx. If this is still wanted I can add this in a other bug.

This patch looks huge but the most is exchanging images and SVGs.

Josiah, please check also if all works as expected in Retina mode. I can't test this.
Assignee: nobody → richard.marti
Status: NEW → ASSIGNED
Attachment #8351227 - Flags: ui-review?(josiah)
Attachment #8351227 - Flags: review?(josiah)
I made a try build (https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=af510c0bb239) to check I introduce no new failures. What I can see there are the same as without the patch. Unfortunately Windows fails with and without patch.
If needed, I can provide a Windows zip file for testing.
Whiteboard: [ux-feature-wanted-31]
(In reply to Richard Marti (:Paenglab) from comment #2)
> I made a try build
> (https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=af510c0bb239) to check I
> introduce no new failures. What I can see there are the same as without the
> patch. Unfortunately Windows fails with and without patch.
> If needed, I can provide a Windows zip file for testing.

I would appreciate a Windows zip, yes.
The link for the zip: https://dl.dropboxusercontent.com/u/23792533/thunderbird-29.0a1.en-US.win32.zip

There is also the Prefs in Tab patch included but this isn't affecting this one. You have only a tab more to test. ;)
Attached patch Patch after bug 953011 (obsolete) — Splinter Review
The same patch like for review but applies after landing of bug 953011.
Comment on attachment 8351513 [details] [diff] [review]
Patch after bug 953011

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

Overall it looks very good (Works great on a retina display). I am for the most part quite pleased with the code, though I have some comments. Some are nits, but the major thing I think I want changed is to use CSS variables instead of pre processor defines. I know Firefox doesn't yet do this, but I believe we should.

Also, not mentioned in the below comments, but could you change all the messenger-LWTheme.css names to messengerLWTheme.css. The dash seems kind of inconsistent to me.

r- because of the CSS variables, but I'm sure it will be fine after that.

Also, as far as the UI goes, it works quite well except for the tabbar border on Windows (Sans Aero). The border color is a little too dark. I'll attach a screenshot. ui-r- for now.

::: mail/base/content/msgMail3PaneWindow.js
@@ +132,5 @@
> +  },
> +
> +  /**
> +   * Append the headerImage to the background-image property of all rulesets in
> +   * browser-lightweightTheme.css.

I don't think you want browser here.

::: mail/themes/linux/mail/linuxShared.inc
@@ +1,4 @@
> +%if 0
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

Minor nit. Move the "*/" down to its own line.

@@ +8,5 @@
> +
> +%define toolbarHighlight rgba(255, 255, 255, .3)
> +%define fgTabTexture linear-gradient(transparent 0px, transparent 2px, hsla(0, 0%, 100%, 0.35) 2px, hsla(0, 0%, 100%, 0.35) 3px, hsla(0, 0%, 100%, 0.65) 3px, hsla(0, 0%, 100%, 0.65) 4px, @toolbarHighlight@)
> +%define fgTabTextureLWT @fgTabTexture@
> +%define fgTabBackgroundColor -moz-dialog

Can we use CSS variables for this instead so we don't need to pre-process this? If you set all the variables on #tabs-toolbar you should still be able to reference all of these. E.G.

/* Variable declarations */
#tabs-toolbar {
  var-toolbarHighlight: rgba(255, 255, 255, .3);
  var-fgTabTexture: linear-gradient(transparent 0px, transparent 2px, hsla(0, 0%, 100%, 0.35) 2px, hsla(0, 0%, 100%, 0.35) 3px, hsla(0, 0%, 100%, 0.65) 3px, hsla(0, 0%, 100%, 0.65) 4px, var(toolbarHighlight))
  var-fgTabTextureLWT: var(fgTabTexture);
  var-fgTabBackgroundColor: -moz-dialog;
}

::: mail/themes/linux/mail/messenger-LWTheme.css
@@ +16,5 @@
> +.tabmail-tab > .tab-stack > .tab-background >
> +  .tab-background-end[selected=true]:-moz-lwtheme::before {
> +  background-attachment: scroll, fixed;
> +  background-color: transparent;
> +  background-image: @fgTabTextureLWT@;/*, lwtHeader;*/

Use CSS variable instead. Also, why is |, lwtHeader;| commented out. Is that on purpose?

@@ +25,5 @@
> +  .tab-background-middle[selected=true]:-moz-lwtheme {
> +  background-attachment: scroll, scroll, fixed;
> +  background-color: transparent;
> +  background-image: url(tabs/active-middle.png),
> +                    @fgTabTextureLWT@;/*, lwtHeader;*/

Ditto.

::: mail/themes/osx/mail/messenger-LWTheme.css
@@ +1,3 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

Move "*/" down.

@@ +16,5 @@
> +.tabmail-tab > .tab-stack > .tab-background >
> +  .tab-background-end[selected=true]:-moz-lwtheme::before {
> +  background-attachment: scroll, fixed;
> +  background-color: transparent;
> +  background-image: @fgTabTextureLWT@;/*, lwtHeader;*/

Use variable if possible. Again, curious about the comment.

@@ +25,5 @@
> +  .tab-background-middle[selected=true]:-moz-lwtheme {
> +  background-attachment: scroll, scroll, fixed;
> +  background-color: transparent;
> +  background-image: url(tabs/active-middle.png),
> +                    @fgTabTextureLWT@;/*, lwtHeader;*/

Ditto.

::: mail/themes/osx/mail/osxShared.inc
@@ +1,4 @@
> +%if 0
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

Again, move closing "*/".

@@ +8,5 @@
> +
> +%define fgTabTexture linear-gradient(transparent, transparent 2px, hsla(0, 0%, 100%, 0.6) 2px, hsla(0, 0%, 100%, 0.6) 3px, hsl(0, 0%, 99%) 3px, hsl(0, 0%, 93%))
> +%define toolbarColorLWT rgba(253, 253, 253, 0.45)
> +%define fgTabTextureLWT linear-gradient(transparent, transparent 2px, rgba(254, 254, 254, 0.72) 2px, @toolbarColorLWT@)
> +%define fgTabBackgroundColor transparent

CSS variables if possible.

::: mail/themes/osx/mail/tabmail.css
@@ +1,4 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> + 

Trailing whitespace.

::: mail/themes/shared/mail/tabs.inc.css
@@ +1,4 @@
> +%if 0
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

Move "*/" down.

@@ +4,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +%endif
> +%define tabHeight 31px
> +%define tabCurveWidth 30px
> +%define tabCurveHalfWidth 15px

CSS variables? Also use theses variables in this code, but I won't mention every instance.

@@ +118,5 @@
> +.tab-background-end[selected=true]::after {
> +  /* position ::after on top of its parent */
> +  -moz-margin-start: -@tabCurveWidth@;
> +  background-size: 100% 100%;
> +  content: "";

Is this content assignment necessary? Kind of annoying.

@@ +126,5 @@
> +
> +.tab-background-start[selected=true]::before,
> +.tab-background-end[selected=true]::before {
> +  /* all ::before pseudo elements */
> +  content: "";

Ditto.

::: mail/themes/windows/mail/messenger-LWTheme.css
@@ +1,3 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

*/ down.

@@ +16,5 @@
> +.tabmail-tab > .tab-stack > .tab-background >
> +  .tab-background-end[selected=true]:-moz-lwtheme::before {
> +  background-attachment: scroll, fixed;
> +  background-color: transparent;
> +  background-image: @fgTabTextureLWT@;/*, lwtHeader;*/

Variable instead.

@@ +25,5 @@
> +  .tab-background-middle[selected=true]:-moz-lwtheme {
> +  background-attachment: scroll, scroll, fixed;
> +  background-color: transparent;
> +  background-image: url(tabs/active-middle.png),
> +                    @fgTabTextureLWT@;/*, lwtHeader;*/

Variable instead.

::: mail/themes/windows/mail/messenger.css
@@ +13,5 @@
> +/*
> + * Draw the bottom border for the tabstrip:
> + */
> +#tabmail {
> +  border-top: 1px solid hsla(209, 67%, 12%, 0.35);

Do you happen to know how this value was derived?

::: mail/themes/windows/mail/tabmail-aero.css
@@ +1,4 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> + 

Trailing whitespace.

@@ +35,5 @@
>  }
>  
>  @media (-moz-windows-default-theme) {
> +  .tab-background-middle[selected=true]:not(:-moz-lwtheme) {
> +    background-color: @customToolbarColor@;

Use the variable if possible.

::: mail/themes/windows/mail/tabmail.css
@@ +1,4 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> + 

Trailing whitespace.

::: mail/themes/windows/mail/windowsShared.inc
@@ +1,4 @@
> +%if 0
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

Move closing "*/".

@@ +11,5 @@
> +%define fgTabBackgroundColor -moz-dialog
> +%define fgTabTextureLWT @fgTabTexture@
> +
> +% Aero-only defines
> +%define customToolbarColor hsl(210, 75%, 92%)

CSS variables if possible.
Attachment #8351513 - Flags: ui-review-
Attachment #8351513 - Flags: review-
Comment on attachment 8351227 [details] [diff] [review]
Patch

I did my review on the other patch.
Attachment #8351227 - Flags: ui-review?(josiah)
Attachment #8351227 - Flags: review?(josiah)
Attached patch Patch with CSS-variables (obsolete) — Splinter Review
This is now the patch with the variables. But I'm not so convinced. I think we are not gaining anything with it. It would be better to fill the variables parts manually. The advantage of the preprocessor includes is, we have a central file with all the defines and can use it in every CSS-file. With only one variables file we would have a to big overhead. And multiple variable files would make it complicated to synchronize them.

And the most important now ;), for .tab-background-middle it doesn't work well. The background-image:  url(tabs/active-middle.png), var(fgTabTexture), none; is correctly shown with the variable expanded in computed style. But the active-middle.png isn't shown. When I remove in DOMi the , var(fgTabTexture), none; the image is shown. And after readding the removed part everything is okay.

Josiah, can you look if you find why this happens?

Also with this fixed I think we should use the preprocessor way for this defines we could use in all files.
Attachment #8351733 - Flags: feedback?(josiah)
Attached image Shadow problem
Oh, forgot to upload that screenshot. Here's the border-shadow problem on Windows 7.
Attached patch new patch (obsolete) — Splinter Review
This patch uses pre processor defines and in tabs.inc.css CSS variables for the tab dimensions. I think this is a good compromise. The tab dimensions are only in this file used and here they can be useful.

All in all this is still the same patch. I moved the tab strip border from #tabmail to #tabs-toolbar. I also made messengerLWTheme.css using the shared directory.

(In reply to Josiah Bruner [:JosiahOne] from comment #7)
> Comment on attachment 8351513 [details] [diff] [review]
> Patch after bug 953011
> 
> Review of attachment 8351513 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mail/base/content/msgMail3PaneWindow.js
> @@ +132,5 @@
> > +  },
> > +
> > +  /**
> > +   * Append the headerImage to the background-image property of all rulesets in
> > +   * browser-lightweightTheme.css.
> 
> I don't think you want browser here.

fixed

> ::: mail/themes/linux/mail/linuxShared.inc
> @@ +1,4 @@
> > +%if 0
> > +/* This Source Code Form is subject to the terms of the Mozilla Public
> > + * License, v. 2.0. If a copy of the MPL was not distributed with this
> > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> 
> Minor nit. Move the "*/" down to its own line.

All CSS files have the MPL header like this. let it like this makes it also easier for future scripted MPL header changes.
> ::: mail/themes/linux/mail/messenger-LWTheme.css
> @@ +16,5 @@
> > +.tabmail-tab > .tab-stack > .tab-background >
> > +  .tab-background-end[selected=true]:-moz-lwtheme::before {
> > +  background-attachment: scroll, fixed;
> > +  background-color: transparent;
> > +  background-image: @fgTabTextureLWT@;/*, lwtHeader;*/
> 
> Also, why is |, lwtHeader;| commented out. Is that on purpose?

Copied from Fx. This is to show where the LightweightThemeListener inserts his data. Moved ahead of the semicolon to show it better.

> ::: mail/themes/osx/mail/tabmail.css
> @@ +1,4 @@
> >  /* This Source Code Form is subject to the terms of the Mozilla Public
> >   * License, v. 2.0. If a copy of the MPL was not distributed with this
> >   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> > + 
> 
> Trailing whitespace.

Fixed.

> @@ +118,5 @@
> > +.tab-background-end[selected=true]::after {
> > +  /* position ::after on top of its parent */
> > +  -moz-margin-start: -@tabCurveWidth@;
> > +  background-size: 100% 100%;
> > +  content: "";
> 
> Is this content assignment necessary? Kind of annoying.

Yes, without it, none is shown on ::after and ::before pseudo elements. I changed to '' to look better.

> ::: mail/themes/windows/mail/messenger.css
> @@ +13,5 @@
> > +/*
> > + * Draw the bottom border for the tabstrip:
> > + */
> > +#tabmail {
> > +  border-top: 1px solid hsla(209, 67%, 12%, 0.35);
> 
> Do you happen to know how this value was derived?

This comes from Fx. Now I'm using it on #tabs-toolbar as background-color like Fx is doing.

> ::: mail/themes/windows/mail/tabmail-aero.css
> @@ +1,4 @@
> >  /* This Source Code Form is subject to the terms of the Mozilla Public
> >   * License, v. 2.0. If a copy of the MPL was not distributed with this
> >   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> > + 
> 
> Trailing whitespace.

Fixed.

> ::: mail/themes/windows/mail/tabmail.css
> @@ +1,4 @@
> >  /* This Source Code Form is subject to the terms of the Mozilla Public
> >   * License, v. 2.0. If a copy of the MPL was not distributed with this
> >   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> > + 
> 
> Trailing whitespace.

Fixed.
Attachment #8351227 - Attachment is obsolete: true
Attachment #8351513 - Attachment is obsolete: true
Attachment #8351733 - Attachment is obsolete: true
Attachment #8351733 - Flags: feedback?(josiah)
Attachment #8355228 - Flags: ui-review?(josiah)
Attachment #8355228 - Flags: review?(josiah)
Forgot to write this patch needs bug 953011 applied first. Newest Windows build with this patch is still under the link in comment 4 available.
Attached patch new patch (obsolete) — Splinter Review
Patch updated for bug 876635.
Attachment #8355228 - Attachment is obsolete: true
Attachment #8355228 - Flags: ui-review?(josiah)
Attachment #8355228 - Flags: review?(josiah)
Attachment #8355282 - Flags: ui-review?(josiah)
Attachment #8355282 - Flags: review?(josiah)
Only unbitrotted.
Attachment #8355282 - Attachment is obsolete: true
Attachment #8355282 - Flags: ui-review?(josiah)
Attachment #8355282 - Flags: review?(josiah)
Attachment #8358989 - Flags: ui-review?(josiah)
Attachment #8358989 - Flags: review?(josiah)
Comment on attachment 8358989 [details] [diff] [review]
patch unbitrotted

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

Code looks fine, however, now OS X has a tabstrip issue. When you use a lightweight theme, the whole top border on the toolbar disappears. Australis does not act this way. I'll upload a screenshot.

Sorry for the slow turnaround.
Attachment #8358989 - Flags: ui-review?(josiah)
Attachment #8358989 - Flags: ui-review-
Attachment #8358989 - Flags: review?(josiah)
Attachment #8358989 - Flags: review+
Comment on attachment 8358989 [details] [diff] [review]
patch unbitrotted

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

So for some weird reason today I re-built TB and am seeing the border now. So in that case, ui-r+. 
You're good to go, thanks Richard!
Attachment #8358989 - Flags: ui-review- → ui-review+
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/c47f6742a8c5
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 29.0
You need to log in before you can comment on or make changes to this bug.