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(VMenu/Selects): properly configure a11y #7385

Merged
merged 33 commits into from
Jun 18, 2019
Merged

fix(VMenu/Selects): properly configure a11y #7385

merged 33 commits into from
Jun 18, 2019

Conversation

johnleider
Copy link
Member

@johnleider johnleider commented Jun 3, 2019

TODO

  • add a11y specific tests
  • focus on select, hit space to open, scrolls document
  • up/down without list shown should cycle
  • down arrow on select causes document scroll before the list pops up

Description

Improved a11y with v-menu and implementing components. Move keyboard interaction to be more in line with native behavior.

  • v-list-item
    • When inside of a v-menu will be a menu-item
    • When inside of a v-list-item-group will be a list-item
    • When inside of a v-select will be an option
    • Can also have its role manually set
  • v-menu
    • Will now skip disabled items when moving through list items
    • Can now move through list items with tab and shift+tab
    • Arrow keys can no longer set listIndex to -1 by pressing the up arrow
  • v-select
    • Can now cycle through options when focused be pressing up or down (only applies to single select)
    • Will now focus the first selected v-list-item (if applicable) when opening the menu
    • Will now maintain the currently active item (highlight when the item index changes) when toggling items on and off
    • Now uses v-simple-checkbox to avoid issues caused by having an unrelated label element (also faster now!)
  • routable
    • Removed explicit applying of the disabled prop. Now only happens on v-btn
  • activatable
    • Now passes a11y attributes through the scoped activator slot v-slot:activator="{ on, attrs }"

Motivation and Context

Provide better a11y support and make select inputs feel more native.

How Has This Been Tested?

jest

Markup:

<template>
  <div class="ma-5 pa-5">
    <v-menu>
      <template v-slot:activator="{ on, attrs }">
        <v-btn
          v-bind="attrs"
          v-on="on"
        >Menu</v-btn>
      </template>

      <v-list>
        <v-list-item
          v-for="i in 5"
          :key="i"
          :disabled="i === 2"
          link
        >
          <v-list-item-title>Hello</v-list-item-title>
        </v-list-item>
      </v-list>
    </v-menu>

    <div class="my-5"></div>

    <v-select
      :items="[1, 2, 3, 4]"
      label="Select"
    ></v-select>

    <div class="my-5"></div>

    <v-select
      :items="[{
        text: 'Foo'
      }, {
        text: 'Bar',
        disabled: true
      }, {
        text: 'Fizz'
      }]"
      label="Select w/ Disabled option"
    ></v-select>

    <div class="my-5"></div>

    <v-autocomplete
      :items="[1,2,3,4]"
      label="Autocomplete"
    ></v-autocomplete>

    <div class="my-5"></div>

    <v-combobox
      :items="[1,2,3,4]"
      label="Combobox"
    ></v-combobox>

    <div class="my-5"></div>

    <v-select
      :items="[1,2,3,4]"
      label="Select multiple"
      multiple
    ></v-select>

    <div class="my-5"></div>

    <v-autocomplete
      :items="[1,2,3,4]"
      label="Autocomplete multiple"
      multiple
    ></v-autocomplete>

    <div class="my-5"></div>

    <v-combobox
      :items="[1,2,3,4]"
      label="Combobox multiple"
    ></v-combobox>
  </div>
</template>

<script>
  export default {
    data: () => ({
      //
    }),
  }
</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 feature but make things better)

Checklist:

  • The PR title is no longer than 64 characters.
  • The PR is submitted to the correct branch (master for bug fixes and documentation updates, dev for new features and breaking changes).
  • My code follows the code style of this project.
  • I've added relevant changes to the documentation (applies to new features and breaking changes in core library)

@codecov
Copy link

codecov bot commented Jun 4, 2019

Codecov Report

Merging #7385 into next will decrease coverage by 0.05%.
The diff coverage is 89.85%.

Impacted file tree graph

@@            Coverage Diff             @@
##             next    #7385      +/-   ##
==========================================
- Coverage   85.78%   85.73%   -0.06%     
==========================================
  Files         329      329              
  Lines        8610     8663      +53     
  Branches     2157     2178      +21     
==========================================
+ Hits         7386     7427      +41     
- Misses       1134     1147      +13     
+ Partials       90       89       -1
Impacted Files Coverage Δ
packages/vuetify/src/mixins/routable/index.ts 97.61% <ø> (ø) ⬆️
packages/vuetify/src/components/VInput/VInput.ts 97.18% <ø> (ø) ⬆️
packages/vuetify/src/components/VList/VList.ts 57.14% <ø> (ø) ⬆️
...ages/vuetify/src/components/VSelect/VSelectList.ts 90% <100%> (+0.38%) ⬆️
...uetify/src/components/VCheckbox/VSimpleCheckbox.ts 90.47% <100%> (ø) ⬆️
...tify/src/components/VAutocomplete/VAutocomplete.ts 99.22% <100%> (+0.02%) ⬆️
packages/vuetify/src/components/VList/VListItem.ts 100% <100%> (ø) ⬆️
packages/vuetify/src/components/VSelect/VSelect.ts 94.77% <100%> (+0.78%) ⬆️
packages/vuetify/src/mixins/activatable/index.ts 100% <100%> (ø) ⬆️
packages/vuetify/src/components/VMenu/VMenu.ts 71.24% <75.86%> (-2.25%) ⬇️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d8fb3cf...6f84d8c. Read the comment docs.

@codecov
Copy link

codecov bot commented Jun 4, 2019

Codecov Report

Merging #7385 into next will decrease coverage by 0.18%.
The diff coverage is 94.69%.

Impacted file tree graph

@@            Coverage Diff             @@
##             next    #7385      +/-   ##
==========================================
- Coverage   87.12%   86.93%   -0.19%     
==========================================
  Files         325      325              
  Lines        8455     8536      +81     
  Branches     2120     2153      +33     
==========================================
+ Hits         7366     7421      +55     
- Misses        999     1025      +26     
  Partials       90       90
Impacted Files Coverage Δ
packages/vuetify/src/components/VInput/VInput.ts 97.18% <ø> (ø) ⬆️
packages/vuetify/src/components/VList/VList.ts 57.14% <ø> (ø) ⬆️
packages/vuetify/src/mixins/routable/index.ts 97.61% <ø> (ø) ⬆️
...ages/vuetify/src/components/VCombobox/VCombobox.ts 89.33% <100%> (-5.27%) ⬇️
...uetify/src/components/VCheckbox/VSimpleCheckbox.ts 90.47% <100%> (ø) ⬆️
packages/vuetify/src/mixins/activatable/index.ts 100% <100%> (ø) ⬆️
...ages/vuetify/src/components/VSelect/VSelectList.ts 90% <100%> (+0.38%) ⬆️
packages/vuetify/src/components/VBtn/VBtn.ts 100% <100%> (ø) ⬆️
packages/vuetify/src/components/VList/VListItem.ts 100% <100%> (ø) ⬆️
...tify/src/components/VAutocomplete/VAutocomplete.ts 99.21% <100%> (+0.01%) ⬆️
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6016cf8...4432081. Read the comment docs.

@johnleider johnleider changed the title fix(VMenu): properly configure a11y fix(VMenu/Selects): properly configure a11y Jun 8, 2019
@jacekkarczmarczyk
Copy link
Member

In multiple examples (select/combo/autocomplete) - i can't tab out of the field, tab just toggles the selection

In menu - tab moves to the next item, I'm not actually sure what should be the correct behaviour here, but my intuition told me that i'd tab out of the menu and it surprised me that it didn't happen.

@KaelWD
Copy link
Member

KaelWD commented Jun 15, 2019

https://www.w3.org/TR/wai-aria-practices/#menubutton

Up/down should open the menu and focus the last/first item when the activator is focused
Up/down should focus the previous/next item in the menu when already open
Tab should close the menu, return focus to the activator, and then proceed to the next item in the tab order

Will now select the first selected v-list-item (if applicable) when opening the menu

Should be "focus the first selected"?

@jacekkarczmarczyk
Copy link
Member

Also should up/down cycle through the items in menu?

@vercel vercel bot temporarily deployed to staging June 17, 2019 15:52 Inactive
@johnleider
Copy link
Member Author

Also should up/down cycle through the items in menu?

resolved

@johnleider
Copy link
Member Author

https://www.w3.org/TR/wai-aria-practices/#menubutton

Up/down should open the menu and focus the last/first item when the activator is focused
Up/down should focus the previous/next item in the menu when already open
Tab should close the menu, return focus to the activator, and then proceed to the next item in the tab order

Will now select the first selected v-list-item (if applicable) when opening the menu

Should be "focus the first selected"?

resolved

@vercel vercel bot temporarily deployed to staging June 17, 2019 15:58 Inactive
@johnleider
Copy link
Member Author

In multiple examples (select/combo/autocomplete) - i can't tab out of the field, tab just toggles the selection

In menu - tab moves to the next item, I'm not actually sure what should be the correct behaviour here, but my intuition told me that i'd tab out of the menu and it surprised me that it didn't happen.

You are correct, default behavior for multiple select is to move on with whatever selection have been made. Single select selects the item and re-focuses the input. Also tab behavior has been removed from menu and will now properly move on.

@dsseng dsseng merged commit e9a816d into next Jun 18, 2019
@dsseng dsseng deleted the fix/menu-a11y branch June 18, 2019 11:55
@lock lock bot locked as resolved and limited conversation to collaborators Jul 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
T: bug Functionality that does not work as intended/expected T: enhancement Functionality that enhances existing features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants