Fix tablist variants invisible under Prism (monochrome palette)#147
Conversation
…ePalette) Co-authored-by: jongalloway <68539+jongalloway@users.noreply.github.com> Agent-Logs-Url: https://github.com/jongalloway/DiagramForge/sessions/12c9ed1a-2f39-46e4-b841-f62269df8ebb
There was a problem hiding this comment.
Pull request overview
Fixes Conceptual tablist rendering under the Prism theme by ensuring tablist layouts use a chromatic palette when the theme palette is monochrome, and by adjusting gradient opt-out behavior. Adds E2E snapshot fixtures to lock in the corrected Prism output for all three tablist variants.
Changes:
- Update tablist layout variants (cards/stacked/flat) to use
ThemePaletteResolver.ResolveEffectivePalette(theme)instead of directly consumingtheme.NodePalette. - Make
render:noGradient=trueconditional based on whether a fallback palette was used. - Add three new Prism-focused E2E fixtures (+ baselines) for tablist cards/stacked/flat.
Reviewed changes
Copilot reviewed 4 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/DiagramForge/Layout/DefaultLayoutEngine.TabList.cs |
Switch tablist palette selection to effective palette; make gradient opt-out conditional. |
tests/DiagramForge.E2ETests/Fixtures/conceptual-tablist-cards-prism.input |
New Prism tablist “cards” fixture input. |
tests/DiagramForge.E2ETests/Fixtures/conceptual-tablist-cards-prism.expected.svg |
Baseline snapshot for Prism tablist “cards”. |
tests/DiagramForge.E2ETests/Fixtures/conceptual-tablist-stacked-prism.input |
New Prism tablist “stacked” fixture input. |
tests/DiagramForge.E2ETests/Fixtures/conceptual-tablist-stacked-prism.expected.svg |
Baseline snapshot for Prism tablist “stacked”. |
tests/DiagramForge.E2ETests/Fixtures/conceptual-tablist-flat-prism.input |
New Prism tablist “flat” fixture input. |
tests/DiagramForge.E2ETests/Fixtures/conceptual-tablist-flat-prism.expected.svg |
Baseline snapshot for Prism tablist “flat”. |
Comments suppressed due to low confidence (3)
src/DiagramForge/Layout/DefaultLayoutEngine.TabList.cs:357
- In the flat variant, the tablist renderer (AppendTabListFlatBand) always renders
stroke="none"for the accent line and title bar, and does not use the node-levelstrokevalue produced by AppendGradientDefs. Withrender:noGradientnow conditional, fallback palettes will cause per-node gradient<defs>to be emitted even though they can’t be used by this renderer, increasing SVG size unnecessarily. Consider keepingrender:noGradientfor the flat layout, or extending the flat renderer/metadata to apply the border gradient if that’s desired.
titleNode.Label.FontSize = titleFontSize;
titleNode.Label.FontWeight = "bold";
titleNode.Label.Color = titleBarTextColor;
if (!useFallbackPalette)
titleNode.Metadata["render:noGradient"] = true;
src/DiagramForge/Layout/DefaultLayoutEngine.TabList.cs:224
- ResolveEffectivePalette(theme) is now used even when theme.NodePalette is null/empty. Previously TabList used theme.NodeFillColor in that case; switching to a derived 8-color palette changes output for themes without an explicit palette. If that behavior change isn’t intended, keep the NodeFillColor fallback for empty palettes and reserve ResolveEffectivePalette for monochrome-but-present palettes (e.g., Prism).
var effectivePalette = ThemePaletteResolver.ResolveEffectivePalette(theme);
bool useFallbackPalette = !ReferenceEquals(effectivePalette, theme.NodePalette);
for (int i = 0; i < titleNodes.Count; i++)
{
var titleNode = titleNodes[i];
double rowY = pad + titleOffset + i * (bandHeight + rowGap);
string catFill = effectivePalette[i % effectivePalette.Count];
src/DiagramForge/Layout/DefaultLayoutEngine.TabList.cs:340
- ResolveEffectivePalette(theme) is now used even when theme.NodePalette is null/empty. That removes the previous single-color fallback to theme.NodeFillColor, and can change TabList styling for themes that intentionally omit NodePalette. Consider keeping the NodeFillColor fallback for empty palettes, and using ResolveEffectivePalette only when an explicitly provided palette is monochrome/unusable.
var effectivePalette = ThemePaletteResolver.ResolveEffectivePalette(theme);
bool useFallbackPalette = !ReferenceEquals(effectivePalette, theme.NodePalette);
double curY = pad + titleOffset;
for (int i = 0; i < titleNodes.Count; i++)
{
var titleNode = titleNodes[i];
int catIdx = GetMetadataInt(titleNode.Metadata, "tablist:categoryIndex");
var items = itemsByCategory.TryGetValue(catIdx, out var list) ? list : [];
int itemCount = items.Count;
string catFill = effectivePalette[i % effectivePalette.Count];
string titleBarFill = ColorUtils.Vibrant(catFill, 2.0);
| var effectivePalette = ThemePaletteResolver.ResolveEffectivePalette(theme); | ||
| bool useFallbackPalette = !ReferenceEquals(effectivePalette, theme.NodePalette); | ||
|
|
||
| for (int i = 0; i < titleNodes.Count; i++) | ||
| { | ||
| var titleNode = titleNodes[i]; | ||
| double rowY = pad + titleOffset + i * (cardHeight + rowGap); | ||
| string catFill = palette[i % palette.Length]; | ||
| string catFill = effectivePalette[i % effectivePalette.Count]; |
There was a problem hiding this comment.
ResolveEffectivePalette(theme) is now used even when theme.NodePalette is null/empty. That changes TabList behavior from the previous fallback of using theme.NodeFillColor for all categories, which can surprise custom themes that intentionally rely on NodeFillColor when no palette is provided. Consider preserving the prior behavior for the null/empty palette case (use NodeFillColor), and only invoking ResolveEffectivePalette when NodePalette is present but monochrome (or otherwise deemed unusable).
This issue also appears in the following locations of the same file:
- line 217
- line 327
There was a problem hiding this comment.
Fixed in 272b1f6. All three variants now only call ResolveEffectivePalette when NodePalette is present (non-null, non-empty) — if no palette is set, they fall back to [theme.NodeFillColor] exactly as before, preserving behavior for custom themes that rely on that path. Additionally, render:noGradient=true is now unconditional for stacked and flat since their band renderers use tablist:tabStroke/contentStroke/accentColor metadata directly and never consume the node-level gradient stroke; the conditional remains only on cards, which does use fill/stroke in its accent rect.
| if (!useFallbackPalette) | ||
| titleNode.Metadata["render:noGradient"] = true; |
There was a problem hiding this comment.
In the stacked variant, the tablist renderer (AppendTabListStackedBand) does not use the node-level stroke value produced by AppendGradientDefs; it uses tablist:tabStroke / tablist:contentStroke instead. Making render:noGradient conditional here will therefore emit per-node <defs> (and compute gradients) that are unused in the final SVG for fallback palettes. Consider either keeping render:noGradient enabled for the stacked layout, or updating the stacked renderer/metadata to actually apply the gradient stroke when that’s the intent.
This issue also appears on line 353 of the same file.
| if (!useFallbackPalette) | |
| titleNode.Metadata["render:noGradient"] = true; | |
| titleNode.Metadata["render:noGradient"] = true; |
|
@copilot apply changes based on the comments in this thread |
…ent for stacked/flat Co-authored-by: jongalloway <68539+jongalloway@users.noreply.github.com> Agent-Logs-Url: https://github.com/jongalloway/DiagramForge/sessions/aa6d6311-d110-4334-812e-299addc9387b
These changes were already applied in 272b1f6 — |
All three tablist variants (cards, stacked, flat) rendered as white-on-white under Prism because
ColorUtils.Vibrant(#FFFFFF, n)returns#FFFFFFunchanged, andrender:noGradient=truewas applied unconditionally, suppressing the border-gradient fallback.Changes
DefaultLayoutEngine.TabList.cs— all three layout methods (LayoutTabListCards,LayoutTabListStacked,LayoutTabListFlat):ResolveEffectivePaletteis only invoked whenNodePaletteis present (non-null, non-empty). When no palette is configured, the original[theme.NodeFillColor]fallback is preserved, keeping prior behavior for custom themes that rely on it.NodePaletteis present but monochrome (e.g. Prism),ResolveEffectivePaletteprovides a chromatic fallback derived fromBorderGradientStopsor hue rotation.render:noGradient=trueis kept unconditional for stacked and flat — their band renderers consumetablist:tabStroke/contentStroke/accentColormetadata directly and never use the node-level gradient stroke, so gradient generation would be wasteful.render:noGradient=trueremains conditional for cards — its accent rect usesfillandstrokedirectly, so the gradient border is genuinely applied and visible.conceptual-tablist-cards-prism,conceptual-tablist-stacked-prism,conceptual-tablist-flat-prismwith generated baselines showing chromatic fills derived from Prism's gradient stops🔒 GitHub Advanced Security automatically protects Copilot coding agent pull requests. You can protect all pull requests by enabling Advanced Security for your repositories. Learn more about Advanced Security.