refactor(tabs): give the editor tab strip's pointer input a single AppKit owner - #2571
Merged
Conversation
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Signed-off-by: Ngô Quốc Đạt <datlechin@gmail.com>
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.
Reordering editor tabs by drag worked by winning an argument. A press on a tab was claimed by a SwiftUI
Button, by the enclosingScrollView, by asimultaneousGesture(DragGesture), and by AppKit's own titlebar window drag, because the strip lives in anNSTitlebarAccessoryViewController. Which one got it depended on timing. This gives the press one owner.Fixes #2438.
What the report turned out to be
The reported symptom, the whole window travelling with the pointer, is already fixed. The video in the issue is 0.68.x, where reordering was an
.onDragdrag-and-drop session; #2472 replaced that with aDragGesturein 0.69.0.Measured at 0.69.0 on a real display, 13 drags across the strip, sidebar shown and hidden, tab centres from 131pt to 1392pt from the window's leading edge, holds of 0.02s and 0.6s: the window never moved once. That includes the 135pt position #2472 named as the unfixed half of this issue.
The second report, losing the ability to "detach them to create separate windows", is native window tabs. Those went with the single-window model in 0.65.0 (#2097), not with the double-click change that comment suspected.
What is still wrong is narrower and worse:
EditorTabReorderUITestshas never passed CI. All four cases are quarantined, and the quarantine note already names the strip's titlebar home as the suspect.hasCrossedcompared a float for equality against a midpoint that arrives from a geometry conversion, so it is 609.99998 as often as it is 610.Two candidate fixes, measured and refuted
Both are recorded so nobody spends the afternoon on them again.
mouseDownCanMoveWindow = falseon an ancestor changes nothing on its own. A standalone harness replicating the strip's exact mounting reportscanMove=trueon every view from the SwiftUI hit view up throughNSTitlebarAccessoryContainerView,NSTitlebarViewandNSThemeFrame, and the window still moved 0.0pt across a 200pt drag at every hold from 0ms to 600ms.NSHostingViewconsumes the mouse-down, so the theme frame never gets the chance the flag would deny it.An
NSPanGestureRecognizeron the hosting view is worse than what shipped. Attached to the strip'sNSHostingViewwithdelaysPrimaryMouseButtonEvents = false, it is deterministically dead wherever SwiftUI has a real control: 0 callbacks over a tab's title text and over its close button, 16 elsewhere, identical across repeated runs. The SwiftUIDragGestureit would replace fires everywhere.The change
EditorTabInteractionViewis the strip's superview and the only thing that receives a press over the track. It is a parent rather than a view laid over the tabs on purpose: a view mounted over the track covers every tab in the accessibility tree however little it draws, which is why the Escape monitor it replaces had to be a background. A parent adds no sibling and shadows nothing.mouseDownCanMoveWindowisfalse, so AppKit's window drag can never take a gesture that started on a tab. The band around the track keeps the default, so the empty chrome either side still moves the window the way Finder's tab bar does.hitTestclaims the track and nothing else. The new-tab button stays an ordinary SwiftUI button.nextEvent(matching:)tracking loop, withNSEventperiodic events mixed in so autoscroll keeps running while the pointer is held still at the edge. That is the shape AppKit controls have always used.NSApp.sendEvent, soCmd+WandCmd+Tstill work mid-drag, which the reorder state already handled.EditorTabRunLayoutis now the single source of the strip's geometry. The tab a user sees, the tab the pointer hits and the tab a drag targets are one rectangle, so they cannot disagree. SwiftUI keeps the drawing, the accessibility tree, and the keyboard and VoiceOver routes to every command.Multiple rows are the issue's own alternative, under Settings > General > Tabs > When tabs stop fitting. Scrolling stays the default because that is what every macOS tab bar does. A wrapped run never overflows, so no tab is off screen, and the titlebar accessory grows with it through
preferredContentSizeso the content below is laid out around the taller band. A wrapped track stops being a capsule: at that height the radius would curve away most of the first row's close target while the pointer still hit-tests the full rectangle.The reorder resolves a wrapped run by projecting the point onto the run it would have been unwrapped, so the midpoint rule stays one rule rather than growing a second one for rows.
Not in this PR
Detaching a tab into its own window. Since 0.65.0 a connection's session lives in exactly one window (
ConnectionWorkspace,activeSessions, keyed by connection), so tearing one tab out means one session visible in two windows. That is a change to the model whose invariantsCLAUDE.mdrecords as having shipped the same bug four times, and stacking it on an unshipped rewrite of the strip would make both unreviewable. The command set and the tear-off gesture are in place behindcanTearOff, which answersfalse; the follow-up branches off this one.Verified
buildPASS.testPASS, 87 cases: the newEditorTabRunLayoutTestsandEditorTabStripInteractionTests, the new crossing-tolerance cases, and all six existingEditorTabStrip*suites including the rasterisation ones, which still match pixel for pixel.TablePro,TableProTestsandTableProUITests. The violations the run reports are all inTableProTests/Core/Vim/VimEngineVisualReselectionTests.swift, untouched here and unlinted at HEAD because.swiftlint.ymlscopes toTablePro.docs/scripts/check-writing-style.shanddocs/scripts/check-docs-against-source.pyboth clean.What could not be verified, and why
TableProUITestsdid not run. Every case inEditorTabReorderUITestsandEditorTabKeepOpenUITestsfails at launch with "The sample database never finished opening". That is not this change: a worktree at cleanmainfails identically, and the app launched by hand from the same build copies the sample, then sits idle at 0% CPU with no window. Five consecutive launches in fresh sandboxes reproduced it.So the new interaction has unit coverage of its model and no end-to-end coverage of the gesture. Two guards are in the branch and should be run by CI or on a healthy machine before this merges:
testDraggingATabNeverMovesTheWindow, which asserts the window's origin across a drag. refactor(tabs): reorder editor tabs by direct manipulation instead of a drag session #2472 explicitly declined to assert it; it is the whole point ofmouseDownCanMoveWindowbeing false here.testAFastDragReordersTheStrip, a press-and-drag with no hold, which is the gesture a person makes. The suite's other cases hold for 0.6s first.The four previously quarantined cases are deliberately left in
.github/macos-ui-test-quarantine.txtfor now: taking them off that list is a claim about CI that this branch has no way to check.No screenshots. The strip is pixel-identical at rest in its default mode, and the wrapped mode could not be captured because the app would not open a window on this machine.
https://claude.ai/code/session_01L7uaHbJBPV1LaWL5QXzxyp