Skip to content

fix(cdk-experimental/accordion): fix disabled trigger button can't be focused when skipDisabled=false #31379

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

Merged
merged 1 commit into from
Jun 25, 2025

Conversation

ok7sai
Copy link
Contributor

@ok7sai ok7sai commented Jun 18, 2025

In short, in the demo page

<button cdkAccordionTrigger disabled>

Makes the button natively disabled and not focusable, so even with skipDisabled=false it can't be focused by the focus manager.

Change it to

<button cdkAccordionTrigger [disabled]="true">

Makes the cdkAccordionTrigger to be disabled without changing the button state.

Also added the inert attribute to the button when it's completely disabled(disabled and skip disabled), so it can't be focused by a pointer.

@ok7sai ok7sai requested a review from wagnermaciel June 18, 2025 08:26
@ok7sai ok7sai requested a review from a team as a code owner June 18, 2025 08:26
@ok7sai ok7sai requested review from mmalerba and removed request for a team June 18, 2025 08:26
@ok7sai ok7sai added the dev-app preview When applied, previews of the dev-app are deployed to Firebase label Jun 18, 2025
Copy link

github-actions bot commented Jun 18, 2025

Deployed dev-app for c8b2e73 to: https://ng-dev-previews-comp--pr-angular-components-31379-dev-wcgip6j4.web.app

Note: As new commits are pushed to this pull request, this link is updated after the preview is rebuilt.

Copy link
Contributor

@wagnermaciel wagnermaciel left a comment

Choose a reason for hiding this comment

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

LGTM once mmalerba's comment is addressed

Side-note:
I noticed that event listeners are being attached per-accordion trigger instead of once on the accordion group. This is bad for initial load performance but isn't a major issue for this particular component since it is rare to have many accordions a page. It might be worth asking Jules to try refactoring this as a future PR.

@ok7sai ok7sai force-pushed the ng-aria-accordion branch from abb4fc4 to f83f0ab Compare June 20, 2025 20:47
@ok7sai ok7sai force-pushed the ng-aria-accordion branch from f83f0ab to c8b2e73 Compare June 20, 2025 20:52
@ok7sai ok7sai added action: merge The PR is ready for merge by the caretaker target: minor This PR is targeted for the next minor release labels Jun 24, 2025
@ok7sai ok7sai merged commit 06d7384 into angular:main Jun 25, 2025
26 of 28 checks passed
@ok7sai ok7sai deleted the ng-aria-accordion branch June 25, 2025 21:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
action: merge The PR is ready for merge by the caretaker dev-app preview When applied, previews of the dev-app are deployed to Firebase target: minor This PR is targeted for the next minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants