Respect explicit outline-color for focus rings in dark mode [chromium/src : main]
Helmut Januschka (Gerrit)
Helmut Januschka added 1 comment![]( "Open in Gerrit")
hi @fma...@chromium.org thanks for review in advance.
offtopic, i love this avatar you have, thought my glassses had a bug!!
Related details
- Florin Malita
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Florin Malita (Gerrit)
Florin Malita added 1 comment![]( "Open in Gerrit")
The change looks good, but the pixel diffs in `chrome/test/data/focus_rings/` seem problematic: we changed from white to a black outline, which is not visible against the black background.
Do you know what's causing that? Looks like the test is using dark mode, so I would expect the outline to be white:
Related details
- Helmut Januschka
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Helmut Januschka (Gerrit)
Helmut Januschka added 2 comments![]( "Open in Gerrit")
The change looks good, but the pixel diffs in `chrome/test/data/focus_rings/` seem problematic: we changed from white to a black outline, which is not visible against the black background.
Do you know what's causing that? Looks like the test is using dark mode, so I would expect the outline to be white:
Good catch. Root cause was that renderer prefs set a custom focus ring color (#101010), and FocusRingColor() was honoring that in dark mode.
thank you from preventing me to break the world!
Related details
- Florin Malita
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Florin Malita (Gerrit)
Florin Malita voted and added 1 comment![]( "Open in Gerrit")
Votes added by Florin Malita
| Code-Review | +1 |
1 comment
Related details
- Helmut Januschka
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Helmut Januschka (Gerrit)
Helmut Januschka voted Commit-Queue+2![]( "Open in Gerrit")
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Respect explicit outline-color for focus rings in dark mode
PaintFocusRing() unconditionally overrides the author-specified
outline-color to white when DarkColorScheme() is true on non-Mac
platforms. This means outline-color: red is silently ignored in
dark mode.
Bug: 480978106, 489304736
Change-Id: Ie602528a2e235a5b1fbd5dd9c4eada41b2425799
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7633026
Reviewed-by: Florin Malita <fma...@chromium.org>
Commit-Queue: Helmut Januschka <hel...@januschka.com>
Cr-Commit-Position: refs/heads/main@{#1595419}
- M third_party/blink/renderer/core/layout/layout_theme.cc
- M third_party/blink/renderer/core/paint/outline_painter.cc
- M third_party/blink/renderer/core/paint/outline_painter_test.cc
- A third_party/blink/web_tests/flag-specific/enable-skia-graphite/virtual/dark-color-scheme/fast/forms/color-scheme/suggestion-picker/date-suggestion-picker-appearance-expected.png
- A third_party/blink/web_tests/flag-specific/enable-skia-graphite/virtual/dark-color-scheme/fast/forms/color-scheme/suggestion-picker/datetimelocal-suggestion-picker-appearance-expected.png
- A third_party/blink/web_tests/flag-specific/enable-skia-graphite/virtual/dark-color-scheme/fast/forms/color-scheme/suggestion-picker/month-suggestion-picker-appearance-expected.png
- A third_party/blink/web_tests/flag-specific/enable-skia-graphite/virtual/dark-color-scheme/fast/forms/color-scheme/suggestion-picker/time-suggestion-picker-appearance-expected.png
- A third_party/blink/web_tests/flag-specific/enable-skia-graphite/virtual/dark-color-scheme/fast/forms/color-scheme/suggestion-picker/week-suggestion-picker-appearance-expected.png
- M third_party/blink/web_tests/platform/linux/virtual/dark-color-scheme/fast/forms/color-scheme/suggestion-picker/date-suggestion-picker-appearance-expected.png
- M third_party/blink/web_tests/platform/linux/virtual/dark-color-scheme/fast/forms/color-scheme/suggestion-picker/datetimelocal-suggestion-picker-appearance-expected.png
- M third_party/blink/web_tests/platform/linux/virtual/dark-color-scheme/fast/forms/color-scheme/suggestion-picker/month-suggestion-picker-appearance-expected.png
- M third_party/blink/web_tests/platform/linux/virtual/dark-color-scheme/fast/forms/color-scheme/suggestion-picker/time-suggestion-picker-appearance-expected.png
- M third_party/blink/web_tests/platform/linux/virtual/dark-color-scheme/fast/forms/color-scheme/suggestion-picker/week-suggestion-picker-appearance-expected.png
- M third_party/blink/web_tests/platform/mac/virtual/dark-color-scheme/fast/forms/color-scheme/suggestion-picker/date-suggestion-picker-appearance-expected.png
- M third_party/blink/web_tests/platform/mac/virtual/dark-color-scheme/fast/forms/color-scheme/suggestion-picker/datetimelocal-suggestion-picker-appearance-expected.png
- M third_party/blink/web_tests/platform/mac/virtual/dark-color-scheme/fast/forms/color-scheme/suggestion-picker/month-suggestion-picker-appearance-expected.png
- M third_party/blink/web_tests/platform/mac/virtual/dark-color-scheme/fast/forms/color-scheme/suggestion-picker/time-suggestion-picker-appearance-expected.png
- M third_party/blink/web_tests/platform/mac/virtual/dark-color-scheme/fast/forms/color-scheme/suggestion-picker/week-suggestion-picker-appearance-expected.png
- D third_party/blink/web_tests/platform/win/virtual/dark-color-scheme/fast/forms/color-scheme/suggestion-picker/date-suggestion-picker-appearance-expected.png
- D third_party/blink/web_tests/platform/win/virtual/dark-color-scheme/fast/forms/color-scheme/suggestion-picker/datetimelocal-suggestion-picker-appearance-expected.png
- D third_party/blink/web_tests/platform/win/virtual/dark-color-scheme/fast/forms/color-scheme/suggestion-picker/month-suggestion-picker-appearance-expected.png
- D third_party/blink/web_tests/platform/win/virtual/dark-color-scheme/fast/forms/color-scheme/suggestion-picker/time-suggestion-picker-appearance-expected.png
- D third_party/blink/web_tests/platform/win/virtual/dark-color-scheme/fast/forms/color-scheme/suggestion-picker/week-suggestion-picker-appearance-expected.png
- A third_party/blink/web_tests/virtual/dark-color-scheme/fast/forms/color-scheme/suggestion-picker/date-suggestion-picker-appearance-expected.png
- A third_party/blink/web_tests/virtual/dark-color-scheme/fast/forms/color-scheme/suggestion-picker/datetimelocal-suggestion-picker-appearance-expected.png
- A third_party/blink/web_tests/virtual/dark-color-scheme/fast/forms/color-scheme/suggestion-picker/month-suggestion-picker-appearance-expected.png
- A third_party/blink/web_tests/virtual/dark-color-scheme/fast/forms/color-scheme/suggestion-picker/time-suggestion-picker-appearance-expected.png
- A third_party/blink/web_tests/virtual/dark-color-scheme/fast/forms/color-scheme/suggestion-picker/week-suggestion-picker-appearance-expected.png
Code-Review: +1 by Florin Malita
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Philip Rogers (Gerrit)
Philip Rogers added 1 comment![]( "Open in Gerrit")
#if !BUILDFLAG(IS_MAC)Q1: will this patch change behavior of some new codepaths, such as `CanvasRenderingContext2D::DrawFocusRing` which uses this function but did not previously use PaintFocusRing?
Q2: Does this change behavior of reading the css style for `-webkit-focus-ring-color`? I think this can be done with `console.log((() => { const d = document.createElement('div'); d.style.color = '-webkit-focus-ring-color'; document.body.appendChild(d); const c = getComputedStyle(d).color; d.remove(); return c; })());`.
Q3: Why do we still have this platform difference at all?
Related details
- Helmut Januschka
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Helmut Januschka (Gerrit)
Helmut Januschka added 1 comment![]( "Open in Gerrit")
#if !BUILDFLAG(IS_MAC)Q1: will this patch change behavior of some new codepaths, such as `CanvasRenderingContext2D::DrawFocusRing` which uses this function but did not previously use PaintFocusRing?
Q2: Does this change behavior of reading the css style for `-webkit-focus-ring-color`? I think this can be done with `console.log((() => { const d = document.createElement('div'); d.style.color = '-webkit-focus-ring-color'; document.body.appendChild(d); const c = getComputedStyle(d).color; d.remove(); return c; })());`.
Q3: Why do we still have this platform difference at all?
A1: Yes, this does affect other FocusRingColor() callers (like CanvasRenderingContext2D::DrawFocusRing) on non-Mac in dark mode.
That is intentional: previously only the outline:auto paint path forced white in dark mode, while other -webkit-focus-ring-color users kept the dark renderer-pref color (#101010), which led to inconsistent and sometimes invisible focus indicators.
A2: Yes, in dark color-scheme contexts this changes computed style for -webkit-focus-ring-color on non-Mac. Since that keyword resolves via LayoutTheme::FocusRingColor(), getComputedStyle(...).color now resolves to white instead of rgb(16, 16, 16) (light mode is unchanged).
A3: The platform split is pre-existing and preserved by this CL. Mac uses LayoutThemeMac::FocusRingColor() (native accent/focus behavior), while non-Mac uses the legacy renderer-pref path with dark-mode visibility handling. This CL only moves where the non-Mac dark override is applied; it does not attempt to unify Mac/non-Mac policy.
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Philip Rogers (Gerrit)
Philip Rogers added 1 comment![]( "Open in Gerrit")
#if !BUILDFLAG(IS_MAC)Helmut JanuschkaQ1: will this patch change behavior of some new codepaths, such as `CanvasRenderingContext2D::DrawFocusRing` which uses this function but did not previously use PaintFocusRing?
Q2: Does this change behavior of reading the css style for `-webkit-focus-ring-color`? I think this can be done with `console.log((() => { const d = document.createElement('div'); d.style.color = '-webkit-focus-ring-color'; document.body.appendChild(d); const c = getComputedStyle(d).color; d.remove(); return c; })());`.
Q3: Why do we still have this platform difference at all?
A1: Yes, this does affect other FocusRingColor() callers (like CanvasRenderingContext2D::DrawFocusRing) on non-Mac in dark mode.
That is intentional: previously only the outline:auto paint path forced white in dark mode, while other -webkit-focus-ring-color users kept the dark renderer-pref color (#101010), which led to inconsistent and sometimes invisible focus indicators.
A2: Yes, in dark color-scheme contexts this changes computed style for -webkit-focus-ring-color on non-Mac. Since that keyword resolves via LayoutTheme::FocusRingColor(), getComputedStyle(...).color now resolves to white instead of rgb(16, 16, 16) (light mode is unchanged).
A3: The platform split is pre-existing and preserved by this CL. Mac uses LayoutThemeMac::FocusRingColor() (native accent/focus behavior), while non-Mac uses the legacy renderer-pref path with dark-mode visibility handling. This CL only moves where the non-Mac dark override is applied; it does not attempt to unify Mac/non-Mac policy.
Isn't this patch basically a regression for non-mac platforms for the other callers of FocusRingColor? In other words, you've fixed PaintFocusRing, but broken FocusRingColor. Or do I misunderstand?
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Helmut Januschka (Gerrit)
Helmut Januschka added 1 comment![]( "Open in Gerrit")
#if !BUILDFLAG(IS_MAC)Helmut JanuschkaQ1: will this patch change behavior of some new codepaths, such as `CanvasRenderingContext2D::DrawFocusRing` which uses this function but did not previously use PaintFocusRing?
Q2: Does this change behavior of reading the css style for `-webkit-focus-ring-color`? I think this can be done with `console.log((() => { const d = document.createElement('div'); d.style.color = '-webkit-focus-ring-color'; document.body.appendChild(d); const c = getComputedStyle(d).color; d.remove(); return c; })());`.
Q3: Why do we still have this platform difference at all?
Philip RogersA1: Yes, this does affect other FocusRingColor() callers (like CanvasRenderingContext2D::DrawFocusRing) on non-Mac in dark mode.
That is intentional: previously only the outline:auto paint path forced white in dark mode, while other -webkit-focus-ring-color users kept the dark renderer-pref color (#101010), which led to inconsistent and sometimes invisible focus indicators.
A2: Yes, in dark color-scheme contexts this changes computed style for -webkit-focus-ring-color on non-Mac. Since that keyword resolves via LayoutTheme::FocusRingColor(), getComputedStyle(...).color now resolves to white instead of rgb(16, 16, 16) (light mode is unchanged).
A3: The platform split is pre-existing and preserved by this CL. Mac uses LayoutThemeMac::FocusRingColor() (native accent/focus behavior), while non-Mac uses the legacy renderer-pref path with dark-mode visibility handling. This CL only moves where the non-Mac dark override is applied; it does not attempt to unify Mac/non-Mac policy.
Isn't this patch basically a regression for non-mac platforms for the other callers of FocusRingColor? In other words, you've fixed PaintFocusRing, but broken FocusRingColor. Or do I misunderstand?
You are right that it changes all non-Mac dark-mode FocusRingColor() callers, not just PaintFocusRing(), and that is intentional: previously only outline:auto forced white while other paths still used #101010, causing inconsistent and often invisible focus rings. This also intentionally addresses https://issues.chromium.org/issues/489304736, where -webkit-focus-ring-color with non-auto outlines in dark mode stayed at #101010 on non-Mac.
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Philip Rogers (Gerrit)
Philip Rogers added 1 comment![]( "Open in Gerrit")
#if !BUILDFLAG(IS_MAC)Helmut JanuschkaQ1: will this patch change behavior of some new codepaths, such as `CanvasRenderingContext2D::DrawFocusRing` which uses this function but did not previously use PaintFocusRing?
Q2: Does this change behavior of reading the css style for `-webkit-focus-ring-color`? I think this can be done with `console.log((() => { const d = document.createElement('div'); d.style.color = '-webkit-focus-ring-color'; document.body.appendChild(d); const c = getComputedStyle(d).color; d.remove(); return c; })());`.
Q3: Why do we still have this platform difference at all?
Philip RogersA1: Yes, this does affect other FocusRingColor() callers (like CanvasRenderingContext2D::DrawFocusRing) on non-Mac in dark mode.
That is intentional: previously only the outline:auto paint path forced white in dark mode, while other -webkit-focus-ring-color users kept the dark renderer-pref color (#101010), which led to inconsistent and sometimes invisible focus indicators.
A2: Yes, in dark color-scheme contexts this changes computed style for -webkit-focus-ring-color on non-Mac. Since that keyword resolves via LayoutTheme::FocusRingColor(), getComputedStyle(...).color now resolves to white instead of rgb(16, 16, 16) (light mode is unchanged).
A3: The platform split is pre-existing and preserved by this CL. Mac uses LayoutThemeMac::FocusRingColor() (native accent/focus behavior), while non-Mac uses the legacy renderer-pref path with dark-mode visibility handling. This CL only moves where the non-Mac dark override is applied; it does not attempt to unify Mac/non-Mac policy.
Helmut JanuschkaIsn't this patch basically a regression for non-mac platforms for the other callers of FocusRingColor? In other words, you've fixed PaintFocusRing, but broken FocusRingColor. Or do I misunderstand?
You are right that it changes all non-Mac dark-mode FocusRingColor() callers, not just PaintFocusRing(), and that is intentional: previously only outline:auto forced white while other paths still used #101010, causing inconsistent and often invisible focus rings. This also intentionally addresses https://issues.chromium.org/issues/489304736, where -webkit-focus-ring-color with non-auto outlines in dark mode stayed at #101010 on non-Mac.
This change will cause a regression where there are invisible focus rings for canvas in the following example:
```
<!DOCTYPE html>
<html>
<head>
<meta charset="utf-8" />
<title>Outline colour</title>
<style>
* {
outline-color: red;
}
div {
padding: 1rem;
margin: 1rem;
border: thin solid gold;
}
p {
color: red;
}
.dark {
color-scheme: dark;
}
.light {
color-scheme: light;
}
.system {
color-scheme: light dark;
}
</style>
</head>
<body>
<div class="dark">
<p>Dark colour scheme</p>
<button>Button</button>
<a href="#">Link</a>
<canvas id="darkc" width="100" height="100"><button id="darkcb">abc</button></canvas>
<script>
darkcb.focus();
var ctx = darkc.getContext('2d');
ctx.beginPath();
ctx.rect(10, 10, 50, 50);
ctx.drawFocusIfNeeded(darkcb);
</script>
</div>
<div class="light">
<p>Light colour scheme</p>
<button>Button</button>
<a href="#">Link</a>
<canvas id="lightc" width="100" height="100"><button id="lightcb">abc</button></canvas>
<script>
lightcb.focus();
var ctx = lightc.getContext('2d');
ctx.beginPath();
ctx.rect(10, 10, 50, 50);
ctx.drawFocusIfNeeded(lightcb);
</script>
</div>
<div class="system">
<p>System colour scheme</p>
<button>Button</button>
<a href="#">Link</a>
<canvas id="systemc" width="100" height="100"><button id="systemcb">abc</button></canvas>
<script>
systemcb.focus();
var ctx = systemc.getContext('2d');
ctx.beginPath();
ctx.rect(10, 10, 50, 50);
ctx.drawFocusIfNeeded(systemcb);
</script>
</div>
</body>
</html>
```
I think it would be best to revert these patches and think more about the changes.
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Helmut Januschka (Gerrit)
Helmut Januschka added 1 comment![]( "Open in Gerrit")
ohhh gosh, super sorry! and highly appreciate your feedback. instead of revert, would it be ok to land a fix on top? CL: https://chromium-review.googlesource.com/c/chromium/src/+/7646168
posted a screenshot of before/after on the bug.
that ultimatly unresolves issue: http://crbug.com/489304736 (but keeps the first one clean solved, and it might require some more targeted approach on css level itself.)
i am feeling confident that this should cleanly solve issue1, please let me know if you want me to revert (and reland basically the same thing) or proceed with the followup.
and again, sorry for breaking things, i will do better next time!
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Helmut Januschka (Gerrit)
Helmut Januschka has created a revert of this change![]( "Open in Gerrit")
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |