[Feature Request] Axe-core rules should be consistent between service and web #571
Comments
ghost
added
the
|
This issue has been marked as ready for team triage; we will triage it in our weekly review and update the issue. Thank you for contributing to Accessibility Insights! |
adarshrema
added
status: needs investigation
and removed
status: ready for triage
labels
|
This issue requires additional investigation by the Accessibility Insights team. When the issue is ready to be triaged again, we will update the issue with the investigation result and add "status: ready for triage". Thank you for contributing to Accessibility Insights! |
lamaks
changed the title
|
The service had combined axe core rules into one places. Therefore any update to axe core/rules will require single place to change either axe core version or rules definition file. This is similar to if we had axe core extended package that should be updated and its version should be updated in service or other consumers. The feature could be postponed. |
…pty-table-header rule) (#2402) #### 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](https://github.com/microsoft/accessibility-insights-web/blob/[email protected]/src/scanner/get-rule-inclusions.ts)). 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's `get-rule-inclusions.ts` mechanism. This update bundles a copy of the current output of that package into `axe-configuration.ts` to form the basis of service's axe configuration, and removes `rule-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 in `page-timeout-config.ts`, even assuming pages with multiple levels of iframe nesting. ##### Motivation * Make it more reliable to keep web and service rules in sync. * Disables the unintentionally-enabled `empty-table-header` best-practice rule ##### Context * Builds on @sfoslund 's work in microsoft/accessibility-insights-web#6395 * Partially implements (but doesn't complete) #571 * I considered doing a fuller implementation that would involve creating a publishing pipeline for the axe-config package, but I didn't want to tie up the fix for the rule inconsistency in service vs web with that process. This is still an incremental improvement that moves us closer to that end state without the NPM package layer. * Incorporates changes from microsoft/accessibility-insights-web#6448 and microsoft/accessibility-insights-web#6449 ##### 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 from `rule-exclusions.ts` during the 4.6.3 update). #### Pull request checklist <!-- If a checklist item is not applicable to this change, write "n/a" in the checkbox --> - [x] Addresses an existing issue: Partially addresses #571 - [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
|
Update: The service is not yet using the config generator, and when I asked if there are any plans to use it, the answer was no. The service code is at https://github.com/microsoft/accessibility-insights-service/blob/main/packages/scanner-global-library/src/axe-scanner/axe-run-options.ts. |
Describe the bug
In the web repo, we use the axe-core rules that are enabled by default (non-experimental) and then disable anything tagged with best-practice. In the service, we keep a list of rules to disable, which may change when axe-core updates. This makes it difficult to consistently run the same rules in web and service.
We should make sure that web and service use a consistent set of rules. One solution is to create an npm package that returns a list of rules, and have both web and service consume that package.
The text was updated successfully, but these errors were encountered: