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(VExpansionPanel): remove duplicate aria-expanded #17284

Merged
merged 2 commits into from
May 4, 2023
Merged

fix(VExpansionPanel): remove duplicate aria-expanded #17284

merged 2 commits into from
May 4, 2023

Conversation

marjev
Copy link
Contributor

@marjev marjev commented May 3, 2023

fixes #15514

Description

Markup:

<template>
  <v-app>
    <v-container>
      <v-expansion-panels>
        <v-expansion-panel
          v-for="i in 3"
          :key="i"
          title="Item"
          text="Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea commodo consequat."
        />
      </v-expansion-panels>
    </v-container>
  </v-app>
</template>

KaelWD
KaelWD previously requested changes May 3, 2023
Copy link
Member

@KaelWD KaelWD left a comment

Choose a reason for hiding this comment

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

Don't just remove the attribute to make your validation tool pass, make it work in a screenreader

@marjev
Copy link
Contributor Author

marjev commented May 3, 2023

@KaelWD I've tested with VO on Safari and it works because there is a child button that actually contains the aria-expanded. With which screen reader did you test this?

VExpansionPanelTitle is the component which contains aria-expanded. Which WCAG criterion are you trying to meet with this concrete aria-expanded which I removed?

@KaelWD KaelWD dismissed their stale review May 3, 2023 19:13

Thanks for clarifying. I hadn't tested this yet, it appeared to just be a low effort "delete the thing making my tool complain" which we get a lot of. If VExpansionPanelTitle already has aria-expanded then yeah this is probably good.

@KaelWD KaelWD added T: bug Functionality that does not work as intended/expected a11y Accessibility issue C: VExpansionPanels VExpansionPanels labels May 3, 2023
@KaelWD KaelWD added this to the v3.2.x milestone May 3, 2023
@marjev
Copy link
Contributor Author

marjev commented May 3, 2023

@KaelWD thank you for following up. I can check with my QA team tomorrow to verify that it also works on the following:

  • NVDA + Firefox
  • JAWS + Chrome

Let me get back to you on those. I want to ensure that I don't accidentally introduce a new issue.

@KaelWD
Copy link
Member

KaelWD commented May 4, 2023

Looks alright in NVDA + Chrome

@KaelWD KaelWD changed the title fix(VExpansionPanel): incorrect use of aria-expanded fix(VExpansionPanel): remove duplicate aria-expanded May 4, 2023
@KaelWD KaelWD merged commit 94335b1 into vuetifyjs:master May 4, 2023
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a11y Accessibility issue C: VExpansionPanels VExpansionPanels 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][2.6.7] Expansion Panel uses aria-expanded attribute on a div
2 participants