1791024 - Focus blue outline should not override HCM colors

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect, P3)

Description

[Sarah Clements [:sclements]]

During a recent accessibility review for Firefox View, it was drawn to our attention that the focus outline in HCM on Windows has a double outline. It looks like this was part of a UX improvement in bug 1776592 but it should probably respect focus colors in HCM mode and not have that blue outline.

We have this issue tracked in bug 1786397 and even though its marked as resolved it'd be preferable if we didn't have to override this hard coded blue outline with media queries. (I don't see this issue in Mac, only in Windows.)

Comment 1

[BugBot [:suhaib / :marco]]

The severity field is not set for this bug.
:spohl, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 2

[Ray Fambro]

I know this is just clean up and pretty low priority, but wanted to check in on status. Any updates or plans to address?

Comment 3

[Emilio Cobos Álvarez [:emilio]]

Can you elaborate on what difference do you see? I'm not sure I'm following. Is the issue that we behave as outline: auto even if you specify another oitline-color? Or something else?

Comment 4

[Sarah Clements [:sclements]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #3)

Can you elaborate on what difference do you see? I'm not sure I'm following. Is the issue that we behave as outline: auto even if you specify another oitline-color? Or something else?

Hi Emilio, the issue is that the blue part of the double outline is hardcoded and overrides HCM focus colors. See Anna's comment and screenshot where this bug was originally reported: https://bugzilla.mozilla.org/show_bug.cgi?id=1786397#c2

Comment 5

[Emilio Cobos Álvarez [:emilio]]

It's not hard-coded? You can set accent-color: Highlight or somesuch to override it?

Comment 6

[Sarah Clements [:sclements]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #5)

It's not hard-coded? You can set accent-color: Highlight or somesuch to override it?

We did manually override it in the style sheet, in a prefers-contrast media query to get around this. I'm curious what the a11y team thinks, but it seems like you shouldn't have to override it (especially since this change was a surprise and it happened to be caught during an accessibility review).

Comment 7

[Morgan Reschenberg [:morgan]]

(In reply to Sarah Clements [:sclements] from comment #6)

(In reply to Emilio Cobos Álvarez (:emilio) from comment #5)

It's not hard-coded? You can set accent-color: Highlight or somesuch to override it?

We did manually override it in the style sheet, in a prefers-contrast media query to get around this. I'm curious what the a11y team thinks, but it seems like you shouldn't have to override it (especially since this change was a surprise and it happened to be caught during an accessibility review).

Hey Sarah, can you tell me more about what you tried here? Where did you attempt this override and what did you see as a result? Also: what platform are you testing on?

Comment 8

[Sarah Clements [:sclements]]

(In reply to Morgan Reschenberg [:morgan] from comment #7)

(In reply to Sarah Clements [:sclements] from comment #6)

(In reply to Emilio Cobos Álvarez (:emilio) from comment #5)

It's not hard-coded? You can set accent-color: Highlight or somesuch to override it?

We did manually override it in the style sheet, in a prefers-contrast media query to get around this. I'm curious what the a11y team thinks, but it seems like you shouldn't have to override it (especially since this change was a surprise and it happened to be caught during an accessibility review).

Hey Sarah, can you tell me more about what you tried here? Where did you attempt this override and what did you see as a result? Also: what platform are you testing on?

Hi Morgan, tgiles did some styling to fix this issue in the Firefox View style sheets and it does solve the issue in firefox view - see the patch in bug 1786397. The crux of the issue I'm trying to get it as should we have to override this? If the answer is "yes", then we can close out this bug (though maybe this should be communicated to folks on the front end?). But my impression is that this was a side-effect of bug 1776592.

Comment 9

[Morgan Reschenberg [:morgan]]

(In reply to Sarah Clements [:sclements] from comment #8)

Hi Morgan, tgiles did some styling to fix this issue in the Firefox View style sheets and it does solve the issue in firefox view - see the patch in bug 1786397. The crux of the issue I'm trying to get it as should we have to override this? If the answer is "yes", then we can close out this bug (though maybe this should be communicated to folks on the front end?). But my impression is that this was a side-effect of bug 1776592.

ahhh gotcha -- yeah this is a bit confusing. the patch that added the dual-outline uses accent-color (which fetches that blue). that colour doesn't get overridden when HCM is enabled the way background, foreground, and link colours do. you could argue we should be overriding accent-color, but I think that override would look different across platforms -- on mac, for example, we probably want to continue to use the system accent color when prefers-contrast is triggered. On windows, though, we might want something more like SelectedItem? Hard to say. I imagine the preferred override also depends on the context we're using accent color in.

That said, I think it's reasonable to change the code in widget/Theme.cpp to use something other than accent colour to draw the double outline when HCM is on. That feels like a localised/safe change, and I can post a patch :)

Comment 10

[Morgan Reschenberg [:morgan]]

Ah looks like someone (emilio?) addressed this by bringing back ComputeFocusRectColors and adding HCM casing. I think this should "just work" without the media query overrides now.

Comment 11

[Sarah Clements [:sclements]]

(In reply to Morgan Reschenberg [:morgan] from comment #10)

Ah looks like someone (emilio?) addressed this by bringing back ComputeFocusRectColors and adding HCM casing. I think this should "just work" without the media query overrides now.

Hi Morgan, apologies for the very long delay... lots going on :) It doesn't seem this issue has been fixed at the widgets/layout level as it has again come up during a11y review of firefoxview-next, see bug 1846603. Do you still want to submit a patch for this or should I use media query overrides the same way we did for old firefoxview?

Comment 12

[Emilio Cobos Álvarez [:emilio]]

I'm confused at why is a solid outline the expectation. We use a double outline to guarantee contrast, in fact, so it'd be weird to avoid that only on HCM.

Comment 13

[Sarah Clements [:sclements]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #12)

I'm confused at why is a solid outline the expectation. We use a double outline to guarantee contrast, in fact, so it'd be weird to avoid that only on HCM.

I think this is a question for the a11y team, but as Anna mentioned in bug :

Expected:
Focus outline is solid CanvasText color outline (refer to the Acorns > Buttons > States > (HCM), State = Focus examples)

Actual:
Focus outline consists of two outlines: a CanvasText (as expected) and blue (on Windows 11 Night Sky HCM) or Accent color (on macOS)

It looks like in the screenshot of her first comment though, that there still is a double outline but the second isn't blue.

Comment 14

[Morgan Reschenberg [:morgan]]

(In reply to Sarah Clements [:sclements] from comment #11)

(In reply to Morgan Reschenberg [:morgan] from comment #10)

Ah looks like someone (emilio?) addressed this by bringing back ComputeFocusRectColors and adding HCM casing. I think this should "just work" without the media query overrides now.

Hi Morgan, apologies for the very long delay... lots going on :) It doesn't seem this issue has been fixed at the widgets/layout level as it has again come up during a11y review of firefoxview-next, see bug 1846603. Do you still want to submit a patch for this or should I use media query overrides the same way we did for old firefoxview?

I'm not sure why the code we have in layout right now doesn't work, so I'll take a closer look.

We should be using a double outline with CanvasText (same as non-HCM) and SelectedItem

Comment 15

[Morgan Reschenberg [:morgan]]

It looks like we do use the rect code I linked above for drawing outlines on <input>'s, but not on links.

:emilio do you know why that'd be true?

Comment 16

[❌ Anna Yeddi [:ayeddi] ❌ On leave until 2027-01-04]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #12)

I'm confused at why is a solid outline the expectation. We use a double outline to guarantee contrast, in fact, so it'd be weird to avoid that only on HCM.

That'd be great, but then we would probably want to have the double outline with the same HCM-ized colors across the chrome UI while on HCM

At the moment, as Morgan mentioned in the comment 14, we are expected to be using HCM colors, while the Fx View has got default, non-HCM color (blue) with CanvasText (refer to the Win screenshot from bug 1846603)

Comment 17

[Emilio Cobos Álvarez [:emilio]]

Isnt the blue just AccentColor, which is the default for outline-style: auto? How is that not an HCM color?

In any case we'd need to special case here to use something that isn't accentColor: https://searchfox.org/mozilla-central/rev/57f94ca1d57ab745242daafc8926690377579b83/widget/Theme.cpp#1430

But if Windows should return a different accent color in HCM we should fix it in widget/windows/nsLookAndFeel rather than special casing everywhere.

Comment 18

[Morgan Reschenberg [:morgan]]

:sclements, to answer your orignal question, I don't think you should use media queries to work around this in the fx view instance. we should fix the issue here. feel free to mark this as blocking if there's another issue filed from a11y review.

Comment 19

[Sarah Clements [:sclements]]

(In reply to Morgan Reschenberg [:morgan] from comment #18)

:sclements, to answer your orignal question, I don't think you should use media queries to work around this in the fx view instance. we should fix the issue here. feel free to mark this as blocking if there's another issue filed from a11y review.

Thanks, I'll do that.

Comment 20

[Morgan Reschenberg [:morgan]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #17)

Isnt the blue just AccentColor, which is the default for outline-style: auto? How is that not an HCM color?

In any case we'd need to special case here to use something that isn't accentColor: https://searchfox.org/mozilla-central/rev/57f94ca1d57ab745242daafc8926690377579b83/widget/Theme.cpp#1430

But if Windows should return a different accent color in HCM we should fix it in widget/windows/nsLookAndFeel rather than special casing everywhere.

I don't know if there's an HCM color that makes semantic sense to use for accent color, since we apply it across lots of different surfaces in our chrome. In this case, I think it should be SelectedItem to match Edge/Chrome's dual focus outlines for HCM (and semantically focused items are "selected" more than they are "button" or something else), but in other cases -- like in the case of our primary buttons, I think? -- it's maybe better to useButtonText.

I agree that specifying edge-cases for HCM all over the place isn't great, but I think focus rings are a bit of a special case and we should handle them explicitly, like we've done for HCM here

Comment 21

[Morgan Reschenberg [:morgan]]

Attachment: Bug 1791024: Make focus ring use Canvastext and Selecteditem when HCM is enabled r?emilio

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fdb6ed958661
Make focus ring use Canvastext and Selecteditem when HCM is enabled r=emilio

Comment 23

[Sandor Molnar[:smolnar]]

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