Skip to content

fix: remove duplicate tabstop in RadioButton.Item - #5050

Open
ErfanBagheri404 wants to merge 2 commits into
callstack:mainfrom
ErfanBagheri404:fix/radiobuttonitem-duplicate-tabstop
Open

fix: remove duplicate tabstop in RadioButton.Item#5050
ErfanBagheri404 wants to merge 2 commits into
callstack:mainfrom
ErfanBagheri404:fix/radiobuttonitem-duplicate-tabstop

Conversation

@ErfanBagheri404

Copy link
Copy Markdown

Fixes #4775

RadioButton.Item renders an outer TouchableRipple plus the inner RadioButton, both focusable on web. Two tabstops per item.

Disable the inner radio button focusability so only the outer touchable is a tabstop.

The item renders an outer TouchableRipple plus the inner RadioButton,
both of which are focusable on web. This makes the item take two Tab
presses to navigate past.

Disable the inner radio button's focusability so only the outer
touchable is a tabstop.

Fixes callstack#4775
// interactive surface. Disable the inner RadioButton's focusability
// so there is only one tabstop per radio button item on web.
focusable: false,
tabIndex: -1,

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

could we also hide inner RadioButton from the accessibility tree?

tabIndex: -1 only removes it from tab sequence - it remains focusable programmatically according to W3C guidance
inner Android & iOS controls still expose role="radio" & aria-checked, so screen readers may encounter 2 radio controls

so could we explicitly hide inner visual control as documented for aria-hidden?

Suggested change
tabIndex: -1,
tabIndex: -1,
accessible: false,
'aria-hidden': true,

this follows the same approach already used by Checkbox.Item

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

what about updating the snapshots & adding regression test here?
existing RadioButton.Item tests use toMatchSnapshot() & those snapshots currently contain focusable={true} for inner radio.

since this PR changes that rendered output but only updates the component file, the snapshot tests will fail when CI runs, I suppose

it would also be useful to guard against the duplicate control explicitly:

it('exposes only one radio control', async () => {
  await render(
    <RadioButton.Item
      label="Radio button"
      value="radio"
      status="unchecked"
    />
  );

  expect(screen.getAllByRole('radio')).toHaveLength(1);
});

The inner RadioButton inside RadioButtonItem exposed its own
role="radio" and aria-checked to screen readers, creating a
duplicate control. Apply accessible={false} + aria-hidden={true}
(matches Checkbox.Item), update snapshots, and add a regression
test asserting getAllByRole('radio') returns exactly 1.

Addresses review comments on callstack#5050.
@ErfanBagheri404

Copy link
Copy Markdown
Author

Both review points are already addressed in the latest push (b3b6612): accessible=false + aria-hidden=true on the inner RadioButton, snapshot updated, and the regression test with getAllByRole radio assertion was added. The review was posted after the push so GitHub still showed the unresolved thread.

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.

RadioButton.Item has duplicate tabstop

2 participants