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(VMenu): add jump to first/last item keyboard navigation #12348

Merged
merged 4 commits into from
Nov 11, 2020

Conversation

janKollars
Copy link
Contributor

@janKollars janKollars commented Oct 4, 2020

pressing HOME in a v-menu selects the first selectable list item, END the last

Description

I am not aware of any related open issues.

Motivation and Context

VSelect uses the VMenu component. On long lists it is convenient to use HOME/END to navigate to the top/bottom. This is an optional behaviour according to the W3C and is implemented in their examples. The <select> HTML element as implemented in Chrome/Firefox (Windows/Linux) provides this functionality too.
Moreover, a menu as defined by the W3C is required to be navigable using these two buttons.

How Has This Been Tested?

A new unit test was created for checking the new behaviour.

Markup:

<template>
  <v-container>
    <v-select
      :items="[
        {
          text: 'foo',
          disabled: true,
        },
        'bar',
        'hello',
        'world'
      ]"
      label="pick one"
    />
  </v-container>
</template>

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)

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 backwards compatible changes and next for non-backwards compatible 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)

The last checklist item is not necessary in my opinion. Please correct me if I am wrong.

@KaelWD KaelWD added C: VMenu VMenu T: enhancement Functionality that enhances existing features labels Oct 5, 2020
@KaelWD KaelWD added this to the v2.4.0 milestone Oct 5, 2020
pressing HOME in a v-menu selects the first selectable list item, END the last
@johnleider johnleider self-assigned this Oct 20, 2020
@@ -432,7 +452,7 @@ export default baseMixins.extend({
this.$nextTick(() => activator && activator.focus())
} else if (
!this.isActive &&
[keyCodes.up, keyCodes.down].includes(e.keyCode)
[keyCodes.up, keyCodes.down, keyCodes.home, keyCodes.end].includes(e.keyCode)
Copy link
Member

Choose a reason for hiding this comment

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

https://www.w3.org/TR/wai-aria-practices/#menubutton only mentions up/down, and none of the sections you linked explicitly state that home/end should also open the menu.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are absolutely correct! While focusing a select, home and end should only select the first/last selectable item and should not open the menu.
Sorry for the inconvenience. I will fix this as soon as I can.

@@ -424,6 +428,22 @@ export default baseMixins.extend({
this.listIndex--
if (tile.tabIndex === -1) this.prevTile()
},
lastTile () {
const tile = this.tiles[this.tiles.length - 1]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const tile = this.tiles[this.tiles.length - 1]
const tile = this.tiles[this.tiles.length - 1]

if (tile.tabIndex === -1) this.prevTile()
},
firstTile () {
const tile = this.tiles[0]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const tile = this.tiles[0]
const tile = this.tiles[0]

@janKollars
Copy link
Contributor Author

I incorporated the feedback. When you test the changes, you might notice a bug: The select loses the focus after up/down/home/end was pressed. This problem was not introduced by my changes.
I created an issue for this #12572

@KaelWD KaelWD merged commit 098db11 into vuetifyjs:dev Nov 11, 2020
KaelWD added a commit that referenced this pull request Nov 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: VMenu VMenu T: enhancement Functionality that enhances existing features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants