Skip to content
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

feat(a11y): make menu buttons in 'More' dropdown selectable with the keyboard #2976

Merged
merged 6 commits into from
Oct 21, 2024

Conversation

jyn514
Copy link
Contributor

@jyn514 jyn514 commented Sep 24, 2024

previously, these were pure divs, without a <button> element, and could not be selected with Tab, rendering them keyboard-inaccessible.

see also #2767 (cc @userquin). that uses a slightly different approach; it adds a disabled anchor instead of using buttons. i think this html is more semantically accurate, but i don't mind changing it if you so desire.

fixes #2974

…keyboard

previously, these were pure divs, without a `<button>` element, and could not be selected with `Tab`, rendering them keyboard-inaccessible.
Copy link

netlify bot commented Sep 24, 2024

Deploy Preview for elk-zone ready!

Name Link
🔨 Latest commit f4a3489
🔍 Latest deploy log https://app.netlify.com/sites/elk-zone/deploys/66f5555b9063570008f4201c
😎 Deploy Preview https://deploy-preview-2976--elk-zone.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Sep 24, 2024

Deploy Preview for elk-docs canceled.

Name Link
🔨 Latest commit f4a3489
🔍 Latest deploy log https://app.netlify.com/sites/elk-docs/deploys/66f5555b06b44800072f5b78

@jyn514
Copy link
Contributor Author

jyn514 commented Sep 24, 2024

8:31:15 PM: Failed during stage 'checking build content for changes': Canceled build due to no content change

i am not sure why this thinks nothing changed; i tested it locally with pnpm dev and it's definitely selectable now where it wasn't before.

@userquin
Copy link
Member

can you update your main and merge changes in this PR?

@userquin
Copy link
Member

userquin commented Sep 25, 2024

i am not sure why this thinks nothing changed; i tested it locally with pnpm dev and it's definitely selectable now where it wasn't before.

What do you mean? The buttons are there (also in the PR preview):

Buttons on StatusActionsMore on local using this PR

imagen

@userquin
Copy link
Member

userquin commented Sep 25, 2024

We should:

  • do the same also in components/account/AccountMoreButton.vue
  • add type="button" when is is button in components/common/dropdown/DropdownItem.vue: iirc type by default is submit

… keyboard

previously, these were pure divs, without a `<button>` element, and could not be selected with `Tab`, rendering them keyboard-inaccessible.
@jyn514
Copy link
Contributor Author

jyn514 commented Sep 26, 2024

can you update your main and merge changes in this PR?
do the same also in components/account/AccountMoreButton.vue

done :)

What do you mean? The buttons are there (also in the PR preview):

i mean the comment the github bot posted on this PR, that says the build was cancelled. maybe i'm misinterpreting it somehow, clearly the preview is showing up.

add type="button" when is is button in components/common/dropdown/DropdownItem.vue: iirc type by default is submit

can you say more? i'm looking at the rendered button and i don't see any submit attribute. does it affect the event type or something like that?

i glanced at DropdownItem but i don't see anything related to submit; the closest is :is="is" in a component. maybe i'm missing something, i've never used react before 😅

image

@jyn514
Copy link
Contributor Author

jyn514 commented Sep 26, 2024

i mean the comment the github bot posted on this PR, that says the build was cancelled. maybe i'm misinterpreting it somehow, clearly the preview is showing up.

ohhhh that's for the docs, not for the live preview of the site. that makes more sense.

@userquin
Copy link
Member

userquin commented Sep 26, 2024

Add this in the script setup:

const type = computed(() => is === 'button' ? 'button' : null)

Then add :type="type" below :is="is" in the template, and so the button will be rendered with the type:

<button type="button" ...

@userquin
Copy link
Member

We should use props destr. instead with defaults (enabled by default in vue 3.5)

@jyn514
Copy link
Contributor Author

jyn514 commented Sep 26, 2024

i added :type="type". it broke at first (apparently NuxtJS can't handle null?) so i just had it fallback to submit .

We should use props destr. instead with defaults (enabled by default in vue 3.5)

i don't know what this means.

the default is `type="submit"` which is semantically wrong and may cause
the browser to try to submit a form.
@jyn514
Copy link
Contributor Author

jyn514 commented Sep 26, 2024

it broke at first (apparently NuxtJS can't handle null?) so i just had it fallback to submit

oh wait apparently this started working again?? the only thing i changed was the order :type appears in the element ...

anyway, i've reverted this change, so now type="" only appears for button elements, not other dropdown items.

@userquin
Copy link
Member

userquin commented Sep 26, 2024

Use undefined instead null in the composable

@userquin
Copy link
Member

i don't know what this means.

I Will fix this

Copy link
Member

@shuuji3 shuuji3 left a comment

Choose a reason for hiding this comment

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

Sorry for taking a while to review 🙏🏻 This is a great accessibility improvement! Thank you so much.😀

screenshot of context menu and accessibility tree view on devtool. it shows all menu items can be focusable and button elements

@shuuji3 shuuji3 added this pull request to the merge queue Oct 21, 2024
Merged via the queue into elk-zone:main with commit dac42e0 Oct 21, 2024
13 checks passed
@jyn514 jyn514 deleted the selectable-menu branch October 21, 2024 12:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

dropdown menu is not keyboard accessible
3 participants