fix: auto-pass main landmark requirements for no-landmark pages (#2644)

#### 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`'s `Requirement` processing: * A new `isVisualizationSupportedForResult` property which `Requirement`s can use to indicate that certain results returned by their analyzers should not support being visualized (by default, all results support visualization) * A new `getInitialManualTestStatus` property which manual `Requirement`s 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-landmark` rule to look for *all* landmarks instead of the old behavior of looking only for main landmarks, use `isVisualizationSupportedForResult` to only visualize the main landmarks (like before), and use `getInitialManualTestStatus` to set a "PASS" status if the scan completes and there are no instances with the landmark role data from the `unique-landmark` rule. New behavior: **no landmarks: "Primary content" and "No repeating content" automatically pass** ![screen recording of no-landmarks interactions](https://user-images.githubusercontent.com/376284/81753659-1ef3ea00-9469-11ea-8646-e394ffff61de.gif) **only non-main landmarks: "Primary content" and "No repeating content" don't auto-pass and show no matching instances to visualize** ![screen recording of banner-landmark-only interactions](https://user-images.githubusercontent.com/376284/81753648-1ac7cc80-9469-11ea-9c90-05f8d7243088.gif) **Mix of main and non-main landmarks: "Primary content" and "No repeating content" don't auto-pass and visualize only the main landmark** ![screen recording of mixed-landmarks interactions](https://user-images.githubusercontent.com/376284/81753619-0a175680-9469-11ea-9d6e-40131889cf46.gif) #### Pull request checklist  - [x] Addresses an existing issue: #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`. - [x] (UI changes only) Added screenshots/GIFs to description above - [x] (UI changes only) Verified usability with NVDA/JAWS
microsoft · May 14, 2020 · 010f34e · 010f34e
1 parent 7d59286
commit 010f34e
Show file tree

Hide file tree

Showing 20 changed files with 1,049 additions and 530 deletions.
diff --git a/src/DetailsView/components/assessment-visualization-enabled-toggle.tsx b/src/DetailsView/components/assessment-visualization-enabled-toggle.tsx
@@ -1,13 +1,12 @@
 // Copyright (c) Microsoft Corporation. All rights reserved.
 // Licensed under the MIT License.
 import { GeneratedAssessmentInstance } from 'common/types/store-data/assessment-result-data';
-import { isEmpty } from 'lodash';
 
 import { BaseVisualHelperToggle } from './base-visual-helper-toggle';
 
 export class AssessmentVisualizationEnabledToggle extends BaseVisualHelperToggle {
-    protected isDisabled(filteredInstances: GeneratedAssessmentInstance<{}, {}>[]): boolean {
-        return isEmpty(filteredInstances);
+    protected isDisabled(instances: GeneratedAssessmentInstance<{}, {}>[]): boolean {
+        return !this.isAnyInstanceVisualizable(instances);
     }
 
     protected isChecked(instances: GeneratedAssessmentInstance<{}, {}>[]): boolean {
@@ -28,10 +27,16 @@ export class AssessmentVisualizationEnabledToggle extends BaseVisualHelperToggle
     };
 
     private isAnyInstanceVisible(instances: GeneratedAssessmentInstance<{}, {}>[]): boolean {
+        const testStep = this.props.assessmentNavState.selectedTestSubview;
         return instances.some(
-            instance =>
-                instance.testStepResults[this.props.assessmentNavState.selectedTestSubview]
-                    .isVisualizationEnabled,
+            instance => instance.testStepResults[testStep].isVisualizationEnabled,
+        );
+    }
+
+    private isAnyInstanceVisualizable(instances: GeneratedAssessmentInstance<{}, {}>[]): boolean {
+        const testStep = this.props.assessmentNavState.selectedTestSubview;
+        return instances.some(
+            instance => instance.testStepResults[testStep].isVisualizationSupported,
         );
     }
 }
diff --git a/src/assessments/assessment-builder.tsx b/src/assessments/assessment-builder.tsx
@@ -10,6 +10,7 @@ import { RequirementComparer } from 'common/assessment/requirement-comparer';
 import { AssessmentVisualizationConfiguration } from 'common/configs/assessment-visualization-configuration';
 import { Messages } from 'common/messages';
 import { ManualTestStatus } from 'common/types/manual-test-status';
+import { InstanceIdToInstanceDataMap } from 'common/types/store-data/assessment-result-data';
 import { FeatureFlagStoreData } from 'common/types/store-data/feature-flag-store-data';
 import { AssessmentScanData, ScanData } from 'common/types/store-data/visualization-store-data';
 import {
@@ -52,6 +53,15 @@ export class AssessmentBuilder {
             requirement.getInstanceStatus = AssessmentBuilder.getInstanceStatus;
         }
 
+        if (!requirement.getInitialManualTestStatus) {
+            requirement.getInitialManualTestStatus = AssessmentBuilder.getInitialManualTestStatus;
+        }
+
+        if (!requirement.isVisualizationSupportedForResult) {
+            requirement.isVisualizationSupportedForResult =
+                AssessmentBuilder.isVisualizationSupportedForResult;
+        }
+
         if (!requirement.getInstanceStatusColumns) {
             requirement.getInstanceStatusColumns = AssessmentBuilder.getInstanceStatusColumns;
         }
@@ -75,6 +85,16 @@ export class AssessmentBuilder {
         return ManualTestStatus.UNKNOWN;
     }
 
+    private static getInitialManualTestStatus(
+        instances: InstanceIdToInstanceDataMap,
+    ): ManualTestStatus {
+        return ManualTestStatus.UNKNOWN;
+    }
+
+    private static isVisualizationSupportedForResult(result: DecoratedAxeNodeResult): boolean {
+        return true;
+    }
+
     private static getInstanceStatusColumns(): Readonly<IColumn>[] {
         return [
             {

diff --git a/src/assessments/landmarks/auto-pass-if-no-landmarks.ts b/src/assessments/landmarks/auto-pass-if-no-landmarks.ts
@@ -0,0 +1,14 @@
+// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT License.
+import { ManualTestStatus } from 'common/types/manual-test-status';
+import { InstanceIdToInstanceDataMap } from 'common/types/store-data/assessment-result-data';
+import { some } from 'lodash';
+
+export function autoPassIfNoLandmarks(instanceData: InstanceIdToInstanceDataMap): ManualTestStatus {
+    const someInstanceHasLandmarkRole = some(
+        Object.values(instanceData),
+        instance => instance.propertyBag != null && instance.propertyBag['role'] != null,
+    );
+
+    return someInstanceHasLandmarkRole ? ManualTestStatus.UNKNOWN : ManualTestStatus.PASS;
+}
diff --git a/src/assessments/landmarks/does-result-have-main-role.ts b/src/assessments/landmarks/does-result-have-main-role.ts
@@ -0,0 +1,13 @@
+// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT License.
+import { DecoratedAxeNodeResult } from 'injected/scanner-utils';
+import { some } from 'lodash';
+
+export function doesResultHaveMainRole(result: DecoratedAxeNodeResult): boolean {
+    // This 'role' data is populated by the unique-landmark rule, which considers
+    // both explicit role attributes and implicit roles based on tag name
+    return (
+        some(result.any, checkResult => checkResult.data['role'] === 'main') ||
+        some(result.all, checkResult => checkResult.data['role'] === 'main')
+    );
+}
diff --git a/src/assessments/landmarks/test-steps/no-repeating-content.tsx b/src/assessments/landmarks/test-steps/no-repeating-content.tsx
@@ -2,6 +2,7 @@
 // Licensed under the MIT License.
 import * as React from 'react';
 
+import { doesResultHaveMainRole } from 'assessments/landmarks/does-result-have-main-role';
 import { VisualizationType } from 'common/types/visualization-type';
 import { link } from 'content/link';
 import * as content from 'content/test/landmarks/no-repeating-content';
@@ -10,10 +11,14 @@ import { AnalyzerConfigurationFactory } from '../../common/analyzer-configuratio
 import { ManualTestRecordYourResults } from '../../common/manual-test-record-your-results';
 import * as Markup from '../../markup';
 import { Requirement } from '../../types/requirement';
+import { autoPassIfNoLandmarks } from '../auto-pass-if-no-landmarks';
 import { LandmarkTestStep } from './test-steps';
 
 const description: JSX.Element = (
-    <span>The main landmark must not contain any blocks of content that repeat across pages.</span>
+    <span>
+        The <Markup.CodeTerm>main</Markup.CodeTerm> landmark must not contain any blocks of content
+        that repeat across pages.
+    </span>
 );
 
 const howToTest: JSX.Element = (
@@ -22,10 +27,33 @@ const howToTest: JSX.Element = (
             The visual helper for this requirement highlights the page's{' '}
             <Markup.CodeTerm>main</Markup.CodeTerm> landmark.
         </p>
+        <p>
+            <Markup.Emphasis>
+                Note: If no landmarks are found, this requirement will automatically be marked as
+                pass.
+            </Markup.Emphasis>
+        </p>
         <ol>
             <li>
-                In the target page, examine the main landmark to verify that it does not contain any
-                blocks of content that repeat across pages (e.g., site-wide navigation links).
+                <p>Examine the target page to verify that all of the following are true:</p>
+                <ol>
+                    <li>
+                        The page has exactly one <Markup.CodeTerm>main</Markup.CodeTerm> landmark,
+                        and
+                    </li>
+                    <li>
+                        The <Markup.CodeTerm>main</Markup.CodeTerm> landmark does not contain any
+                        blocks of content that repeat across pages (such as site-wide navigation
+                        links).
+                    </li>
+                </ol>
+                <p>
+                    Exception: If a page has nested <Markup.CodeTerm>document</Markup.CodeTerm> or{' '}
+                    <Markup.CodeTerm>application</Markup.CodeTerm> roles (typically applied to{' '}
+                    <Markup.Tag tagName="iframe" /> or <Markup.Tag tagName="frame" /> elements),
+                    each nested document or application may <Markup.Emphasis>also</Markup.Emphasis>{' '}
+                    have one <Markup.CodeTerm>main</Markup.CodeTerm> landmark.
+                </p>
             </li>
             <ManualTestRecordYourResults isMultipleFailurePossible={true} />
         </ol>
@@ -38,11 +66,13 @@ export const NoRepeatingContent: Requirement = {
     description,
     howToTest,
     isManual: true,
+    getInitialManualTestStatus: autoPassIfNoLandmarks,
+    isVisualizationSupportedForResult: doesResultHaveMainRole,
     guidanceLinks: [link.WCAG_1_3_1, link.WCAG_2_4_1],
     getAnalyzer: provider =>
         provider.createRuleAnalyzer(
             AnalyzerConfigurationFactory.forScanner({
-                rules: ['main-landmark'],
+                rules: ['unique-landmark'],
                 key: LandmarkTestStep.noRepeatingContent,
                 testType: VisualizationType.LandmarksAssessment,
             }),

diff --git a/src/assessments/landmarks/test-steps/primary-content.tsx b/src/assessments/landmarks/test-steps/primary-content.tsx
@@ -2,6 +2,7 @@
 // Licensed under the MIT License.
 import * as React from 'react';
 
+import { doesResultHaveMainRole } from 'assessments/landmarks/does-result-have-main-role';
 import { VisualizationType } from 'common/types/visualization-type';
 import { link } from 'content/link';
 import * as content from 'content/test/landmarks/primary-content';
@@ -10,19 +11,48 @@ import { AnalyzerConfigurationFactory } from '../../common/analyzer-configuratio
 import { ManualTestRecordYourResults } from '../../common/manual-test-record-your-results';
 import * as Markup from '../../markup';
 import { Requirement } from '../../types/requirement';
+import { autoPassIfNoLandmarks } from '../auto-pass-if-no-landmarks';
 import { LandmarkTestStep } from './test-steps';
 
 const description: JSX.Element = (
-    <span>The main landmark must contain all of the page's primary content.</span>
+    <span>
+        The <Markup.CodeTerm>main</Markup.CodeTerm> landmark must contain all of the page's primary
+        content.
+    </span>
 );
 
 const howToTest: JSX.Element = (
     <div>
-        <p>The visual helper for this requirement highlights the page's main landmark.</p>
+        <p>
+            The visual helper for this requirement highlights the page's{' '}
+            <Markup.CodeTerm>main</Markup.CodeTerm> landmark.
+        </p>
+        <p>
+            <Markup.Emphasis>
+                Note: If no landmarks are found, this requirement will automatically be marked as
+                pass.
+            </Markup.Emphasis>
+        </p>
         <ol>
             <li>
-                In the target page, examine the <Markup.CodeTerm>main</Markup.CodeTerm> landmark to
-                verify that it contains all of the page's primary content.
+                <p>Examine the target page to verify that all of the following are true:</p>
+                <ol>
+                    <li>
+                        The page has exactly one <Markup.CodeTerm>main</Markup.CodeTerm> landmark,
+                        and
+                    </li>
+                    <li>
+                        The <Markup.CodeTerm>main</Markup.CodeTerm> landmark contains all of the
+                        page's primary content.
+                    </li>
+                </ol>
+                <p>
+                    Exception: If a page has nested <Markup.CodeTerm>document</Markup.CodeTerm> or{' '}
+                    <Markup.CodeTerm>application</Markup.CodeTerm> roles (typically applied to{' '}
+                    <Markup.Tag tagName="iframe" /> or <Markup.Tag tagName="frame" /> elements),
+                    each nested document or application may <Markup.Emphasis>also</Markup.Emphasis>{' '}
+                    have one <Markup.CodeTerm>main</Markup.CodeTerm> landmark.
+                </p>
             </li>
             <ManualTestRecordYourResults isMultipleFailurePossible={true} />
         </ol>
@@ -35,11 +65,13 @@ export const PrimaryContent: Requirement = {
     description,
     howToTest,
     isManual: true,
+    getInitialManualTestStatus: autoPassIfNoLandmarks,
+    isVisualizationSupportedForResult: doesResultHaveMainRole,
     guidanceLinks: [link.WCAG_1_3_1, link.WCAG_2_4_1],
     getAnalyzer: provider =>
         provider.createRuleAnalyzer(
             AnalyzerConfigurationFactory.forScanner({
-                rules: ['main-landmark'],
+                rules: ['unique-landmark'],
                 key: LandmarkTestStep.primaryContent,
                 testType: VisualizationType.LandmarksAssessment,
             }),

diff --git a/src/assessments/types/requirement.ts b/src/assessments/types/requirement.ts
@@ -5,6 +5,7 @@ import { ManualTestStatus } from 'common/types/manual-test-status';
 import {
     AssessmentNavState,
     GeneratedAssessmentInstance,
+    InstanceIdToInstanceDataMap,
 } from 'common/types/store-data/assessment-result-data';
 import { FeatureFlagStoreData } from 'common/types/store-data/feature-flag-store-data';
 import { DetailsViewActionMessageCreator } from 'DetailsView/actions/details-view-action-message-creator';
@@ -39,10 +40,17 @@ export interface Requirement {
     addFailureInstruction?: string;
     infoAndExamples?: ContentPageComponent;
     isManual: boolean;
+    // This is for semi-manual cases where we can't present a list of instances like an assisted
+    // test would, but can infer a PASS or FAIL state. If not specified, acts like () => UNKNOWN.
+    getInitialManualTestStatus?: (instances: InstanceIdToInstanceDataMap) => ManualTestStatus;
     guidanceLinks: HyperlinkDefinition[];
     columnsConfig?: InstanceTableColumn[];
     getAnalyzer?: (provider: AnalyzerProvider) => Analyzer;
     getVisualHelperToggle?: (props: VisualHelperToggleConfig) => JSX.Element;
+    // Any results this returns false for will be omitted from visual helper displays, but still
+    // present for the purposes of instance lists or getInitialManualTestStatus. By default, all
+    // results support visualization.
+    isVisualizationSupportedForResult?: (result: DecoratedAxeNodeResult) => boolean;
     visualizationInstanceProcessor?: VisualizationInstanceProcessorCallback<
         PropertyBags,
         PropertyBags

diff --git a/src/background/assessment-data-converter.ts b/src/background/assessment-data-converter.ts
@@ -29,6 +29,7 @@ export class AssessmentDataConverter {
         stepName: string,
         generateInstanceIdentifier: (instance: UniquelyIdentifiableInstances) => string,
         getInstanceStatus: (result: DecoratedAxeNodeResult) => ManualTestStatus,
+        isVisualizationSupported: (result: DecoratedAxeNodeResult) => boolean,
     ): AssessmentInstancesMap {
         let instancesMap: AssessmentInstancesMap = {};
 
@@ -51,6 +52,7 @@ export class AssessmentDataConverter {
                     stepName,
                     ruleResult,
                     getInstanceStatus,
+                    isVisualizationSupported,
                 );
             }
         });
@@ -98,6 +100,7 @@ export class AssessmentDataConverter {
         testStep: string,
         ruleResult: DecoratedAxeNodeResult,
         getInstanceStatus: (result: DecoratedAxeNodeResult) => ManualTestStatus,
+        isVisualizationSupported: (result: DecoratedAxeNodeResult) => boolean,
     ): GeneratedAssessmentInstance {
         const target: string[] = elementAxeResult.target;
         let testStepResults = {};
@@ -114,6 +117,7 @@ export class AssessmentDataConverter {
             ruleResult,
             elementAxeResult,
             getInstanceStatus,
+            isVisualizationSupported,
         );
 
         let actualPropertyBag = {
@@ -163,6 +167,7 @@ export class AssessmentDataConverter {
             status: ManualTestStatus.UNKNOWN,
             isCapturedByUser: false,
             failureSummary: null,
+            isVisualizationSupported: true,
             isVisualizationEnabled: true,
             isVisible: true,
         };
@@ -172,12 +177,14 @@ export class AssessmentDataConverter {
         ruleResult: DecoratedAxeNodeResult,
         elementAxeResult: HtmlElementAxeResults,
         getInstanceStatus: (result: DecoratedAxeNodeResult) => ManualTestStatus,
+        isVisualizationSupported: (result: DecoratedAxeNodeResult) => boolean,
     ): TestStepResult {
         return {
             id: ruleResult.id,
             status: getInstanceStatus(ruleResult),
             isCapturedByUser: false,
             failureSummary: ruleResult.failureSummary,
+            isVisualizationSupported: isVisualizationSupported(ruleResult),
             isVisualizationEnabled: false,
             isVisible: true,
         };