Skip to content

Fix SelectStart.pointer_start_offset applying scroll delta with wrong sign#6653

Open
vishnujayvel wants to merge 1 commit into
Textualize:mainfrom
vishnujayvel:fix-6639-selectstart-scroll-delta-sign
Open

Fix SelectStart.pointer_start_offset applying scroll delta with wrong sign#6653
vishnujayvel wants to merge 1 commit into
Textualize:mainfrom
vishnujayvel:fix-6639-selectstart-scroll-delta-sign

Conversation

@vishnujayvel

Copy link
Copy Markdown

Summary

Fixes #6639.

SelectStart.pointer_start_offset (src/textual/selection.py:141-147) was
adding the container's scroll delta instead of subtracting it:

return (
    self.container.region.offset
    + self.container_pointer_delta
    + (self.container.scroll_offset - self.container_initial_scroll_offset)
)

The compositor places children at placement_offset - scroll_offset
(src/textual/_compositor.py:631), so as a container scrolls (e.g. while
select-auto-scroll, added in #6440, is active), on-screen content moves in
the opposite direction to an increase in scroll_offset. The selection
anchor 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 in
    SelectStart.pointer_start_offset.
  • tests/test_selection.py: added
    test_select_start_offset_tracks_container_scroll_direction, a regression
    test 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 what
    Screen._start_auto_scroll's timer callback does) with no further mouse
    movement, and asserts the selection anchor
    (SelectState.ordered_offsets) moves in the direction consistent with the
    compositor rather than jumping ahead of the fixed pointer position.
  • CHANGELOG.md: added an Unreleased / Fixed entry crediting the issue.

Test plan

  • New regression test fails on main (unpatched) and passes after the fix:
    # before fix
    FAILED tests/test_selection.py::test_select_start_offset_tracks_container_scroll_direction
    AssertionError: assert Offset(x=0, y=1) == Offset(x=0, y=-1)
    
    # after fix
    tests/test_selection.py .......... [100%] 15 passed
    
  • tests/test_selection.py (all 15 tests, including the 14 pre-existing
    ones): pass.
  • Full non-snapshot suite (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 main checkout in this environment,
    caused by the textual-dev optional package not being installed here,
    not by this diff).
  • Snapshot suite: 31 pre-existing snapshot mismatches, all reproduced
    identically on a clean main checkout before this change (environment
    SVG/font rendering differences, not caused by this diff). None of them
    are new regressions from this fix.
  • black --check and isort --check pass clean on both changed files.

Checklist (per CONTRIBUTING.md)

  • Updated CHANGELOG.md
  • Formatted with black (black --check clean)
  • Docstrings present in the style of the surrounding code
  • Added regression test per the "fixing a bug" guideline, linked to the
    originating issue

… 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

SelectStart.pointer_start_offset applies the scroll delta with the wrong sign during cross-widget selection auto-scroll

1 participant