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

fix(VSelect/VMenu): remove duplicate IDs, add aria-activedescendant #10226

Merged
merged 19 commits into from
Feb 6, 2020

Conversation

KaelWD
Copy link
Member

@KaelWD KaelWD commented Jan 15, 2020

Description

Removed duplicated IDs in VSelectList caused by inheritAttrs
The v-card styles also make no visual difference so I removed that along with the redundant DOM element
Added aria-activedescendant to menu activators and v-select hidden input, auto ids on v-list-item in menu

Motivation and Context

fixes #9680

Markup:

<template>
  <v-container>
    <v-menu offset-y>
      <template v-slot:activator="{ attrs, on }">
        <v-btn
          color="primary"
          dark
          v-bind="attrs"
          v-on="on"
        >
          Dropdown
        </v-btn>
      </template>
      <v-list>
        <v-list-item
          v-for="(item, index) in items"
          :key="index"
          :id="`my-custom-id-${index}`"
          @click=""
        >
          <v-list-item-title>{{ item }}</v-list-item-title>
        </v-list-item>
      </v-list>
    </v-menu>

    <v-select
      :items="items"
      label="Month"
      outlined
    ></v-select>
  </v-container>
</template>

<script>
  export default {
    data: () => ({
      items: ['January', 'February', 'March', 'April'],
    })
  }
</script>

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Improvement/refactoring (non-breaking change that doesn't add any features but makes things better)

@KaelWD KaelWD added T: bug Functionality that does not work as intended/expected a11y Accessibility issue C: VSelect VSelect labels Jan 15, 2020
@KaelWD KaelWD added this to the v2.2.x milestone Jan 15, 2020
@KaelWD KaelWD self-assigned this Jan 15, 2020
@KaelWD
Copy link
Member Author

KaelWD commented Jan 15, 2020

@rkrasnan can you test this fix? zeit isn't behaving so you'll have to checkout the branch, copy my markup to packages/vuetify/dev/Playground.vue, then run yarn && yarn build && yarn dev

johnleider
johnleider previously approved these changes Jan 15, 2020
Copy link
Member

@johnleider johnleider left a comment

Choose a reason for hiding this comment

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

Pending verification from NVDA.

@rkrasnan

This comment has been minimized.

@KaelWD KaelWD added C: VMenu VMenu C: VList VList labels Jan 17, 2020
@TravisBuddy
Copy link

TravisBuddy commented Jan 20, 2020

Hey @KaelWD,
Your changes look good to me!

View build log

TravisBuddy Request Identifier: 7c165250-48f0-11ea-b51a-43bce7cb815c

@rkrasnan
Copy link
Contributor

Testing with NVDA.

v-menu

  • Menu button is tab accessible.
  • It is expandable with either tab or space.
  • Pressing down reads "January".
  • Pressing down again does not read anything, it should read "February".

v-select

  • Input is not tab accessible.
  • Clicking the select expands the menu
  • Arrow navigation does not work in the expanded menu
  • Nothing is read to the screen reader

@johnleider
Copy link
Member

cc @phiter @KaelWD

@phiter
Copy link
Contributor

phiter commented Jan 30, 2020

The issue faced by @rkrasnan not reading the items after the first is due to this playground example generating the same id for every item my-custom-id. Using my-custom-id-${index} makes it work.

But v-select is indeed not working properly on this branch. It's not focusable at all.

@KaelWD
Copy link
Member Author

KaelWD commented Jan 31, 2020

image

@KaelWD KaelWD changed the title fix(VSelect): remove duplicate IDs in list fix(VSelect/VMenu): remove duplicate IDs, add aria-activedescendant Jan 31, 2020
@KaelWD KaelWD requested a review from phiter January 31, 2020 07:45
Copy link
Member

@johnleider johnleider left a comment

Choose a reason for hiding this comment

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

I'm not sure if the activeTile references can be computed, but they are currently duplicated.

@phiter
Copy link
Contributor

phiter commented Feb 1, 2020

VSelect items are now being read as you navigate through them, but there are some weird issues still.
I talked to Kael on Discord about this, but basically VSelect is reading the input field as "edit blank".

Also if the VSelect is closed and you press arrow to change the item, it just reads "blank" after each keypress.

Here's a video: https://www.youtube.com/watch?v=LdFbpNOrE-k

Here is the proper implementation details with the elements and aria roles to implement an accessible VSelect.

https://www.w3.org/TR/wai-aria-practices-1.1/examples/listbox/listbox-collapsible.html

We could follow some of this.

The implementation uses a button with the currently selected item as a label inside the button, but I don't know if it works well since VSelect extends VInput.

@KaelWD KaelWD force-pushed the master branch 3 times, most recently from 9e00958 to de967ac Compare February 5, 2020 07:33
KaelWD and others added 5 commits February 7, 2020 01:15
Co-Authored-By: John Leider <john@vuetifyjs.com>
Co-Authored-By: John Leider <john@vuetifyjs.com>
Co-Authored-By: John Leider <john@vuetifyjs.com>
Co-Authored-By: John Leider <john@vuetifyjs.com>
@KaelWD KaelWD force-pushed the fix/9680-vselect-duplicate-id branch from 371db44 to 805a707 Compare February 6, 2020 14:18
@KaelWD
Copy link
Member Author

KaelWD commented Feb 6, 2020

activeTile references can be computed

Maybe, but I know refs aren't reactive so I can't be bothered

basically VSelect is reading the input field as "edit blank"
We could follow some of this

Yeah that'd be nice but maybe for a later time, this is a big improvement already and is ready to go now.

@KaelWD KaelWD merged commit b2e5691 into master Feb 6, 2020
@KaelWD KaelWD deleted the fix/9680-vselect-duplicate-id branch February 6, 2020 16:04
rachael-smith pushed a commit to rachael-smith/vuetify that referenced this pull request Feb 7, 2020
…uetifyjs#10226)

* fix(VSelect): remove duplicate IDs in list

fixes vuetifyjs#9680

* feat(VMenu): apply activedescendant with ids on list items

* revert: undo changes to v-select

* feat(VSelect): add aria-activedescendant

* test: update snapshots

* fix(VSelect): set correct aria-readonly value

broken by vuetifyjs#7385

* fix(VSelect): set activedescendant to list item instead of title

* fix(VAutocomplete): set aria-activedescendant

* fix(VMenu): prevent TypeError before list is rendered

* Update packages/vuetify/src/components/VAutocomplete/VAutocomplete.ts

Co-Authored-By: John Leider <john@vuetifyjs.com>

* Update packages/vuetify/src/components/VAutocomplete/VAutocomplete.ts

Co-Authored-By: John Leider <john@vuetifyjs.com>

* Update packages/vuetify/src/components/VSelect/VSelect.ts

Co-Authored-By: John Leider <john@vuetifyjs.com>

* Update packages/vuetify/src/components/VSelect/VSelect.ts

Co-Authored-By: John Leider <john@vuetifyjs.com>

* style: rearrange stuff

* fix: import getObjectValueByPath

* fix(VSelect): remove duplicate import

Co-authored-by: John Leider <john@vuetifyjs.com>
@lock lock bot locked as resolved and limited conversation to collaborators Mar 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
a11y Accessibility issue C: VList VList C: VMenu VMenu C: VSelect VSelect T: bug Functionality that does not work as intended/expected
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug Report] v-select accessibility aria-labelledby doesn't work
5 participants