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

Bug/menu item missing aria labels #3417

Merged
merged 11 commits into from Jul 18, 2023
Merged

Conversation

blunteshwar
Copy link
Contributor

@blunteshwar blunteshwar commented Jul 6, 2023

Description

Added aria-haspopup,aria-expanded and aria-controls to MenuItem with subitem.

Related issue(s)

#3248

Motivation and context

The following attributes { aria-expanded , aria-haspopup and aria-controls } did not exist for a MenuItem with subitem so I added them.

How has this been tested?

Uploading Screen Recording 2023-07-12 at 8.18.21 PM.mov…

  • Test case 1
    1. Go here
    2. Do this
  • Test case 2
    1. Go here
    2. Do this

Screenshots (if appropriate)

Screenshot 2023-07-07 at 2 34 19 PM

Types of changes

  • [ x] 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 change)
  • Chore (minor updates related to the tooling or maintenance of the repository, does not impact compiled assets)

Checklist

  • [ x] I have signed the Adobe Open Source CLA.
  • [ x] My code follows the code style of this project.
  • [ x] If my change required a change to the documentation, I have updated the documentation in this pull request.
  • [ x] I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • [ x] All new and existing tests passed.
  • [ x] I have reviewed at the Accessibility Practices for this feature, see: Aria Practices

Best practices

This repository uses conventional commit syntax for each commit message; note that the GitHub UI does not use this by default so be cautious when accepting suggested changes. Avoid the "Update branch" button on the pull request and opt instead for rebasing your branch against main.

@github-actions
Copy link

github-actions bot commented Jul 6, 2023

Tachometer results

Chrome

action-menu permalink

Version Bytes Avg Time vs remote vs branch
npm latest 738 kB 228.52ms - 232.46ms - unsure 🔍
-3% - +0%
-6.03ms - +0.33ms
branch 738 kB 230.84ms - 235.84ms unsure 🔍
-0% - +3%
-0.33ms - +6.03ms
-

menu permalink

Version Bytes Avg Time vs remote vs branch
npm latest 420 kB 309.90ms - 314.88ms - unsure 🔍
-1% - +2%
-3.02ms - +5.31ms
branch 421 kB 307.92ms - 314.58ms unsure 🔍
-2% - +1%
-5.31ms - +3.02ms
-

picker permalink

Version Bytes Avg Time vs remote vs branch
npm latest 593 kB 769.91ms - 792.81ms - unsure 🔍
-3% - +1%
-20.54ms - +10.78ms
branch 592 kB 775.56ms - 796.92ms unsure 🔍
-1% - +3%
-10.78ms - +20.54ms
-

split-button permalink

Version Bytes Avg Time vs remote vs branch
npm latest 671 kB 2047.68ms - 2051.05ms - unsure 🔍
-0% - +0%
-1.63ms - +3.64ms
branch 670 kB 2046.34ms - 2050.39ms unsure 🔍
-0% - +0%
-3.64ms - +1.63ms
-
Firefox

action-menu permalink

Version Bytes Avg Time vs remote vs branch
npm latest 738 kB 403.32ms - 424.44ms - unsure 🔍
-6% - +1%
-26.55ms - +6.39ms
branch 738 kB 411.32ms - 436.60ms unsure 🔍
-2% - +6%
-6.39ms - +26.55ms
-

menu permalink

Version Bytes Avg Time vs remote vs branch
npm latest 420 kB 560.01ms - 599.91ms - unsure 🔍
-4% - +4%
-26.05ms - +23.09ms
branch 421 kB 567.10ms - 595.78ms unsure 🔍
-4% - +4%
-23.09ms - +26.05ms
-

picker permalink

Version Bytes Avg Time vs remote vs branch
npm latest 593 kB 1078.21ms - 1123.51ms - unsure 🔍
-2% - +3%
-19.70ms - +34.46ms
branch 592 kB 1078.64ms - 1108.32ms unsure 🔍
-3% - +2%
-34.46ms - +19.70ms
-

split-button permalink

Version Bytes Avg Time vs remote vs branch
npm latest 671 kB 2179.27ms - 2196.37ms - unsure 🔍
-1% - +0%
-12.03ms - +10.15ms
branch 670 kB 2181.69ms - 2195.83ms unsure 🔍
-0% - +1%
-10.15ms - +12.03ms
-

@blunteshwar blunteshwar marked this pull request as ready for review July 7, 2023 09:06
@blunteshwar blunteshwar linked an issue Jul 7, 2023 that may be closed by this pull request
1 task
Copy link
Collaborator

@najikahalsema najikahalsema left a comment

Choose a reason for hiding this comment

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

How did you test this? Did you utilise the accessibility tree or screen reader?

packages/accordion/src/AccordionItem.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@Westbrook Westbrook left a comment

Choose a reason for hiding this comment

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

There’s a bunch of white space addition, was the intentional?

packages/menu/src/MenuItem.ts Outdated Show resolved Hide resolved
packages/menu/src/MenuItem.ts Outdated Show resolved Hide resolved
@blunteshwar
Copy link
Contributor Author

There’s a bunch of white space addition, was the intentional?

extra white spaces have been removed now

@blunteshwar
Copy link
Contributor Author

How did you test this? Did you utilise the accessibility tree or screen reader?

I have used both screen reader and accessibility tree

@@ -425,6 +430,11 @@ export class MenuItem extends LikeAnchor(Focusable) {
this.handleSubmenuPointerenter
);
submenu.addEventListener('change', this.handleSubmenuChange);
if (!submenu.id) {
// if the consumer has already applied an ID to this element then we won't explicitly aplly ID
Copy link
Contributor

Choose a reason for hiding this comment

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

please remove the comments here!

@@ -631,6 +631,7 @@ export class Menu extends SpectrumElement {
}
item.focused = this.hasVisibleFocusInTree();
this.setAttribute('aria-activedescendant', item.id);

Copy link
Contributor

Choose a reason for hiding this comment

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

remove this white space

Rajdeepc
Rajdeepc previously approved these changes Jul 18, 2023
@najikahalsema najikahalsema merged commit 0d04869 into main Jul 18, 2023
47 checks passed
@najikahalsema najikahalsema deleted the bug/menu-item-missing-Aria-labels branch July 18, 2023 18:33
TarunAdobe added a commit that referenced this pull request Jul 19, 2023
* fix(meter): added role meter progressbar in meter component (#3459)

Co-authored-by: Rajdeep Chandra <rajdeepc@adobe.com>

* docs: updated swatch mixed documentations (#3455)

Co-authored-by: Rajdeep Chandra <rajdeepc@adobe.com>

* fix(textfield): update focus state when [multiline][quiet] (#3452)

* fix: menu item missing aria labels (#3417)

* testing

* fix(menu): menu-item with submenu lacks aria-haspopup and aria-expanded

* fix(menu): added aria-expanded and aria-haspopup to menu-item

* fix(menu): fixed the aria-controls attribute of menu-item

* fix(menu): reviewed changes

* fix(menu): reviewed changes-2

* fix(menu): removed the comment

* fix(menu): removed white space

* fix(menu): white space removed

* chore: bump @web/test-runner from 0.16.1 to 0.17.0

Bumps [@web/test-runner](https://github.com/modernweb-dev/web/tree/HEAD/packages/test-runner) from 0.16.1 to 0.17.0.
- [Release notes](https://github.com/modernweb-dev/web/releases)
- [Changelog](https://github.com/modernweb-dev/web/blob/master/packages/test-runner/CHANGELOG.md)
- [Commits](https://github.com/modernweb-dev/web/commits/@web/test-runner@0.17.0/packages/test-runner)

---
updated-dependencies:
- dependency-name: "@web/test-runner"
  dependency-type: direct:development
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>

* chore: bump webpack-bundle-analyzer from 4.8.0 to 4.9.0

Bumps [webpack-bundle-analyzer](https://github.com/webpack-contrib/webpack-bundle-analyzer) from 4.8.0 to 4.9.0.
- [Release notes](https://github.com/webpack-contrib/webpack-bundle-analyzer/releases)
- [Changelog](https://github.com/webpack-contrib/webpack-bundle-analyzer/blob/master/CHANGELOG.md)
- [Commits](webpack-contrib/webpack-bundle-analyzer@v4.8.0...v4.9.0)

---
updated-dependencies:
- dependency-name: webpack-bundle-analyzer
  dependency-type: direct:development
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>

* chore: bump netlify-cli from 12.12.0 to 15.9.0

Bumps [netlify-cli](https://github.com/netlify/cli) from 12.12.0 to 15.9.0.
- [Release notes](https://github.com/netlify/cli/releases)
- [Changelog](https://github.com/netlify/cli/blob/main/CHANGELOG.md)
- [Commits](netlify/cli@v12.12.0...v15.9.0)

---
updated-dependencies:
- dependency-name: netlify-cli
  dependency-type: direct:development
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <support@github.com>

* chore: bump husky from 8.0.2 to 8.0.3

Bumps [husky](https://github.com/typicode/husky) from 8.0.2 to 8.0.3.
- [Release notes](https://github.com/typicode/husky/releases)
- [Commits](typicode/husky@v8.0.2...v8.0.3)

---
updated-dependencies:
- dependency-name: husky
  dependency-type: direct:development
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>

* chore: bump word-wrap from 1.2.3 to 1.2.4

Bumps [word-wrap](https://github.com/jonschlinkert/word-wrap) from 1.2.3 to 1.2.4.
- [Release notes](https://github.com/jonschlinkert/word-wrap/releases)
- [Commits](jonschlinkert/word-wrap@1.2.3...1.2.4)

---
updated-dependencies:
- dependency-name: word-wrap
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <support@github.com>

* chore: bump @types/react from 18.0.25 to 18.2.15

Bumps [@types/react](https://github.com/DefinitelyTyped/DefinitelyTyped/tree/HEAD/types/react) from 18.0.25 to 18.2.15.
- [Release notes](https://github.com/DefinitelyTyped/DefinitelyTyped/releases)
- [Commits](https://github.com/DefinitelyTyped/DefinitelyTyped/commits/HEAD/types/react)

---
updated-dependencies:
- dependency-name: "@types/react"
  dependency-type: direct:development
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>

---------

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: Rajdeep Chandra <rajdeep.chandra@bsil.com>
Co-authored-by: Rajdeep Chandra <rajdeepc@adobe.com>
Co-authored-by: Piyush Vashisht <pvashish@adobe.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug][a11y]: MenuItem with submenu lacks aria-haspopup and aria-expanded
4 participants