microsoft / microsoft/microsoft-ui-reactor

Harden RichTextBlock reactive scroll-anchor (#649 follow-up): clamp-filter intent refresh, unbounded armed state, restore/nested-host coverage

Open
#718 1 comment 0 reactions 0 assignees View on GitHub
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.