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(axe-core-4.6): update to axe-core 4.6.3, promote link-in-text-block and meta-viewport #6334

Merged
merged 29 commits into from
Jan 30, 2023

Conversation

dbjorge
Copy link
Contributor

@dbjorge dbjorge commented Jan 10, 2023

Details

This PR updates axe-core to its latest version, 4.6.2, from 4.4.1.

Notable code changes beyond the simple version bump include:

  • link-in-text-block failures have been promoted from "needs review" to "automated checks" (removed from needs review with explicit code change, added to automated checks implicitly due to new axe-core tagging)
  • meta-viewport failures have been promoted from "unreported" to "automated checks" (implicitly due to new axe-core tagging)
  • WCAG mappings for a few rules have changed (we pull these automatically from axe-core's tags, so there is no corresponding code change)
  • Per discussion with PM, link-in-text-block incomplete results are staying unreported (not being promoted to "needs review")
  • frame-title-unique and no-autoplay-audio are explicitly disabled. They were promoted in axe-core from experimental/best-practice to "wcag-correspondant, but needs-review-only", but we want to omit them from our needs review checks pending further investigation
Motivation

See 1960423

Context
  • We are considering a separate PR to pull in a temporary version of the aria-required-children message improvements we've suggested upstream as axe-core#3842 (which would hopefully be removed later once we update to axe-core 4.7). Leaving that for a separate PR.
  • While testing this locally, I noticed an issue where the target page console would show an error during scans suggesting axe-core scans were being run multiple times concurrently. I also observed this is main without this PR - I don't think it's a regression and will be following up separately.

Pull request checklist

  • Addresses an existing issue: 1960423
  • Ran yarn fastpass
  • Added/updated relevant unit test(s) (and ran yarn test)
  • Verified code coverage for the changes made. Check coverage report at: <rootDir>/test-results/unit/coverage
  • PR title AND final merge commit title both start with a semantic tag (fix:, chore:, feat(feature-name):, refactor:). See CONTRIBUTING.md.
  • [n/a] (UI changes only) Added screenshots/GIFs to description above
  • [n/a] (UI changes only) Verified usability with NVDA/JAWS

@dbjorge dbjorge requested a review from a team as a code owner January 10, 2023 19:55
return axe.run({ include: [options.selector] } as ElementContext, {
runOnly: { type: 'tag', values: ['wcag2a', 'wcag21a', 'wcag2aa', 'wcag21aa'] },
rules: options.rules,
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a no-op change, required because upstream axe.run typings became more strict in this update

@dbjorge dbjorge changed the title feat(axe-core-4.6): update to axe-core 4.6.2, promote link-in-text-block and meta-viewport feat(axe-core-4.6): update to axe-core 4.6.3, promote link-in-text-block and meta-viewport Jan 24, 2023
Copy link
Contributor Author

@dbjorge dbjorge left a comment

Choose a reason for hiding this comment

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

@JGibson2019 and @shanisebarona 's changes since mine LGTM! (can't mark my own PR as approved)

@dbjorge dbjorge merged commit ed75fda into microsoft:main Jan 30, 2023
@dbjorge dbjorge deleted the axe-core-4.6.1 branch January 30, 2023 23:48
dbjorge added a commit that referenced this pull request Feb 1, 2023
…ard row (#6388)

#### Details

This PR implements a feature to add a new "Related paths" row to result
cards that represent axe-core results which included any check results
with `relatedNodes` properties.

It adds this information to:
* Cards in the fast pass automated checks and needs review results
* FastPass exported reports
* "axe results" style reports in the report package
* "combined" style reports in the report package (what we usually
consider as "service reports") - the service already sends enough
information for us to populate this

Screenshot of FastPass Automated Checks showing an
`aria-required-children` violation with Related paths:
![screenshot of FastPass card with "Related paths" card row added in
between "Snippet" and "How to fix" rows, showing a list of targets
formatted similarly to the old "Path"
row](https://user-images.githubusercontent.com/376284/215625581-3fe63469-bc8b-49ee-8a08-3bf1c4e67542.png)

Screenshot of FastPass Report Export showing the same info:
![screenshot of FastPass report export with "Related paths" card row
added in between "Snippet" and "How to fix" rows, showing a list of
targets formatted similarly to the old "Path"
row](https://user-images.githubusercontent.com/376284/215625717-b7632986-e2d4-4328-80fd-7e419dc54674.png)

Screenshot of FastPass Automated Checks' `color-contrast` results on a
page where `axe-core` only reports related paths for some results - note
that cards for results where the data is present show it, and results
where axe-core didn't report that data omit it.
![screenshot of two FastPass cards, one with Related paths and one
without](https://user-images.githubusercontent.com/376284/215625877-2225d666-c98f-4d5b-a62a-5652daa7cf33.png)

Screenshot of combined report with related path field:
![screenshot of combined report card with a related
path](https://user-images.githubusercontent.com/376284/215629181-7a6866f7-d124-44eb-afbe-a327217db50d.png)

##### Motivation

This is primarily to improve the actionability of the
`aria-required-children` "unknown" violation path which we added support
for during our 4.6.3 update in #6334. That path uses a how to fix
message of the form `"Element has children which are not allowed (see
related nodes)"`, which isn't very actionable without this PR since we
don't expose "related nodes" anywhere. This specific message might be
updated in a future version of axe-core (see
dequelabs/axe-core#3842)

This secondarily improves the actionability of a few other rules that
axe-core exposes related nodes for - particularly, `color-contrast`
violations where axe knows the node that is responsible for the
background color of a violation will now present that background node as
a "related path".

##### Context

In addition to this change, we'll want to make a docs change to
https://accessibilityinsights.io/info-examples/web/aria-required-children/
to make this specific form of `aria-required-children` violation easier
for a user to understand and take action on.

#### Pull request checklist
<!-- If a checklist item is not applicable to this change, write "n/a"
in the checkbox -->
- [x] Addresses an existing issue: 2020748
- [x] Ran `yarn fastpass`
- [x] Added/updated relevant unit test(s) (and ran `yarn test`)
- [x] Verified code coverage for the changes made. Check coverage report
at: `<rootDir>/test-results/unit/coverage`
- [x] PR title *AND* final merge commit title both start with a semantic
tag (`fix:`, `chore:`, `feat(feature-name):`, `refactor:`). See
`CONTRIBUTING.md`.
- [x] (UI changes only) Added screenshots/GIFs to description above
- [x] (UI changes only) Verified usability with NVDA/JAWS
dbjorge added a commit to microsoft/accessibility-insights-service that referenced this pull request Feb 2, 2023
…lock, meta-viewport, and "Related paths" in reports (#2383)

#### Details

This PR:

* Updates `axe-core` to v4.6.3
* Updates `accessibility-insights-report` to corresponding v4.6.3
* Removes `link-in-text-block` and `meta-viewport` from the rule
exclusion list, matching the new AI4Web behavior enabling them as
automated checks
* Enables a new report feature to include "Related paths" for failure
cards of axe results with "relatedNodes" info by propogating this axe
property in `combined-report-data-converter.ts`
* Bumps the `accessibility-insights-scan` minor version to `1.6.0` in
anticipation of a corresponding release for that package

Screenshot of combined report segment with new Related paths UX
generated using the following CLI command in this PR branch: `node
C:\repos\accessibility-insights-service\packages\cli\dist\ai-scan-cli.js
--crawl --url
https://developer.microsoft.com/en-us/fluentui#/controls/web/detailslist
--maxUrls 5`

![screenshot highlighting new "Related paths" field in a failure card in
a combined
report](https://user-images.githubusercontent.com/376284/216456916-648a9aaa-c18e-4a6a-b8cd-b1b741d1799a.png)


##### Motivation

Keep service rules version in sync with AI4Web, which has begun release
validation today for its corresponding release (2.37.0)

##### Context

* [web 4.6.3 PR
(#6334)](microsoft/accessibility-insights-web#6334)
* [web "Related paths" PR with screenshots of new report UI
(#6388)](microsoft/accessibility-insights-web#6388)

#### Pull request checklist
<!-- If a checklist item is not applicable to this change, write "n/a"
in the checkbox -->

- [x] Addresses an existing issue: 2017450
- [x] Added relevant unit test for your changes. (`yarn test`)
- [x] Verified code coverage for the changes made. Check coverage report
at: `<rootDir>/test-results/unit/coverage`
- [x] Ran precheckin (`yarn precheckin`)
- [ ] Validated in an Azure resource group
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.

None yet

3 participants