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

Allow passing inputRef to RAC <Radio> #6067

Merged
merged 12 commits into from
Apr 3, 2024

Conversation

sookmax
Copy link
Contributor

@sookmax sookmax commented Mar 16, 2024

Closes #6054, closes #6123

To implement the behavior described in #6054 (comment), I needed to pass tabbable: true to getFocusableTreeWalker(). This behavior is tested by the unit test, but hard to tell in the story.

Not sure if we still need to expose individual inputRefs for each <Radio /> inside the group as done in #5967.

Oh and should we document this behavior somewhere in the docs?

Let me know what you guys think!

This PR exposes inputRef through props in <Radio>, practically the same as #5967.

✅ 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:

Docs

🧢 Your Project:

Copy link
Member

@snowystinger snowystinger left a comment

Choose a reason for hiding this comment

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

Looks pretty good, couple small comments.
To your question about documentation. We'll need to change the type on RadioGroup to handle this. In React Spectrum side, we have a FocusableRef type which we can base a new type on over here. We don't want to actually use that type because it obscures access to the DOM Node itself in a way we don't want to do over here.

@Mrtly
Copy link

Mrtly commented Mar 18, 2024

Thanks @sookmax for jumping on this! I'll try to test this shortly. It'd be great if you could add the scrollIntoView() method to the imperative handle, so a form handler can both focus and scroll to the invalid field. I am not sure if all browsers automatically scroll on focus. thoughts on this?

edit to add: checking out the story now, it seems to work well but the focus lacks visual clarity without a focus ring. On inspection, the Dog label has the data-focused=“true” property but per the stylesheet only the data-focus-visible will receive a ring/shadow. Would it be possible to add data-focus-visible when focusing on the radio label programmatically?

@sookmax
Copy link
Contributor Author

sookmax commented Mar 18, 2024

@snowystinger thanks for the feedback! I updated the story to include what you requested.

Let me know if there's anything else I should add or change!

radiogroup_focusable_ref_nerde.mov

@sookmax
Copy link
Contributor Author

sookmax commented Mar 18, 2024

@Mrtly

I am not sure if all browsers automatically scroll on focus. thoughts on this?

It seems like the default behavior per HTMLElement: focus() method:

"By default the browser will scroll the element into view after focusing it, and it may also provide visible indication of the focused element (typically by displaying a "focus ring" around the element)."

And so I thought calling focus() on the ref would be enough to bring the radio group that may possibly be out of screen into view, without calling scrollIntoView() separately again. But calling scrollIntoView() will still work if you choose to call it, it'll just scroll to the radio group's div. I'm just not sure if we want to implement the same behavior for ref.scrollIntoView() as we did for ref.focus(). We need opinions from the core team members for this, I think.

Would it be possible to add data-focus-visible when focusing on the radio label programmatically?

I think this is impossible though I'm not 100% sure. From my understanding, the focus-visible state is controlled only by keyboard interactions, and we can't manipulate it with HTMLElement.focus(). But again, I might be missing something, so It would be better to get the answer from one of the core members!

@sookmax sookmax changed the title Add focusable ref feature to <RadioGroup> Allow passing inputRef to RAC <RadioGroup> Apr 2, 2024
@sookmax sookmax changed the title Allow passing inputRef to RAC <RadioGroup> Allow passing inputRef to RAC <Radio> Apr 2, 2024
snowystinger
snowystinger previously approved these changes Apr 2, 2024
Copy link
Member

@snowystinger snowystinger left a comment

Choose a reason for hiding this comment

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

thank you for the quick pivot!

@reidbarber reidbarber merged commit 0b2a838 into adobe:main Apr 3, 2024
27 checks passed
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] Radio - pass ref to input element add forwarded focusable ref on RadioGroup (rac)
6 participants