Repository navigation
Add Element content support to button-family controls - #1483
Alexandre Zollinger Chohfi (azchohfi) merged 7 commits into
Conversation
Alexandre Zollinger Chohfi (azchohfi)
left a comment
There was a problem hiding this comment.
Nice work. Moving this into a shared WithContent strategy is the right call, and the selftest matrix is thorough. CI is green apart from Docs build.
- Docs build fails.
controls.mdwas compiled with--skip-reference, so the eight<!-- ref:X -->markers stayed raw. None of them resolve, because reference pages only exist for hooks. The CI compile renders them as inline code, warnsREACTOR_DOC_REFMARKER_001for each, and the page no longer matches what's committed. I'd drop the markers from the template and rerundotnet run --project tools/Reactor.DocPipeline -- compile --no-screenshots. .Set(c => c.Content = <UIElement>)stops working. See the inline comment onContentControlContent.cs.- Accessibility checks only know about
Button.AccessibilityScanner.GetChildren(line 238) andCheckIconButton(line 264) only matchButtonElement, andREACTOR_A11Y_001only matchesButton(expr, action). SoToggleButton(Icon(...), on, setOn)with no.AutomationName()isn't flagged, and content inside the new controls isn't scanned. At minimum, I'd extend the scanner; the analyzer could be a follow-up. - Please add a CHANGELOG
[Unreleased]entry: Added for the new overloads, Changed for the Button content path, citing #1482.
Smaller: the default AutomationName still comes from Label even when ContentElement replaces the visible text. Either document that, or skip Label in ResolveCaptionForElement when content is set.
|
Thanks for the detailed review! Merged main and preserved setter-supplied native Content across mount/update by leaving undeclared child slots alone. Extended the runtime accessibility scanner across the button family, made accessible names follow visible content, removed the invalid reference markers, and added changelog entries including the null-overload ambiguity. The added live regression checks failed 66 assertions before the fix and now pass. Validation: 34 focused unit tests and 210 button/DevTools checks passed, documentation compilation passed, and Reactor Release builds with zero warnings/errors. The full unit run had two suggester-test failures; both also reproduce on unmodified main at db7be26. |
There was a problem hiding this comment.
🟡 Changes recommended
Two active issues remain: Element-to-native-setter transitions can leave buttons empty, and text-to-icon transitions can clear an explicit accessible name.
2 open issues
What changed in this PR
Extends Reactor’s button-family controls with declarative Element content, addressing #1482 and bringing icon-and-label composition beyond ordinary Button.
Changes:
- Adds content overloads and slots, including three-state variants.
- Shares content reconciliation and updates accessible-name handling and scanning.
- Adds unit and live WinUI coverage, a compiled example, documentation, and API-index entries.
| File | Description |
|---|---|
| tests/Reactor.Tests/ButtonContentTests.cs | Tests factories, update-skip safety, and accessibility scanning. |
| tests/Reactor.AppTests.Host/SelfTest/SelfTestFixtureRegistry.cs | Registers the content fixture. |
| tests/Reactor.AppTests.Host/SelfTest/Fixtures/ButtonContentFixture.cs | Tests content transitions, state, cleanup, setters, and names. |
| src/Reactor/Elements/Dsl.cs | Adds Element-content factory overloads. |
| src/Reactor/Core/V1Protocol/V1HandlerAdapter.cs | Preserves undeclared setter-supplied content. |
| src/Reactor/Core/V1Protocol/Descriptor/ContentControlContent.cs | Shares text and Element content handling. |
| src/Reactor/Core/V1Protocol/ChildrenStrategy.cs | Documents null and empty child semantics. |
| src/Reactor/Core/Reconciler.cs | Derives accessible names from visible content. |
| src/Reactor/Core/Element.cs | Adds content slots and integrates shared reconciliation. |
| src/Reactor/Core/AccessibilityScanner.cs | Scans button-family content and checks accessible names. |
| skills/reactor.api.txt | Indexes the new public API. |
| plugins/reactor/skills/reactor-dsl/references/reactor.api.txt | Updates the plugin’s matching API index. |
| docs/guide/controls.md | Adds generated content-composition guidance. |
| docs/_pipeline/templates/controls.md.dt | Adds the source documentation. |
| docs/_pipeline/apps/controls/App.cs | Adds a compiled icon-and-label example. |
| CHANGELOG.md | Records content support, behavior changes, and overload ambiguity. |
Review effort: Balanced
Alexandre Zollinger Chohfi (azchohfi)
left a comment
There was a problem hiding this comment.
Thanks, this covers everything I raised last round. The setter-content fix and the extra live checks look right.
What's left:
- Docs build fails again, this time on
docs/guide/control-reconciler-protocol.md. That page embeds thechildren-strategiessnippet fromChildrenStrategy.cs, and the new<para>onSingleContentsits inside it. Rerunningdotnet run --project tools/Reactor.DocPipeline -- compile --no-screenshotsand committing should fix it. - Both Copilot threads hold up when I trace the code. On
Reconciler.csI added a second, broader problem: the default-name update now writes""on every element update. Details in that thread. - A11Y_001 now fires on a plain unlabeled
CheckBox/RadioButton, e.g.samples/TodoApp/Program.cs:185. The message says "Icon-only button" with WCAG 1.1.1, which reads oddly for a checkbox, and it doesn't honor.LabeledBy()the way A11Y_003 does. Either skip CheckBox/RadioButton when there's noContentElement, or acceptLabeledByand reword the message for the whole family. - The null-child skip in
V1HandlerAdapternow applies to everySingleContentcontrol, third-party ones included. Worth a line under Changed, sinceSingleContentis public.
Everything else in CI is green.
…content # Conflicts: # CHANGELOG.md
|
Addressed the remaining review comments, synced with main, and refreshed the generated docs and changelog; all 78 targeted unit tests, 386 native assertions, and the full docs compile pass. |
Alexandre Zollinger Chohfi (azchohfi)
left a comment
There was a problem hiding this comment.
Thanks, this addresses everything from the last round, including both Copilot findings, and the new live checks cover each transition.
One thing I'd like changed before merge: ChildRemovalHandledByHandler. It adds an internal switch to the public SingleContent record that changes how V1HandlerAdapter handles an Element→null transition. That gives the engine-wide strategy a mode only built-in code can turn on and the docs don't describe. Because it sits inside the children-strategies snippet, the internal member and its button-specific comment are now rendered in the public control-reconciler-protocol.md. It also makes the adapter rely on a property entry having already unmounted the child; if that ever doesn't run, the child leaks.
Could the button helper own this transition itself instead of reaching into the engine's dispatch? If engine support really is needed, make it a public, documented option on SingleContent, covered in the CHANGELOG and the protocol page, so third-party handlers can use it too.
CI note: the Selftests (2/2) failure is ContentDialogDispose_MountSiblingAfterDisposal, which flakes on main with the same three assertions (runs 43099122335, 42986375926). It passes 5/5 locally on this head, so it's unrelated.
|
Thanks! Removed |
Alexandre Zollinger Chohfi (azchohfi)
left a comment
There was a problem hiding this comment.
Thanks, moving ownership into the helper is much cleaner, and the engine contract is back to what it was. Element/text/setter transitions, unmount, and the diagnostics walk (ReactorSource_RootRemount still finds Button content inside a closed flyout) all check out, and CI is green. LGTM.
Two optional nits:
- The
ImperativeBridgedunmount hook gives every button-family control a ReactorState, even ones with no callbacks, such as a plainCheckBoxlabel. Probably fine, but worth knowing. - The new CHANGELOG line describes an internal helper and doesn't cite #1482. It could fold into the existing
Button and the button family share content reconciliation…entry.
78a2aac
into
microsoft:main
|
Merged, thanks for working through all the rounds on this. I'm sending a small follow-up PR for the two nits from my last review, so no need to pick those up. It adds an optional |

Summary
ContentElementslots to the missing button-family wrappers, including three-state variantsButton, preserving child state and cleanup when content changes, becomes empty, or returns to textLinked issue
Fixes #1482
Test plan
Empty()regression checks were observed failing before the shared content strategy was corrected, then passing after the fixRisk / breaking changes
Existing string and command overloads remain available. Custom content takes precedence over the string fallback; setting
ContentElementtonullrestores the fallback. A declared empty Element clears the content.