-
Notifications
You must be signed in to change notification settings - Fork 6.8k
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
base: main
Are you sure you want to change the base?
Conversation
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"> |
There was a problem hiding this comment.
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"
There was a problem hiding this comment.
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?
There was a problem hiding this 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.
abb4fc4
to
f83f0ab
Compare
… focused when skipDisabled=false
f83f0ab
to
c8b2e73
Compare
In short, in the demo page
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
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.