Skip to content

Mac: stop selection auto-scroll when the view leaves the hierarchy - #631

Open
jizhi0v0 wants to merge 2 commits into
migueldeicaza:mainfrom
jizhi0v0:spike/mouseup-autoscroll-repro
Open

Mac: stop selection auto-scroll when the view leaves the hierarchy#631
jizhi0v0 wants to merge 2 commits into
migueldeicaza:mainfrom
jizhi0v0:spike/mouseup-autoscroll-repro

Conversation

@jizhi0v0

@jizhi0v0 jizhi0v0 commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Problem

The selection auto-scroll timer added in f971cbf (fixing #309) is torn down only from mouseUp. A view taken out of the view hierarchy while a drag is in flight never receives that mouseUp, so the timer keeps running — it scrolls a view that is no longer on screen, and since the timer block captures the view weakly, once the view goes away nothing is left that can invalidate it and the run loop holds it forever.

mouseExited, the didResignKeyNotification observer and deinit all leave it alone, so mouseUp really is the only exit today.

How it was verified

A standalone AppKit probe (custom NSView logging every mouse event plus a 20 Hz tick):

  • Removed the view from its superview mid-drag → no mouseUp was ever delivered, and the tick kept reporting the drag as in progress for as long as the probe was left running.
  • Control: pressed inside, released outside the windowmouseUp delivered normally. The ordinary drag-past-the-edge case f971cbf fixed is unaffected by this change.

Closing a tab or a split pane during a selection drag is exactly the first sequence.

Fix

Tear the timer down in viewDidMoveToWindow() when the view has left its window, with deinit as a backstop.

Tests

Tests/SwiftTermTests/SelectionAutoScrollTimerLifecycleTests.swift covers both directions:

  • removing the view mid-drag stops the timer (asserting the timer itself is gone, not merely that the viewport stopped moving);
  • an ordinary in-flight drag keeps auto-scrolling until mouseUp.

The second test is there to rule out a tempting but wrong class of fix: polling whether the button is still held. NSEvent.pressedMouseButtons looks like the obvious signal, but AppKit documents it as "not suitable for tracking", and it reports no button pressed during accessibility drag lock as well as in synthetic test drags — a fix built on it would stop legitimate drags.

Full suite: 700 tests in 62 suites, all passing.

The selection auto-scroll timer added in f971cbf is torn down only from
mouseUp. A view taken out of the view hierarchy while a drag is in flight
never receives that mouseUp, so the timer keeps running: it scrolls a view
that is no longer on screen, and because the timer block captures the view
weakly, once the view goes away nothing is left that can invalidate it and
the run loop holds it forever.

Verified with a standalone AppKit probe: a view removed from its superview
mid-drag logged no mouseUp at all, while a 20 Hz tick kept reporting the
drag as in progress for as long as the probe was left running. Releasing
the button outside the window, by contrast, delivered mouseUp normally, so
the ordinary drag-past-the-edge case f971cbf fixed is unaffected.

Tear the timer down in viewDidMoveToWindow() when the view has left its
window, with deinit as a backstop. Closing a tab or a split pane during a
selection drag is exactly this sequence.

The tests cover both directions: removing the view mid-drag stops the
timer, and an ordinary in-flight drag keeps auto-scrolling until mouseUp.
The second one matters because it rules out fixes that guess at whether
the button is still held: NSEvent.pressedMouseButtons looks like the
obvious signal, but AppKit documents it as "not suitable for tracking",
and it reports no button during accessibility drag lock and in synthetic
test drags.
@jizhi0v0
jizhi0v0 force-pushed the spike/mouseup-autoscroll-repro branch from 2850f3a to 27a26f3 Compare August 13, 2026 16:14
@jizhi0v0 jizhi0v0 changed the title Mac: stop selection auto-scroll when the mouse button is no longer down Mac: stop selection auto-scroll when the view leaves the hierarchy Aug 13, 2026
@jizhi0v0

Copy link
Copy Markdown
Contributor Author

Heads-up: I force-pushed a different implementation than the one I originally opened this with, before anyone had to spend review time on it.

The first version polled NSEvent.pressedMouseButtons from inside the timer callback, on the theory that the drag could end without a mouseUp when the app loses key window. Two problems with that, both my fault:

  1. I never actually reproduced the app-switch scenario — it was inferred from AppKit's tracking semantics, not observed. When I built a probe to check, what I could reproduce deterministically was something else: a view removed from the hierarchy mid-drag never gets its mouseUp. Releasing the button outside the window, which I had assumed was the risky case, delivers mouseUp normally.
  2. NSEvent.pressedMouseButtons is documented as "not suitable for tracking", and it reports no button during accessibility drag lock — so that version could have stopped auto-scroll during a perfectly legitimate drag.

The current version tears the timer down in viewDidMoveToWindow() (plus deinit), which targets the reproduced case directly, and the tests now assert both that the timer is gone after view removal and that an ordinary drag keeps scrolling until mouseUp.

Happy to adjust further — in particular, if you'd rather also cover viewDidMoveToSuperview() for views that move between superviews inside the same window, say the word.

Gating the teardown on `window == nil` missed two moves that also cost the
view its mouseUp: reparenting within the same window, and moving directly
from one window to another. Probed with a minimal AppKit view logging the
callbacks: viewDidMoveToWindow fires for both, with a non-nil window, so
the earlier condition skipped them. It fires for the first insertion too,
where no drag is in flight and the teardown is a no-op.

viewDidMoveToSuperview is not needed: viewDidMoveToWindow already covers
every hierarchy move, same-window reparents included.

Adds a test for the same-window reparent, and drops two comments that
claimed more than was actually verified.
@jizhi0v0

Copy link
Copy Markdown
Contributor Author

Answering my own question from the previous comment, rather than leaving it for you.

I checked what AppKit actually calls, with a minimal view logging the hierarchy callbacks:

move viewDidMoveToWindow fires window at that point
first insertion yes non-nil
reparent within the same window yes non-nil
direct move to another window yes non-nil
removeFromSuperview yes nil

So two things: gating the teardown on window == nil was wrong — it skipped the same-window reparent and the direct window-to-window move, both of which also cost the view its mouseUp — and viewDidMoveToSuperview is not needed, since viewDidMoveToWindow already covers every hierarchy move.

Pushed a follow-up that tears the drag down unconditionally there (a no-op on first insertion, where nothing is in flight), with a test for the same-window reparent. I also dropped two comments that claimed more coverage than I had actually verified.

701 tests in 62 suites passing.

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.

1 participant