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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

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.

@@ -40,7 +40,7 @@ <h3 class="example-accordion-header">

<div class="example-accordion-container">
<h3 class="example-accordion-header">
<button cdkAccordionTrigger value="item2" disabled>
<button cdkAccordionTrigger value="item2" [disabled]="true">
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we fix this so they behave the same way? I find it very odd that disabled does not mean the same thing as [disabled]="true"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm I am not sure if implicitly removing the disabled attribute from a button will surprise developers or not(e.g. expecting a css selector button.foo:disabled would work)

Is there a similar case in Angular components or CDKs that an input name shares the same name of a native attribute and what's the best practice of handling that?

In this case I can optionally bind the disabled attribute like

'[attr.aria-disabled]': 'pattern.disabled()', // soft disabled.
'[attr.disabled]': 'pattern.tabindex() < 0 ? true : null', // hard disabled for elements supports disabled attribute.
'[attr.inert]': 'pattern.tabindex() < 0 ? true : null', // hard disabled for other elements.

thoughts?

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dev-app preview When applied, previews of the dev-app are deployed to Firebase
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants