984978 - Adjust TB's theme for Windows 8 and up

Status: RESOLVED FIXED

(Thunderbird :: Theme, defect)

Description

[Richard Marti (:Paenglab)]

This bug is to implement bug 960517 for TB

Comment 1

[Richard Marti (:Paenglab)]

Attachment: Part1: Win8-mainWindow.patch

This is the biggest patch for the main window. It styles the tabs, the toolbar with the buttons and the trees.

Comment 2

[Richard Marti (:Paenglab)]

Attachment: Part2: Win8-messageHeaders.patch

This patch styles the messageheader buttons and the attachment list.

Comment 3

[Richard Marti (:Paenglab)]

Attachment: Part3: Win8-AB.patch

This is more or less a copy of the main patch adapted to the Addressbook.

Comment 4

[Richard Marti (:Paenglab)]

Attachment: Part4: Win8-composer.patch

This is more or less a copy of the main patch adapted to the Compozer. I included the addressbook sidebar part in this patch because it is only visible in compozer.

Comment 5

[Richard Marti (:Paenglab)]

The main difference to bug 960517 is, I used @media (-moz-windows-default-theme) and (-moz-os-version: windows-win8) to style the Win8 part because FX uses the same styling for Win7 Basic and Win8 themes. TB uses for Basic themes the native button/menulist appearance for better readability on High Contrast themes.

Part5 with the Chat adjustments follows a bit later.

Comment 6

[Richard Marti (:Paenglab)]

Attachment: Part5: Win8-chat.patch

This patch styles the chat tab content. I've added also some preprocessor code to include the MPL header only one time.

Florian, I added you only with f? because I don't know if you have a Win8 box.

Comment 7

[Florian Quèze [:florian]]

Comment on attachment 8393055 [details] [diff] [review]
Part5: Win8-chat.patch

I don't, but clokep does.

Comment 8

[Richard Marti (:Paenglab)]

Attachment: Part1: Win8-mainWindow.patch

Oops, found a trailing whitespace.

Comment 9

[Richard Marti (:Paenglab)]

Attachment: Part3: Win8-AB.patch

The same whitespace. This comes from copy-n-paste.

Comment 10

[Richard Marti (:Paenglab)]

Attachment: Part4: Win8-composer.patch

Again whitespace. Sorry for the bug spam.

Comment 11

[Patrick Cloke [:clokep]]

Comment on attachment 8393055 [details] [diff] [review]
Part5: Win8-chat.patch

I tested (the chat portion of) this on Windows 8.1 and it seems fine. Very blue-ish.

Comment 12

[Patrick Cloke [:clokep]]

Attachment: Chat screenshot

Comment 13

[Stanzilla]

looks good!

Comment 14

[Richard Marti (:Paenglab)]

Attachment: Part1: Win8-mainWindow.patch

Fixed some Win8 High Contrast issues:
- Search boxes where not readable (see Fx bug 986324)
- Menu background in titlebar was black.
- I needed to use the Australis toolbarbutton styling also in HC because the buttons where black holes in toolbars gradient (on dark HC themes). I can't remove the gradient because it's needed for the tabs.

Comment 15

[Richard Marti (:Paenglab)]

Attachment: Part2: Win8-messageHeaders.patch

Fixed some Win8 High Contrast issues:
- #msgHeaderView had background-color: #f8f8f8 also on dark HC themes.

Comment 16

[Richard Marti (:Paenglab)]

Attachment: Part3: Win8-AB.patch

Fixed some Win8 High Contrast issues:
- Search boxes where not readable (see Fx bug 986324)
- Removed the toolbar gradient on HC themes.

Comment 17

[Richard Marti (:Paenglab)]

Attachment: Part4: Win8-composer.patch

Fixed some Win8 High Contrast issues:
- Search boxes where not readable (see Fx bug 986324)
- Removed the toolbar gradient on HC themes.

Comment 18

[Richard Marti (:Paenglab)]

Attachment: Part1: Win8-mainWindow.patch

Sorry to spam this bug but this patch needed a update to tip.

Comment 19

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8402276 [details] [diff] [review]
Part1: Win8-mainWindow.patch

Redirecting to JosiahOne, because I've got zero time right now. :/

Comment 20

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8402276 [details] [diff] [review]
Part1: Win8-mainWindow.patch

Actually, I might be able to take some of the UI reviews, once my build is done.

Comment 21

[Josiah Bruner [:jsbruner]]

Just noting that it might take a while before I get to this. I have a Win 8 VM setup, but have been unable to get any builds to complete successfully.

Comment 22

[Mike Conley (:mconley) (:⚙️)]

(In reply to Josiah Bruner [:JosiahOne] from comment #21)
> Just noting that it might take a while before I get to this. I have a Win 8
> VM setup, but have been unable to get any builds to complete successfully.

A try push might be your best bet.

Comment 23

[Richard Marti (:Paenglab)]

I started a try build: https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=1c8ae0fe2456

Comment 24

[Richard Marti (:Paenglab)]

Attachment: Part1: Win8-mainWindow.patch

Part1 was bitrotted by bug 995747.

Comment 25

[Josiah Bruner [:jsbruner]]

Comment on attachment 8411135 [details] [diff] [review]
Part1: Win8-mainWindow.patch

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

In general I like this a lot, but here are some of the major issues:

- Main Toolbar button's active state cuts of borders, we should keep the borders.
- White lines under items in the folderpane. There seems to be some white lines that appear under some entries in the folder pane that aren't selected/hovered. It may be a drawing issue, but we could probably work around that by changing one of the parent's background color to match the entries.
- Message header buttons don’t have enough padding-right.
- Message header is blue when opened in a tab (intentional?)

I'll attach screenshots of all these problems.

Those are just general problems and not directed specifically at this patch. Review of the actual patch is below, just minor nits, so I'd give a r+ except for the major problems. So r-.

(I'm r- only to remind me that things to need be addressed, even if most of the issues don't revolve around this patch)

::: mail/themes/windows/mail/mailWindow1-aero.css
@@ +244,5 @@
> +}
> +
> +@media (-moz-windows-default-theme) and (-moz-os-version: windows-vista),
> +       (-moz-windows-default-theme) and (-moz-os-version: windows-win7) {
> +  /* < Win8 */

I don't think this comment is necessary. The @media query explains it well enough.

@@ +287,3 @@
>  
> +@media (-moz-windows-default-theme) and (-moz-os-version: windows-win8) {
> +  /* >= Win8 */

Ditto.

@@ +345,4 @@
>  
> +@media (-moz-windows-default-theme) and (-moz-os-version: windows-vista),
> +       (-moz-windows-default-theme) and (-moz-os-version: windows-win7) {
> +  /* < Win8 */

Ditto.

::: mail/themes/windows/mail/primaryToolbar-aero.css
@@ +67,5 @@
>  }
>  
> +@media (-moz-windows-default-theme) and (-moz-os-version: windows-vista),
> +       (-moz-windows-default-theme) and (-moz-os-version: windows-win7) {
> +  /* < Win8 */

Remove the comment.

@@ +106,5 @@
> +}
> +
> +@media (-moz-windows-default-theme) and (-moz-os-version: windows-vista),
> +       (-moz-windows-default-theme) and (-moz-os-version: windows-win7) {
> +  /* < Win8 */

Ditto.

@@ +190,5 @@
>    }
> +}
> +
> +@media (-moz-os-version: windows-win8) {
> +  /* >= Win8 */

Ditto.

@@ +248,5 @@
> +
> +@media (-moz-windows-default-theme) {
> +  /* Separator between menu and split type buttons */
> +  .toolbarbutton-1:not(:hover):not(:active):not([open]):not([checked])
> +   > .toolbarbutton-menubutton-dropmarker::before {

This makes the separator hide when hovering over it when disabled. You need to do what I did here: https://bug995681.bugzilla.mozilla.org/attachment.cgi?id=8405824

::: mail/themes/windows/mail/quickFilterBar-aero.css
@@ +2,3 @@
>   * 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. Could you remove this?

@@ +38,5 @@
>  }
>  
> +@media (-moz-windows-default-theme) and (-moz-os-version: windows-vista),
> +       (-moz-windows-default-theme) and (-moz-os-version: windows-win7) {
> +  /* < Win8 */

Unneeded comment.

@@ +71,5 @@
>  }
>  
> +@media (-moz-os-version: windows-vista),
> +       (-moz-os-version: windows-win7) {
> +  /* < Win8 */

Ditto.

@@ +99,5 @@
>  }
>  
> +@media (-moz-windows-default-theme) and (-moz-os-version: windows-vista),
> +       (-moz-windows-default-theme) and (-moz-os-version: windows-win7) {
> +  /* < Win8 */

Ditto.

::: mail/themes/windows/mail/tabmail-aero.css
@@ +53,5 @@
>  }
>  
> +@media (-moz-windows-default-theme) and (-moz-os-version: windows-vista),
> +       (-moz-windows-default-theme) and (-moz-os-version: windows-win7) {
> +  /* < Win8 */

Remove comment.

Comment 26

[Josiah Bruner [:jsbruner]]

Comment on attachment 8402211 [details] [diff] [review]
Part4: Win8-composer.patch

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

I really like this, just one UI-related thing, when you press down on the format toolbar buttons, the icon shifts over down and right quite a bit. It shouldn't. r+ with that fixed.

::: mail/themes/windows/mail/addrbook/abContactsPanel-aero.css
@@ +5,5 @@
>  %include abContactsPanel.css
>  
> +@media (-moz-windows-default-theme) and (-moz-os-version: windows-vista),
> +       (-moz-windows-default-theme) and (-moz-os-version: windows-win7) {
> +  /* < Win8 */

Comment again.

@@ +36,5 @@
> +}
> +
> +@media (-moz-windows-default-theme) and (-moz-os-version: windows-vista),
> +       (-moz-windows-default-theme) and (-moz-os-version: windows-win7) {
> +  /* < Win8 */

Comment.

@@ +79,3 @@
>  
> +@media (-moz-windows-default-theme) and (-moz-os-version: windows-win8) {
> +  /* >= Win8 */

Comment.

::: mail/themes/windows/mail/compose/messengercompose-aero.css
@@ +45,5 @@
>  }
>  
> +@media (-moz-os-version: windows-vista),
> +        (-moz-os-version: windows-win7) {
> +  /* < Win8 */

Comment again.

@@ +54,5 @@
> +}
> +
> +@media (-moz-windows-default-theme) and (-moz-os-version: windows-vista),
> +       (-moz-windows-default-theme) and (-moz-os-version: windows-win7) {
> +  /* < Win8 with default theme */

Ditto.

@@ +138,5 @@
>  }
>  
> +@media (-moz-windows-default-theme) and (-moz-os-version: windows-vista),
> +       (-moz-windows-default-theme) and (-moz-os-version: windows-win7) {
> +  /* < Win8 */

Ditto.

@@ +239,5 @@
>    }
>  }
>  
> +@media (-moz-windows-default-theme) and (-moz-os-version: windows-win8) {
> +  /* >= Win8 */

Ditto.

@@ +256,5 @@
> +  .toolbarbutton-1:not([disabled=true]):-moz-any(:hover,[open]) >
> +  .toolbarbutton-menubutton-dropmarker,
> +  .toolbarbutton-1:not([disabled=true]):not([checked=true]):not([open]):not(:active):hover,
> +  .toolbarbutton-1:not([buttonover]):not([open]):not(:active):hover >
> +  .toolbarbutton-menubutton-dropmarker:not([disabled]) {

Note to self: This is super ugly, fix it with JS attributes.

@@ +307,5 @@
> +
> +@media (-moz-windows-default-theme) {
> +  /* Separator between menu and split type buttons */
> +  .toolbarbutton-1:not(:hover):not(:active):not([open]):not([checked])
> +   > .toolbarbutton-menubutton-dropmarker::before {

Same thing as the other patch. separator disappears on hover when disabled.

@@ +429,5 @@
>  }
>  
> +@media (-moz-windows-default-theme) and (-moz-os-version: windows-vista),
> +       (-moz-windows-default-theme) and (-moz-os-version: windows-win7) {
> +  /* < Win8 */

Comment.

@@ +741,5 @@
>  }
>  
> +@media (-moz-windows-default-theme) and (-moz-os-version: windows-vista),
> +       (-moz-windows-default-theme) and (-moz-os-version: windows-win7) {
> +  /* < Win8 */

Comment.

@@ +755,5 @@
> +  }
> +}
> +
> +@media (-moz-windows-default-theme) and (-moz-os-version: windows-win8) {
> +  /* >= Win8 */

Ditto.

@@ +1121,5 @@
>  }
>  
> +@media (-moz-windows-default-theme) and (-moz-os-version: windows-vista),
> +       (-moz-windows-default-theme) and (-moz-os-version: windows-win7) {
> +  /* < Win8 */

Ditto.

Comment 27

[Josiah Bruner [:jsbruner]]

Comment on attachment 8402210 [details] [diff] [review]
Part3: Win8-AB.patch

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

Looks. r+ with the nits addressed.

::: mail/themes/windows/mail/addrbook/addressbook-aero.css
@@ +36,5 @@
>  }
>  
> +@media (-moz-os-version: windows-vista),
> +        (-moz-os-version: windows-win7) {
> +  /* < Win8 */

I'm not going to mention all of them this time, but please kill all these comment lines.

@@ +215,5 @@
> +
> +@media (-moz-windows-default-theme) {
> +  /* Separator between menu and split type buttons */
> +  .toolbarbutton-1:not(:hover):not(:active):not([open]):not([checked])
> +   > .toolbarbutton-menubutton-dropmarker::before {

Same thing as before with the hovered+disabled.

Comment 28

[Josiah Bruner [:jsbruner]]

Comment on attachment 8402209 [details] [diff] [review]
Part2: Win8-messageHeaders.patch

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

Also looks good for the most part. Except the blue background when in a tab probably needs to be addressed in this patch.

::: mail/themes/windows/mail/attachmentList-aero.css
@@ +91,5 @@
> +}
> +
> +@media (-moz-windows-default-theme) and (-moz-os-version: windows-vista),
> +       (-moz-windows-default-theme) and (-moz-os-version: windows-win7) {
> +  /* < Win8 */

Comment again.

@@ +128,5 @@
>    }
>  }
> +
> +@media (-moz-windows-default-theme) and (-moz-os-version: windows-win8) {
> +  /* >= Win8 */

Ditto.

::: mail/themes/windows/mail/messageHeader-aero.css
@@ +142,5 @@
>  }
>  
> +@media (-moz-windows-default-theme) and (-moz-os-version: windows-vista),
> +       (-moz-windows-default-theme) and (-moz-os-version: windows-win7) {
> +  /* < Win8 */

Ditto.

@@ +217,5 @@
> +}
> +
> +@media (-moz-windows-default-theme) and (-moz-os-version: windows-vista),
> +       (-moz-windows-default-theme) and (-moz-os-version: windows-win7) {
> +  /* < Win8 */

Ditto

@@ +241,5 @@
>    }
>  }
>  
> +@media (-moz-windows-default-theme) and (-moz-os-version: windows-win8) {
> +  /* >= Win8 */

Ditto.

Comment 29

[Josiah Bruner [:jsbruner]]

Comment on attachment 8393055 [details] [diff] [review]
Part5: Win8-chat.patch

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

Looks good. Thanks.

::: mail/themes/windows/mail/chat-aero.css
@@ +6,1 @@
>  %include chat.css

Blank line before the %include please.

@@ +172,5 @@
>  }
>  
> +@media (-moz-windows-default-theme) and (-moz-os-version: windows-vista),
> +       (-moz-windows-default-theme) and (-moz-os-version: windows-win7) {
> +  /* < Win8 */

Comment again. This is the only one I will mention, fix others as well please.

::: mail/themes/windows/mail/chat.css
@@ +6,1 @@
>  %include ../../../components/im/themes/chat.css

Blank line before the include.

Comment 30

[Josiah Bruner [:jsbruner]]

Attachment: Padding issue screenshot

Comment 31

[Richard Marti (:Paenglab)]

Attachment: Part1: Win8-mainWindow.patch

Addressed comments. Now with toolbarbutton styling like http://people.mozilla.org/~shorlander/mockups-interactive/australis-interactive-mockups/windows8.html

Comment 32

[Richard Marti (:Paenglab)]

Attachment: Part2: Win8-messageHeaders.patch

Addressed comments.

Comment 33

[Richard Marti (:Paenglab)]

Attachment: Part3: Win8-AB.patch

Addressed comments.

Comment 34

[Josiah Bruner [:jsbruner]]

If you could throw a new Try Build together that would be great.

Comment 35

[Richard Marti (:Paenglab)]

Attachment: Part4: Win8-composer.patch

Addressed comments.

Comment 36

[Richard Marti (:Paenglab)]

Attachment: Part5: Win8-chat.patch

Addressed comments.

Comment 37

[Richard Marti (:Paenglab)]

Try build: https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=8dae4945a846

Comment 38

[Richard Marti (:Paenglab)]

Attachment: Part2: Win8-messageHeaders.patch

Found a whitespace.

Comment 39

[Josiah Bruner [:jsbruner]]

Comment on attachment 8412934 [details] [diff] [review]
Part1: Win8-mainWindow.patch

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

Fantastic! Only one nit and you're good to land. In the message header the "Other Actions" button looks like it has WAY to much padding-right, though it actually doesn't. Because of the dropdown arrow, the text is not centered properly, which makes it look like we need to move it to the right.

On OS X, we only use the text (no dropdown arrow), I suggest we do the same here.

I'll attach a screenshot.

Comment 40

[Josiah Bruner [:jsbruner]]

Attachment: Margin illusion

Comment 41

[Richard Marti (:Paenglab)]

Attachment: Part2: Win8-messageHeaders.patch

"Other Actions" padding improved.

Comment 42

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/748dcbeb8dbc
https://hg.mozilla.org/comm-central/rev/14948c94ca0d
https://hg.mozilla.org/comm-central/rev/53a9fca6dc70
https://hg.mozilla.org/comm-central/rev/e422ced708f9
https://hg.mozilla.org/comm-central/rev/d806f9b3564f