Closed
Bug 952953
Opened 10 years ago
Closed 10 years ago
Use the Fx Australis tabs
Categories
(Thunderbird :: Theme, defect)
Thunderbird
Theme
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)
20.85 KB,
image/png
|
Details | |
157.71 KB,
patch
|
jsbruner
:
review+
jsbruner
:
ui-review+
|
Details | Diff | Splinter Review |
124.93 KB,
image/png
|
Details |
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.
Assignee | ||
Comment 1•10 years ago
|
||
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)
Assignee | ||
Comment 2•10 years ago
|
||
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.
Updated•10 years ago
|
Whiteboard: [ux-feature-wanted-31]
Comment 3•10 years ago
|
||
(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.
Assignee | ||
Comment 4•10 years ago
|
||
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. ;)
Assignee | ||
Comment 5•10 years ago
|
||
The same patch like for review but applies after landing of bug 953011.
Assignee | ||
Updated•10 years ago
|
Blocks: AustralisBird
Comment 7•10 years ago
|
||
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 8•10 years ago
|
||
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)
Assignee | ||
Comment 9•10 years ago
|
||
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)
Comment 10•10 years ago
|
||
Oh, forgot to upload that screenshot. Here's the border-shadow problem on Windows 7.
Assignee | ||
Comment 11•10 years ago
|
||
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)
Assignee | ||
Comment 12•10 years ago
|
||
Try-build: https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=f77dfbb27f81
Assignee | ||
Comment 13•10 years ago
|
||
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.
Assignee | ||
Comment 14•10 years ago
|
||
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)
Assignee | ||
Comment 15•10 years ago
|
||
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 16•10 years ago
|
||
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 17•10 years ago
|
||
Comment 18•10 years ago
|
||
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+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 19•10 years ago
|
||
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.
Description
•