Fix SelectStart.pointer_start_offset applying scroll delta with wrong sign#6653
Open
vishnujayvel wants to merge 1 commit into
Open
Fix SelectStart.pointer_start_offset applying scroll delta with wrong sign#6653vishnujayvel wants to merge 1 commit into
vishnujayvel wants to merge 1 commit into
Conversation
… sign SelectStart.pointer_start_offset added the container's scroll delta instead of subtracting it. The compositor places children at `placement_offset - scroll_offset`, so as a container scrolls during select-auto-scroll, the selection anchor needs to move the same way the content does on screen (subtract the delta), not the opposite way (add it). With the wrong sign, the anchor could be pushed past the (unmoved) pointer position, inverting/collapsing cross-widget selections mid-drag, as introduced by Textualize#6440. Adds a regression test that simulates a scroll tick (as performed by the select-auto-scroll timer) without any further mouse movement and asserts the anchor moves in the correct direction relative to the fixed pointer position. Reported by @PilsnerChamp, who also suggested the one-character sign fix applied here. Fixes Textualize#6639
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
Fixes #6639.
SelectStart.pointer_start_offset(src/textual/selection.py:141-147) wasadding the container's scroll delta instead of subtracting it:
The compositor places children at
placement_offset - scroll_offset(
src/textual/_compositor.py:631), so as a container scrolls (e.g. whileselect-auto-scroll, added in #6440, is active), on-screen content moves in
the opposite direction to an increase in
scroll_offset. The selectionanchor needs to track that same movement, i.e. the scroll delta must be
subtracted, not added. With the wrong sign, the anchor moves the wrong way
relative to the (unmoved) pointer position, which can push it past the
pointer mid-drag and cause cross-widget selections to invert, collapse, and
restart while auto-scroll is advancing.
I've been enjoying Textual and wanted to contribute something back, so I
started digging through the open issues looking for a good first fix — this
one turned out to be a clean, self-contained bug, and the fix works.
Full credit to @PilsnerChamp (#6639), who reported it, identified the root
cause, and suggested the one-character sign fix. I independently traced the
compositor's placement math to confirm the sign is wrong before applying it,
and added a regression test. @PilsnerChamp mentioned they'd be happy to open
a PR ("I'm happy to open a PR ... if welcome") — I hope this is a welcome
head start rather than stepping on toes; more than happy to defer or hand it
over if they'd prefer to land it themselves.
Changes
src/textual/selection.py: flip+to-for the scroll-delta term inSelectStart.pointer_start_offset.tests/test_selection.py: addedtest_select_start_offset_tracks_container_scroll_direction, a regressiontest that starts a selection inside a scrollable container, then simulates
a single select-auto-scroll tick (
scroller.scroll_y += 2; screen._update_select(), mirroring exactly whatScreen._start_auto_scroll's timer callback does) with no further mousemovement, and asserts the selection anchor
(
SelectState.ordered_offsets) moves in the direction consistent with thecompositor rather than jumping ahead of the fixed pointer position.
CHANGELOG.md: added anUnreleased / Fixedentry crediting the issue.Test plan
main(unpatched) and passes after the fix:tests/test_selection.py(all 15 tests, including the 14 pre-existingones): pass.
pytest tests/ --ignore=tests/snapshot_tests):3012 passed, 1 skipped, 4 xfailed, 1 pre-existing failure
(
tests/test_features.py::test_textual_env_var, unrelated to this change— fails identically on a clean
maincheckout in this environment,caused by the
textual-devoptional package not being installed here,not by this diff).
identically on a clean
maincheckout before this change (environmentSVG/font rendering differences, not caused by this diff). None of them
are new regressions from this fix.
black --checkandisort --checkpass clean on both changed files.Checklist (per CONTRIBUTING.md)
CHANGELOG.mdblack --checkclean)originating issue