1665336 - With OS in high contrast black mode and TB default theme, TB toolbars and unread messages in thread pane don't use OS high-contrast colors

Status: VERIFIED FIXED

(Thunderbird :: Disability Access, defect)

Description

[Thomas D. (:thomas8)]

+++ This bug was initially created as a clone of Bug #1661229 +++

Another minor nit. For TB default theme (read: use OS colors), if OS has high contrast, we should use that everywhere consistently without exceptions. Our dark theme toolbar theming isn't high contrast, especially for hover/focus/pressed states.

Steps to reproduce:

1. Enable high-contrast mode in Windows 10, by doing the following:
   Press Alt+Shift+Print, or:
   1.1. Open Windows Settings (Windows+I).
   1.2. Go to Ease of access -> High-contrast.
   1.3. Turn on the high-contrast mode, using the respective toggle.
   1.4. From the "High-contrast theme" combo-box, select "High-Contrast Black".
2. Enable default theme in Thunderbird to pick up OS high contrast colors
3. Observe colors (expecting high-contrast OS colors):

• TB main toolbar, including hover and pressed effects of buttons (compare with Windows Explorer button hover)
• Hover/select read vs. unread message in thread pane
• Focus global search input

Actual results:

• Toolbar does not pick OS high contrast colors. That's inconsistent and defeats the purpose, especially for hover/pressed effects etc. Hovered buttons are supposed to stand out in screaming greenish (as seen in Explorer), in TB it's just faint grey.
• whereas we get high and contrast right for read messages, it fails for unread messages (white text on light screamy selection green bg --> very little contrast)
• focusing global search box does not show any focus ring (should have high contrast green focus ring like quick filter search input)

Expected results:

• Apply OS high-contrast colors consistently everywhere in TB UI without random exceptions

Comment 1

[Thomas D. (:thomas8)]

Attachment: Screenshot 2: selected/hovered unread messages in thread pane don't have high contrast

unread vs. read messages in thread pane on hover/select: unread w/o high contrast

Comment 2

[Thomas D. (:thomas8)]

Attachment: Screenshot 1: Toolbars and their buttons (hover/pressed) don't use OS high contrast colors (compare: Explorer)

Forgot to attach screenshot in comment 0, here it is.

Comment 3

[Richard Marti (:Paenglab)]

Attachment: 1665336-high-contrast-button.patch

This fixes the issues mostly only on Windows 10 and only partially on Windows 7 because there the Classic theme would also be affected which makes it looking weird.

For easier testing I started a try: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=d0ba8a63dc6fa42d18d437ce75fe1519eb511685

Comment 4

[Alessandro Castellani [:aleca]]

Comment on attachment 9177107 [details] [diff] [review]
1665336-high-contrast-button.patch

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

My VM is throwing a fit, sorry.
Better ask Thomas to review this since he's on Windows.

Comment 5

[Thomas D. (:thomas8)]

Comment on attachment 9177107 [details] [diff] [review]
1665336-high-contrast-button.patch

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

LGTM - I have tested the try and this seems to do what it should, so I guess the CSS is right (too deep). 1½ nits:
1: Please add high contrast text color for hovering unread messages.
½: Pls add me as reviewer in the commit message.

::: mail/themes/windows/mail/messenger.css
@@ +165,5 @@
> +  :root:not(:-moz-lwtheme) #ab-toolbox > toolbar,
> +  :root:not(:-moz-lwtheme) .mail-toolbox > toolbar,
> +  :root:not(:-moz-lwtheme) #event-toolbox > toolbar,
> +  :root:not(:-moz-lwtheme) #compose-toolbox > toolbar,
> +  :root:not(:-moz-lwtheme) #navigation-toolbox > toolbar,

I'm surprised that we're not just applying this to all toolbars, but Richard says it's on purpose...

@@ +366,5 @@
>    fill: currentColor;
>  }
>  
>  treechildren::-moz-tree-row {
> +  min-height: 1.8em;

Good catch!

Comment 6

[Richard Marti (:Paenglab)]

Attachment: 1665336-high-contrast-button.patch

Fixed the hover with adding
:root:not(:-moz-lwtheme) treechildren::-moz-tree-cell-text(untagged, unread, hover) to color: HighlightText !important;

Comment 7

[Thomas D. (:thomas8)]

Comment on attachment 9178031 [details] [diff] [review]
1665336-high-contrast-button.patch

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

Thank you, this looks good. I have some cosmetic proposals to reorder CSS function arguments and some selectors for improved readability, so that their similarity against tiny differences comes out clearer.

::: mail/themes/windows/mail/mailWindow1.css
@@ +521,5 @@
> +
> +  :root:not(:-moz-lwtheme)
> +    treechildren::-moz-tree-cell-text(untagged, unread, hover),
> +  :root:not(:-moz-lwtheme)
> +    treechildren::-moz-tree-cell-text(selected, untagged, unread) {

If possible, can we reorder the attributes to have the same order in each selector so that the only difference (selected vs. hover) becomes clearer? Common attributes first, different attribute last.
```	
    treechildren::-moz-tree-cell-text(untagged, unread, selected) {
```

::: mail/themes/windows/mail/messenger.css
@@ +190,5 @@
> +
> +  :root:not(:-moz-lwtheme)
> +  treechildren::-moz-tree-image(folderNameCol, newMessages-true),
> +  :root:not(:-moz-lwtheme)
> +  treechildren::-moz-tree-cell-text(folderNameCol, isServer-true, biffState-NewMail),

please move this selector to come last, so that surrounding similar selectors are together for ease of reading and telling apart

@@ +194,5 @@
> +  treechildren::-moz-tree-cell-text(folderNameCol, isServer-true, biffState-NewMail),
> +  :root:not(:-moz-lwtheme)
> +  treechildren::-moz-tree-cell-text(folderNameCol, newMessages-true),
> +  :root:not(:-moz-lwtheme)
> +  treechildren::-moz-tree-cell-text(folderNameCol, specialFolder-Inbox, newMessages-true) {

Please reorder attributes, newMessages-true before specialFolder-Inbox

::: mail/themes/windows/mail/searchBox.css
@@ +52,5 @@
>  
>  @media (-moz-windows-default-theme: 0) {
> +  .gloda-search:not(:-moz-lwtheme),
> +  .searchBox:not(:-moz-lwtheme),
> +  .themeableSearchBox:not(:-moz-lwtheme) {

This screams for a searchbox meta class, but that's for another bug

Comment 8

[Richard Marti (:Paenglab)]

The reordering of the attributes needs to be made everywhere in the tree to be better searchable. Better we do this in a dedicated bug.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/7728f0fae5c3
Add special rules for the Windows 10 high contrast theme. r=thomas8

Comment 10

[Richard Marti (:Paenglab)]

Comment on attachment 9178031 [details] [diff] [review]
1665336-high-contrast-button.patch

[Approval Request Comment]
User impact if declined: contrast not everywhere correct
Testing completed (on c-c, etc.): on c-c
Risk to taking this patch (and alternatives if risky): low, would only affect high contrast users

Comment 11

[Richard Marti (:Paenglab)]

Attachment: 1665336-high-contrast-button-ESR.patch

Bug 1661229 needs to be applied first.

[Approval Request Comment]
User impact if declined: contrast not everywhere correct
Testing completed (on c-c, etc.): on c-c
Risk to taking this patch (and alternatives if risky): low, would only affect high contrast users

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/8659834500ef
Follow-up: change min-height to height on treechildren::-moz-tree-row. r=me DONTBUILD

Comment 13

[Richard Marti (:Paenglab)]

Attachment: 1665336-high-contrast-button-ESR.patch

Bug 1661229 needs to be applied first.

[Approval Request Comment]
User impact if declined: contrast not everywhere correct
Testing completed (on c-c, etc.): on c-c
Risk to taking this patch (and alternatives if risky): low, would only affect high contrast users

Updated the wrong min-height to height (see https://hg.mozilla.org/comm-central/rev/8659834500ef follow-up).

Comment 14

[Wayne Mery (:wsmwk)]

Comment on attachment 9178031 [details] [diff] [review]
1665336-high-contrast-button.patch

[Triage Comment]
Approved for beta

esr78 patch to be taken after 82.0b2

Comment 15

[Wayne Mery (:wsmwk)]

Comment on attachment 9178240 [details] [diff] [review]
1665336-high-contrast-button-ESR.patch

[Triage Comment]
Approved for esr78

Comment 16

[Rob Lemley [:rjl]]

Thunderbird 82.0b2:
https://hg.mozilla.org/releases/comm-beta/rev/25df91834ad2

Comment 17

[Rob Lemley [:rjl]]

Richard, some things have changed on 78 in messenger.css since you originally made the patch for it, could you rebase it?

Comment 18

[Richard Marti (:Paenglab)]

Rob, have you first applied bug 1661229 like in comment 13 written?

Comment 19

[Rob Lemley [:rjl]]

(In reply to Richard Marti (:Paenglab) from comment #18)

Rob, have you first applied bug 1661229 like in comment 13 written?

Oh wow... I had looked, noted that bug 1661229 was marked fixed for 78 and that there were changes from it present in the Mercurial log... but did not see that another patch had appeared needed to be uplifted. Thanks, I got it now.

Comment 20

[Rob Lemley [:rjl]]

Thunderbird 78.3.2:
https://hg.mozilla.org/releases/comm-esr78/rev/541af566d30a

Comment 21

[Anje]

Query: comment 5
treechildren::-moz-tree-row {

• min-height: 1.8em;

Version 78.3.2
Suddenly there is an increased amount of space between the list of messages in the Thread Pane. That reduces the number of items that can be displayed in a given space. The line/row height has been significantly increased.

I am currently being asked in Support Forum on how to remove the unwanted extra spacing, so wondered if it had occurred due to changes in this recent update to do with High Contrast ?

Comment 22

[Richard Marti (:Paenglab)]

This won't be changed as it is now consistent on all Windows themes.

Comment 23

[Anje]

Thanks you for confirming; at least I know what has caused the change and can fix using userChrome.css.

Comment 24

[Ramona Gavrilescu]

Confirming this issue as verified fixed on Win 11 using ESR 128.0(220240703144221).