LT-22676: Stop browse views dragging the selected row back on scroll - #1156
jasonleenaylor wants to merge 2 commits into
Conversation
Clear the row rectangle XmlBrowseViewBase saves for AdjustScrollRange1 as soon as MakeSelectionVisible returns, so a lazy-box expansion caused by the user scrolling no longer forces the selected row back on screen. Guard the idle selection cleanup so installing its replacement insertion point does not queue another cleanup pass. Re-asserting the same index now scrolls only when the row shows a different object, and PostLayoutInit goes through ScrollSelectedRowIntoView. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1156 +/- ##
==========================================
+ Coverage 38.89% 38.95% +0.06%
==========================================
Files 1518 1520 +2
Lines 352262 352514 +252
Branches 40597 40638 +41
==========================================
+ Hits 137005 137329 +324
+ Misses 185976 185904 -72
Partials 29281 29281
🚀 New features to boost your workflow:
|
| if (value >= 0 && m_wantScrollIntoView) | ||
| // Never touch the editing selection here (LT-12092). Scroll only when the | ||
| // row now shows a different object, e.g. after a re-sort (LT-22676). | ||
| if (value >= 0 && m_wantScrollIntoView && GetNewSelectionObject(value) != m_hvoOldSel) |
There was a problem hiding this comment.
Jumping to the record that's already current no longer brings it into view. Now that the user can scroll the current row off screen, Find Lexical Entry (or any JumpToRecord) on that same record goes through RecordClerk.JumpToIndex. Because the index hasn't changed, it just re-sends RecordNavigation. RecordBrowseView.OnRecordNavigation then sets the same index again, and this branch skips the scroll because the object is the same. The list stays where the user left it, and the entry they just chose isn't visible.
Can an explicit jump still scroll the row into view, while other re-asserts of the current record keep leaving the scroll position alone? ScrollSelectedRowIntoView looks like the right hook if the jump path can reach it.
JumpToRecord now tells the RecordNavigation payload when it asked for the record that was already current, and RecordBrowseView scrolls the selected row into view for that case. JumpToIndex on the current index still leaves the scroll position alone, so refreshes do not drag the row back. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
3 participants
Clear the row rectangle
XmlBrowseViewBasesaves forAdjustScrollRange1as soon as
MakeSelectionVisiblereturns, so a lazy-box expansion causedby the user scrolling no longer forces the selected row back on screen.
Guard the idle selection cleanup so installing its replacement insertion
point does not queue another cleanup pass; that loop re-scrolled the row
on every idle tick whenever the current row had no editable cell.
Re-asserting the same index now scrolls only when the row shows a
different object, and
PostLayoutInitgoes throughScrollSelectedRowIntoView.Why it failed
Three things in
XmlBrowseViewBasecooperated, all confirmed with runtimetracing in Bulk Edit Phoneme Features on Sena 3:
SaveSelectionInfo(called fromMakeSelectionVisible) left the selectedrow's rectangle armed after the call finished. The Views engine raises
AdjustScrollRangeon every lazy-box expansion or contraction, so the firstuser scroll that reached unexpanded rows hit
AdjustScrollRange1with thestale anchor and it snapped the view back to the row (the LT-3607 code from
2006, meant only for the expansions inside
MakeSelectionVisibleitself).RemoveRootBoxSelectionOnIdledestroyed a non-editable insertion point inthe current row and re-created it through
SetDefaultInsertionPointInRow,which also force-scrolls the row into view. Installing the new selection
raised SelectionChanged, which queued the idle pass again. In any browse view
whose current row has no editable cell this ran about 80 times a second
(about 1,800
MakeSelectionVisiblecalls in seven idle seconds) and undid apage-down within 12 ms. It was also a permanent idle CPU drain.
SelectedIndexsetter always scrolled the rowinto view, re-arming (1) each time a clerk re-asserted the current record.
The editing selection is never touched, so LT-12092 behaviour is unchanged.
Verification
XMLViewsTests: 119 pass with-CommentHygiene -TokenHygiene. Two of thefive new tests fail on
main(anchor survivesMakeSelectionVisible;same-row re-assert scrolls) and pass on the branch; a third guards the
LT-3607 behaviour and passes on both.
MakeSelectionVisiblecalls in startup + 10 s droppedfrom about 1,800 to 7. Page-down and page-up in Bulk Edit Phoneme Features
and Bulk Edit Wordforms (about 4,400 rows) leave the view where it was
scrolled with record 1 still current.
filter/sort changes, Find/Jump (need keyboard focus).
Fixes LT-22676; same defect as LT-20523 (kept open per Beth's request, to be
tested against this build).
Next: approve, or ask for the idle-loop unit test (needs a Mediator in the
fake browse view) before merge.
Preflight review details
Code Review Summary
Branch: LT-22676-browse-scroll-yank
Base: origin/main
Date: 2026-09-23
Review model: Claude Fable 5.1 (Claude Code)
Files changed: 2
Overview
Purpose (author): in every
XmlBrowseViewBasebrowse or bulk-edit view, once arow is selected the user cannot scroll it out of view; the viewport is dragged
back (LT-22676, duplicate of LT-20523). The branch changes only when the view
scrolls; the editing selection is never touched (LT-12092 constraint).
Analysis found three cooperating mechanisms, all confirmed with runtime tracing
in Bulk Edit Phoneme Features on Sena 3 before and after the change: a stale
SaveSelectionInfoanchor consumed byAdjustScrollRange1on the next lazy-boxexpansion; an infinite idle loop in
RemoveRootBoxSelectionOnIdle(about 80passes a second, each force-scrolling the row) whenever the current row has no
editable cell; and the same-index branch of the
SelectedIndexsetter alwaysscrolling. The fixes are scoped to
XmlBrowseViewBase;SimpleRootSiteis notedited.
Contract/API Changes
XmlBrowseViewBase.MakeSelectionVisible(IVwSelection, bool, bool, bool)isnow overridden (base is
protected virtualinSimpleRootSite); no subclassin the repository overrides it.
internal void ScrollSelectedRowIntoView();PostLayoutInitdelegatesto it. No public surface change.
SelectedIndexto the current index with the sameobject no longer scrolls the row into view. Callers that re-assert the current
index (
RecordBrowseView.OnRecordNavigation,ShowRecord) keep scrolling whenthe object at that index changed (filter/sort reload).
Findings
Critical - Must address before merge
None.
Important - Should address before merge
No unit test covers the idle-loop guard (m_fReplacingIdleSelection).(author: the fake browse view has no Mediator/IdleQueue; adding one is an
explicit decision deferred to a follow-up. Covered live: idle
MakeSelectionVisiblecalls in startup + 10 s dropped from about 1,800 to 7.)Minor - Consider
Same-index re-assert no longer scrolls an off-screen current row back(author: intended; approved in theinto view when the object is unchanged.
plan as the distinction between "selection changed" and "selection
re-asserted". Window-size changes are already handled by
SimpleRootSite.OnSizeChanged.)Required Validation / Evidence
.\test.ps1 -SkipNative -TestProject XMLViewsTests -CommentHygiene -TokenHygiene:119 passed; comment and token hygiene clean. Two of the five new tests fail
on
mainand pass on the branch; a third guards the LT-3607 behaviour.Features and Bulk Edit Wordforms leave the view where it was scrolled with
record 1 still current; idle loop gone.
the separate 3 px nudge, now LT-22489); Bulk Edit Phoneme Features scrolling.
filter/sort change, Find/Jump. The editing selection code is untouched.
.mcp.json,Docs/,Installer/) are not part of this branch and were left alone.Positive Observations
of the 2006/2011 scrolling code, and the LT-3607 and LT-12092 behaviours are
preserved and tested.
FakeXmlBrowseViewBase, with anObjectListPublisherfor the row list.Interview Notes
for
ScrollSelectedRowIntoViewto beinternal, since its only caller isPostLayoutInit.forgotten (needs a Mediator in the fake; ask first).
to be tested against this build.
Suggested Review Focus
RemoveRootBoxSelectionOnIdleguard: confirm the replacement insertionpoint should not queue a second cleanup pass.
SelectedIndex: agree that re-asserting the sameobject should not scroll.
MakeSelectionVisibleoverride: anchor cleared infinallycovers theearly-return paths in the base method.
🤖 Generated with Claude Code
This change is