Fix search not scrolling to result past view #19571
Open
+11
−20
Conversation
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 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 theSearchRequestwhen conducting a search. This would get toTerminal::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 callTerminal::_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.