Skip to content

fix(select, action-sheet): use radio role for options#30769

Merged
thetaPC merged 12 commits into
mainfrom
FW-6818
Dec 8, 2025
Merged

fix(select, action-sheet): use radio role for options#30769
thetaPC merged 12 commits into
mainfrom
FW-6818

Conversation

@thetaPC

@thetaPC thetaPC commented Nov 5, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

Issue number: internal

What is the current behavior?

The screen reader does not announce when an option is selected within the action sheet interface. This is because the action sheet uses standard buttons, which do not support a detectable selected state via native properties or ARIA attributes like aria-checked or aria-selected, creating an inconsistent user experience across different interface types.

What is the new behavior?

  • Updated the action sheet buttons to accept role="radio"
  • Added keyboard navigation to follow the pattern for radio group
  • Added test

Does this introduce a breaking change?

  • Yes
  • No

Other information

Basic

@github-actions github-actions Bot added the package: core @ionic/core package label Nov 5, 2025
@vercel

vercel Bot commented Nov 5, 2025 •
edited
Loading

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
ionic-framework Ready Ready Preview Comment Dec 8, 2025 8:24pm

@thetaPC thetaPC changed the title Fw 6818 fix(select): use aria description for selected option Nov 5, 2025
@thetaPC thetaPC changed the title fix(select): use aria description for selected option fix(select): use aria description for selected option within action sheet Nov 5, 2025
@thetaPC thetaPC marked this pull request as ready for review November 5, 2025 21:11
@thetaPC thetaPC requested a review from a team as a code owner November 5, 2025 21:11
@thetaPC thetaPC requested a review from gnbm November 5, 2025 21:11
ShaneK
ShaneK approved these changes Nov 7, 2025
View reviewed changes

@ShaneK ShaneK left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good to me! Extremely minor grammatical nits, feel free to ignore if you don't wanna run CI just for them 🤷‍♂️

Comment thread core/src/components/select/test/a11y/select.e2e.ts Outdated
Comment thread core/src/components/select/test/a11y/select.e2e.ts Outdated
@thetaPC @brandyscarney
refactor(action-sheet): accept radio role buttons
e1f83b0 
Co-authored-by: Brandy Smith <6577830+brandyscarney@users.noreply.github.com>
@thetaPC thetaPC changed the title fix(select): use aria description for selected option within action sheet fix(select, action-sheet): use radio role for options Nov 13, 2025
ShaneK
ShaneK requested changes Nov 14, 2025
View reviewed changes

@ShaneK ShaneK left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looking good, just a couple of minor things

Comment thread core/src/components/action-sheet/action-sheet.tsx Outdated
Comment thread core/src/components/action-sheet/action-sheet.tsx
@Watch('buttons')
buttonsChanged() {
// Initialize activeRadioId when buttons change
if (this.hasRadioButtons) {

@ShaneK ShaneK Nov 14, 2025 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

My only concern here is this.hasRadioButtons is only set in connectedCallback, I believe this means if buttons are set dynamically this value may become stale and this process may not trigger when it's supposed to. That's sort of an edge case for many, but it may be worth re-computing this.hasRadioButtons here, if you feel like this could be a concern. Up to you!

@thetaPC thetaPC Nov 15, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

c150a61

@thetaPC @ShaneK
Update core/src/components/action-sheet/action-sheet.tsx
626370b 
Co-authored-by: Shane <shane@shanessite.net>
brandyscarney
brandyscarney reviewed Nov 25, 2025
View reviewed changes

@brandyscarney brandyscarney left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Overall this looks good, just one concern on the role!

Comment thread core/src/components/select/select.tsx Outdated
Comment thread core/src/components/action-sheet/action-sheet.tsx Outdated
Comment thread core/src/components/action-sheet/action-sheet.tsx
Comment thread core/src/components/action-sheet/action-sheet.tsx
@thetaPC @brandyscarney
Update core/src/components/action-sheet/action-sheet.tsx
73149c9 
Co-authored-by: Brandy Smith <brandyscarney@users.noreply.github.com>
@thetaPC thetaPC removed the request for review from gnbm December 8, 2025 19:10
brandyscarney
brandyscarney requested changes Dec 8, 2025
View reviewed changes

@brandyscarney brandyscarney left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Small request to use the isSelected variable but otherwise this looks good!

Comment thread core/src/components/select/select.tsx Outdated
@thetaPC @brandyscarney
Update core/src/components/select/select.tsx
d3e0d53 
Co-authored-by: Brandy Smith <brandyscarney@users.noreply.github.com>
brandyscarney
brandyscarney approved these changes Dec 8, 2025
View reviewed changes

@brandyscarney brandyscarney left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good! 🚀

@thetaPC thetaPC added this pull request to the merge queue Dec 8, 2025
Merged via the queue into main with commit 1c89cf0 Dec 8, 2025
51 checks passed
@thetaPC thetaPC deleted the FW-6818 branch December 8, 2025 21:01
@pjaecks-carecompass pjaecks-carecompass mentioned this pull request Jan 16, 2026
Open
3 tasks
@aeharding aeharding mentioned this pull request Apr 18, 2026
Closed
3 tasks
@aeharding aeharding mentioned this pull request May 1, 2026
Merged
2 tasks
pull Bot pushed a commit to LoadsAForks/ionic-framework that referenced this pull request May 6, 2026
@aeharding
fix(action-sheet): restore action-sheet-selected class on non-radio b…
c18502f 
…uttons (ionic-team#31109)

Issue number: resolves ionic-team#31090

---------

## What is the current behavior?

Buttons configured with `role: "selected"` no longer receive the
`action-sheet-selected` CSS class. Userland styling that targets
`.action-sheet-selected` (the documented hook for marking the active
option — bold text, custom checkmarks, etc.) silently stopped working as
of `8.7.12`.

This regressed in ionic-team#30769 (`fix(select, action-sheet): use radio role for
options`). The new render path computes the button's class as:

```tsx
class={{
  ...buttonClass(b),
  'action-sheet-selected': isActiveRadio,
}}
```

`buttonClass(b)` already emits `'action-sheet-selected': true` for
`role: "selected"` (via `[action-sheet-${button.role}]]: button.role !==
undefined`), but the second key with the same name overrides it. For any
non-radio button `isActiveRadio` is `false`, so the class is dropped
from the rendered `<button>`.

### Repro

```html
<ion-action-sheet id="sheet" header="Choose"></ion-action-sheet>
<script type="module">
  await customElements.whenDefined("ion-action-sheet");
  const sheet = document.getElementById("sheet");
  sheet.buttons = [
    { text: "Option A" },
    { text: "Option B", role: "selected" },
    { text: "Cancel", role: "cancel" },
  ];
  await sheet.present();
</script>
```

In `8.7.11` the Option B button gets `action-sheet-selected`. In
`8.7.12+` it does not.

## What is the new behavior?

- Only override `action-sheet-selected` based on `isActiveRadio` when
the button actually participates in the radio group
(`b.htmlAttributes?.role === 'radio'`). For non-radio buttons, the class
map produced by `buttonClass(b)` is left untouched, so `role:
"selected"` keeps emitting `action-sheet-selected` exactly as it has
since the component was introduced.
- Adds a spec-test regression covering the `role: "selected"` case.

This preserves the new radio-group behavior introduced in ionic-team#30769 (when
the button is a radio, `action-sheet-selected` follows `activeRadioId`)
and restores the documented public API.

## Does this introduce a breaking change?

- [ ] Yes
- [x] No

## Other information

- The minified output for the lazy + custom-elements bundles becomes
`Object.assign(Object.assign({}, buttonClass(b)), isRadio &&
{"action-sheet-selected": isActiveRadio})`. When `isRadio` is `false`,
`Object.assign(target, false)` is a no-op and the class set by
`buttonClass(b)` is preserved.
- Verified in a real consumer app on 8.8.4: with this patch built and
installed locally, `role: "selected"` once again renders `class="...
action-sheet-selected ..."` on the `<button>`.


Made with [Cursor](https://cursor.com)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ShaneK ShaneK ShaneK approved these changes
@brandyscarney brandyscarney brandyscarney approved these changes

Assignees

No one assigned

Labels

package: core @ionic/core package

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@thetaPC @ShaneK @brandyscarney