Closed Bug 1022568 Opened 6 years ago Closed 4 years ago

Default and hover style of buttons is illegible when using black-on-white or white-on-black High Contrast mode

Categories

(Toolkit :: Themes, defect, P1, major)

x86_64
Windows 8.1
defect
Points:
2

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: Unfocused, Assigned: dao)

References

(Blocks 1 open bug)

Details

(Keywords: access)

Attachments

(4 files, 1 obsolete file)

Attached image Screenshot
The default style of buttons doesn't make the label legible when using black-on-white High Contrast mode. See attached screenshot.
Flags: firefox-backlog?
Blocks: fx-high-contrast
No longer blocks: 1022566
Flags: firefox-backlog? → firefox-backlog+
Points: --- → 2
Keywords: access
Priority: -- → P1
Whiteboard: p=2
Jim, any idea how we should address this? Setting 'color: HighlightText' for button[default="true"] in toolkit/themes/windows/global/button.css would work with high-contrast themes but not with the default theme on Windows 10. Do we need to expose another platform color for this?
Flags: needinfo?(jmathies)
Component: Themes → Widget: Win32
Product: Toolkit → Core
Severity: normal → major
Attached file default colors.html
Flags: needinfo?(jmathies)
(In reply to Dão Gottwald [:dao] from comment #1)
> Jim, any idea how we should address this? Setting 'color: HighlightText' for
> button[default="true"] in toolkit/themes/windows/global/button.css would
> work with high-contrast themes but not with the default theme on Windows 10.
> Do we need to expose another platform color for this?

Looks like we're using COLOR_HIGHLIGHT for the button face and something besides COLOR_HIGHLIGHTTEXT for the button text, which seems wrong. Those two colors are for items in controls like lists. We have  COLOR_BTNFACE and COLOR_BTNTEXT.
Flags: needinfo?(dao+bmo)
(In reply to Jim Mathies [:jimm] from comment #4)
> (In reply to Dão Gottwald [:dao] from comment #1)
> > Jim, any idea how we should address this? Setting 'color: HighlightText' for
> > button[default="true"] in toolkit/themes/windows/global/button.css would
> > work with high-contrast themes but not with the default theme on Windows 10.
> > Do we need to expose another platform color for this?
> 
> Looks like we're using COLOR_HIGHLIGHT for the button face and something
> besides COLOR_HIGHLIGHTTEXT for the button text, which seems wrong. Those
> two colors are for items in controls like lists. We have  COLOR_BTNFACE and
> COLOR_BTNTEXT.

We use COLOR_BTNTEXT for the label:

https://dxr.mozilla.org/mozilla-central/rev/8c9c4e816e86f903c1d820f3f29715dc070a5a4a/toolkit/themes/windows/global/button.css#23

The background for button[default="true"] comes from -moz-appearance: button;. Does widget code use COLOR_HIGHLIGHT for that? If so, under which conditions? Because it doesn't seem to do that with the default theme, so making the label use COLOR_HIGHLIGHTTEXT would be wrong there.
Flags: needinfo?(dao+bmo) → needinfo?(jmathies)
(In reply to Dão Gottwald [:dao] from comment #5)
> (In reply to Jim Mathies [:jimm] from comment #4)
> > (In reply to Dão Gottwald [:dao] from comment #1)
> > > Jim, any idea how we should address this? Setting 'color: HighlightText' for
> > > button[default="true"] in toolkit/themes/windows/global/button.css would
> > > work with high-contrast themes but not with the default theme on Windows 10.
> > > Do we need to expose another platform color for this?
> > 
> > Looks like we're using COLOR_HIGHLIGHT for the button face and something
> > besides COLOR_HIGHLIGHTTEXT for the button text, which seems wrong. Those
> > two colors are for items in controls like lists. We have  COLOR_BTNFACE and
> > COLOR_BTNTEXT.
> 
> We use COLOR_BTNTEXT for the label:
> 
> https://dxr.mozilla.org/mozilla-central/rev/
> 8c9c4e816e86f903c1d820f3f29715dc070a5a4a/toolkit/themes/windows/global/
> button.css#23
> 
> The background for button[default="true"] comes from -moz-appearance:
> button;. Does widget code use COLOR_HIGHLIGHT for that? If so, under which
> conditions? Because it doesn't seem to do that with the default theme, so
> making the label use COLOR_HIGHLIGHTTEXT would be wrong there.

I've confirmed the text is eColorID_buttontext. But I haven't found the source of that button background yet. In Win7 it's the window background color, and nothing in nsLookAndFeel gets queried to for it. I'll try debugging this on win8 where I can see the difference.
For win7 and lower these themes are technically classic themes, so we fall back on the classic rendering path and use DrawFrameControl to render the button. 

For Win8 and up, these themes are desktop themes designed to look like the old classic high contrast themes. Hence we end up running through a different code path and use DrawThemedBackground to render the button.

In the themed path, here [1] is where we request "defaulted" state. Technically this constant should come out of vsstyles.h but we're still using our old defines here. The proper constant is PBS_DEFAULTED, which has the same value as TS_FOCUSED. This is not the bug. 

With all standard themes adding PBS_DEFAULTED gives us a highlight around the button.

With os<=Win7 high contrast themes, we lose the border and keep the same button face color.

With os>=Win8 PBS_DEFAULTED changes the button face to the highlight color. This is correct behavior AFAICT, you can see this on default buttons for system widgets like the file picker.

However we aren't using an appropriate text color with this, and hence we end up with black text and a dark button face [2].

I'd like to add a new css block here wrapped in a accessibility media query to change the button text. However AFAICT we don't support an accessibility related css media query or system metric (bug 425598 is currently wontfix).

Dao, can you suggest a way forward here? Should we reopen bug 425598 or maybe try to use some other css query or metric?

[1] http://searchfox.org/mozilla-central/rev/8eb4fd2c7be150b0aa1b05c0f3707e82dc8f2dc8/widget/windows/nsNativeThemeWin.cpp#904
[2] http://searchfox.org/mozilla-central/rev/8eb4fd2c7be150b0aa1b05c0f3707e82dc8f2dc8/toolkit/themes/windows/global/button.css#23
Flags: needinfo?(jmathies) → needinfo?(dao+bmo)
(In reply to Jim Mathies [:jimm] from comment #7)
> I'd like to add a new css block here wrapped in a accessibility media query
> to change the button text. However AFAICT we don't support an accessibility
> related css media query or system metric (bug 425598 is currently wontfix).
> 
> Dao, can you suggest a way forward here? Should we reopen bug 425598 or
> maybe try to use some other css query or metric?

We could reopen bug 425598, but... Could we just use the default theme query instead? All non-default themes are high-contrast ones on Win 8 and later, right?
Flags: needinfo?(dao+bmo) → needinfo?(jmathies)
Attached patch patch? (obsolete) — Splinter Review
Like this? I haven't tested this yet.
Comment on attachment 8788946 [details] [diff] [review]
patch?

Yes, that works for both the dark and light high contrast themes.
Flags: needinfo?(jmathies)
Attachment #8788946 - Flags: feedback+
I'd be happy to r+ a finalized patch.
Flags: needinfo?(dao+bmo)
hover is broken too.
Component: Widget: Win32 → Themes
Flags: needinfo?(dao+bmo)
Product: Core → Toolkit
Summary: Default style of buttons is illegible when using black-on-white High Contrast mode → Default and hover style of buttons is illegible when using black-on-white or white-on-black High Contrast mode
Version: unspecified → Trunk
Attached patch patchSplinter Review
Assignee: nobody → dao+bmo
Attachment #8788946 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8789303 - Flags: review?(jmathies)
Attachment #8789303 - Flags: review?(jmathies) → review+
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2699eae6dafb
Set appropriate text color on default and hovered buttons for high-contrast themes on Windows 8 and later. r=jimm
https://hg.mozilla.org/mozilla-central/rev/2699eae6dafb
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Blocks: 1348573
Depends on: 1518018
You need to log in before you can comment on or make changes to this bug.