Skip to content

Text on an accent fill gets a measured ink in every theme, and the FinOps row marks become theme brushes Dark can actually see (#3577, bug arm) - #3589

Merged
erikdarlingdata merged 1 commit into
devfrom
fix/3577-lane
Sep 18, 2026
Merged

erikdarlingdata merged 1 commit into
devfrom
fix/3577-lane

Conversation

@erikdarlingdata

Copy link
Copy Markdown
Owner

Arm (A) of #3577 — the two contrast defects the report names, plus the sweep of the same defect class in the same themes. Arm (B) — user-maintainable colors with a reset to default — is out of scope for this PR pending the maintainer's ruling, so no Closes keyword; the issue stays open.

Everything here is measured, not eyeballed: WCAG 2.x relative-luminance contrast, computed from the shipped hex values (script below). Targets are AA — ≥ 4.5:1 for text, ≥ 3:1 for a non-text marker. Screenshots are impossible from this lane (no Windows); verification is the measured ratios plus a clean build of every affected project.

Defect 1 — Cool Breeze, selected tab title (reported)

The selected tab read 2.71:1 (#1A2A3A on the #1E6FA8 accent) — and not because the theme chose that. Every TabItem style sets a light Foreground in its IsSelected trigger (#EEF4FA on Cool Breeze). It never applied. A string header becomes a TextBlock, and the themes' app-level implicit TextBlock style out-ranks the Foreground that TextBlock would inherit from the TabItem: WPF looks an implicit style up past the template boundary into Application.Resources (FrameworkElement.FindImplicitStyleResource → FindResourceInTree stops at the templated parent, then continues into the app dictionary), and a style setter beats an inherited value. So the header always rendered ForegroundBrush on the accent fill. The repo already knew this shape — RecommendationsTab.xaml and ExcludedDatabasesDialog.xaml both carry "Foreground is restated because the themes' implicit TextBlock style would otherwise win" — it just hadn't been traced to the tab header.

The same dead setter is why Dark's selected tab is 1.99:1 (#E4E6EB on #2eaef1 — you can see it in the README's plan-viewer screenshot, the "Actual Plan – QS 42087" tab), and why Light's trigger literal, a light #E4E6EB, never turned Light's selected tab light-on-cyan.

Fix (mechanism): an empty implicit <Style TargetType="TextBlock"/> inside the header ContentPresenter.Resources. It is found first in the lookup and shadows the app-level one inside the header only, so the header text inherits the TabItem's Foreground the way stock WPF intends. Explicit Foregrounds on custom header elements (the alert badge's white) are local values and still win. The base Foreground setter moves from ForegroundDimBrush to ForegroundBrush so the unselected tab looks exactly as it always has (Dim never applied either).

Fix (values): a new per-theme AccentForegroundColor / AccentForegroundBrush — the ink for text or glyphs on any accent fill:

Theme Ink on Accent on AccentHover on AccentPressed
Cool Breeze #FFFFFF (already the grid-selection ink) 5.39 4.56 (see hover note) 7.34
Dark #111217 (the darkest surface tone) 7.52 9.49 6.01
Light #1A1D23 (= ForegroundColor, no visual change) 6.79 8.57 5.42

Cool Breeze's AccentHoverColor moves #2B87C8 → #267BB8: no ink passed on the old shade (white 3.89, page text 3.76) and it sits under text on a selected+hovered tab, a highlighted combo item and a hovered accent button. The new shade is still lighter than the #1E6FA8 rest state (ΔE 5.1 vs 10 before — a subtler hover cue; the trade is stated in the XAML comment).

Defect 2 — Dark, "Mark To Do / Done / Do Not Do" rows (reported)

The marks are not theme brushes at all: DataGridRowMarks (PerformanceMonitor.Ui) painted three fixed 20%-alpha overlays — #22C55E, #D97706, #DC2626 — as the row background. On Dark's #111217 rows they composite to #143625 / #392614 / #3A161A: 1.41 / 1.30 / 1.17:1 against the unmarked row. On a dark panel that is "bad visible", exactly as reported. On the light themes the same overlays produce pastel tints that read fine (and the report doesn't mention them), so this is a per-theme problem with a per-theme answer.

Fix: three theme keys — RowMarkDoneBrush, RowMarkToDoBrush, RowMarkDoNotBrush — in every dictionary. Light and Cool Breeze carry the #2645 originals unchanged (Color="#22C55E" Opacity="0.2" reproduces #3322C55E exactly). Dark builds its own from the theme's SuccessColor / WarningColor / ErrorColor, at the opacity that pushes the mark as far from the row as possible while the row's own text stays ≥ 4.5:1 on both row backgrounds (#111217 primary / #1c1f25 alternating, #E4E6EB text) — that constraint, not the 3:1 marker target, is the binding one; one step brighter and the row's values fail AA:

Mark Before (composite) vs row / alt After Composite vs row / alt Row text
Done #143625 1.41 / 1.44 SuccessColor @ .47 #46674A 2.94 / 2.87 5.10 / 4.61
To Do #392614 1.30 / 1.33 WarningColor @ .39 #6E5E2D 2.94 / 2.92 5.09 / 4.53
Do Not #3A161A 1.17 / 1.15 ErrorColor @ .62 #944E50 3.09 / 2.93 4.85 / 4.52

The marker lands at 2.9–3.1:1 rather than a clean 3.0 everywhere because the two constraints leave a band about 0.02 luminance wide; CIE76 ΔE from the row goes 21–25 → 44–49.

DataGridRowMarks.Apply now paints by SetResourceReference (so a marked row follows a theme switch instead of holding the palette it was painted under; still a local value, so precedence over the row style's selection/hover triggers is unchanged) and falls back to the original literals only when the key is absent from the host's resources — a mark can never silently paint nothing. Lite.Tests/DataGridRowMarkTests.EveryThemeDefinesTheBrushKeysTheMarksPaintWith pins the three keys in all six Lite/Darling theme files, spelled as the code looks them up. This is the one load-bearing edit outside the theme dictionaries.

Sweep — the same defect class in the same themes

Cool Breeze, every selected/hover pair in the theme dictionary (text on background; ✗ = below 4.5):

Surface Before After
TabItem selected #1A2A3A on #1E6FA8 2.71 ✗ #FFFFFF on #1E6FA8 5.39
TabItem selected + hover on #2B87C8 3.76 ✗ on #267BB8 4.56
ComboBoxItem selected / highlighted 2.71 ✗ / 3.76 ✗ 5.39 / 4.56
AccentButton rest / hover / pressed 2.71 ✗ / 3.76 ✗ / 1.99 ✗ 5.39 / 4.56 / 7.34
Calendar selected day / month 2.71 ✗ 5.39
TabCloseButton × on an unselected tab hard-coded White on #EEF4FA 1.11 ✗ (invisible) inherits header ink: #1A2A3A 13.20
DataGrid selected row (cell ink) #FFFFFF on accent 5.39 unchanged (now via the shared key)
TabItem unselected / hover 13.20 / 9.24 unchanged
Sub-tab selected text; underline vs window 11.53; 4.25 (non-text) unchanged
ListViewItem selected / hover 9.24 / 13.20 unchanged
MenuItem highlighted text 9.24 unchanged (highlight-vs-menu 1.43 is a hover cue, not text — noted)
FilteredColumnHeader 7.58 unchanged
TextBox selection (accent @ .4 over well) 6.43 unchanged
Base Button pressed (AccentPressed under page text) 1.99 ✗ left as-is — transient mouse-down state; the base template is under every button and I did not want that blast radius here

Dark, semantic marker brushes vs the row backgrounds (#111217 / #1c1f25; text threshold 4.5): all pass — SuccessBrush 9.30 / 8.20, WarningBrush 13.26 / 11.70, ErrorBrush 6.26 / 5.53, InfoBrush 7.52 / 6.64, WarningTextBrush 10.50 / 9.27, ApexBorderBrush 7.14 / 6.30, DeadlockVictimBrush 6.18 / 5.45, DeadlockEdgeBrush 6.04 / 5.33, ForegroundMutedBrush 8.40 / 7.41, ForegroundDimBrush 11.51 / 10.16, filtered-header text on #3D5A80 5.66. The row marks were the only failing markers.

Dark, the text-on-accent class — the same pair the report named for Cool Breeze, failing worse here, and unavoidable once the tab template is fixed (the Dark trigger value had to be chosen anyway):

Surface Before After
TabItem selected / selected+hover #E4E6EB on #2eaef1 1.99 ✗ / on #5bc4f5 1.58 ✗ #111217: 7.52 / 9.49
ComboBoxItem selected / highlighted 1.99 ✗ / 1.58 ✗ 7.52 / 9.49
AccentButton rest / hover / pressed 1.99 ✗ / 1.58 ✗ / 2.49 ✗ 7.52 / 9.49 / 6.01
Calendar selected day / month 1.99 ✗ 7.52
DataGrid selected row (GridSelectionForegroundBrush, DataGridCell trigger, SystemColors.Highlight*TextBrushKey) 1.99 ✗ 7.52
TabCloseButton × on selected / unselected tab White 2.49 ✗ / 15.36 7.52 / 12.30
Base Button pressed 2.49 ✗ left as-is (same reason as above)

⚠️ The Dark grid-selection row is the most visible change in this PR — the default theme's selected row goes from near-white-on-cyan to near-black-on-cyan. It is the same pair as the tab at the same 1.99:1, and leaving it would have put two different inks on the same accent in one theme. If you'd rather keep the old look there, it is three keys per Dark copy (GridSelectionForegroundBrush, the two Highlight*TextBrushKeys) plus the DataGridCell trigger; everything else stands on its own.

Light: every pair above already passed (its page text is dark and its accent is light); the template mechanism is applied there too for parity, and the ink key equals its existing values, so no visual change on Light — except the plan-tab × on an unselected white tab, which was invisible and now isn't.

Parity ports

  • Darling viewer: Darling/PerformanceMonitor.Darling.Viewer/Themes/{Dark,Light,CoolBreeze}Theme.xaml are 1:1 with Lite's (the same generator applied to both; diff shows only Darling's pre-existing viewer-only block). The viewer's per-server sub-tabs, plan tabs, accent buttons, combo dropdowns and calendars use these implicit styles; its main tabs (DarkTabItem, ViewerSelectionBrush) were already fine and are untouched. ThemeParityLiteDarlingTests intersection: 69 shared keys per theme, 0 mismatches; ThemeCompletenessTests key sets equal (90 Lite / 98 Darling).
  • deprecated/Dashboard: carries the identical TabItem/ComboBoxItem/AccentButton/Calendar/grid-selection/TabCloseButton defects and the same AccentHoverColor; the same fix lands in its three dictionaries in its idiom (its DataGridCell trigger carried the same literal and takes the same key; its grid-selection block sits earlier in the file, which is why the generator was run against it separately). No row marks there (it has none). Dashboard.Tests/ThemeParityTests (Dashboard↔Lite): 66 shared keys per theme, 0 mismatches — a nudged AccentHoverColor in Lite alone would have failed it, since PerformanceMonitor.Ui trips the core path filter that runs it.

Verification

  • dotnet build -c Release -p:EnableWindowsTargeting=true on Lite, Lite.Tests, Darling.Tests, the Darling viewer, Dashboard and Dashboard.Tests: 0 warnings, 0 errors each (the markup compiler accepted ContentPresenter.Resources and Opacity on the brushes; BAML regenerated).
  • The test executables need Microsoft.WindowsDesktop.App and cannot run here. The text-parsing assertions of ThemeCompletenessTests (both apps), ThemeParityLiteDarlingTests, Dashboard's ThemeParityTests, XamlStaticResourceHygieneTests (every {StaticResource} in each theme file resolves in that file, and every new key is defined before first use) and the new key pin were replicated line-for-line in Python against the working tree: all green. CI runs the real ones.
  • Contrast script (WCAG relative luminance; sRGB alpha composite for the marks): python3 contrast.py '#1A2A3A' '#1E6FA8' → 2.71:1. Happy to check it in under tools/ if wanted; it is a 20-line script and I kept it out of the tree to keep the diff to the defect.

Out-of-lane findings (reported, not fixed)

  • SuccessButton (all three themes) puts a light literal on SuccessBrush — 1.9:1 on Dark — but has zero references in Lite or Darling. Dead style; left alone.
  • The base Button IsPressed state (accent-pressed fill under page text) fails in Cool Breeze (1.99) and Dark (2.49) for the duration of a mouse-down. Fixing it means touching the template under every button in the app; flagged rather than folded in.
  • Row marks are conveyed by color alone (no glyph/text), which is a WCAG 1.4.1 matter independent of contrast — a product-scope question, adjacent to arm (B).
  • Explicit Foreground="{DynamicResource ForegroundBrush}" on a cell (severity columns, DataGridCellText-less columns) on a selected accent row stays page-text-on-accent; DataGridCellText handles the common case and nothing in this PR makes any such cell worse.

CHANGELOG entry handed to the coordinator separately; CHANGELOG.md is untouched here.

…nOps row marks become theme brushes Dark can actually see (#3577)

Two contrast defects from a community report, both measured rather than
eyeballed (WCAG relative-luminance contrast; AA wants 4.5:1 for text, 3:1
for a non-text marker).

The selected tab on Cool Breeze read 2.71:1 - and not because the theme
chose that. Every TabItem style set a light Foreground for the selected
state, but a string header becomes a TextBlock, and the themes' app-level
implicit TextBlock style out-ranks the Foreground that TextBlock inherits
from the TabItem, so the setter never reached the text: the header always
rendered ForegroundBrush on the accent. The same dead setter meant Dark's
selected tab was 1.99:1 (visible in the README's plan-viewer screenshot)
and Light's would have been light-on-cyan had it ever applied. An empty
implicit TextBlock style inside the header presenter shadows the app-level
one, so the header inherits the TabItem's Foreground the way stock WPF
intends, and a new per-theme AccentForegroundColor/Brush is the ink for
text on any accent fill: white on Cool Breeze (5.39:1), the darkest
surface tone on Dark (7.52:1), the page text on Light (6.79:1, no change).
The same ForegroundBrush-on-accent pair recurred on the highlighted combo
item, the accent button, the selected calendar day and Dark's grid
selection (all 1.99:1 on Dark, 2.71:1 on Cool Breeze); they take the same
ink through the same key. Cool Breeze's AccentHoverColor moves one step
toward the accent (#2B87C8 -> #267BB8) because no ink passed on the old
shade (white 3.89:1); white is 4.56:1 on the new one. TabCloseButton drops
its hard-coded White so the x inherits the header ink - it was invisible on
the light themes' unselected tabs.

The FinOps "Mark Done / To Do / Do Not Do" tints were three fixed 20%
overlays in DataGridRowMarks, which composite to 1.2-1.4:1 against Dark's
near-black rows - the operator could not find the rows they had marked.
They are theme brushes now (RowMarkDoneBrush / RowMarkToDoBrush /
RowMarkDoNotBrush, painted by resource reference so a marked row follows a
theme switch). Light and Cool Breeze keep the shipped tints; Dark builds
its own from the theme's Success / Warning / Error colors at the opacity
that pushes the mark as far from the row as it can go while the row text
holds >= 4.5:1 on both row backgrounds (marks 2.9-3.1:1, text 4.5-5.1:1).
The literals stay as the fallback for a host without the keys, and a test
pins the keys in every theme file of both apps.

Darling viewer theme copies ported 1:1; the deprecated Dashboard twin
carries the identical tab/accent defect and gets the same fix (no marks
there - it has none). Arm (B) of the report - user-maintainable colors
with reset - is not in this change.
@erikdarlingdata
erikdarlingdata enabled auto-merge (squash) September 18, 2026 15:20

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — reviewed the WCAG contrast fixes and DataGridRowMarks resource-key refactor for correctness, Lite/Darling parity, and the theme-key fallback path. DataGridRowMarks.Apply's null/lookup branching is sound (ClearValue vs SetResourceReference vs literal fallback), the empty implicit TextBlock style shadowing trick checks out against the app-level implicit style it's meant to shadow (confirmed it really does set Foreground in each theme file), and diff'ing Lite vs Darling theme files post-patch confirms they're still 1:1 aside from the pre-existing viewer-only block. Dashboard correctly has no row-mark keys since it never calls into DataGridRowMarks. No SQL touched, so the T-SQL style rules don't apply here.

@erikdarlingdata
erikdarlingdata merged commit 91029fb into dev Sep 18, 2026
8 checks passed
@erikdarlingdata
erikdarlingdata deleted the fix/3577-lane branch September 18, 2026 15:29
erikdarlingdata added a commit that referenced this pull request Sep 18, 2026
…m Settings, with a measured contrast readout and a reset to default (#3577, arm B)

Arm (B) of #3577. Arm (A) (#3589) fixed the two contrast defects the
report named; this is the request itself: user-maintainable colors with
a reset to default, in Lite and the Darling viewer.

- A per-user theme-overrides.json (Lite: beside settings.json; the
  viewer: beside viewer-settings.json, local to the machine), keyed by
  theme then by palette color, holding only what the operator changed.
- A closed list of twelve of the eighteen palette colors, declared once
  in PerformanceMonitor.Ui (ThemeColorOverrides.Slots) with the reason
  the other six are withheld; pinned in Lite.Tests.
- ThemeManager regenerates a theme that carries overrides from its
  embedded XAML text (ThemeXamlRewriter) instead of merging an override
  dictionary, because the theme's own brushes and every style setter
  resolve their {StaticResource} at load and a merged dictionary cannot
  reach them. The compiled BAML stays the path for a theme without
  overrides. A bad file, key, value or parse falls back to the stock
  theme with a logged warning; nothing here can take the app down.
- A Colors section (shared ThemeColorsPanel) under each Settings
  window's theme combo: swatch, hex, HSV picker, and the WCAG ratio
  against the surface each color renders on, colour-coded 4.5 / 3.
  Warns, never refuses. Apply / Reset to default / Open file; a
  FileSystemWatcher re-applies outside edits.
- WcagContrast brings arm (A)'s out-of-tree script into the Ui project;
  the tests pin it to the numbers #3589 published.
- Docs: Lite README "Themes and colors", a paragraph in the Darling
  README's viewer section. No Dashboard port (frozen twin).
erikdarlingdata added a commit that referenced this pull request Sep 18, 2026
…m Settings, with a measured contrast readout and a reset to default (#3577, arm B) (#3606)

* The operator can maintain the twelve palette colors of each theme from Settings, with a measured contrast readout and a reset to default (#3577, arm B)

Arm (B) of #3577. Arm (A) (#3589) fixed the two contrast defects the
report named; this is the request itself: user-maintainable colors with
a reset to default, in Lite and the Darling viewer.

- A per-user theme-overrides.json (Lite: beside settings.json; the
  viewer: beside viewer-settings.json, local to the machine), keyed by
  theme then by palette color, holding only what the operator changed.
- A closed list of twelve of the eighteen palette colors, declared once
  in PerformanceMonitor.Ui (ThemeColorOverrides.Slots) with the reason
  the other six are withheld; pinned in Lite.Tests.
- ThemeManager regenerates a theme that carries overrides from its
  embedded XAML text (ThemeXamlRewriter) instead of merging an override
  dictionary, because the theme's own brushes and every style setter
  resolve their {StaticResource} at load and a merged dictionary cannot
  reach them. The compiled BAML stays the path for a theme without
  overrides. A bad file, key, value or parse falls back to the stock
  theme with a logged warning; nothing here can take the app down.
- A Colors section (shared ThemeColorsPanel) under each Settings
  window's theme combo: swatch, hex, HSV picker, and the WCAG ratio
  against the surface each color renders on, colour-coded 4.5 / 3.
  Warns, never refuses. Apply / Reset to default / Open file; a
  FileSystemWatcher re-applies outside edits.
- WcagContrast brings arm (A)'s out-of-tree script into the Ui project;
  the tests pin it to the numbers #3589 published.
- Docs: Lite README "Themes and colors", a paragraph in the Darling
  README's viewer section. No Dashboard port (frozen twin).

* The watcher's re-read of theme-overrides.json is a log line on any failure, never a throw (#3577 review)

Review on #3606: ReloadFromDiskIfChanged caught only IOException, and it
runs from a DispatcherTimer.Tick on the UI thread with no caller above
it - an UnauthorizedAccessException from an ACL change, an AV scan or a
sync client's lock would have crashed the app over a file watcher,
against the class header's own contract.

- The read is behind ThemeManager.OverridesFileReader (File.ReadAllText
  in the app); IOException still retries up to MaxReloadRetries, and
  anything else - or retries exhausted - is LogWarning + the theme stays
  as it was. The method returns what it did (ReloadOutcome).
- The Tick handler is the last line of defence: a broad catch around the
  whole reload, for whatever nobody anticipated in the rest of the path.
- Pinned in Lite.Tests/ThemeColorOverrideTests: UnauthorizedAccess ->
  Failed, one warning naming it, loaded overrides untouched; IOException
  retries to the cap then warns; unchanged text is a no-op; no file
  configured is a no-op.

* The regeneration test reads its brushes on the STA thread that built them (#3577 CI)

First CI run: all six RegeneratingATheme_MovesEveryBrushTheXamlDerivesFromAnOverriddenColor
cases failed reading SolidColorBrush.Color from the runner's thread - the
parsed brushes are unfrozen DispatcherObjects owned by the STA thread.
The product regeneration itself parsed cleanly with no warnings on every
theme; the test now extracts plain Color structs inside the STA body and
asserts on those. No product change.
erikdarlingdata added a commit that referenced this pull request Sep 18, 2026
…wn page, and text on a status fill gets a measured ink in every theme (#3609) (#3627)

Light shipped WarningColor #F57F17 at 2.47:1 and InfoColor #2eaef1 (the
accent itself) at 2.32:1 against BackgroundColor; Cool Breeze's
WarningColor was 2.09:1. All three sit under the 3:1 non-text floor the
new Settings contrast readout (#3606) paints red, and WarningBrush is
TEXT in the viewer (fleet counts, seat state, coverage note), so the bar
was 4.5:1, not 3:1.

- Light WarningColor -> #AE4F08 (4.99 page / 5.35 card / 4.77 well /
  4.72 alt row), InfoColor -> #0369A1 (5.53 / 5.93 / 5.29 / 5.23; same
  201-degree hue as the accent, decoupled from it). Cool Breeze
  WarningColor -> #9E4A0B (4.82 / 5.51; 4.41 / 4.08 where it is only a
  marker). Dark untouched.
- No amber clears 4.5:1 on Light's page AND under the #1A1A1A the
  Recommendations badge painted on it (the band is empty), so the ink is
  a theme key, StatusForegroundBrush, the way #3589 gave the accent
  AccentForegroundColor: white on the light themes, #111217 on Dark. A
  brush with a literal, not a nineteenth <Color>, so the eighteen-color
  palette pins in ThemeColorOverrideTests stand. The Recommendations
  badge (Lite, viewer, Dashboard) and the viewer's alert / attention
  badges take it; the latter were hard-coded White, 1.41:1 on Dark's
  amber.
- Values and the ink key mirrored to the Darling viewer and the
  deprecated Dashboard so ThemeParityLiteDarlingTests and the Dashboard
  ThemeParityTests hold (70 and 67 shared keys, 0 mismatches).
- Lite.Tests/ThemeStatusContrastTests pins every theme's four status
  colors at 3:1 on both page surfaces, Warning/Info at 4.5:1, the ink at
  4.5:1 on every status fill, and the three consumers on the key. Text
  parse + WcagContrast, no WPF object; executed locally, fails on dev.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant