microsoft / microsoft/microsoft-ui-reactor
Harden RichTextBlock reactive scroll-anchor (#649 follow-up): clamp-filter intent refresh, unbounded armed state, restore/nested-host coverage
- Dominant language
- C#
- Stars
- 646
- Forks
- 54
- Avg merge
- 1d 3h
- Merged PRs (30d)
- 84
Description
Follow-up to #649 (merged, closes #487). The final pre-merge pr-review of the reactive scroll-anchor (`src/Reactor/Core/Reconciler.RichTextScrollAnchor.cs`) surfaced four MEDIUM findings that were deferred as non-blockers, plus a missing regression test for the H1 pool-reuse bug that **was** fixed before merge (commit `45ca1c50`). Tracking them here so they aren't lost.
### Sequencing (important)
This is intentionally sequenced **after #717** (the root-cause fix that coalesces the inline-UI reattach into the same committed layout pass so the ScrollViewer never samples the transient collapsed extent). If #717 makes the reactive anchor provably redundant, revisit whether to **harden** it (these items) or **remove** it entirely. Do not start this work before #717 resolves.
### Correctness
- **M1 — clamp-filter intent refresh:** `LastScrollableHeight` is only refreshed when `Intended == NaN`, so on a *second* arm a real clamp can be recorded as the new intent and never restored — i.e. the very scroll drift the anchor exists to prevent can resurface across successive mutations.
- **M2 — unbounded armed state on permanent shrink:** a permanent content shrink leaves the anchor armed indefinitely — the `!recovered` early-return has no bounded wait and never reaches the corr-b retry budget its comment claims. It should consume the bounded budget and disarm.
### Test coverage
- **M3 — restore offset is under-asserted:** restore is only tested from the bottom; a regression that restored to `ScrollableHeight` instead of the intended *mid* offset would still pass. Add a mid-offset restore assertion.
- **M4 — nested scroll hosts:** no fixture covers nearest-ancestor scroll-host selection when scroll hosts are nested; that selection is currently unguarded.
- **H1 pool-reuse regression test:** add a fixture exercising pooled-ScrollViewer rent/return that locks the `45ca1c50` fix — exactly one ViewChanged/LayoutUpdated handler pair survives a pool cycle, and a pending dispatcher-deferred restore no-ops on a recycled ScrollViewer (no cross-renter stale-scroll replay).
_Found during pre-merge review of #649; none are merge-blockers for that PR (H1 was fixed; these are the residual mediums + coverage)._
Contributor guide
Research direction
Start only after #717 resolves, then read src/Reactor/Core/Reconciler.RichTextScrollAnchor.cs and inspect the existing restore, nested-host, and pooled-ScrollViewer coverage. Verify the clamp intent refresh and bounded disarm behavior, add mid-offset, nested-host, and pool-reuse regression coverage, and confirm one handler pair survives reuse without stale deferred restoration.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- csharp
- Domain
- desktop, frontend, testing
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 38/100