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(VSelects): focus / activation of chips and clearable w/ kb #18838

Merged
merged 3 commits into from
Mar 29, 2024

Conversation

tmasrat
Copy link
Contributor

@tmasrat tmasrat commented Dec 7, 2023

Description

If an icon is clickable, keyboard navigation to the icon should be allowed. If onClick is defined, I am setting the tabindex to 0
fixes #18482

Markup:

<template>
  <v-app>
    <v-container>
      <v-select
          clearable
          label="Select"
          :items="['California', 'Colorado', 'Florida', 'Georgia', 'Texas', 'Wyoming']"
      ></v-select>
    </v-container>
  </v-app>
</template>

@MajesticPotatoe MajesticPotatoe added T: bug Functionality that does not work as intended/expected C: VIcon labels Dec 11, 2023
@tmasrat
Copy link
Contributor Author

tmasrat commented Dec 14, 2023

@johnleider this is ready to be reviewed again

@johnleider
Copy link
Member

I don't have a good idea atm but I think running that function on every icon render is going to get expensive. @KaelWD, what are your thoughts?

@johnleider johnleider force-pushed the master branch 8 times, most recently from 7cd11a6 to d0765f1 Compare December 17, 2023 01:32
@tmasrat
Copy link
Contributor Author

tmasrat commented Dec 21, 2023

@johnleider @KaelWD any update on this one? This change is a prerequisite to #18493

@johnleider
Copy link
Member

I wouldn't expect too much movement on PRs until after Christmas.

@johnleider johnleider force-pushed the master branch 2 times, most recently from f931c2e to 748056c Compare December 28, 2023 22:41
@ibmalo95
Copy link

ibmalo95 commented Jan 8, 2024

@johnleider @KaelWD any more thoughts or suggestions on this one? We would really appreciate any help you can give so we can get this issue fixed. Thanks!

@ibmalo95
Copy link

@tmasrat would it work if we just add these properties directly to where vicon is added in the autocomplete component? We know we need them for autocompletes and might be better than trying to make it work for all icons. I'm not at all familiar with how the code is setup so not sure if that would work or not, but just a thought.

@johnleider
Copy link
Member

@johnleider @KaelWD any more thoughts or suggestions on this one? We would really appreciate any help you can give so we can get this issue fixed. Thanks!

I'm drawing a blank. I'll add it as a task item for next week and spend some more thought time on it.

@tmasrat
Copy link
Contributor Author

tmasrat commented Jan 19, 2024

@johnleider is there a way to pass down properties to the VIcon component from the Vselect/Vautocomplete? That would limit the fix to those two components only until we figure out a way to fix all icons

@tmasrat
Copy link
Contributor Author

tmasrat commented Jan 22, 2024

@johnleider I just pushed a new solution. It should fix all components that has VField as a child (VTextArea, VCombobox, VAutocomplete, VSelect...). There are conflicts on the PR but it's not related to the changes I made
Please let me know what you think about the new solution

@johnleider
Copy link
Member

Did you merge a branch into this?

@tmasrat
Copy link
Contributor Author

tmasrat commented Jan 24, 2024

Yes, I merged the upstream master into it

@johnleider
Copy link
Member

My suggestion would be to run git reflog and go back to the commit before your merge, then force push that up. It appears that you did something equivalent to git merge --ff-only instead of having an actual merge commit.

@tmasrat tmasrat force-pushed the VIcon-accessibility branch from a363828 to 837aad9 Compare January 25, 2024 15:16
@tmasrat
Copy link
Contributor Author

tmasrat commented Jan 25, 2024

@johnleider this is ready to be reviewed. Thanks!

@johnleider johnleider requested a review from KaelWD January 26, 2024 14:58
@tmasrat
Copy link
Contributor Author

tmasrat commented Feb 12, 2024

@KaelWD @johnleider any updates on this?

@johnleider
Copy link
Member

@vuetifyjs/core-team I'd like everyone to chime in on this please.

@blalan05
Copy link
Member

blalan05 commented Mar 4, 2024

I agree provide/inject seems like a lot. Also this only solves clearable, I think there maybe a different solution that can also cover append/prepend icons, but as an option, not mandatory if hasClick is true.

@yuwu9145
Copy link
Member

yuwu9145 commented Mar 15, 2024

  • Should be just as simple as tabindex={ listener ? 0 : -1 } in InputIcon, then tabindex will fall through to VIcon
  • Need to do more work around blur, currently tab blur input will blur whole field, it's unable to focus icon via tab
Screenshot 2024-03-15 at 10 04 24 pm

@johnleider
Copy link
Member

johnleider commented Mar 18, 2024

Partial fix here 9cbcea9. However, it isn't enough for the keyboard to actually trigger the clear. Pushing those changes to this P.R.

@johnleider
Copy link
Member

@tmasrat can you confirm that this still resolves your issue? I made some changes.

@tmasrat
Copy link
Contributor Author

tmasrat commented Mar 26, 2024

@johnleider thanks for the update! It works for a case where you tab into the close button on a dropdown menu but it doesn't work for the chips. It tabs into the close icon but it opens the dropdown instead of closing the selected option

<template>
  <v-app>
    <v-container>
      <v-select
        :items="['California', 'Colorado', 'Florida', 'Georgia', 'Texas', 'Wyoming']"
        label="Select"
        chips
        closable-chips
        multiple
      ></v-select>
    </v-container>
  </v-app>
</template>

@johnleider johnleider force-pushed the VIcon-accessibility branch from d758ead to 0759ba4 Compare March 28, 2024 19:11
@johnleider
Copy link
Member

Can you check now?

@tmasrat
Copy link
Contributor Author

tmasrat commented Mar 28, 2024

@johnleider the clearable chips are working but the other issue came back. The clear button is not clearing the selected value. It's opening the dropdown

<template>
  <v-app>
    <v-container>
      <v-select
        :items="['California', 'Colorado', 'Florida', 'Georgia', 'Texas', 'Wyoming']"
        label="Select"
        clearable
      ></v-select>
    </v-container>
  </v-app>
</template>

@johnleider johnleider added S: stale This issue is untriaged and hasn't seen any activity in at least six months. and removed S: stale This issue is untriaged and hasn't seen any activity in at least six months. labels Mar 28, 2024
@johnleider
Copy link
Member

3rd times the charm.

@tmasrat
Copy link
Contributor Author

tmasrat commented Mar 28, 2024

Everything works now 🥳 . Thank you @johnleider

@johnleider johnleider added this to the v3.5.x milestone Mar 28, 2024
@johnleider johnleider changed the title fix(VIcon): fix accessibility (add tabindex) fix(VSelect/Auto/Combo): focus and activation of chips and clearable using keyboard Mar 28, 2024
@johnleider johnleider merged commit 384d89e into vuetifyjs:master Mar 29, 2024
10 checks passed
@johnleider johnleider changed the title fix(VSelect/Auto/Combo): focus and activation of chips and clearable using keyboard fix(VSelects): focus / activation of chips and clearable w/ kb Mar 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: VIcon 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][3.3.21] Accessibility: v-select and v-autocomplete Clearable icon not keyboard accessible
6 participants