microsoft / fast

The adaptive interface system for modern web experiences.
https://www.fast.design
Other
9.28k stars 595 forks source link

fix: Setting button disabled should also set aria-disabled #6783

Closed m-akinc closed 1 year ago

m-akinc commented 1 year ago

🐛 Bug Report

Setting the disabled attribute on the button should also set aria-disabled on that element.

💻 Repro or Code Sample

Set disabled on a FAST button and see that aria-disabled is not true.

🤔 Expected Behavior

See title.

😯 Current Behavior

Button's aria-disabled is not true.

💁 Possible Solution

Add <template aria-disabled=${x => x.disabled}> to the button template.

🔦 Context

This is causing problems for some Playwright tests we are trying to write. The Playwright toBeDisabled() assertion will only succeed if native form elements have the disabled attribute or non-native custom elements have aria-disabled=true. This means our FAST-based button with the disabled attribute is currently failing that assertion.

chrisdholt commented 1 year ago

I don't think this would be valid or expected - the actual element receiving focus is the button within the shadow root. The host is considered generic, there is no role or anything which would necessitate aria-disabled and adding said role would likely trigger a new violation as the ARIA attribute is now present when not necessary. This sounds like a function of the test and how it's written. Here's our playwright test to validate the disabled state (first test): https://github.com/microsoft/fast/blob/master/packages/web-components/fast-foundation/src/button/button.pw.spec.ts

m-akinc commented 1 year ago

I think that makes sense, thanks.