MarkdownTextBlock: ThemeResource support, DP migration & performance fixes

[niels9001]

MarkdownTextBlock: ThemeResource support, DP migration & performance fixes

Summary

This PR modernizes the MarkdownTextBlock control's theming architecture and fixes several performance/memory issues. The control now supports proper {ThemeResource} brushes that update automatically on Light/Dark/HighContrast theme switches — previously, theme changes had no effect on markdown styling.

Addresses: #611

---

What changed

Theming & DependencyProperty migration

Previously, all styling lived in plain C# classes (MarkdownThemes with auto-properties, wrapped by MarkdownConfig). This meant {ThemeResource} couldn't target them, and theme switches (Light ↔ Dark) had no effect on markdown brush colors.

New architecture: All 67+ styling properties are now DependencyProperties directly on MarkdownTextBlock. The intermediary MarkdownThemes and MarkdownConfig classes are deleted entirely. TextElement renderers (MyHeading, MyCodeBlock, MyQuote, MyTable, etc.) receive the MarkdownTextBlock control reference and read DPs directly from it at render time.

<controls:MarkdownTextBlock
    H1Foreground="{ThemeResource MyCustomBrush}"
    CodeBlockBackground="{StaticResource CodeBg}"
    InlineCodeCornerRadius="4" />

DPs added to MarkdownTextBlock.Properties.cs

Category	Properties
Headings (18)	H1–H6 × FontSize, Foreground, FontWeight, Margin
Inline Code (7)	InlineCodeBackground, InlineCodeForeground, InlineCodeBorderBrush, InlineCodeBorderThickness, InlineCodeCornerRadius, InlineCodePadding, InlineCodeFontSize, InlineCodeFontWeight
Code Blocks (9)	CodeBlockBackground, CodeBlockForeground, CodeBlockBorderBrush, CodeBlockBorderThickness, CodeBlockPadding, CodeBlockMargin, CodeBlockFontFamily, CodeBlockCornerRadius
Quotes (8)	QuoteBackground, QuoteForeground, QuoteBorderBrush, QuoteBorderThickness, QuoteMargin, QuotePadding, QuoteBarMargin, QuoteCornerRadius
Tables (6)	TableHeadingBackground, TableBorderBrush, TableBorderThickness, TableCellPadding, TableMargin, TableCornerRadius
Horizontal Rule (4)	HorizontalRuleBrush, HorizontalRuleThickness, HorizontalRuleMargin
Links (1)	LinkForeground
Images (3)	ImageMaxWidth, ImageMaxHeight, ImageStretch
Paragraphs/Lists (4)	ParagraphMargin, ParagraphLineHeight, ListBulletSpacing, ListGutterWidth
Bold (1)	BoldFontWeight
Config (3)	BaseUrl, ImageProvider, SVGRenderer (previously on MarkdownConfig)

Default style (MarkdownTextBlock.xaml)

• ThemeDictionaries with Default, Light, and HighContrast dictionaries define all brush resources (e.g., MarkdownTextBlockH1Foreground → TextFillColorPrimaryBrush). HighContrast uses SystemColorButtonTextColorBrush, SystemColorHotlightColorBrush, etc.
• Non-brush values (font sizes, margins, thicknesses, corner radii, font weights) are set as literal values in style setters — not as named resources.
• Follows the same pattern as SettingsControls in this repo.

Rendering architecture changes

• WinUIRenderer — Removed Config property. Constructor now takes (MyFlowDocument, MarkdownTextBlock). Already had a MarkdownTextBlock property.
• All 11 TextElement classes — Constructors changed from MarkdownThemes/MarkdownConfig parameter to MarkdownTextBlock. Each reads DPs directly (e.g., _control.H1FontSize, _control.CodeBlockBackground).
• All 8 ObjectRenderers + HtmlWriter — Pass renderer.MarkdownTextBlock instead of renderer.Config.Themes.
• Batched re-render — All theme DPs share a single OnThemePropertyChanged callback that uses DispatcherQueue.TryEnqueue with a boolean guard to coalesce multiple simultaneous DP changes (e.g., 20+ brush updates on theme switch) into one re-render.
• ActualThemeChanged handler as belt-and-suspenders for theme switching, using the same batching guard.

Performance & memory fixes

1. Fixed HttpClient socket leak (MyImage.cs)
   Each image load created new HttpClient(), never disposed. HttpClient maintains connection pools internally, so per-instance creation leaks socket handles. Replaced with a static readonly HttpClient shared across all image loads.

2. Fixed Image.Loaded event handler leak (MyImage.cs)
   _image.Loaded += LoadImage was never unsubscribed. On every theme-change re-render, the old MyImage objects remained rooted via the event handler delegate, preventing garbage collection. Now unsubscribes immediately at the top of the handler.

3. Fixed ActualThemeChanged event leak (MarkdownTextBlock.xaml.cs)
   The event was subscribed in the constructor but never unsubscribed. Moved to Loaded/Unloaded lifecycle handlers, matching the pattern used by the old CommunityToolkit MarkdownTextBlock control.

4. Eliminated per-image DefaultSVGRenderer allocation (MyImage.cs)
   Every image without a custom SVG renderer created new DefaultSVGRenderer(). Since it's stateless, replaced with a static readonly singleton.

5. Fixed double-scan on HtmlNodeCollection (HtmlWriter.cs)
   node.ChildNodes.Remove(node.ChildNodes.FirstOrDefault(...)) scanned the collection twice — once to find the node, once to remove it. Refactored to a single FirstOrDefault + null-check + Remove.

6. Improved text buffer growth strategy (WinUIRenderer.cs)
   When text exceeded the 1024-char buffer, the entire buffer was replaced with an exact-fit ToCharArray() — likely too small for the next call, causing repeated reallocations. Now uses a doubling strategy (Math.Max(length, buffer.Length * 2)).

Sample updates

• Example sample simplified — no longer needs Config binding; defaults come from the XAML style automatically.
• Custom Theme sample updated to bind directly to control DPs via x:Bind function bindings.
• "Apply Changes" button removed from theme options — changes are now live via bindings.

---

Breaking changes

• MarkdownConfig and MarkdownThemes classes are deleted. Users who were setting Config should set properties directly on the control:

  - markdown.Config = new MarkdownConfig { Themes = new MarkdownThemes { H1FontSize = 32 } };
  + markdown.H1FontSize = 32;

• BaseUrl, ImageProvider, SVGRenderer are now DPs on MarkdownTextBlock (previously on MarkdownConfig):

  - markdown.Config = new MarkdownConfig { ImageProvider = myProvider };
  + markdown.ImageProvider = myProvider;

Testing

• XAML compiler: zero errors across all target frameworks
• Existing ImageProviderConstraintTest updated to use new DP API
• Theme switching verified: Light ↔ Dark ↔ HighContrast all update brushes correctly

[niels9001]

@Arlodotexe @michael-hawker can we get this reviewed? We are still blocked by the textblocks not changing colors..

[Lorenz5600]

Yes, a review would be great. We're also blocked by unreadable hint headers.

[niels9001]

@Arlodotexe this PR has been pending for 4 months now.. When can we expect a review?

[Arlodotexe]

@Arlodotexe this PR has been pending for 4 months now.. When can we expect a review?

Thanks for bumping @niels9001. Organizing has been chaotic, especially with Michael leaving. I'm going through and reviewing ALL PRs with deliberate priority orientation-- this was on the list, but thanks to your ping and existing priority criteria it's now second to top of my agenda now (just behind repo-wide CI fixes).

I'll be attending this until we've closed it. Expect a review by today or tomorrow.

[Arlodotexe]

Great PR @niels9001, many much-needed quality improvements here!

Reviewed with a fine-tooth comb, took me approx. ~6h 15m from first source file to all changed files reviewed.

11 review comments added. Some to address or simply clarify before closing, some to defer to later.

Reviewed in order of source, then samples, then tests. All other changes not commented on were validated for internal consistency and overall correctness.

To top it off, I've visually validated via a local temp sample on both WASDK and UWP that all new style assignments and DPs were referencing valid and usable brushes in both Light/Dark themes at sample-control-level ThemeResource swap (not system, see "Default" vs "Dark" review comments):

Human/AI attribution & disclosure:

• All file-by-file review and validation (structural, functional, design, performance) was done 100% by human hand based on:
  • Direct diff reading
  • MSLearn docs checks
  • Personal and community-based knowledge sourcing
  • AI-delegated local/repro validation (see below).
• AI agent assistance was used:
  • To scaffold/verify throwaway UWP and WinUI 3 <StaticResource ...> repros.
  • At the end to surface and attend to deferred review items from the area/task log.
  • To scaffold the focused theme-resource coverage sample (screenshots above)

Full verbatim area/task log available on request.

[Arlodotexe]

• This seems to be the whole "shared callback batches multiple simultaneous updates" setup.
  • I'm not convinced that this is doing what we expect it to.
  • Let me logically walk through the code:
    • Style change happens
    • Multiple properties are changed at the same time
    • DP callbacks don't fire perfectly concurrently
    • OnThemePropertyChanged is called multiple times, overlapping and sequentially
    • The very first entry sets self._themePropertyChangeQueued = true;
    • The second/third/etc method entry is dropped due to the if gate on _themePropertyChangeQueued needing to be false
    • The first method entry continues on the DispatcherQueue and unsets _themePropertyChangeQueued to false
    • ApplyText is called on the dispatcher queue, still caused by the first entry.
  • This looks like a clear issue to me:
    • Since the first method entry is the only one that can call ApplyText, it may finish before all DP updates can happen if DP callbacks aren't perfectly concurrent, meaning it may also call ApplyText multiple times while flipping _themePropertyChangeQueued back and forth in both concurrent and sequential update scenarios.
    • That means one property change will always work, two property changes may even work concurrently, but the odds of duplicating the ApplyText call may go up the more property changes are made via a single style swap.
  • I'll want to flag this, but alternatives are an open question:
    • Should we be using a debounce?
    • Should we find another way to wait until all DP callbacks are done firing?
  • If the intent is to batch updates rather than prevent duplicate/concurrent ApplyText calls, then either way ApplyText should be handled by the final DP callback entry, not the first one.
  • If the intent is to prevent duplicate/concurrent ApplyText calls and not to batch together rapid-fire DP updates, the comment should be updated to say that clearly but the code will work as-is.

[Arlodotexe]

"Default" is ambiguous about whether it means Dark or Light-- depends on application and system settings. It'll cause problems when switching themes.

Docs here explicitly call this out and recommend against it, with a full explainer as to why.

[Arlodotexe]

Is it intentional that ImageMaxWidth and ImageMaxHeight are both 0 by default?

[Arlodotexe]

Same method shape and concern as flagged in this comment

[Arlodotexe]

Looking here, the bool param here is called rerender. Internally, this method body uses the param to gate whether _renderer.ReloadDocument is called.

It's unclear from the code alone why this param value was changed from false to true in this method for this PR.

@niels9001 Any insight you can provide on this? If the rationale can't be found, we should probably expose this param through the containing Build method so it can be deliberately tuned by the needs of each caller. If it can be changed arbitrarily like this (unless it was deliberate but not obvious), then it may be underspecified throughout the codebase.

[Arlodotexe]

On MUX: RichEditBox exists in WinUI 2/MUX, but RichTextBlock only exists in WinUI 3/MUX while the UWP version of RichTextBox comes from the platform under WUX, so the namespace here needs adjusted slightly.

Our globalusings should auto-include MUXC on WASDK and WUXC on UWP out of the box, so simply shortening it should work.

Suggested change

	/// Represents a flow document that wraps a <see cref="Microsoft.UI.Xaml.Controls.RichTextBlock"/> for rendering markdown or HTML content.
	/// Represents a flow document that wraps a <see cref="RichTextBlock"/> for rendering markdown or HTML content.

[Arlodotexe]

Same comment as this one regarding MUX vs WUX on RichTextBlock:

Suggested change

	/// <summary>Gets or sets the underlying <see cref="Microsoft.UI.Xaml.Controls.RichTextBlock"/>.</summary>
	/// <summary>Gets or sets the underlying <see cref="RichTextBlock"/>.</summary>

[Arlodotexe]

This should be exposed as a property that can be set on the MarkdownTextBlock itself so it can be changed by the code consumer and shared everywhere (not just once per rendered image).

Not a blocker to closing this PR; it's an improvement over what we had but it still leaves room for further improvement.

[Arlodotexe]

• Reading as: Immediately unsubs the _image.Loaded -= LoadImage; event, making the handler a one-time use (unless resubscribed at end of this method body)
• I'm not seeing the re-sub to the _image.Loaded event.
  • This changes the behavior, specifically making each MyImage instance single-use.
  • Once an image is loaded, it cannot be re-loaded, even if the MyImage instance is removed and re-added to the visual tree.
  • If the intent here is to prevent the event body from running more than once at a time (semaphore-like), we should be resubscribing the event at the end of the method body.

[Arlodotexe]

This doesn't need to be nullable-- the null suppression in the body was a red flag that helped catch it, but I verified locally and the only reference to this ctor in our own code is from HtmlWriter.cs L69 where the provided reference renderer.MarkdownTextBlock is never null.

This means the nullability is an artifact of the prior MarkdownConfig? config being nullable. I can't think of any reason this control needs to operate without a MarkdownTextBlock instance attached, so let's align it with the other TextElement controls and make it non-nullable.

It should clean things up here nicely.