-
Notifications
You must be signed in to change notification settings - Fork 53
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: use axe run options from accessibility-insights-web (disables empty-table-header rule) #2402
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dbjorge
changed the title
Axe config from web
fix: use axe run options from accessibility-insights-web (disables empty-table-header rule)
Feb 22, 2023
This was referenced Feb 22, 2023
lamaks
approved these changes
Feb 22, 2023
dbjorge
added a commit
to microsoft/accessibility-insights-web
that referenced
this pull request
Feb 22, 2023
…6448) #### Details `axe-core` removed its `restoreScroll` option in mid-2020, when they updated the only rule that scrolled the page to no longer ever do that in [PR 2388](dequelabs/axe-core#2388). This PR removes the obsolete option from our axe-config. ##### Motivation * Code cleanup, axe-core doesn't care whether the option is passed or not. * Preparation for [using the axe-config output in service](microsoft/accessibility-insights-service#2402) ##### Context n/a #### Pull request checklist <!-- If a checklist item is not applicable to this change, write "n/a" in the checkbox --> - [n/a] Addresses an existing issue: #0000 - [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`. - [n/a] (UI changes only) Added screenshots/GIFs to description above - [n/a] (UI changes only) Verified usability with NVDA/JAWS
abassey2
pushed a commit
to msa2984/accessibility-insights-web
that referenced
this pull request
Feb 22, 2023
…icrosoft#6448) #### Details `axe-core` removed its `restoreScroll` option in mid-2020, when they updated the only rule that scrolled the page to no longer ever do that in [PR 2388](dequelabs/axe-core#2388). This PR removes the obsolete option from our axe-config. ##### Motivation * Code cleanup, axe-core doesn't care whether the option is passed or not. * Preparation for [using the axe-config output in service](microsoft/accessibility-insights-service#2402) ##### Context n/a #### Pull request checklist <!-- If a checklist item is not applicable to this change, write "n/a" in the checkbox --> - [n/a] Addresses an existing issue: #0000 - [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`. - [n/a] (UI changes only) Added screenshots/GIFs to description above - [n/a] (UI changes only) Verified usability with NVDA/JAWS
abassey2
pushed a commit
to msa2984/accessibility-insights-web
that referenced
this pull request
Feb 22, 2023
…icrosoft#6448) #### Details `axe-core` removed its `restoreScroll` option in mid-2020, when they updated the only rule that scrolled the page to no longer ever do that in [PR 2388](dequelabs/axe-core#2388). This PR removes the obsolete option from our axe-config. ##### Motivation * Code cleanup, axe-core doesn't care whether the option is passed or not. * Preparation for [using the axe-config output in service](microsoft/accessibility-insights-service#2402) ##### Context n/a #### Pull request checklist <!-- If a checklist item is not applicable to this change, write "n/a" in the checkbox --> - [n/a] Addresses an existing issue: #0000 - [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`. - [n/a] (UI changes only) Added screenshots/GIFs to description above - [n/a] (UI changes only) Verified usability with NVDA/JAWS
dbjorge
added a commit
to microsoft/accessibility-insights-web
that referenced
this pull request
Mar 1, 2023
#### Details This PR: * Updates the new `@accessibility-insights/axe-config` package to not include an entry for `frame-tested`. We include this rule in AI4Web's scans, but then post-process the results such that we don't present it to users as an automated checks failure (we instead present it as a special warning bar). This change means that a user relying on axe-config won't include `frame-tested`, which will avoid the appearance of issues reported by `axe-config` but not by automated checks. ##### Motivation * Preparation for [using the axe-config output in service](microsoft/accessibility-insights-service#2402) ##### Context n/a #### Pull request checklist <!-- If a checklist item is not applicable to this change, write "n/a" in the checkbox --> - [n/a] Addresses an existing issue: #0000 - [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`. - [n/a] (UI changes only) Added screenshots/GIFs to description above - [n/a] (UI changes only) Verified usability with NVDA/JAWS
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Details
Today, Service decides which axe-core rules to use via a
RuleExclusion
mechanism that starts from axe-core's default ruleset an disables rules we don't use in web.This is inconsistent with how accessibility-insights-web defines its ruleset (via a combination of axe-core's rule tags and explicit overrides). The difference in mechanisms led to a mistake during the axe-core 4.6.3 update where the rulesets became inconsistent between web and service.
To ensure consistency between web and service, this PR replaces
rule-exclusion.ts
with a new mechanism based on the recently-added@accessibility-insights/axe-config
package in the web repo. That package builds an axe config blob based on web'sget-rule-inclusions.ts
mechanism. This update bundles a copy of the current output of that package intoaxe-configuration.ts
to form the basis of service's axe configuration, and removesrule-exclusion.ts
entirely as obsolete.Besides keeping the rules in sync, this also means adds a new axe-core config option that was previously overridden by web but not service (
"pingWaitTime": 1000
instead of the default 500ms). I decided to keep the new value from the web config for consistency with web and because both values are still very comfortably within the margin of error set by the page level timeouts used inpage-timeout-config.ts
, even assuming pages with multiple levels of iframe nesting.Motivation
empty-table-header
best-practice ruleContext
Testing
Besides the PR checklist, I validated that a local build of the scan package against test site https://www.catalog.update.microsoft.com/Home.aspx produced a report consistent with [email protected]'s results (no violations), where previously the service reported a slightly distinct set of errors (one violation of the
empty-table-header
rule, which was accidentally omitted fromrule-exclusions.ts
during the 4.6.3 update).Pull request checklist
yarn test
)<rootDir>/test-results/unit/coverage
yarn precheckin
)