spec(047): close V1-protocol descriptor regressions (default stays V1 OFF)#443
Merged
Conversation
Add an optional AfterChildrenMount hook to the V1 handler surface, invoked by V1HandlerAdapter.Mount after the children strategy (including inline items-binder strategies) has mounted every child. DescriptorHandler forwards to a new ControlDescriptor.AfterChildrenMount delegate. Lets handlers subscribe events that must wire strictly after children-add (e.g. TabView.SelectionChanged, which WinUI fires spuriously during the first tab add when subscribed during the prop-apply phase). Additive and inert until a descriptor populates the callback (Batch T). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The V1 TabItemsHost children strategy (backing PivotDescriptor, which is registered/live under V1, and the carved TabViewDescriptor) rebuilt every container on each Update: it unmounted+remounted all TabViewItem/PivotItem containers and their content subtrees. That re-triggers the tab-strip entrance animation and steals focus from descendant controls on any render that touches the host element. Switch the Update path to an index-keyed in-place positional reconcile, mirroring the legacy UpdateTabView/UpdatePivot arms: - keep existing containers for shared indices; reconcile each container's Content via ReconcileV1Child and only reassign when the realized control reference actually changes (avoids WinUI detach/reattach dropping queued setState); - refresh container metadata (header/icon/closable) through a new optional UpdateContainer callback supplied per descriptor; - unmount+remove excess tail; mount+append surplus new items; - fall back to full rebuild only on engine-invariant break / count drift. Extend the Desc_Pivot and Desc_TabView selftests with same-shape update assertions proving container AND content-subtree instance identity is preserved (would fail on the old rebuild path). Spec 047 §14 Phase 3 prelude. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Close the NavigationHost dispatch carve (Path B delegate handler). Expose MountNavigationHost/UpdateNavigationHost as internal and add NavigationHostHandler delegating to them; register it in RegisterV1BuiltInHandlers. Unmount stays on the flag-independent UnmountRecursive intercept (fires before the V1 unmount arm), so teardown is byte-identical V1 ON =/= V1 OFF. Validated: core build green; Navigation_NavHost* selftests (16 asserts) pass under both V1 ON and V1 OFF. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Close the GridView dispatch carve with a hand-coded GridViewHandler (Path B delegate) mirroring ListViewHandler. Expose MountGridView/UpdateGridView as internal and delegate to them; register the handler in RegisterV1BuiltInHandlers. The GridViewDescriptor stays unregistered (its ItemsHost<> strategy pre-mounts every item with no virtualization, which would regress the recycle contract). Validated: core build green; all GridView selftests (RareControl, KLR, SelectionEvt, TemplatedListHL, CoreCov, Desc) pass under both V1 ON and V1 OFF (0 failures). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Route the seven overlay elements (ContentDialog, Flyout, MenuBar, CommandBar, MenuFlyout, Popup, CommandBarFlyout) through V1 handler dispatch via decorator-style Path B handlers that delegate to the existing internal MountXxx/UpdateXxx engine bodies and return ContinueDefaultTraversal on unmount. Because ContinueDefaultTraversal makes the engine fall through to the same UnmountRecursive type-based recursion that runs when the V1 flag is OFF, mount/update/unmount are byte-identical V1 ON ≡ V1 OFF. Made the 14 overlay Mount*/Update* methods internal, added OverlayDecoratorHandlers.cs, registered all 7 in RegisterV1BuiltInHandlers, and updated the carve xmldoc. Validated A|B: overlay selftests (ContentDialog/Flyout/CommandBar/ MenuBar/Popup families) pass with identical counts and 0 failures under both V1 ON and V1 OFF. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…lude) Route TabViewElement through V1 handler dispatch via TabViewHandler, a Path B delegate that calls the COMPLETE legacy MountTabView/UpdateTabView bodies (drag pipeline, pinnable headers, strip header/footer, in-place tab-content reconcile, conditional SelectedIndex). This is distinct from the still-unregistered TabViewDescriptor + TabItemsHost port, which leaves those features on the legacy arm; the delegate runs the full feature set because it IS the legacy code, so it has none of the descriptor's gaps. UpdateTabView returns null (pure in-place reconcile), so the void-Update IElementHandler shape preserves identity. Made MountTabView/UpdateTabView internal. Unmount is byte-identical V1 ON =/= V1 OFF: a WinUI.TabView is an ItemsControl that both unmount paths pool without child recursion (no Panel/Border/ContentControl branch matches), and CollectSelf reproduces the V1-OFF fall-through. Mount/update are unchanged from the previously-carved V1-ON path; registering the handler only makes the parity-safe unmount arm fire. Validated A|B: TabView (5) + Pivot (3) selftests pass identically under V1 ON and V1 OFF. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…prelude) ButtonDescriptor only expressed the string-Label fast path and dropped ButtonElement.ContentElement, so element-content buttons (e.g. PropertyGrid expand buttons) rendered blank under V1 ON. Replace the registered descriptor with a delegate ButtonHandler that runs the complete legacy MountButton/ UpdateButton bodies (enabled/disabled-focusable, Click trampoline, setters AND ContentElement). Decorator shape returns ContinueDefaultTraversal so unmount recurses into ContentControl content in both paths, matching V1 OFF cleanup of an element child. ButtonDescriptor retained for its isolated selftests + perf bench. A|B validated: PropertyGrid/Button/Categor selftests 0 failures under V1 ON and V1 OFF. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The V1 Panel<> children strategy (V1HandlerAdapter) reconciles children by
index with no keyed reconcile, losing WinUI control identity on keyed
reorder/reverse/swap/remove-middle. Legacy Update{Flex,Stack,Grid,Canvas,
RelativePanel,WrapGrid} run ReconcileChildren -> ChildReconciler (spec-042
keyed LIS) and re-apply attached props, so V1 ON diverged from V1 OFF on
~17 identity-preservation selftests.
Route all 6 panels through IDecoratorElementHandler delegates that invoke the
complete legacy Mount*/Update* bodies (identical to V1 OFF). ContinueDefaultTraversal
on unmount so teardown falls through to the same WinUI.Panel child-recursion
V1 OFF uses. Update returns the legacy result when non-null (RelativePanel may
substitute the control on count change) else keeps the existing control.
Descriptor types retained for isolated selftests + perf bench.
Made the 6 Mount*/Update* methods internal for delegate access.
A|B validated (V1 ON == V1 OFF, 0 failures): FlexColumn, Keyed*, MultiCycle,
Reconciler_KeyedList, Stack, Grid, Canvas, RelativePanel, WrapGrid. Also fixes
as side effects: AAF_MoveSpring + AAF_FlexColumnMove (animation) and
ConditionalRendering_Toggle_HiddenAgain.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The V1 LazyStackDescriptor port used ItemsRepeater directly as TControl (a descriptor's RentControl returns a single control, with no place for a wrapping ScrollViewer). Legacy MountLazyStack wraps the ItemsRepeater in a ScrollViewer with orientation-appropriate scrollbars + ScrollViewerSetters. Under V1 ON the ScrollViewer was therefore absent, failing every FindControl<ScrollViewer> fixture (LazyVStack/HStack_ScrollViewer, LazyHStack_HScrollEnabled, ScrollPop/Back, HookPaging_Scroll*) and the component-cleanup-on-unmount gate (EFR_LazyStackUnmount) — no ScrollViewer for the engine to recurse through on teardown. Add RegisterDecoratorHandlerForDerivedTypes<TBase> (the derived-type analogue of RegisterDecoratorHandler) and route the Lazy*Stack family through a Path B LazyStackHandler decorator that runs the complete legacy MountLazyStack/ UpdateLazyStack bodies. ContinueDefaultTraversal on unmount so the engine recurses ScrollViewer -> ItemsRepeater -> realized rows (running each row component's UseEffect cleanup), identical to V1 OFF. Descriptor retained for isolated selftests. Made MountLazyStack/UpdateLazyStack internal for delegate access. A|B validated (V1 ON == V1 OFF, 0 failures): Lazy, Scroll, HookPaging, DataGridScroll. EFR_LazyStackUnmount_AtLeastOneCleanup now after=5 (was 0). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Convert CheckBox, Expander, and templated list (ListView/GridView/FlipView) V1 descriptors to decorator handlers that delegate Mount/Update to the legacy engine bodies, restoring byte-identical parity with V1 OFF. - CheckBoxHandler: legacy UpdateCheckBox only suppresses echo when the target differs from the current value, fixing ConditionalRendering toggle swallow. - ExpanderHandler: restores ContentTransitions + late callback subscription. - TemplatedListHandler: registered on common base TemplatedListElementBase so typed ListView<T>/GridView<T>/FlipView<T> route through legacy MountTemplatedList + control-typed Update*, restoring ApplyMoveAnimations (fixes AAF_MoveSpring/FlexColumnMove implicit-offset attach). Made the corresponding legacy Mount*/Update* methods internal. A|B validated under real V1 ON vs OFF: 0 failures, equal check counts. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Closes the remaining V1-protocol descriptor regressions so that, when Reactor.UseV1Protocol is ON, behavior matches the legacy path byte-for-byte. The library default remains V1 OFF. Each fix follows a Path-B delegate recipe: a new handler in V1Protocol.Handlers whose Mount/Update delegate to the existing internal Reconciler.Mount*/Update* bodies and returns ContinueDefaultTraversal on unmount so the engine recurses identically to V1 OFF.
Changes:
- New decorator/standard handlers (Button, CheckBox, Expander, TabView, NavigationHost, GridView, 7 overlays, 6 panels, Lazy*Stack, TemplatedList) all delegating to legacy bodies, with corresponding
internalvisibility bumps on theMount*/Update*methods. - New
RegisterDecoratorHandlerForDerivedTypes<TBase>API + newAfterChildrenMountengine hook (interface default + descriptor surface + adapter invocation) to support post-children-add subscription (e.g. TabView SelectionChanged). TabItemsHostrewritten from full rebuild to in-place index-positional reconcile with an optionalUpdateContainercallback; selftest fixtures extended to assert container/content identity preservation across same-shape updates.
Show a summary per file
| File | Description |
|---|---|
| src/Reactor/Core/Reconciler.cs | Registers new handlers; comments updated to mark overlays/TabView/NavigationHost/GridView as ported; adds RegisterDecoratorHandlerForDerivedTypes |
| src/Reactor/Core/Reconciler.Mount.cs | Bumps Mount* methods from private to internal so V1 handlers can delegate |
| src/Reactor/Core/Reconciler.Update.cs | Bumps Update* methods from private to internal for delegation |
| src/Reactor/Core/V1Protocol/IElementHandler.cs | Adds optional AfterChildrenMount default no-op |
| src/Reactor/Core/V1Protocol/V1HandlerAdapter.cs | Calls AfterChildrenMount after children strategy mount |
| src/Reactor/Core/V1Protocol/ChildrenStrategy.cs | TabItemsHost now does in-place positional reconcile with UpdateContainer hook |
| src/Reactor/Core/V1Protocol/Descriptor/ControlDescriptor.cs | Adds AfterChildrenMount descriptor property + delegate type |
| src/Reactor/Core/V1Protocol/Descriptor/DescriptorHandler.cs | Forwards AfterChildrenMount to the descriptor callback |
| src/Reactor/Core/V1Protocol/Descriptor/Descriptors/ButtonDescriptor.cs | Doc note: descriptor superseded by ButtonHandler |
| src/Reactor/Core/V1Protocol/Descriptor/Descriptors/TabViewDescriptor.cs | Adds UpdateContainer for in-place header/icon/closable refresh |
| src/Reactor/Core/V1Protocol/Descriptor/Descriptors/PivotDescriptor.cs | Adds UpdateContainer for in-place header refresh |
| src/Reactor/Core/V1Protocol/Handlers/ButtonHandler.cs | New delegate handler (ContentElement coverage + unmount parity) |
| src/Reactor/Core/V1Protocol/Handlers/CheckBoxHandler.cs | New delegate handler (legacy echo-suppression) |
| src/Reactor/Core/V1Protocol/Handlers/ExpanderHandler.cs | New delegate handler (callback/template wiring) |
| src/Reactor/Core/V1Protocol/Handlers/TabViewHandler.cs | New handler routing TabView through V1 via legacy bodies |
| src/Reactor/Core/V1Protocol/Handlers/NavigationHostHandler.cs | New handler for NavigationHost |
| src/Reactor/Core/V1Protocol/Handlers/GridViewHandler.cs | New handler mirroring ListViewHandler |
| src/Reactor/Core/V1Protocol/Handlers/OverlayDecoratorHandlers.cs | Seven overlay decorator handlers |
| src/Reactor/Core/V1Protocol/Handlers/PanelDelegateHandlers.cs | Six panel decorator handlers (Flex/Stack/Grid/Canvas/RelativePanel/WrapGrid) |
| src/Reactor/Core/V1Protocol/Handlers/LazyStackHandler.cs | New decorator handler for derived Lazy*Stack types |
| src/Reactor/Core/V1Protocol/Handlers/TemplatedListHandler.cs | New decorator handler dispatching by control type |
| tests/Reactor.AppTests.Host/SelfTest/Fixtures/Spec047V1ProtocolDescriptorFixtures.cs | Adds same-shape in-place reconcile assertions for TabView and Pivot |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 22/22 changed files
- Comments generated: 0
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.
Summary
Closes the remaining V1-protocol descriptor regressions so that, when
Reactor.UseV1Protocolis ON, the rendered WinUI behavior is byte-identical to the legacy path (V1 ON ≡ V1 OFF). The library default stays V1 OFF — this PR does not flip the production switch (see "Not in scope" below).Each fix follows the proven Path-B delegate recipe: convert a buggy V1 descriptor into an
IDecoratorElementHandlerthat delegates Mount/Update to the existing, unchanged legacyMount*/Update*engine bodies, returningContinueDefaultTraversalfor unmount.What changed
TabItemsHostreconciles in place (no full rebuild → no tab re-animation)ContentElementgapRegisterDecoratorHandlerForDerivedTypes<TBase>API)UpdateCheckBoxonly suppresses when target ≠ current value (fixes swallowed 2nd toggle)ContentTransitions+ late callback subscription<T>MountTemplatedList+ control-typedUpdate*, restoringApplyMoveAnimationsEngine prelude also included: post-children mount hook (A1).
Validation
REACTOR_USE_V1_PROTOCOL=true) ×3 = 0 failures.Not in scope (deliberately)
This PR keeps
UseV1Protocoldefault OFF. Flipping the production default is gated on items that require separate governance:TypeRegistryTests.Override_Builtin_Type_Mount_Is_Dispatched— under V1 ON, overriding a built-in viaRegisterTypethrows by design (spec §13 Q17). Needs a test update + release notes when the flip happens.RichEditBoxElementTests.Mount_Dispatches_RichEditBoxElement— headless COM test artifact (RentControl<T>genericnew T()vs legacy direct ctor); not a render regression.Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com