Closed Bug 1022582 Opened 6 years ago Closed 5 years ago

Checkboxes and radio buttons in about:preferences lack any indication they're checked/selected when using High Contrast mode

Categories

(Toolkit :: Themes, defect)

30 Branch
All
Windows XP
defect
Not set
Points:
3

Tracking

()

VERIFIED FIXED
mozilla37
Iteration:
37.2
Tracking Status
firefox37 --- verified

People

(Reporter: Unfocused, Assigned: Paenglab)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 6 obsolete files)

Attached image Screenshot
The checkboxes and radio buttons in about:preferences lack any indication they're checked/selected when using High Contrast mode, making the two states indistinguishable. See attached screenshot.
Flags: firefox-backlog+
Possibly the same issue as bug 582951 and bug 575974 (both for non-chrome content pages).
Assignee: nobody → ntim007
Status: NEW → ASSIGNED
OS: Windows 8.1 → Windows XP
Hardware: x86_64 → All
I'm planning on using the SVG here : https://bug1014208.bugzilla.mozilla.org/attachment.cgi?id=8427995

And I'm planning to use Highlight as high contrast color. This looks similar to original color on Windows, which is great, but it doesn't look similar at all on OSX or Linux.

So, Is there a solution to this ? or should I simply use the hardcoded color (the checkmark would be visible in that case, but not respecting the system settings).
Flags: needinfo?(jaws)
We may want to try to get bug 425598 fixed, as that will allow us to use the desired colors at all times except for high contrast themes. I don't know of a special system color that we can use that will keep the shade of blue that is desired.
Depends on: 425598
Flags: needinfo?(jaws)
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #3)
> We may want to try to get bug 425598 fixed, as that will allow us to use the
> desired colors at all times except for high contrast themes. I don't know of
> a special system color that we can use that will keep the shade of blue that
> is desired.

That sounds like a perfect use case for the default theme metric. I.e. use the custom color for the default theme, use a system color otherwise.
(In reply to Dão Gottwald [:dao] from comment #4)
> That sounds like a perfect use case for the default theme metric. I.e. use
> the custom color for the default theme, use a system color otherwise.

Huh, for some reason I was under the belief that the -moz-windows-default-theme metric matched to true even with high contrast themes, but I just tested it out and my memory was wrong.

Looks like this bug can move forward then. Tim, can you add some rules to the styling that do the following:

#ifdef XP_WIN
@media not all and (-moz-windows-default-theme) {
  .checkbox { list-style-image: url(checkbox.svg); }
}
#endif

and then the checkbox.svg can just use -moz-DialogText normally and GrayText when disabled. Just like we did for the dropdown.
No longer depends on: 425598
Attached patch HC-Radio-Check.patch (obsolete) — Splinter Review
Tim, sorry to chime in to your bug and you don't mind, but it's a rainy day and I had some time.

Jared what do you think about this? 

Classic is also not a -moz-windows-default-theme and the used rule makes the radio and checkmark also under Classic black instead of the blue. So this would make bug 425598 still preferable to be fixed.
Attachment #8523417 - Flags: feedback?(jaws)
Comment on attachment 8523417 [details] [diff] [review]
HC-Radio-Check.patch

> xul|*.checkbox-check[checked] {
>-  background-image: url("chrome://global/skin/in-content/check.png"),
>-                    /* !important needed to override toolkit !important rule */
>-                    linear-gradient(#fff, rgba(255,255,255,0.8)) !important;
>+  list-style-image: url("chrome://global/skin/in-content/check.svg#check");
>+  /* !important needed to override toolkit !important rule */
>+  background-image: linear-gradient(#fff, rgba(255,255,255,0.8)) !important;
> }
> 
>+%ifdef XP_WIN
>+@media not all and (-moz-windows-default-theme) {
>+  xul|*.checkbox-check[checked] {
>+    list-style-image: url("chrome://global/skin/in-content/check.svg#check-hc");
>+  }
>+}
>+%endif

-moz-dialogText can be white, so using that on a hardcoded white background isn't sane.

Note that this bug also affects Linux with browser.display.use_document_colors=false.
Assignee: ntim007 → richard.marti
Comment on attachment 8523417 [details] [diff] [review]
HC-Radio-Check.patch

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

::: toolkit/themes/shared/in-content/radio.svg
@@ +8,5 @@
> +    use {
> +      fill: #2292d0;
> +    }
> +    use[id$="-hc"] {
> +      fill: -moz-DialogText;

Highlight would match better for third-party themes, and would still work with High contrast themes.
(In reply to Tim Nguyen [:ntim] from comment #8)
> 
> Highlight would match better for third-party themes, and would still work
> with High contrast themes.

Yeah, this would be on Classic a dark blue and on the dark HC themes a different color to the border/text like we also have on normal themes.

Jared, what do you think?
Comment on attachment 8523417 [details] [diff] [review]
HC-Radio-Check.patch

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

::: toolkit/themes/shared/in-content/check.svg
@@ +12,5 @@
> +      fill: -moz-DialogText;
> +    }
> +  </style>
> +  <defs style="display: none;">
> +    <path id="check-shape" d="M 9.39,16.5 16.28,6 14.77,4.5 9.37,12.7 6.28,9.2 4.7,10.7 z"/>

You can use a <circle> tag here.
Blocks: 1013692
Attached patch HC-radio-check.patch (obsolete) — Splinter Review
I'm turning this to a review now.

I'm using now Highlight as color on non-default themes to address Dão's comment 7. With this it imitates more the original Chameleon's style with the different colors of the borders and inner elements.
Attachment #8523417 - Attachment is obsolete: true
Attachment #8523417 - Flags: feedback?(jaws)
Attachment #8527162 - Flags: review?(jaws)
Comment on attachment 8527162 [details] [diff] [review]
HC-radio-check.patch

> xul|*.checkbox-check[checked] {
>-  background-image: url("chrome://global/skin/in-content/check.png"),
>-                    /* !important needed to override toolkit !important rule */
>-                    linear-gradient(#fff, rgba(255,255,255,0.8)) !important;
>+  list-style-image: url("chrome://global/skin/in-content/check.svg#check");
>+  /* !important needed to override toolkit !important rule */
>+  background-image: linear-gradient(#fff, rgba(255,255,255,0.8)) !important;

How is this different from the background-image for the unchecked state?

> }
> 
>+%ifdef XP_WIN
>+@media not all and (-moz-windows-default-theme) {
>+  xul|*.checkbox-check[checked] {
>+    list-style-image: url("chrome://global/skin/in-content/check.svg#check-hc");
>+  }
>+}
>+%endif

What ensures that this is actually visible on the background? Highlight can be any color just like -moz-dialogText, so this doesn't really address comment 7.

What about Linux?
Attachment #8527162 - Flags: review?(jaws) → review-
Component: Preferences → Themes
Product: Firefox → Toolkit
(In reply to Dão Gottwald [:dao] from comment #12)
> Comment on attachment 8527162 [details] [diff] [review]
> HC-radio-check.patch
> 
> > xul|*.checkbox-check[checked] {
> >-  background-image: url("chrome://global/skin/in-content/check.png"),
> >-                    /* !important needed to override toolkit !important rule */
> >-                    linear-gradient(#fff, rgba(255,255,255,0.8)) !important;
> >+  list-style-image: url("chrome://global/skin/in-content/check.svg#check");
> >+  /* !important needed to override toolkit !important rule */
> >+  background-image: linear-gradient(#fff, rgba(255,255,255,0.8)) !important;
> 
> How is this different from the background-image for the unchecked state?

Good catch, I remove it.

> >+%ifdef XP_WIN
> >+@media not all and (-moz-windows-default-theme) {
> >+  xul|*.checkbox-check[checked] {
> >+    list-style-image: url("chrome://global/skin/in-content/check.svg#check-hc");
> >+  }
> >+}
> >+%endif
> 
> What ensures that this is actually visible on the background? Highlight can
> be any color just like -moz-dialogText, so this doesn't really address
> comment 7.

With Windows HC-themes the background-image isn't shown. This is also why the check and radio aren't shown and need the list-style-image. Then the Highlight, and also the -moz-dialogText, are drawn over the -moz-dialog of http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/windows/global/global.css#32. I could also go back to -moz-dialogText which better correlates to -moz-dialog than Highlight.

> What about Linux?

Linux still uses #2292d0.
(In reply to Richard Marti (:Paenglab) from comment #13)
> > What ensures that this is actually visible on the background? Highlight can
> > be any color just like -moz-dialogText, so this doesn't really address
> > comment 7.
> 
> With Windows HC-themes the background-image isn't shown. This is also why
> the check and radio aren't shown and need the list-style-image. Then the
> Highlight, and also the -moz-dialogText, are drawn over the -moz-dialog of
> http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/windows/global/
> global.css#32. I could also go back to -moz-dialogText which better
> correlates to -moz-dialog than Highlight.

My question wasn't limited to high-contrast themes. As for high-contrast, is the background-*color* not shown either? (This would mean that we effectively flip browser.display.use_document_colors for these themes.)

> > What about Linux?
> 
> Linux still uses #2292d0.

And is that sufficient? See comment 7.
Attached patch HC-radio-check.patch (obsolete) — Splinter Review
(In reply to Dão Gottwald [:dao] from comment #14)
> (In reply to Richard Marti (:Paenglab) from comment #13)
> > > What ensures that this is actually visible on the background? Highlight can
> > > be any color just like -moz-dialogText, so this doesn't really address
> > > comment 7.
> > 
> > With Windows HC-themes the background-image isn't shown. This is also why
> > the check and radio aren't shown and need the list-style-image. Then the
> > Highlight, and also the -moz-dialogText, are drawn over the -moz-dialog of
> > http://mxr.mozilla.org/mozilla-central/source/toolkit/themes/windows/global/
> > global.css#32. I could also go back to -moz-dialogText which better
> > correlates to -moz-dialog than Highlight.
> 
> My question wasn't limited to high-contrast themes. As for high-contrast, is
> the background-*color* not shown either? (This would mean that we
> effectively flip browser.display.use_document_colors for these themes.)

I've added background: HighlightText to give a contrasting color for the background. This would then apply third party themes.

> > > What about Linux?
> > 
> > Linux still uses #2292d0.
> 
> And is that sufficient? See comment 7.

What do you propose to do for Linux?
Attachment #8527162 - Attachment is obsolete: true
Attachment #8527175 - Flags: review?(dao)
Blocks: 1105418
Attached patch HC-radio-check.patch (obsolete) — Splinter Review
Updated to tip.
Attachment #8527175 - Attachment is obsolete: true
Attachment #8527175 - Flags: review?(dao)
Attachment #8529874 - Flags: review?(dao)
Comment on attachment 8529874 [details] [diff] [review]
HC-radio-check.patch

>+%ifdef XP_WIN
>+@media not all and (-moz-windows-default-theme) {
>+  xul|*.checkbox-check[checked] {
>+    list-style-image: url("chrome://global/skin/in-content/check.svg#check-hc");
>+    background: HighlightText;
>+  }
>+}
>+%endif

So this doesn't work well with browser.display.use_document_colors=false, because we'll use the image but not the background color. As I understand it, using -moz-fieldText or -moz-dialogText rather than Highlight for the icon (and -moz-field or -moz-dialog for the background) is going to be the safer choice, then. And we'll need to do this on linux too.

nit: please rename -hc to -native
Attachment #8529874 - Flags: review?(dao) → review-
Attached patch HC-radio-check.patch (obsolete) — Splinter Review
Changed the native checkmark- and radio-button icons to -moz-dialogText and -moz-dialog.

Linux is using now always the native colors. Is this okay like this?
Attachment #8529874 - Attachment is obsolete: true
Attachment #8532026 - Flags: review?(dao)
(In reply to Richard Marti (:Paenglab) from comment #18)
> Created attachment 8532026 [details] [diff] [review]
> HC-radio-check.patch
> 
> Changed the native checkmark- and radio-button icons to -moz-dialogText and
> -moz-dialog.
> 
> Linux is using now always the native colors. Is this okay like this?

I'm afraid -moz-dialog will make the checkbox look disabled on windows 3rd party themes, since -moz-dialog is usually gray. I think -moz-field and -moz-fieldText would be better here.
Comment on attachment 8532026 [details] [diff] [review]
HC-radio-check.patch

>--- a/toolkit/themes/shared/in-content/common.inc.css
>+++ b/toolkit/themes/shared/in-content/common.inc.css
>@@ -393,49 +393,53 @@
> xul|*.checkbox-check {
...
>+  background-image: linear-gradient(#fff, rgba(255,255,255,0.8)) !important;
...
> }

>+  xul|*.checkbox-check[checked] {
...
>+    background: -moz-dialog;
>+  }

The background image is close to opaque and since you use !important, you're not removing it with "background", so the background color won't have much effect. This seems relevant for dark non-high-contrast themes.

Also, with growing divergence across platforms it might be a good idea to move the some of this stuff out of the shared file. I'm not a fan of ifdefs in the shared file when we already have specific theme files.
Attachment #8532026 - Flags: review?(dao) → review-
Attached patch HC-radio-check.patch (obsolete) — Splinter Review
Moved the styles to the platform files.
Attachment #8532026 - Attachment is obsolete: true
Attachment #8532216 - Flags: review?(dao)
Comment on attachment 8532216 [details] [diff] [review]
HC-radio-check.patch

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

::: toolkit/themes/linux/global/in-content/common.css
@@ +51,5 @@
> +}
> +
> +xul|*.checkbox-check[checked] {
> +  list-style-image: url("chrome://global/skin/in-content/check.svg#check-native");
> +  background-color: -moz-dialog;

Please use -moz-field, otherwise the checkboxes will look gray (and disabled) on Linux default theme and Windows third-party themes.

Also, I don't see why the background-color is only applied for the checked state.

@@ +66,5 @@
> +}
> +
> +xul|*.radio-check[selected] {
> +  list-style-image: url("chrome://global/skin/in-content/radio.svg#radio-native");
> +  background: -moz-dialog;

Same here.

Also, you used background-color earlier for checkbox-check, and now you're using background.

::: toolkit/themes/shared/in-content/check.svg
@@ +8,5 @@
> +    use {
> +      fill: #2292d0;
> +    }
> +    use[id$="-native"] {
> +      fill: -moz-dialogText;

Please use -moz-fieldText instead of -moz-dialogText.

::: toolkit/themes/shared/in-content/radio.svg
@@ +8,5 @@
> +    use {
> +      fill: #2292d0;
> +    }
> +    use[id$="-native"] {
> +      fill: -moz-dialogText;

Same comment about the color.

::: toolkit/themes/windows/global/in-content/common.css
@@ +32,5 @@
> +    list-style-image: url("chrome://global/skin/in-content/check.svg#check-native");
> +    background-color: -moz-dialog;
> +  }
> +}
> +

Same comments apply here.

@@ +49,5 @@
> +    list-style-image: url("chrome://global/skin/in-content/radio.svg#radio-native");
> +    background-color: -moz-dialog;
> +  }
> +}
> +

Same here.
(In reply to Tim Nguyen [:ntim] from comment #22)
> Comment on attachment 8532216 [details] [diff] [review]
> HC-radio-check.patch
> 
> Review of attachment 8532216 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/themes/linux/global/in-content/common.css
> @@ +51,5 @@
> > +}
> > +
> > +xul|*.checkbox-check[checked] {
> > +  list-style-image: url("chrome://global/skin/in-content/check.svg#check-native");
> > +  background-color: -moz-dialog;
> 
> Please use -moz-field, otherwise the checkboxes will look gray (and
> disabled) on Linux default theme and Windows third-party themes.

I checked on my Linux Mint and -moz-dialogText is a dark grey and this doesn't look disabled. I'll let it land like it is now. If it doesn't work on other themes this can be changed in a new bug.

> Also, I don't see why the background-color is only applied for the checked
> state.

In non-checked state the original background-color is used to better match the original page background-color.

> @@ +66,5 @@
> > +}
> > +
> > +xul|*.radio-check[selected] {
> > +  list-style-image: url("chrome://global/skin/in-content/radio.svg#radio-native");
> > +  background: -moz-dialog;
> 
> Same here.
> 
> Also, you used background-color earlier for checkbox-check, and now you're
> using background.

Fixed, use background-color now.
Attachment #8532216 - Attachment is obsolete: true
Attachment #8534543 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/56ba684600e1
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla37
Iteration: --- → 37.2
Flags: qe-verify?
Flags: qe-verify? → qe-verify+
QA Contact: camelia.badau
Blocks: 1047591
Verified fixed on Windows 7 64bit, Windows XP 32bit and Windows 8 32bit using latest Nightly 37.0a1 (buildID: 20141214030205).
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.