feat(#479): clickable inline RichTextHyperlink (OnClick mode)#501
Merged
Conversation
Adds Action? OnClick to RichTextHyperlink plus a factory Hyperlink(string text, Action onClick). When OnClick is set, the WinUI Hyperlink.Click event invokes the delegate and NavigateUri is left unset so the platform does not also navigate. Use to make a single rich-text run interactive (open an editor, dispatch a command) without escaping to a hosted native subtree — much lighter than `InlineUI(Button(...))` for virtualized lists of fragment rows. ## Reconciler wiring - Mount: when `OnClick` is non-null, subscribe a static `Click` handler and skip `NavigateUri`. - Update: handle navigate↔click mode transitions (clear/restore `NavigateUri`, attach/detach the static handler). The per-Hyperlink action is stored in a `ConditionalWeakTable<Hyperlink, Action>` so closure swaps across renders are a cheap map update — no event detach/attach churn even when callers pass a fresh closure per render. - CWT keys are weak; entries die when the Hyperlink is GC'd after `RichTextBlock.Blocks.Clear()` (no leak on full rebuilds). ## Drive-by: charting handler registration While verifying the demo, found that `D3Line`/`D3Path`/ `D3PathTranslated` constructed `LineElement`/`PathElement` records with `new()` directly, bypassing the registration touch in the `Line(...)`/`Path2D()` factories. `D3Rect` and `D3Circle` already routed correctly; aligned the remaining three so the spec-048 `ControlRegistry` entry is installed on first call. Without this any chart that uses lines or paths crashes with "No handler is registered for element type 'Microsoft.UI.Reactor.Core.LineElement'" when mounted in a context where `Factories.Line(...)` / `Factories.Path2D()` hasn't been touched elsewhere first. ## Tests - Unit: 4 new tests in MoreCoverageTests covering factory shape, null-guard, URI-mode no-op, and record-equality tracking `OnClick`. - Selftest: `CoreCov_RichTextHyperlink_OnClickMode` (7 assertions) exercising click-mode mount, identity preservation across closure swaps, and the URI↔click transitions in both directions. - Full Reactor.Tests suite green (9214 passed). ## Demo TestApp's Rich Text + Inline UI page swaps the inline `Button` re-roll for the new clickable hyperlink so the new shape is exercised end-to-end. The chart paragraphs now mount cleanly thanks to the D3 factory fix. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Comment on lines
+238
to
+243
| foreach (var block in rtb.Blocks) | ||
| { | ||
| if (block is not Microsoft.UI.Xaml.Documents.Paragraph p) continue; | ||
| foreach (var inline in p.Inlines) | ||
| if (inline is Microsoft.UI.Xaml.Documents.Hyperlink hl) return hl; | ||
| } |
Comment on lines
+241
to
+242
| foreach (var inline in p.Inlines) | ||
| if (inline is Microsoft.UI.Xaml.Documents.Hyperlink hl) return hl; |
| { | ||
| var l = link?.NavigateUri ?? new Uri("about:blank"); | ||
| l = l.ToString().Length < 1 ? l = new Uri("about:blank") : l; | ||
| try { hl.NavigateUri = l; } catch { hl.NavigateUri = new Uri("about:blank"); } |
There was a problem hiding this comment.
Pull request overview
Adds a new “click mode” for rich-text inline hyperlinks so RichTextBlock fragments can be interactive without hosting UIElements, and aligns a few D3 charting helpers with spec-048 handler registration expectations.
Changes:
- Introduces
RichTextHyperlink.OnClickplus a new DSL overloadHyperlink(string text, Action onClick)that suppresses navigation and invokes the delegate viaHyperlink.Click. - Updates the reconciler inline mount/update paths to support navigate↔click transitions with a static click handler +
ConditionalWeakTableaction storage. - Fixes D3 charting helpers (
D3Line/D3Path/D3PathTranslated) to ensure handler registration is touched before first mount; updates demo + docs; adds unit + selftest coverage.
Show a summary per file
| File | Description |
|---|---|
| tests/Reactor.Tests/MoreCoverageTests.cs | Adds unit tests covering the new Hyperlink(Action) factory and record equality on OnClick. |
| tests/Reactor.AppTests.Host/SelfTest/SelfTestFixtureRegistry.cs | Registers the new selftest fixture in both the discovery list and constructor switch. |
| tests/Reactor.AppTests.Host/SelfTest/Fixtures/CoreCoverageFixtures.cs | Adds a selftest verifying click-mode NavigateUri suppression and navigate↔click transitions. |
| src/Reactor/Elements/Dsl.cs | Adds Hyperlink(string, Action) factory overload with null-checking and sentinel URI. |
| src/Reactor/Core/Reconciler.Update.cs | Implements click-mode mount/update wiring for RichTextHyperlink using a static handler + ConditionalWeakTable. |
| src/Reactor/Core/Element.cs | Extends RichTextHyperlink with Action? OnClick and documents the two hyperlink modes. |
| src/Reactor/Charting/D3Charts.cs | Routes line/path helpers through registration-touch paths (spec-048) to avoid “No handler registered” crashes. |
| samples/Reactor.TestApp/Demos/RichTextDemo.cs | Updates demo to use the new clickable inline hyperlink instead of InlineUI(Button(...)). |
| docs/guide/text-and-media.md | Documents the new clickable Hyperlink(string, Action) overload. |
| docs/_pipeline/templates/text-and-media.md.dt | Updates the docs template source to include the new overload. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 10/10 changed files
- Comments generated: 2
Comment on lines
241
to
245
| /// <summary>Creates a path from SVG path data string. Accepts null pathData gracefully (renders nothing).</summary> | ||
| public static PathElement D3Path(string? pathData, Brush? stroke = null, Brush? fill = null, double strokeWidth = 1.5) => | ||
| new() | ||
| Path2D() with | ||
| { | ||
| Data = pathData != null ? PathDataParser.Parse(pathData) : null, |
Comment on lines
252
to
256
| /// <summary>Creates a path from SVG path data with a translate transform. Accepts null pathData gracefully (renders nothing).</summary> | ||
| public static PathElement D3PathTranslated(string? pathData, double translateX, double translateY, Brush? stroke = null, Brush? fill = null, double strokeWidth = 1.5) => | ||
| new() | ||
| Path2D() with | ||
| { | ||
| Data = pathData != null ? PathDataParser.Parse(pathData) : null, |
…uards Per CR feedback from codemonkeychris on PR #501: - Remove nested try/catch around about:blank fallback in UpdateHyperlinkInPlace. 'about:blank' always parses and the NavigateUri assignment for it never throws — if it ever did that's a real bug we want to surface, not swallow. - Drop redundant 'link?' null-conditionals inside 'case RichTextHyperlink link:' (Mount path) — the pattern variable is guaranteed non-null. Also drop the useless self-assignment in the empty-URI ternary. Both were preserved verbatim from the pre-existing block; cleaning them up while in the file. Selftest CoreCov_RichTextHyperlink_OnClickMode still 7/7 green; relevant Reactor.Tests filter still 196/196 green. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Per Copilot CR feedback on PR #501: routing through Path2D()/Line(...) factories meant D3Path/D3PathTranslated allocated twice per call (the factory's ew PathElement() then a with { ... } clone). In chart hot paths that's wasteful. Switch to a direct V1.Reg<TElement, TControl, THandler>.Done touch (spec 048 §7 pattern, idempotent after first call) and a single ew PathElement { ... } / ew LineElement { ... } allocation. Same registration semantics, half the allocations. Chart unit tests still 1303/1303 green. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| var uri = next.NavigateUri ?? new Uri("about:blank"); | ||
| if (uri.ToString().Length < 1) uri = new Uri("about:blank"); | ||
| try { wh.NavigateUri = uri; } | ||
| catch { wh.NavigateUri = new Uri("about:blank"); } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #479.
Adds
Action? OnClicktoRichTextHyperlinkand a new factory overloadHyperlink(string text, Action onClick). WhenOnClickis set the WinUIHyperlink.Clickevent fires the delegate andNavigateUriis left unset so the platform does not also navigate. Use to make a single rich-text run interactive (open an editor, dispatch a command) inside a virtualized list of fragment rows — much lighter thanInlineUI(Button(...))because no UIElement is hosted, just a click-wiredDocuments.Hyperlink.API shape
Reconciler wiring
OnClickis non-null, subscribe a staticClickhandler and skipNavigateUri.NavigateUri, attach/detach the static handler). The per-Hyperlink action lives in aConditionalWeakTable<Hyperlink, Action>so closure swaps across renders are a cheap map update — no event detach/attach churn even when callers pass a fresh closure per render.RichTextBlock.Blocks.Clear()(no leak on full rebuilds).Drive-by: charting handler registration
While verifying the demo I found that
D3Line/D3Path/D3PathTranslatedconstructed their element records withnew()directly, bypassing the spec-048 registration touch in theLine(...)/Path2D()factories.D3RectandD3Circlealready routed correctly; aligned the remaining three so theControlRegistryentry is installed on first call. Without this any chart that uses lines or paths crashes with ""No handler is registered for element type 'Microsoft.UI.Reactor.Core.LineElement'"" when mounted in a context whereFactories.Line(...)/Factories.Path2D()hasn't been touched elsewhere first (e.g. the TestApp's Rich Text demo).Tests
MoreCoverageTests)ArgumentNullExceptionon null action, URI-mode no-op, record-equality trackingOnClick(4 new tests, all green).CoreCov_RichTextHyperlink_OnClickMode)NavigateUri; identity preserved across closure swap; URI→click transition clearsNavigateUri; click→URI restores it. 7 assertions, all green.Reactor.Testssuite — 9214 passed, 0 failed. All existingRichText/InlineUIselftests still green.Demo
TestApp's Rich Text + Inline UI page swaps the inline
Buttonre-roll for the new clickable hyperlink so the new shape is exercised end-to-end. Chart paragraphs now mount cleanly thanks to the D3 factory fix.Docs
docs/_pipeline/templates/text-and-media.md.dt+ compiled output updated to list the newHyperlink(string text, Action onClick)helper.