Skip to content

Add Element content support to button-family controls - #1483

Merged
Alexandre Zollinger Chohfi (azchohfi) merged 7 commits into
microsoft:mainfrom
kusutori:codex/button-element-content
Nov 12, 2026
Merged

Alexandre Zollinger Chohfi (azchohfi) merged 7 commits into
microsoft:mainfrom
kusutori:codex/button-element-content

Conversation

@kusutori

@kusutori kusutoriクスとり (kusutori) commented Nov 9, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • Add Element-content overloads and ContentElement slots to the missing button-family wrappers, including three-state variants
  • Share string/Element content reconciliation with Button, preserving child state and cleanup when content changes, becomes empty, or returns to text
  • Add unit and live WinUI tests, a compiled usage example, documentation, and API-index updates
ToggleButton(
    HStack(8, Icon(FontIcon("\uE8B9")).AccessibilityHidden(), TextBlock("Images")),
    selected,
    setSelected);

Linked issue

Fixes #1482

Test plan

  • Solution-wide Release build passed with 0 errors (14 existing dependency-audit / AnyCPU Win2D warnings)
  • 22 unit tests passed covering content factories, update-skip safety, control-specific arguments, and existing command bindings
  • 130 live WinUI checks passed across 11 content scenarios, plus 3 related DevTools checks. Covers text/Element/empty transitions, state retention, latest callbacks, fallback restoration and cleanup
  • The text-to-Empty() regression checks were observed failing before the shared content strategy was corrected, then passing after the fix
  • Compiled the controls documentation example and regenerated both byte-identical API indices

Risk / breaking changes

Existing string and command overloads remain available. Custom content takes precedence over the string fallback; setting ContentElement to null restores the fallback. A declared empty Element clears the content.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

  1. Docs build fails. controls.md was 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, warns REACTOR_DOC_REFMARKER_001 for each, and the page no longer matches what's committed. I'd drop the markers from the template and rerun dotnet run --project tools/Reactor.DocPipeline -- compile --no-screenshots.
  2. .Set(c => c.Content = <UIElement>) stops working. See the inline comment on ContentControlContent.cs.
  3. Accessibility checks only know about Button. AccessibilityScanner.GetChildren (line 238) and CheckIconButton (line 264) only match ButtonElement, and REACTOR_A11Y_001 only matches Button(expr, action). So ToggleButton(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.
  4. 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.

Comment thread docs/_pipeline/templates/controls.md.dt Resolved
Comment thread src/Reactor/Elements/Dsl.cs Resolved
@kusutori

Copy link
Copy Markdown
Contributor Author

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.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 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

Comment thread src/Reactor/Core/Reconciler.cs Resolved

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks, this covers everything I raised last round. The setter-content fix and the extra live checks look right.

What's left:

  1. Docs build fails again, this time on docs/guide/control-reconciler-protocol.md. That page embeds the children-strategies snippet from ChildrenStrategy.cs, and the new <para> on SingleContent sits inside it. Rerunning dotnet run --project tools/Reactor.DocPipeline -- compile --no-screenshots and committing should fix it.
  2. Both Copilot threads hold up when I trace the code. On Reconciler.cs I added a second, broader problem: the default-name update now writes "" on every element update. Details in that thread.
  3. 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 no ContentElement, or accept LabeledBy and reword the message for the whole family.
  4. The null-child skip in V1HandlerAdapter now applies to every SingleContent control, third-party ones included. Worth a line under Changed, since SingleContent is public.

Everything else in CI is green.

@kusutori

Copy link
Copy Markdown
Contributor Author

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.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

@kusutori

Copy link
Copy Markdown
Contributor Author

Thanks! Removed ChildRemovalHandledByHandler; the shared button helper now owns reconciliation and cleanup using the existing Imperative strategy. Added checks for Element/setter precedence and retained child state, and regenerated the protocol docs. Synced with main; 78 unit tests and 324 native/DevTools assertions pass, along with the Reactor Release build and full docs generation.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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 ImperativeBridged unmount hook gives every button-family control a ReactorState, even ones with no callbacks, such as a plain CheckBox label. 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.

@azchohfi
Alexandre Zollinger Chohfi (azchohfi) merged commit 78a2aac into microsoft:main Nov 12, 2026
42 checks passed
@azchohfi

Copy link
Copy Markdown
Collaborator

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 Unmount callback to the Imperative children strategy, so the button content helper no longer gives every button-family control a ReactorState. Callback-free controls go back to being cleaned up by the generic ContentControl walk. It also folds the extra CHANGELOG line into the existing #1482 entry. I'll tag you on it.

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.

[Bug] ToggleButton and other button wrappers lack Element content overloads

3 participants