Fix keyboard reordering by restoring focus on sortable wrapper#85474
Fix keyboard reordering by restoring focus on sortable wrapper#85474MobileMage wants to merge 9 commits intoExpensify:mainfrom
Conversation
Removes the tabIndex={-1} override that prevented dnd-kit's KeyboardSensor
from activating. Adds an onFocus handler to maintain WCAG single tab stop
by redirecting stray focus and making nested elements non-tabbable. Extends
handleKeyDown to forward Enter to inner buttons.
melvin-bot
bot
requested review from
eVoloshchak and
heyjennahay
and removed request for
a team
|
@eVoloshchak Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| onFocus={(e) => { | ||
| for (const element of e.currentTarget.querySelectorAll<HTMLElement>('button, [tabindex]:not([tabindex="-1"])')) { | ||
| element.tabIndex = -1; | ||
| } |
There was a problem hiding this comment.
❌ CONSISTENCY-2 (docs)
The CSS selector string 'button, [tabindex]:not([tabindex="-1"])' is hardcoded inline. The file already establishes a pattern of extracting selectors to named constants (PRESSABLE_SELECTOR on line 7). This new selector should follow the same convention for consistency and readability.
Extract the selector to a named constant at the top of the file:
const FOCUSABLE_ELEMENTS_SELECTOR = 'button, [tabindex]:not([tabindex="-1"])';Then reference it in the onFocus handler:
for (const element of e.currentTarget.querySelectorAll<HTMLElement>(FOCUSABLE_ELEMENTS_SELECTOR)) {Please rate this suggestion with 👍 or 👎 to help us improve! Reactions are used to monitor reviewer efficiency.
Codecov Report❌ Looks like you've decreased code coverage for some files. Please write tests to increase, or at least maintain, the existing level of code coverage. See our documentation here for how to interpret this table.
|
|
Bug:
Basically, at this point focused utem and the item controlled with arrow keys become detached. I suspect we need to unsubscribe from arrow navigation if the item is unfocused Screen.Recording.2026-03-17.at.23.21.03.movSame issue on mWeb (please include recordings for all platforms) Screen.Recording.2026-03-17.at.23.28.10.movcc: @MobileMage |
|
@MobileMage can you merge main and address feedback ^ |
|
@MobileMage do you have an ETA for this PR |
|
Should de done today @eVoloshchak |
Kapture.2026-03-19.at.17.07.13.mp4@eVoloshchak I've fixed it by adding onBlur to the sortable items, one catch is that you'd have to hit tab twice (first one cancels the current drag), is that acceptable? |
The onBlur handler in SortableItem dispatches a synthetic Escape keydown to cancel dnd-kit keyboard drags when focus leaves the wrapper. This event was also caught by the app's global keyboard shortcut system (react-native-key-command captures on document), triggering EscapeHandler and closing the sidebar/modal. Extract the synthetic dispatch into cancelDndKeyboardDrag with a synchronous module-level flag. Guard bindHandlerToKeydownEvent (the entry point for all KeyboardShortcut subscribers) so the entire shortcut system is skipped during the synthetic dispatch. Also handle Tab in onKeyDownCapture during an active drag so the drag is cancelled before the browser moves focus, allowing both cancel and focus move in a single Tab press.
I think yes, but I'd like to test how intuitive that is in practice. Could you push the changes please? |
|
Pushed @eVoloshchak |
| // Cancel drag on Tab but let default Tab behavior move focus naturally. | ||
| // This must happen in capture phase (before blur) so the drag ends | ||
| // before the browser moves focus, avoiding a render that eats the Tab. | ||
| if (isDragging && e.key === 'Tab') { |
There was a problem hiding this comment.
| if (isDragging && e.key === 'Tab') { | |
| if (isDragging && e.key === CONST.KEYBOARD_SHORTCUTS.TAB.shortcutKey) { |
|
Bug:
Expected result: as the last element is focused, pressing Tab will focus the first element again (back button at the top) Screen.Recording.2026-03-20.at.13.23.49.mov |
|
@MobileMage bump ^^ |
The cancelDndKeyboardDrag helper dispatches a synthetic Escape event that reaches focus-trap's bubble-phase listener, deactivating the trap and breaking Tab cycling. Add escapeDeactivates guard so focus-trap ignores the synthetic event while the dnd cancellation flag is set. Also add CONST.KEYBOARD_SHORTCUTS.TAB and use it in SortableItem.
| DEFAULT: {input: keyInputSpace}, | ||
| }, | ||
| }, | ||
| TAB: { |
There was a problem hiding this comment.
This isn't needed, TAB key already exists (line 963)
|
Desktop web is looking good! @MobileMage, I noticed the screenshots/videos section is missing videos for native and mWeb. Please test every update for the PR, running all of the test cases on all platforms that are in the checklist. Screen.Recording.2026-03-25.at.16.31.17.movBut the same scenario on native fails, you're navigated away after pressing Space Screen.Recording.2026-03-25.at.16.35.16.mov |
|
Will prioritize this tomorrow |
heyjennahay
left a comment
heyjennahay
left a comment
There was a problem hiding this comment.
Product review not required
|
Thanks! @eVoloshchak I looked into this and the issue is that dnd-kit's KeyboardSensor only works on web. On native, we use react-native-draggable-flatlist which is touch-only, so when you hit Space it just falls through to the regular onPress and navigates away. I'm thinking I could build a simple keyboard reorder mode for native separately from dnd-kit. Basically Space puts the item into a "moving" state, arrow keys swap its position in the list, and Space/Escape exits. No drag animations tho. Also included videos for android and ios mweb testing. Worth doing? |
Yeah, that seems fine to me, this is a pretty rare edge case |
…order-single-tabstop-v2
On native, dnd-kit's KeyboardSensor doesn't work because we use react-native-draggable-flatlist which is touch-only. This adds a simple keyboard reorder mode: - Arrow keys navigate focus between items (via useArrowKeyFocusManager) - Space enters moving mode for the focused item - Arrow Up/Down swap the item position while in moving mode - Space confirms the reorder, Escape cancels - Touch drag cancels keyboard moving mode Also passes isKeyboardMoving through renderItem params so consumers can disable navigation (onPress) during keyboard reorder.
|
@MobileMage, is this ready for review? Screen.Recording.2026-04-07.at.08.41.29.mov |
|
Nope, this is proving really difficult. @eVoloshchak |
|
I dug into the native Space behavior I can't intercept it on iOS without native code @eVoloshchak |
|
@MobileMage, would using |
|
Yeah, that's what I used when testing, when Full Keyboard Access is enabled, iOS consumes Space at the system level to trigger accessibility activation on the focused view (calling |
4 participants
Explanation of Change
Removes the
tabIndex={-1}override on the SortableItem wrapper that was preventing dnd-kit's KeyboardSensor from activating. Adds anonFocushandler to maintain WCAG single-tab-stop behavior by making nested interactive children non-tabbable and redirecting stray focus to the wrapper. ExtendshandleKeyDownto forward Enter to inner buttons since they are no longer directly tabbable.Fixed Issues
$ #79247
PROPOSAL: #79247 (comment)
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
Kapture.2026-03-27.at.22.38.02.mp4
iOS: Native
iOS: mWeb Safari
Kapture.2026-03-27.at.22.38.46.mp4
MacOS: Chrome / Safari
Kapture.2026-03-09.at.17.31.10.mp4