1348573 - Buttons are still illegible in focused / active / checked states with High Contrast black and High Contrast white themes

Status: RESOLVED FIXED

(Toolkit :: Themes, defect)

Comment 1

[Dão Gottwald [:dao]]

Attachment: Bug 1348573 - Fix button text color in focused / active / checked states with High Contrast black and High Contrast white themes.

Review commit: https://reviewboard.mozilla.org/r/121646/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/121646/

Comment 2

[:Gijs (he/him)]

Comment on attachment 8848763 [details]
Bug 1348573 - Fix button text color in focused / active / checked states with High Contrast black and High Contrast white themes.

https://reviewboard.mozilla.org/r/121646/#review126080

How do I test this? What is this fixing that's broken? It's not obvious from the commit message or comment #0 of the bug, and I don't see this having any effect on about:preferences, about:support, and I'm assuming it's not supposed to affect web pages... Please update the commit message and comment on the bug with more details about what you're trying to accomplish.

Comment 3

[Dão Gottwald [:dao]]

This is a followup to bug 1022568 (already linked under Depends on).

You can test this with buttons using the default style, and this excludes about:preferences and about:support because they're not using that style. You can test this with the dialog in attachment 8436833 [details] or the bookmark properties dialog, for instance.

I don't think any of this information belongs in the commit message.

Comment 4

[:Gijs (he/him)]

(In reply to Dão Gottwald [::dao] from comment #3)
> This is a followup to bug 1022568 (already linked under Depends on).
> 
> You can test this with buttons using the default style, and this excludes
> about:preferences and about:support because they're not using that style.
> You can test this with the dialog in attachment 8436833 [details] or the
> bookmark properties dialog, for instance.

Which of these uses the "open" or "checked" attribute which your patch references? I don't see any off-hand.

I also still see the same styling before and after the patch on the cancel button of the about:config dialog, on HCM black on Win10, if I mousedown on it, then mousedown off the button. So I don't think the patch is quite right.

> I don't think any of this information belongs in the commit message.

Not "try these dialogs to reproduce", no, but a more precise message about what kinds of button styling this fixes (and why just a text-color change is both necessary and sufficient) would be helpful in order to understand the patch.

Comment 5

[Dão Gottwald [:dao]]

(In reply to :Gijs from comment #4)
> (In reply to Dão Gottwald [::dao] from comment #3)
> > This is a followup to bug 1022568 (already linked under Depends on).
> > 
> > You can test this with buttons using the default style, and this excludes
> > about:preferences and about:support because they're not using that style.
> > You can test this with the dialog in attachment 8436833 [details] or the
> > bookmark properties dialog, for instance.
> 
> Which of these uses the "open" or "checked" attribute which your patch
> references? I don't see any off-hand.

You'd have to modify the DOM for that. Not sure if we have UI that directly uses these.

> I also still see the same styling before and after the patch on the cancel
> button of the about:config dialog, on HCM black on Win10, if I mousedown on
> it, then mousedown off the button. So I don't think the patch is quite right.

This seems like an invalidation problem to me, not a problem with the CSS at hand.

> > I don't think any of this information belongs in the commit message.
> 
> Not "try these dialogs to reproduce", no, but a more precise message about
> what kinds of button styling this fixes

Not sure what's still unclear here. This affects all XUL buttons in said states using the default style, which is all what toolkit/themes/windows/global/button.css is really about.

> (and why just a text-color change is
> both necessary and sufficient) would be helpful in order to understand the
> patch.

It's necessary because native buttons change appearance in various states and the button label color needs to reflect that.

Comment 6

[:Gijs (he/him)]

(In reply to Dão Gottwald [::dao] from comment #5)
> (In reply to :Gijs from comment #4)
> > (In reply to Dão Gottwald [::dao] from comment #3)
> > > This is a followup to bug 1022568 (already linked under Depends on).
> > > 
> > > You can test this with buttons using the default style, and this excludes
> > > about:preferences and about:support because they're not using that style.
> > > You can test this with the dialog in attachment 8436833 [details] or the
> > > bookmark properties dialog, for instance.
> > 
> > Which of these uses the "open" or "checked" attribute which your patch
> > references? I don't see any off-hand.
> 
> You'd have to modify the DOM for that. Not sure if we have UI that directly
> uses these.

So should we just get rid of all the open/checked styling in this file (except for the 'special' [type=disclosure] button)? If we have no UI that uses it, what purpose do the styles serve but to complicate things?

> > > I don't think any of this information belongs in the commit message.
> > 
> > Not "try these dialogs to reproduce", no, but a more precise message about
> > what kinds of button styling this fixes
> 
> Not sure what's still unclear here. This affects all XUL buttons in said
> states using the default style, which is all what
> toolkit/themes/windows/global/button.css is really about.

So, a paragraph along the lines of:

This expands the change of foreground color for hovered buttons to
keyboard-focused ones, and avoids changing the foreground color if the button
is disabled, active, open or checked, because in high contrast mode the
buttons' background color will then revert to the 'standard' non-hover/focus color.
The background colors are governed by moz-appearance.

would make sense to append to the commit message.

Comment 7

[Dão Gottwald [:dao]]

(In reply to :Gijs from comment #6)
> > You'd have to modify the DOM for that. Not sure if we have UI that directly
> > uses these.
> 
> So should we just get rid of all the open/checked styling in this file
> (except for the 'special' [type=disclosure] button)? If we have no UI that
> uses it, what purpose do the styles serve but to complicate things?

No, because Firefox may well be using them (I'm just not sure), and even if it currently doesn't it may start using them again any time, and last but not least Firefox is not the only consumer of this.

> > > > I don't think any of this information belongs in the commit message.
> > > 
> > > Not "try these dialogs to reproduce", no, but a more precise message about
> > > what kinds of button styling this fixes
> > 
> > Not sure what's still unclear here. This affects all XUL buttons in said
> > states using the default style, which is all what
> > toolkit/themes/windows/global/button.css is really about.
> 
> So, a paragraph along the lines of:
> 
> This expands the change of foreground color for hovered buttons to
> keyboard-focused ones, and avoids changing the foreground color if the button
> is disabled, active, open or checked,

This is really just reading out the diff. I don't think this kind of verbosity adds value.

> because in high contrast mode the
> buttons' background color will then revert to the 'standard' non-hover/focus
> color.
> The background colors are governed by moz-appearance.

I can add that.

Comment 8

[Dão Gottwald [:dao]]

Comment on attachment 8848763 [details]
Bug 1348573 - Fix button text color in focused / active / checked states with High Contrast black and High Contrast white themes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/121646/diff/1-2/

Comment 9

[Dão Gottwald [:dao]]

I also switched from :-moz-focusring to :focus since this doesn't have anything to do with focus rings, and it shouldn't matter how the button was focused.

Comment 10

[:Gijs (he/him)]

Comment on attachment 8848763 [details]
Bug 1348573 - Fix button text color in focused / active / checked states with High Contrast black and High Contrast white themes.

https://reviewboard.mozilla.org/r/121646/#review126096

Thanks!

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e6405802bfc0
Fix button text color in focused / active / checked states with High Contrast black and High Contrast white themes. r=Gijs

Comment 12

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/e6405802bfc0