Keyboard pointer movement on arrow keys, ancestor stack on Tab/Shift+Tab#383
Open
aidenybai wants to merge 10 commits into
Open
Keyboard pointer movement on arrow keys, ancestor stack on Tab/Shift+Tab#383aidenybai wants to merge 10 commits into
aidenybai wants to merge 10 commits into
Conversation
…tack on tab/shift+tab Replace discrete element-stepping arrow navigation with a continuous hold-to-move scheme: while an arrow key is held, the pointer position advances at a steady speed and the highlight follows whatever element it crosses. On release, the selection snaps onto the element under the resting pointer, matching mouse-driven freeze behavior. The previous ancestor-stack traversal that ArrowUp/ArrowDown used (stepping out to a parent and back to the child via history) is now bound to Tab and Shift+Tab. Tests are updated to exercise hold-to-move via keyboard.down/up plus a delay, and parent-stack tests are switched to Tab/Shift+Tab. Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
commit: |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
1 issue found across 7 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…still held If Tab fired while an arrow key was physically held, the rAF loop kept walking the pointer and the eventual arrow keyup would snap-freeze on the element under the moved pointer, overwriting the ancestor selection chosen by Tab. Stop the rAF immediately and mark the held arrow keys as suppressed. Subsequent keydown repeats and the eventual keyup are still tracked so we know when every arrow has been released, but they no longer reseed, unfreeze, or snap-freeze. Reported by Cursor Bugbot on #383. Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
…n blank space Holding an arrow key unfreezes the selection so the highlight can follow the moving pointer. Previously, if the user released the arrow with the pointer over blank space or a non-grabbable element, the snap-freeze branch did nothing — leaving the user in active+hovering with no frozen element, so the next mouse move would pull the box away. Cache the element we seeded keyboard movement from and freeze on it as a fallback whenever hit-testing at the resting pointer turns up nothing grabbable. Reset the seed on release, blur, or deactivate. Reported by cubic on #383. Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
| if (elementAtPointer && isValidGrabbableElement(elementAtPointer)) { | ||
| selectAndFocusElement(elementAtPointer); | ||
| } | ||
| } |
There was a problem hiding this comment.
Pointer loop ignores interaction locks
Medium Severity
The keyboard pointer requestAnimationFrame tick and arrow keyup snap-freeze only bail out for activation and prompt mode. They do not mirror handleKeyboardPointerMovement, which skips dragging, shift multi-select, and an open context menu. Held arrows can keep moving the pointer during a drag, and releasing them can call selectAndFocusElement (repositioning the context menu) while those modes are active.
Reviewed by Cursor Bugbot for commit db740ce. Configure here.
Contributor
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/react-grab/src/core/index.tsx">
<violation number="1" location="packages/react-grab/src/core/index.tsx:2277">
P2: Reset `keyboardPointerLastTimestamp` when stopping keyboard pointer movement so the next hold starts from 0 ms.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Comment on lines
+2277
to
+2282
| if (heldKeyboardPointerKeys.size > 0 || keyboardPointerRafId !== null) { | ||
| stopKeyboardPointerMovement(); | ||
| if (heldKeyboardPointerKeys.size > 0) { | ||
| isKeyboardPointerSuppressed = true; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
P2: Reset keyboardPointerLastTimestamp when stopping keyboard pointer movement so the next hold starts from 0 ms.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/react-grab/src/core/index.tsx, line 2277:
<comment>Reset `keyboardPointerLastTimestamp` when stopping keyboard pointer movement so the next hold starts from 0 ms.</comment>
<file context>
@@ -2238,6 +2269,18 @@ export const init = (rawOptions?: Options): ReactGrabAPI => {
+ // keyup don't reactivate the loop or snap-freeze over the ancestor
+ // selection. heldKeyboardPointerKeys stays populated so we can detect
+ // when every arrow has actually been released.
+ if (heldKeyboardPointerKeys.size > 0 || keyboardPointerRafId !== null) {
+ stopKeyboardPointerMovement();
+ if (heldKeyboardPointerKeys.size > 0) {
</file context>
Suggested change
| if (heldKeyboardPointerKeys.size > 0 || keyboardPointerRafId !== null) { | |
| stopKeyboardPointerMovement(); | |
| if (heldKeyboardPointerKeys.size > 0) { | |
| isKeyboardPointerSuppressed = true; | |
| } | |
| } | |
| if (heldKeyboardPointerKeys.size > 0 || keyboardPointerRafId !== null) { | |
| keyboardPointerLastTimestamp = 0; | |
| stopKeyboardPointerMovement(); | |
| if (heldKeyboardPointerKeys.size > 0) { | |
| isKeyboardPointerSuppressed = true; | |
| } | |
| } |
…oard pointer mode and route Tab from overlay focus Two interaction edge cases caught on review: 1. The rAF tick and arrow keyup branch only bailed for activated + prompt-mode, while the keydown gate also blocked dragging, shift multi-select, and an open context menu. Held arrows could keep walking the pointer once one of those modes started, and the eventual keyup snap-freeze would override the context menu's selection. Centralize the predicate in isKeyboardPointerContextValid() and use it from keydown, the rAF tick, and the keyup snap-freeze decision. 2. Tab/Shift+Tab were never routed when the keyboard event originated from inside data-react-grab-ignore-events overlay focus (selection label, ancestor menu items). Arrow keys had a special case here; extend the same case to Tab so ancestor stepping keeps working from overlay-focused contexts. Reported by Cursor Bugbot on #383. Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
…ld-to-move When the user starts holding an arrow key, we unfreeze so the highlight follows the moving pointer. keyboardSelectedElement was left pointing at the previous Tab/snap-freeze target. handleSingleClick's fallback chain (selectedElementUnderPointer → frozenElement → keyboardSelectedElement) could then reach back to that stale element and copy the wrong node if a click landed on blank space during a hold. Drop the reference at the same time we unfreeze. Reported by Cursor Bugbot on #383. Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
… held The pointermove listener still ran while arrow keys were physically held, causing two interactions: 1. After Tab froze an ancestor while arrows were still down, any incidental mouse motion saw isFrozenPhase() and called actions.unfreeze() + clearArrowNavigation(), undoing the ancestor selection before the eventual arrow keyup. 2. While arrows actively drove the pointer, mouse motion called handlePointerMove which retargeted pointer/detectedElement in parallel with the rAF tick. Bail out of the pointermove handler entirely whenever an arrow key is in heldKeyboardPointerKeys, so the keyboard owns the pointer until all arrows are released. Reported by Cursor Bugbot on #383. Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
…e pointer Previous fix returned early from pointermove whenever any arrow key was held, which broke drag repositioning and preview updates if the user happened to start a drag while an arrow key was still down. Narrow the gate: skip pointermove only when arrows are held AND isKeyboardPointerContextValid() (i.e. nothing else — drag, shift-multi-select, prompt mode, context menu — has taken over). When one of those modes preempts keyboard pointer movement, the rAF tick already exits, and pointermove can flow through to drive that mode's own pointer logic. Reported by Cursor Bugbot on #383. Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
…AF after interrupts Two further interaction issues caught on review: 1. Starting hold-to-move called clearArrowNavigation() which also wiped the ancestorStackNavigator history. After a brief arrow nudge, Shift+Tab could no longer step back through prior Tab selections. Split clearArrowNavigation into closeArrowNavigationMenu (just hides the stacked-ancestors menu) plus clearHistory, and use the menu-only variant from the keyboard pointer movement path so the back-navigation stack survives. 2. The rAF tick exited and stopped scheduling itself whenever isKeyboardPointerContextValid() turned false (drag, shift multi- select, prompt mode, context menu). Once the interruption ended, movement stayed idle until the user released and re-pressed arrows. Keep scheduling the tick but skip the position update while interrupted, so the loop transparently resumes when the gating mode ends. Reported by Cursor Bugbot on #383. Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
…w keys are held Previous implementation only moved the underlying pointer position and let the selection box re-snap to whichever element happened to be under it. With horizontally-laid-out wide elements (e.g. a full-width <li>), holding ArrowLeft/Right kept the box locked on the same element so the overlay didn't appear to move at all — and the user reported up/down having the same problem on their layout. Render the selection box from a free-floating bounds signal that's seeded from the held-element's geometry on first arrow press and translated in place by the per-frame pointer delta. The box now slides smoothly in the held direction regardless of how elements are laid out underneath. On release, clear the floating bounds and snap-freeze onto whatever element is under the resting pointer (or fall back to the seed when over blank space). Also bump selectionVisible() so the overlay stays rendered even if the pointer slides over a non-grabbable region while held. Strengthen the keyboard-pointer-movement e2e tests so they actually assert the box translates >50px in each axis instead of just checking visibility. Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/react-grab/src/core/index.tsx">
<violation number="1" location="packages/react-grab/src/core/index.tsx:2277">
P2: Reset `keyboardPointerLastTimestamp` when stopping keyboard pointer movement so the next hold starts from 0 ms.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
…sition instead of inflating the box Previous attempt rendered the selection box itself as a free-floating rectangle the size of whatever element seeded the hold. That made the overlay look enormous when the seed was a wide container, and there was no separate indicator showing where the keyboard pointer actually was. Replace the box-translation approach with a small fixed-size cursor indicator (a 14px rounded circle) drawn at the keyboard pointer position while any arrow key is held. Behaviors: - The cursor only renders during a keyboard hold; clears on release/Tab-suppression/blur/deactivate. - The selection box continues to follow whichever element is under the pointer (live preview of what would snap on release), instead of being detached and translated as before. - Snap-on-release keeps its existing fallback to the seed element when the pointer rests over blank space. Plumb a new keyboardPointerCursor: Position | null prop through ReactGrabRendererProps and render it as a small DOM element in renderer.tsx. New constant KEYBOARD_POINTER_CURSOR_SIZE_PX = 14. Tests now query the [data-react-grab-keyboard-cursor] element directly to verify it appears, translates >50px in each axis under a hold, and disappears on release. Co-authored-by: Aiden Bai <aidenybai@users.noreply.github.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 7289125. Configure here.
| if (heldKeyboardPointerKeys.size > 0) { | ||
| isKeyboardPointerSuppressed = true; | ||
| } | ||
| } |
There was a problem hiding this comment.
Tab leaves keyboard cursor visible
Medium Severity
When Tab is pressed while arrow keys are still held, handleAncestorStackNavigation stops the keyboard pointer loop and freezes the ancestor selection, but it never clears keyboardPointerCursor. The fake cursor keeps showing at the last arrow-driven position until every arrow key is released, even though the highlight and pointer already jump to the ancestor via selectAndFocusElement.
Reviewed by Cursor Bugbot for commit 7289125. Configure here.
Contributor
There was a problem hiding this comment.
2 issues found across 5 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/react-grab/src/core/index.tsx">
<violation number="1" location="packages/react-grab/src/core/index.tsx:2244">
P2: Clear `keyboardPointerCursor` when switching to Tab/Shift+Tab ancestor-stack navigation while arrow keys are held; otherwise the fake cursor can remain visible at a stale position until the arrow keyup path runs.</violation>
</file>
<file name="packages/react-grab/src/components/renderer.tsx">
<violation number="1" location="packages/react-grab/src/components/renderer.tsx:46">
P2: The keyboard pointer cursor is slightly misaligned from the real pointer because border width is added on top of the configured size. This offsets the visual cursor by ~2px and can make snap-preview positioning look inaccurate.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| // the user can see where the pointer is moving while held. The | ||
| // selection box continues to follow whichever element is under | ||
| // it (live preview of what would snap on release). | ||
| setKeyboardPointerCursor(cursorStart); |
Contributor
There was a problem hiding this comment.
P2: Clear keyboardPointerCursor when switching to Tab/Shift+Tab ancestor-stack navigation while arrow keys are held; otherwise the fake cursor can remain visible at a stale position until the arrow keyup path runs.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/react-grab/src/core/index.tsx, line 2244:
<comment>Clear `keyboardPointerCursor` when switching to Tab/Shift+Tab ancestor-stack navigation while arrow keys are held; otherwise the fake cursor can remain visible at a stale position until the arrow keyup path runs.</comment>
<file context>
@@ -2237,25 +2219,29 @@ export const init = (rawOptions?: Options): ReactGrabAPI => {
+ // the user can see where the pointer is moving while held. The
+ // selection box continues to follow whichever element is under
+ // it (live preview of what would snap on release).
+ setKeyboardPointerCursor(cursorStart);
// Close the ancestors menu since the user is now moving freely,
</file context>
| width: `${KEYBOARD_POINTER_CURSOR_SIZE_PX}px`, | ||
| height: `${KEYBOARD_POINTER_CURSOR_SIZE_PX}px`, | ||
| "border-radius": "50%", | ||
| border: `2px solid ${OVERLAY_BORDER_COLOR_DEFAULT}`, |
Contributor
There was a problem hiding this comment.
P2: The keyboard pointer cursor is slightly misaligned from the real pointer because border width is added on top of the configured size. This offsets the visual cursor by ~2px and can make snap-preview positioning look inaccurate.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/react-grab/src/components/renderer.tsx, line 46:
<comment>The keyboard pointer cursor is slightly misaligned from the real pointer because border width is added on top of the configured size. This offsets the visual cursor by ~2px and can make snap-preview positioning look inaccurate.</comment>
<file context>
@@ -28,6 +32,26 @@ export const ReactGrabRenderer: Component<ReactGrabRendererProps> = (props) => {
+ width: `${KEYBOARD_POINTER_CURSOR_SIZE_PX}px`,
+ height: `${KEYBOARD_POINTER_CURSOR_SIZE_PX}px`,
+ "border-radius": "50%",
+ border: `2px solid ${OVERLAY_BORDER_COLOR_DEFAULT}`,
+ "background-color": OVERLAY_FILL_COLOR_DEFAULT,
+ "pointer-events": "none",
</file context>
Suggested change
| border: `2px solid ${OVERLAY_BORDER_COLOR_DEFAULT}`, | |
| border: `2px solid ${OVERLAY_BORDER_COLOR_DEFAULT}`, | |
| "box-sizing": "border-box", |
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
2 participants
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.
What changed
The arrow-key behavior in
react-grabhas been redesigned.Before
ArrowLeft/ArrowRightstepped through siblings/descendants in discrete jumps.ArrowUp/ArrowDownwalked out to ancestors and back through a navigation stack.After
ArrowUp/ArrowDownused to handle (step out to a parent, step back via history).Why
The user wanted the box to "literally move right or left" while the key is held and snap to "whatever element" on release — making keyboard control feel like a slow-motion mouse cursor instead of a list-style navigator. The ancestor-stack stepping is still useful but earns its own dedicated keys.
Implementation notes
packages/react-grab/src/core/arrow-navigation.tsis renamed in spirit: the module now exportscreateAncestorStackNavigatorwithstepUp/stepDown. The horizontal sibling/descendant logic was deleted.core/index.tsx:Set<string>.requestAnimationFrameloop while at least one arrow key is held; per-frame it advancespointer()byKEYBOARD_POINTER_SPEED_PX_PER_MS * deltaMsalong the held direction vector and re-runsgetElementAtPosition.KEYBOARD_POINTER_VIEWPORT_PADDING_PX.selectAndFocusElement).Tab/Shift+Tabare intercepted in the same activated state and routed through the ancestor stack navigator. The stacked-ancestors menu (arrowNavigationState) is still shown — only the trigger keys changed.deactivateRenderer) clear held keys and cancel the rAF loop so a lost keyup never leaves the loop running.MIN_HORIZONTAL_NAV_SIZE_PXandHORIZONTAL_NAV_RECT_MATCH_TOLERANCE_PX. AddedKEYBOARD_POINTER_SPEED_PX_PER_MSandKEYBOARD_POINTER_VIEWPORT_PADDING_PX.Tests
e2e/keyboard-navigation.spec.ts:Keyboard Pointer Movementblock exercises the hold semantics viakeyboard.down/keyboard.upwith a delay, validates that holdingArrowDownslides the highlight onto a different element, and confirms snap-freeze survives subsequent mouse motion.ArrowUp Vertical Traversaltests were converted toTab Ancestor Stack Navigation(Tab / Shift+Tab), preserving their original intent.e2e/disabled-elements.spec.tsto use Tab / Shift+Tab where it previously asserted ancestor traversal acrosspointer-events: noneelements.holdArrowKey(key, durationMs),pressTab,pressShiftTab.react-grab.expect.tsLLM prompt for the new keybinds.Verification
pnpm build✓pnpm typecheck✓pnpm lint✓pnpm format✓Summary by cubic
Redesigned keyboard navigation in
react-grab: hold arrow keys to move a virtual pointer shown as a small on-screen cursor, then snap-freeze on release; Tab/Shift+Tab navigate the ancestor stack. Tab takes priority over held arrows, movement pauses during drag/multi-select/context-menu, and releasing over blank space snaps back to the seed element.Bug Fixes
Migration
Written for commit 7289125. Summary will update on new commits. Review in cubic