fix: auto-pass main landmark requirements for no-landmark pages #2644
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
|
Per discussion from Karan+Fer+Peter: we think as a followup from this work, we'd want to look into similar autopass behavior for |
|
I think this new behavior would benefit from an e2e test validating it; I have it mostly implemented already, but again, leaving it for a separate PR to try to keep the scope of this one more limited, it's already quite large |
checked and right now, those two are the only two requirements that will benefit from this at the moment. |
karanbirsingh
suggested changes
May 14, 2020
karanbirsingh
approved these changes
May 14, 2020
This was referenced May 14, 2020
dbjorge
added a commit
that referenced
this pull request
May 15, 2020
#### Description of changes Adds new end to end tests that verify the auto-pass-on-no-landmarks behavior introduced in #2644. Updates the product code to add a new data-automation-id, no UI impact. #### Pull request checklist <!-- If a checklist item is not applicable to this change, write "n/a" in the checkbox --> - [x] Addresses an existing issue: e2e test for behavior in #2521 - [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
that referenced
this pull request
May 15, 2020
…ning (#2687) #### Description of changes Per discussion in #2644, the new manual auto-pass behavior we added for some of the Landmarks requirements would also be nice to have for the Semantics > CSS content and Sequence > CSS positioning requirements when their visualization analyzers find no relevant elements. This PR generalizes the "auto-pass if no landmarks" behavior to a "auto-pass if no results relevant to the requirement" behavior and adds it in to the 2 CSS requirements. To do this, it updates the assessment store to only pass instance data with results relevant to the current requirement when it calls a requirement's getInitialManualTestStatus function. I considered an alternative approach of leaving assessment store as-is and creating a factory function for each requirement to specify its own `autoPassIfNoResultsFor(key)` rather than a single `autoPassIfNoResults`; I liked that because it avoided adding more complexity to `assessment-store`, which is already a very complex class, but I felt that it was better for data encapsulation if requirements were systematically prevented from seeing/depending on other requirements' data, and I thought the encapsulation was more valuable.  #### 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`. - [x] (UI changes only) Added screenshots/GIFs to description above - [x] (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.
Description of changes
#2521 tracks an issue where it wasn't clear what the expectations on a user were for testing the Landmarks "primary content" and "no repeating content" requirements on pages with no landmarks.
The proposed fix for that was a text update + making the requirements pass automatically on pages that have no landmarks.
This fix involves a significant bit of new behavior. We wanted to keep the 2 requirements in question as manual requirements, not assisted requirements, since it's possible for a page to have no main landmarks and for a user to need to manually enter a failure not associated with any instance we might show in an instance list. However, this is the first time we're assigning auto-pass behavior to a manual test; in all other cases where we support auto-pass, it works by looking for an assisted requirement with no matching instances.
This new behavior is especially challenging because the set of instances required for auto-passing ("are there any landmarks on the page?") is different from the set of instances required for visualization ("what are all the main landmarks on the page?").
To support this, I've added 2 new features to
AssessmentStore'sRequirementprocessing:isVisualizationSupportedForResultproperty whichRequirements can use to indicate that certain results returned by their analyzers should not support being visualized (by default, all results support visualization)getInitialManualTestStatusproperty which manualRequirements can use to infer an initial pass/fail state after scanning for results (by default, a manual requirement is put in the UNKNOWN state regardless of analysis results)These are used by the updated requirements by updating their analyzers to use the
unique-landmarkrule to look for all landmarks instead of the old behavior of looking only for main landmarks, useisVisualizationSupportedForResultto only visualize the main landmarks (like before), and usegetInitialManualTestStatusto set a "PASS" status if the scan completes and there are no instances with the landmark role data from theunique-landmarkrule.New behavior:
no landmarks: "Primary content" and "No repeating content" automatically pass
only non-main landmarks: "Primary content" and "No repeating content" don't auto-pass and show no matching instances to visualize
Mix of main and non-main landmarks: "Primary content" and "No repeating content" don't auto-pass and visualize only the main landmark
Pull request checklist
yarn fastpassyarn test)<rootDir>/test-results/unit/coveragefix:,chore:,feat(feature-name):,refactor:). SeeCONTRIBUTING.md.