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: add trapFocus to RadioButtonGroup #7044

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
16 changes: 11 additions & 5 deletions src/js/components/RadioButtonGroup/RadioButtonGroup.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,12 @@ const RadioButtonGroup = forwardRef(
defaultValue,
disabled,
focusIndicator = true,
gap,
name,
onChange,
options: optionsProp,
trapFocus = false,
value: valueProp,
gap,
...rest
},
ref,
Expand Down Expand Up @@ -83,7 +84,12 @@ const RadioButtonGroup = forwardRef(
// Chrome behaves differently in that focus is given to radio buttons
// when the user selects one, unlike Safari and Firefox.
setTimeout(() => {
setFocus(true);
// In case the focus needs to be trapped inside the input instead of the
// Box wrapper
if (trapFocus) {
Copy link
Collaborator

@taysea taysea Dec 5, 2023

Choose a reason for hiding this comment

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

Hey Shimi! Thanks for the proposal. I understand the use case, but I'm trying to think of a better way to handle it. A couple reactions:

  • The reuse of trapFocus as a prop name doesn't quite feel applicable here because in the case of Drop/Layer, trapFocus indicates that something should behave like a modal and not allow tab stops to continue to anywhere else on the page. That is a bit different from the intent here to place the focus on an individual radiobutton.
  • setFocus in the code is just a boolean to track if focus is set. I'm not following why we would set it to something other than a boolean here. Also, if I try to use arrows to navigate amongst the options in your storybook example, I can't. I would expect to be able to use keyboard to navigate.

I don't have a suggestion at the moment for a better path, but just wanted to drop my reactions and let you know that I'm thinking on it.

Copy link
Member Author

@ShimiSun ShimiSun Dec 5, 2023

Choose a reason for hiding this comment

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

Thanks for the feedback @taysea, appreciate your time and thoughts on this.
I agree it is not trivial. I sent the initial proposal to get the convo started so I appreciate you chiming in.

Here are some follow-up thoughts.

  • I had a similar thought after filing the PR regarding the name of the prop. A name like 'setFirstInputFocus', 'defaultFocusToFirst', 'focusFirstInGroup' might be more self-explanatory - let me know if you like any of those, or whether we should branch off one of them.
  • Yes I know, I'll need to work on it further, this is why the PR is still in Draft. I was seeking initial thoughts from the team before I continued to invest in the solution.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for working on this, I'm wondering if a potential workaround you team could use is before setting the focus on input.current check if role="radiogroup". If so instead of focusing input.current do something like this:

const inputEl = input.current?.querySelectorAll('input[type=radio]')[0];
inputEl.focus();

// console.log('optionRefs.current[0]', optionRefs.current[0]);
setFocus(optionRefs.current[0]);
} else setFocus(true);
}, 1);
};

Expand All @@ -92,7 +98,7 @@ const RadioButtonGroup = forwardRef(
if (onChange) {
event.persist(); // extract from React synthetic event pool
// event.target.value gives value as a string which needs to be
// manually typecasted according to the type of original option value.
// manually typecasting according to the type of original option value.
// return the original option value attached with the event.
const adjustedEvent = event;
adjustedEvent.value = optionValue;
Expand Down Expand Up @@ -140,7 +146,7 @@ const RadioButtonGroup = forwardRef(
optionValue === value ||
(value === undefined && !index) ||
// when nothing has been selected, show focus
// on the first radiobutton
// on the first radio-button
(value === '' && index === 0);

if (optionRest.checked) {
Expand All @@ -165,7 +171,7 @@ const RadioButtonGroup = forwardRef(
// so that the FormField has focus style. However, we still
// need to visually indicate when a RadioButton is active.
// In RadioButton, if focus = true but focusIndicator = false,
// we will apply the hover treament.
// we will apply the hover treatment.
focusIndicator={focusIndicator}
id={id}
value={optionValue}
Expand Down
1 change: 1 addition & 0 deletions src/js/components/RadioButtonGroup/index.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ export interface RadioButtonGroupProps {
value: string | number | boolean;
}
)[];
trapFocus?: boolean;
value?: string | number | boolean | object;
}

Expand Down
1 change: 1 addition & 0 deletions src/js/components/RadioButtonGroup/propTypes.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ if (process.env.NODE_ENV !== 'production') {
}),
),
]).isRequired,
trapFocus: PropTypes.bool,
value: PropTypes.oneOfType([
PropTypes.string,
PropTypes.number,
Expand Down
46 changes: 46 additions & 0 deletions src/js/components/RadioButtonGroup/stories/TrapFocus.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
import React, { useEffect, useState } from 'react';

import { Box, RadioButtonGroup } from 'grommet';

export const TrapFocus = () => {
const [value, setValue] = useState({ value: '' });

const input = React.useRef();

useEffect(() => {
setTimeout(() => {
input.current?.focus();
console.log('here', input.current);
}, 500);
}, []);

const postMethods = [
{ label: 'FTP', value: 'FTP' },
{
label: 'File System',
value: 'FileSystem',
},
{
label: 'FTP & File System',
value: 'FTPCopy',
},
];

// In this example there is no keyboard access to the RBG component
// The trapFocus should fix the initial focus
return (
<Box align="center" pad="large">
<RadioButtonGroup
ref={input}
name="radio"
options={postMethods}
value={value}
onChange={(event) => setValue(event.value)}
// trapFocus
/>
</Box>
);
};
export default {
title: 'Input/RadioButtonGroup/Trap Focus',
};