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

commandbar accessiblity #14805

Closed
reijam opened this issue Aug 28, 2020 · 16 comments
Closed

commandbar accessiblity #14805

reijam opened this issue Aug 28, 2020 · 16 comments

Comments

@reijam
Copy link

reijam commented Aug 28, 2020

see attached images

image
image

package.json

{
  "name": "fluent-ui-react-repo",
  "version": "7.0.0",
  "description": "Reusable React components for building experiences for Microsoft 365.",
  "private": true,
  "repository": {
    "type": "git",
    "url": "https://github.com/microsoft/fluentui"
  },
  "scripts": {
    "a11ytest": "cd apps && cd a11y-tests && npm run test",
    "build": "lerna run build --stream",
    "build:fluentui:docs": "gulp build:docs",
    "build:min": "yarn buildto @fluentui/react-next @fluentui/react-northstar --min",
    "build:prod": "lerna run build --stream -- --production",
    "buildci": "yarn build && yarn test && yarn lint && yarn bundle",
    "buildci:prod": "yarn build:prod && yarn test && yarn lint && yarn bundle:prod",
    "builddemo": "node ./scripts/monorepo/buildTo.js fabric-website-resources",
    "buildto": "node ./scripts/monorepo/buildTo.js",
    "bundle": "lerna run bundle --stream",
    "bundle:prod": "lerna run bundle --stream -- --production",
    "bundlesize": "cd scripts && npm run bundlesize",
    "bundlesizecollect": "cd scripts && just-scripts bundle-size-collect",
    "checkchange": "beachball check  --scope \"!packages/fluentui/*\" --changehint \"Run 'yarn change' to generate a change file\"",
    "clean": "lerna run clean --stream",
    "codepen": "cd packages/office-ui-fabric-react && node ../../scripts/local-codepen.js",
    "code-style": "lerna run code-style --stream",
    "create-package": "plop --plopfile ./scripts/create-package/plopfile.ts --dest . --require ./scripts/ts-node-register",
    "change": "beachball change --scope \"!packages/fluentui/*\"",
    "dom-test": "cd apps/dom-tests && just-scripts jest-dom-with-webpack",
    "generate-version-files": "yarn workspace @uifabric/build just generate-version-files",
    "lage": "lage",
    "lint": "lerna run lint --stream",
    "lint:log": "FORCE_COLOR=0 lerna run lint --stream --no-bail > lint.log 2>&1",
    "sync:beachball": "beachball sync --scope \"!packages/fluentui/*\"",
    "publish:beachball": "beachball publish --scope \"!packages/fluentui/*\"",
    "check-for-changed-files": "cd scripts && just-scripts check-for-modified-files",
    "perf": "cross-env PERF=true gulp perf --times=100",
    "perf:debug": "cross-env PERF=true gulp perf:debug --debug",
    "preinstall": "node ./scripts/use-yarn-please.js",
    "prelint": "yarn satisfied && yarn syncpack:list-mismatches && node ./scripts/no-tslint.js",
    "postinstall": "node ./scripts/postinstall.js",
    "prettier": "node scripts/prettier.js",
    "rebuild": "node ./scripts/invalidate-just-cache.js && npm run build",
    "release:fluentui:minor": "yarn workspace @uifabric/build just fluentui:publish:minor",
    "release:fluentui:patch": "yarn workspace @uifabric/build just fluentui:publish:patch",
    "satisfied": "satisfied --skip-invalid",
    "scrub": "node ./scripts/scrub.js",
    "start:legacy": "yarn workspace @uifabric/fabric-website-resources start",
    "start": "node scripts/start.js",
    "start-exp": "yarn workspace @uifabric/experiments start",
    "stats:build": "gulp stats",
    "stats:save": "gulp stats:save",
    "syncpack:list-mismatches": "syncpack list-mismatches --prod --dev --source \"{apps/*,packages/!(web-components),packages/fluentui/*,scripts,.}/package.json\"",
    "syncpack:fix": "syncpack fix-mismatches --prod --dev --source \"{apps/*,packages/!(web-components),packages/fluentui/*,scripts,.}/package.json\"",
    "test": "lerna run test --stream",
    "test:fluentui:circulars": "gulp test:circulars:run",
    "test:fluentui:e2e": "gulp test:e2e",
    "test:fluentui:visual": "gulp screener",
    "test:fluentui:projects": "gulp test:projects",
    "update-api": "cd packages/office-ui-fabric-react && npm run update-api",
    "update-snapshots": "lerna run update-snapshots --stream",
    "update-a11y": "cd apps/a11y-tests && npm run update-snapshots",
    "vrtest": "cd apps && cd vr-tests && npm run screener"
  },
  "devDependencies": {
    "beachball": "^1.31.2",
    "cross-env": "^5.1.4",
    "danger": "^6.0.5",
    "gulp": "^4.0.2",
    "lerna": "^3.15.0",
    "lage": "^0.17.5",
    "lint-staged": "^10.2.9",
    "sass-loader": "^6.0.6",
    "satisfied": "^1.1.1",
    "syncpack": "^4.5.4"
  },
  "license": "MIT",
  "workspaces": {
    "packages": [
      "apps/*",
      "packages/*",
      "scripts",
      "packages/fluentui/*"
    ],
    "nohoist": [
      "packages/web-components/typescript",
      "packages/web-components/typescript/**",
      "packages/web-components/ts-loader",
      "packages/web-components/ts-loader/**",
      "packages/web-components/ts-node",
      "packages/web-components/ts-node/**",
      "packages/web-components/mocha",
      "packages/web-components/mocha/**",
      "packages/web-components/webpack",
      "packages/web-components/webpack/**"
    ]
  },
  "resolutions": {
    "eslint": "^7.1.0",
    "webpack": "^4.35.0",
    "autoprefixer": "9.7.6",
    "copy-to-clipboard": "3.2.0"
  }
}
@paulgildea
Copy link
Member

@reijam Thanks for the feedback.

Could you provide more information for a reproduction? Possibly link to a codepen? That would help our team understand if this is a recent regression.

Thanks!

@msft-github-bot
Copy link
Contributor

Thanks for taking the time to enter an issue. However, it seems that there aren't enough details here for this issue to be actionable.

When issues are created, we need details such as:

  1. Which Fluent UI component is causing the issue
  2. Which package name and version the component is from
  3. Specific, complete steps to reproduce the issue
  4. What behaviors and attributes are missing or incorrect
  5. What you expected and what is actually happening
  6. Confirmation that the problem reproduces in isolation

Without a clear understanding of these details, it's not possible to take clear action on issues. We are unable to meet your expectations, properly address the root cause, and make changes without affecting the expectations of other consumers.

Please provide these additional details as you are able. The default issue template provides an outline of these details and is viewable when creating a new issue. Additionally, if this is an accessibility issue, please see Accessibility Troubleshooting in our wiki for more guidance. If these details cannot be provided, please kindly close the issue.

Thank you for your patience.

@reijam
Copy link
Author

reijam commented Aug 31, 2020

if you look at the picture carefully you will see that all the answers are there. Hence the image...

if you look at the package.json it has all the version numbers

@PurpleGranite
Copy link

I came here specifically to report accessibility issues with the Command Bar - so glad to see an existing issue open.

Something has changed recently, though I can't put my finger on exactly when, as I first spotted the change in SharePoint Online rather than in my own code.

Sometime in the last few weeks, SharePoint Online has picked up a new version of the controls where the background to the Command Bar is white rather than grey, and where text size seems to have decreased slightly.

As the automated sample from @reijam shows, there are a number of issues that can be detected via tools.

What can't be shown is the impact that the specific change from grey to white as a background for the Command Bar has on magnifier users like myself. Where the Command Bar background is the same as the rest of the page background, it removes a necessary visual cue indicating a change of function / role.

Correcting the missing aria tags will take care of this for screen reader users, and users with no sight impairments are likely unaffected with the current version - but those of us who use magnification software at relatively high zoom levels need a proper visual indication of when we're looking at a set of related controls on a Command Bar, versus other page areas.

Please consider either reverting the change in background colour, or changing the background to some colour other than the page background (and remaining within the WCAG AA colour contrast guidelines for other controls on the Command Bar).

@XianghanWang
Copy link
Member

our team meet same issue. Could we add some simple accessibility tests before submit the code change?

@cschlechty
Copy link
Member

cschlechty commented Sep 4, 2020

This is a result of Accessibility Insights updating their rule set. It doesn't look past the grouping to find the menuitems (or rather checks that the group has the correct children) now. Group isn't valid (according to wai-aria 1.1) unless there's a menuitemradio set. The groups for the left and right sections should probably just be removed.

@reijam a quick reminder that not everyone can pull information from screenshots, especially when the alt is "image". It is always preferable to write the specific details of the issue. If you expand the error, in the "..." menu, there is an option for copying the details. In the screenshot, there are three flagged issues, two of which look to be non-command bar.

@PurpleGranite you should probably file a separate issue for contrast changes. It's separate from what this is flagging.

@PurpleGranite
Copy link

@PurpleGranite you should probably file a separate issue for contrast changes. It's separate from what this is flagging.

Well I'm sorry @cschlechty, I jumped on this as a related issue because in literally every other Microsoft repository on GitHub when I open a new issue, I get shut down as being a duplicate, even when it isn't, or the response is to pass the buck to another team and then everything goes deathly silent - and I've seen it time-and-again for others posting accessibility issues as well.

It actually feels like you guys - despite having a "Chief Accessibility Officer" - are actually trying at most turns to put barriers in the way of people from the community reporting accessibility issues! You need some consistency across your entire estate, because as it stands there's nothing but mixed messages.

Sure, I'll go log a separate issue - but only if you're able to provide some assurance beforehand that it'll be taken seriously and resolved, because it feels like community reports of accessibility issues simply don't mean much to you as an organisation.

@cschlechty
Copy link
Member

@PurpleGranite we take issues reported seriously. I can't guarantee that it will be resolved to your satisfaction, but as separate issue it can be triaged, tagged, and managed better. There is greater transparency and clarity for all. Putting multiple issues into one often results in one or more issue getting missed or not addressed as the discussions grow.

@reijam
Copy link
Author

reijam commented Sep 8, 2020

thanks for the tip did not realize that it was a challenge to copy from image... will fix next time.

@joschect
Copy link
Contributor

@reijam Even for sighted users it can be useful to have the actual text of the error.

After digging a bit it seems that this particular error is popping up because we are setting a role of group on a child. This makes sense logically as it's a resizegroup, but unfortunately within the context of a menubar a group can only contain children with role menuitemradio. It should be a quick fix to remove the group label.

Regarding the second reported issue, that I'm seeing is that the split buttons don't have names on the menu button portion. This is understandable as the buttons themselves are not a valid tabstop. I'm not sure about the fix and Ill need to try some things out.

Title: WCAG 1.3.1: Ensures elements with an ARIA role that require child roles contain them (div[data-focuszone-id="FocusZone296"])
Tags: Accessibility, WCAG 1.3.1, aria-required-children

Issue: Ensures elements with an ARIA role that require child roles contain them (aria-required-children - https://dequeuniversity.com/rules/axe/4.0/aria-required-children?application=msftAI)

Target application: Home - Fluent UI - https://developer.microsoft.com/en-us/fluentui#/controls/web/commandbar

Element path: div[data-focuszone-id="FocusZone296"]

Snippet: <div class="ms-FocusZone css-240 ms-CommandBar root-332" role="menubar" aria-label="Use left and right arrow keys to navigate between commands" data-focuszone-id="FocusZone296">

How to fix: 
Fix any of the following:
  Required ARIA children role not present: menuitemradio, menuitem, menuitemcheckbox

Environment: Chrome version 85.0.4183.83

====

This accessibility issue was found using Accessibility Insights for Web 2.21.1 (axe-core 4.0.1), a tool that helps find and fix accessibility issues. Get more information & download this tool at http://aka.ms/AccessibilityInsights.

@reijam
Copy link
Author

reijam commented Sep 10, 2020

let me know if you need a beta tester. i am your guy!

@shuotli
Copy link

shuotli commented Sep 17, 2020

Our team is running into the same issue. Let me know if you need a tester.

@JustSlone
Copy link
Collaborator

There is some more information on the aria-required-children error in this issue #14993.

@jeffersoneagley
Copy link

Just ran into this too, seems like accessibilityinsights may have gotten some new rules lately and one of them seems to think that menubar > group > menuitem isn't ok anymore and wants a direct parent.

@msft-github-bot
Copy link
Contributor

@joschect

Gentle ping that this issue needs attention.

@joschect
Copy link
Contributor

This was fixed in #15511

@microsoft microsoft locked as resolved and limited conversation to collaborators Nov 22, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests