Skip to content

fix(#3842): DnD Nested Drop Regions example can't focus child drop target #3843

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 16 commits into from
Jun 9, 2023
Merged

fix(#3842): DnD Nested Drop Regions example can't focus child drop target #3843

merged 16 commits into from
Jun 9, 2023

Conversation

majornista
Copy link
Collaborator

@majornista majornista commented Dec 13, 2022 •
edited
Loading

Problem is with how we determine the dropTarget within the DragManager onFocus event handler. We need to prioritize the event.target matching a validDropTarget over a validDropTarget containing the event.target, otherwise the parent element may match first.

Closes #3842

✅ Pull Request Checklist:

  • Included link to corresponding React Spectrum GitHub Issue 3842.
  • Added/updated unit tests and storybook for this change (for new code or code which already has tests).
  • Filled out test instructions.
  • Updated documentation (if it already exists for this component).
  • Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

  1. Open https://reactspectrum.blob.core.windows.net/reactspectrum/c42fb35488449eeae9fcb38139f02336ad9bc550/storybook/index.html?path=/story/drag-and-drop--nested-drop-regions&providerSwitcher-express=false&providerSwitcher-toastPosition=bottom
  2. Move keyboard focus to the draggable element labelled "Drag me".
  3. Press Enter key to start drag and drop operation.
  4. Focus should move to the parent of the two nested drop targets.
  5. Press Tab key to move focus to the child drop target.
  6. Focus should move to the child drop target so that a user can complete the drag and drop operation using the keyboard alone.
  7. Press Enter to complete the drag and drop operation.

🧢 Your Project:

Adobe/Accessibility

fix(#3842): DnD Nested Drop Regions example can't focus child drop ta…
c42fb35 
…rget

Problem is with how we determine the dropTarget within the DragManager onFocus event handler. We need to prioritize the event.target matching a validDropTarget over a validDropTarget containing the event.target, otherwise the parent element may match first.
reidbarber
reidbarber previously approved these changes Dec 15, 2022
View reviewed changes
Copy link
Member

@reidbarber reidbarber left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@rspbot
Copy link

rspbot commented Jan 18, 2023

Build successful! 🎉

@rspbot
Copy link

rspbot commented Jan 23, 2023

Build successful! 🎉

@rspbot
Copy link

rspbot commented Jan 25, 2023

Build successful! 🎉

LFDanLu
LFDanLu reviewed Jan 26, 2023
View reviewed changes
packages/@react-aria/dnd/test/dnd.test.js
Comment on lines +2048 to +2049
expect(onDropEnter2).toHaveBeenCalledTimes(1);
expect(onDropEnter).toHaveBeenCalledTimes(0);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strange, this means the test is showing that the nested drop region got focus first right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I seem to recall the test behaving in reverse order, but the storybook story behaving as expected. It was a while ago.

@rspbot
Copy link

rspbot commented Feb 1, 2023

Build successful! 🎉

@rspbot
Copy link

rspbot commented Feb 6, 2023

Build successful! 🎉

@rspbot
Copy link

rspbot commented Feb 14, 2023

Build successful! 🎉

@rspbot
Copy link

rspbot commented Feb 25, 2023

Build successful! 🎉

@rspbot
Copy link

rspbot commented Mar 23, 2023

Build successful! 🎉

@rspbot
Copy link

rspbot commented Mar 30, 2023

Build successful! 🎉

@rspbot
Copy link

rspbot commented May 15, 2023

Build successful! 🎉

@rspbot
Copy link

rspbot commented May 17, 2023

Build successful! 🎉

@rspbot
Copy link

rspbot commented May 25, 2023

Build successful! 🎉

devongovett
devongovett approved these changes Jun 9, 2023
View reviewed changes
Copy link
Member

@devongovett devongovett left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will figure out the test later, and maybe look into sorting the valid drop targets so parents always come before children regardless of which order they are registered in. #2071

@rspbot
Copy link

rspbot commented Jun 9, 2023

Build successful! 🎉

@rspbot
Copy link

rspbot commented Jun 9, 2023

## API Changes

unknown top level export { type: 'identifier', name: 'Column' }
unknown top level export { type: 'identifier', name: 'Column' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }

@devongovett devongovett merged commit 6a858e3 into main Jun 9, 2023
@devongovett devongovett deleted the fix-nested-drag-and-drop-example branch June 9, 2023 23:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@devongovett devongovett devongovett approved these changes

@LFDanLu LFDanLu LFDanLu approved these changes

@reidbarber reidbarber reidbarber left review comments

@snowystinger snowystinger Awaiting requested review from snowystinger

Assignees

No one assigned

Labels

accessibility bug Something isn't working drag and drop

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

DnD Nested Drop Regions example can't focus child drop target

5 participants

@majornista @rspbot @devongovett @reidbarber @LFDanLu