Skip to content

Commit ee6060b

Browse files
Fix search not scrolling to result past view (#19571)
## Summary of the Pull Request Fixes a bug where search would not scroll to results just below the viewport. This was caused by code intended to scroll the search result in such a way that it isn't covered by the search box. The scroll offset is calculated in `TermControl::_calculateSearchScrollOffset()` then handed down in the `SearchRequest` when conducting a search. This would get to `Terminal::ScrollToSearchHighlight()` where the offset is applied to the search result's position so that we would scroll to the adjusted position. The adjustment was overly aggressive in that it would apply it to both "start" and "end". In reality, we don't need to apply it to "end" because it wouldn't be covered by the search box (we only scroll to end if it's past the end of the current view anyways). The fix applies the adjustment only to "start" and only does so if it's actually in the first few rows that would be covered by the search box. That unveiled another bug where `Terminal::_ScrollToPoints()` would also be too aggressive about scrolling the "end" into view. In some testing, it would generally end up scrolling to the end of the buffer. To fix this cascading bug, I just had `_ScrollToPoints()` just call `Terminal::_ScrollToPoint()` (singular, not plural) which is consistently used throughout the Terminal code for selection (so it's battle tested). `_ScrollToPoints()` was kept since it's still used for accessibility when selecting a new region to keep the new selection in view. It's also just a nice wrapper that ensures a range is visible (or at least as much as it could be). ## References and Relevant Issues Scroll offset was added in #17516 ## Validation Steps Performed ✅ search results that would be covered by the search box are still adjusted ✅ search results that are past the end of the view become visible ✅ UIA still selects properly and brings the selection into view ## PR Checklist Duncan reported this bug internally, but there doesn't seem to be one on the repo.
1 parent 81cdb07 commit ee6060b

File tree

2 files changed

+11
-20
lines changed

2 files changed

+11
-20
lines changed

src/cascadia/TerminalCore/Terminal.cpp

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1297,9 +1297,15 @@ void Terminal::ScrollToSearchHighlight(til::CoordType searchScrollOffset)
12971297
if (_searchHighlightFocused < _searchHighlights.size())
12981298
{
12991299
const auto focused = til::at(_searchHighlights, _searchHighlightFocused);
1300-
const auto adjustedStart = til::point{ focused.start.x, std::max(0, focused.start.y - searchScrollOffset) };
1301-
const auto adjustedEnd = til::point{ focused.end.x, std::max(0, focused.end.y - searchScrollOffset) };
1302-
_ScrollToPoints(adjustedStart, adjustedEnd);
1300+
1301+
// Only adjust the y coordinates if "start" is in a row that would be covered by the search box
1302+
auto adjustedStart = focused.start;
1303+
const auto firstVisibleRow = _VisibleStartIndex();
1304+
if (focused.start.y > firstVisibleRow && focused.start.y < firstVisibleRow + searchScrollOffset)
1305+
{
1306+
adjustedStart.y = std::max(0, focused.start.y - searchScrollOffset);
1307+
}
1308+
_ScrollToPoints(adjustedStart, focused.end);
13031309
}
13041310
}
13051311

src/cascadia/TerminalCore/terminalrenderdata.cpp

Lines changed: 2 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -152,29 +152,14 @@ const til::point_span* Terminal::GetSearchHighlightFocused() const noexcept
152152
// - The updated scroll offset
153153
til::CoordType Terminal::_ScrollToPoints(const til::point coordStart, const til::point coordEnd)
154154
{
155-
auto notifyScrollChange = false;
156155
if (coordStart.y < _VisibleStartIndex())
157156
{
158-
// recalculate the scrollOffset
159-
_scrollOffset = ViewStartIndex() - coordStart.y;
160-
notifyScrollChange = true;
157+
_ScrollToPoint(coordStart);
161158
}
162159
else if (coordEnd.y > _VisibleEndIndex())
163160
{
164-
// recalculate the scrollOffset, note that if the found text is
165-
// beneath the current visible viewport, it may be within the
166-
// current mutableViewport and the scrollOffset will be smaller
167-
// than 0
168-
_scrollOffset = std::max(0, ViewStartIndex() - coordStart.y);
169-
notifyScrollChange = true;
161+
_ScrollToPoint(coordEnd);
170162
}
171-
172-
if (notifyScrollChange)
173-
{
174-
_activeBuffer().TriggerScroll();
175-
_NotifyScrollEvent();
176-
}
177-
178163
return _VisibleStartIndex();
179164
}
180165

0 commit comments

Comments
 (0)