Skip to content
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.

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

fix: tabbing past contenteditable=false in containing focusscope #7340

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

nwidynski
Copy link
Contributor

Closes #7321

✅ Pull Request Checklist:

  • Included link to corresponding React Spectrum GitHub Issue.
  • 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:

🧢 Your Project:

@nwidynski nwidynski closed this Nov 8, 2024
@nwidynski nwidynski reopened this Nov 8, 2024
@@ -176,6 +176,7 @@ export function useSpinButton(
return {
spinButtonProps: {
role: 'spinbutton',
tabIndex: isDisabled ? -1 : undefined,
Copy link
Member

Choose a reason for hiding this comment

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

We can see here that a disabled spin button only has aria-disabled, not disabled

Is there a reason you chose to use tabIndex to solve this, rather than expand the definition of disabled in FocusScope?
https://github.com/adobe/react-spectrum/blob/main/packages/%40react-aria/focus/src/FocusScope.tsx#L270

I'll bring it up with the team to see if we have any preferences.

Copy link
Contributor Author

@nwidynski nwidynski Nov 12, 2024

Choose a reason for hiding this comment

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

Ah no, I didnt look at the semantics of a spinbutton at all. We can definitely try and add this to FocusManager instead 👍

I assume this would look at div[disabled]?

Copy link
Member

Choose a reason for hiding this comment

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

ah, no, i meant in FocusScope adding [aria-disabled] next to [disabled]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure whether I like using aria semantics to change focus order to be honest, feels wrong.

Copy link
Member

@snowystinger snowystinger Nov 14, 2024

Choose a reason for hiding this comment

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

Sorry, just looked at this again more closely.
The issue is just that we're too accepting of contenteditable https://github.com/adobe/react-spectrum/blob/main/packages/%40react-aria/focus/src/FocusScope.tsx#L283
useDateSegment disabled has it set to false, so it's not a focusable element

We should restrict FocusScope to be any content editable EXCLUDING false (since contenteditable can be a couple different values https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/contenteditable#value)

but we should be able to do the exclude query selector
[contenteditable]:not([contenteditable^="false"])

@snowystinger snowystinger changed the title fix: spinbutton tabindex when disabled fix: tabbing past contenteditable=false in containing focusscope Nov 14, 2024
@@ -280,7 +280,7 @@ const focusableElements = [
'embed',
'audio[controls]',
'video[controls]',
'[contenteditable]'
'[contenteditable]:not([contenteditable^="false"])'
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the quick fix. Would you mind adding a test?

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.

[RAC] Disabled DatePicker stops Tab focus moving forward when used inside Modal & Dialog
2 participants