fix: add banner for unresponsive iframes (#6330)

#### Details This PR adds logic to detect when iframes have been skipped in an axe scan via that frames-tested rule. When iframes have been skipped, it shows a yellow banner to notify the user. ##### Motivation Addresses issue [#6128](#6128) ##### Context Screenshot of banner with fastpass (shows up similarly in assessment UI): [Screenshot description: Yellow banner across top of details view with text "there are iframes in the target page that are non-responsive. These frames have been skipped and are not included in the result.] <img width="636" alt="iframes-skipped-screenshot" src="https://user-images.githubusercontent.com/16010855/211618204-cfc09d02-4db5-44f6-a4b9-ed235ef863fb.png"> #### Pull request checklist  - [x] Addresses an existing issue: [#6128](#6128) - [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 · Jan 17, 2023 · 2186a7e · 2186a7e
1 parent d93c537
commit 2186a7e
Show file tree

Hide file tree

Showing 24 changed files with 161 additions and 20 deletions.
diff --git a/src/DetailsView/components/iframe-skipped-warning.tsx b/src/DetailsView/components/iframe-skipped-warning.tsx
@@ -0,0 +1,18 @@
+// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT License.
+import { NewTabLink } from 'common/components/new-tab-link';
+import { NamedFC } from 'common/react/named-fc';
+import * as React from 'react';
+
+export const IframeSkippedWarningContainerAutomationId = 'iframe-skipped-warning-container';
+
+export const IframeSkippedWarning = NamedFC('IframeSkippedWarning', () => (
+    <div data-automation-id={IframeSkippedWarningContainerAutomationId}>
+        There are iframes in the target page that are non-responsive. These frames have been skipped
+        and are not included in results. If you get this warning unexpectedly in a page where you
+        expect the iframes to be responsive, please
+        <NewTabLink href={'https://github.com/microsoft/accessibility-insights-web/issues/6347'}>
+            let us know.
+        </NewTabLink>
+    </div>
+));
diff --git a/src/DetailsView/components/warning-configuration.tsx b/src/DetailsView/components/warning-configuration.tsx
@@ -2,6 +2,7 @@
 // Licensed under the MIT License.
 import { ReactFCWithDisplayName } from 'common/react/named-fc';
 import { ScanIncompleteWarningId } from 'common/types/store-data/scan-incomplete-warnings';
+import { IframeSkippedWarning } from 'DetailsView/components/iframe-skipped-warning';
 import {
     AssessmentIframeWarning,
     AssessmentIframeWarningDeps,
@@ -24,8 +25,10 @@ export type WarningConfiguration = {
 
 export const assessmentWarningConfiguration: WarningConfiguration = {
     'missing-required-cross-origin-permissions': AssessmentIframeWarning,
+    'frame-skipped': IframeSkippedWarning,
 };
 
 export const fastpassWarningConfiguration: WarningConfiguration = {
     'missing-required-cross-origin-permissions': FastPassIframeWarning,
+    'frame-skipped': IframeSkippedWarning,
 };
diff --git a/src/common/types/store-data/scan-incomplete-warnings.ts b/src/common/types/store-data/scan-incomplete-warnings.ts
@@ -2,4 +2,4 @@
 // Licensed under the MIT License.
 
 // This is a string-enum, but we only have one type right now
-export type ScanIncompleteWarningId = 'missing-required-cross-origin-permissions';
+export type ScanIncompleteWarningId = 'missing-required-cross-origin-permissions' | 'frame-skipped';
diff --git a/src/injected/analyzers/base-analyzer.ts b/src/injected/analyzers/base-analyzer.ts
@@ -49,7 +49,7 @@ export class BaseAnalyzer implements Analyzer {
             scanResult: originalAxeResult,
             testType: config.testType,
             scanIncompleteWarnings:
-                this.scanIncompleteWarningDetector.detectScanIncompleteWarnings(),
+                this.scanIncompleteWarningDetector.detectScanIncompleteWarnings(originalAxeResult),
         };
 
         return {

diff --git a/src/injected/analyzers/notification-text-creator.ts b/src/injected/analyzers/notification-text-creator.ts
@@ -16,7 +16,7 @@ export class NotificationTextCreator {
     };
 
     public needsReviewText: TextGenerator = (results: UnifiedResult[]) => {
-        const warnings = this.scanIncompleteWarningDetector.detectScanIncompleteWarnings();
+        const warnings = this.scanIncompleteWarningDetector.detectScanIncompleteWarnings(null);
         if (isEmpty(results) && isEmpty(warnings)) {
             return 'Congratulations!\n\nNeeds review found no instances to review on this page.';
         }

diff --git a/src/injected/analyzers/unified-result-sender.ts b/src/injected/analyzers/unified-result-sender.ts
@@ -64,7 +64,7 @@ export class UnifiedResultSender {
         notificationMessage: TextGenerator,
     ): UnifiedScanCompletedPayload => {
         const scanIncompleteWarnings =
-            this.scanIncompleteWarningDetector.detectScanIncompleteWarnings();
+            this.scanIncompleteWarningDetector.detectScanIncompleteWarnings(results);
 
         let telemetry: ScanIncompleteWarningsTelemetryData = null;
 

diff --git a/src/injected/scan-incomplete-warning-detector.ts b/src/injected/scan-incomplete-warning-detector.ts
@@ -3,6 +3,7 @@
 import { BaseStore } from 'common/base-store';
 import { PermissionsStateStoreData } from 'common/types/store-data/permissions-state-store-data';
 import { ScanIncompleteWarningId } from 'common/types/store-data/scan-incomplete-warnings';
+import { ScanResults } from 'scanner/iruleresults';
 import { IframeDetector } from './iframe-detector';
 
 export class ScanIncompleteWarningDetector {
@@ -11,14 +12,18 @@ export class ScanIncompleteWarningDetector {
         private permissionsStateStore: BaseStore<PermissionsStateStoreData, Promise<void>>,
     ) {}
 
-    public detectScanIncompleteWarnings = () => {
+    public detectScanIncompleteWarnings = (results: ScanResults | null) => {
         const warnings: ScanIncompleteWarningId[] = [];
+
         if (
             this.iframeDetector.hasIframes() &&
             !this.permissionsStateStore.getState().hasAllUrlAndFilePermissions
         ) {
             warnings.push('missing-required-cross-origin-permissions');
+        } else if (results && results.framesSkipped) {
+            warnings.push('frame-skipped');
         }
+
         return warnings;
     };
 }
diff --git a/src/scanner/get-rule-inclusions.ts b/src/scanner/get-rule-inclusions.ts
@@ -4,7 +4,10 @@ import { IRuleConfiguration } from 'scanner/iruleresults';
 import { DictionaryStringTo } from 'types/common-types';
 
 export type RuleIncluded =
+    // Rules included in automated checks by default
     | { status: 'included'; reason?: string }
+    // Rules included when running any scan
+    | { status: 'included-always'; reason: string }
     | { status: 'excluded'; reason: string };
 
 export const explicitRuleOverrides: DictionaryStringTo<RuleIncluded> = {
@@ -36,6 +39,10 @@ export const explicitRuleOverrides: DictionaryStringTo<RuleIncluded> = {
         status: 'included',
         reason: 'best practice rule that was investigated with no known false positives, implemented as an automated check.',
     },
+    'frame-tested': {
+        status: 'included-always',
+        reason: 'Tests for unresponsive frames, enables iframe skipped warning',
+    },
 };
 
 // all the rules we enable in needs review

diff --git a/src/scanner/iruleresults.d.ts b/src/scanner/iruleresults.d.ts
@@ -91,6 +91,7 @@ export interface ScanResults {
     timestamp: string;
     targetPageUrl: string;
     targetPageTitle: string;
+    framesSkipped?: boolean;
 }
 
 export interface RuleDecorations {

diff --git a/src/scanner/result-decorator.ts b/src/scanner/result-decorator.ts
@@ -29,6 +29,7 @@ export class ResultDecorator {
             timestamp: results.timestamp,
             targetPageUrl: results.url,
             targetPageTitle: this.documentUtils.title(),
+            framesSkipped: results.incomplete.some(result => result.id === 'frame-tested'),
         };
 
         return scanResults;

diff --git a/src/scanner/scan-parameter-generator.ts b/src/scanner/scan-parameter-generator.ts
@@ -21,10 +21,16 @@ export class ScanParameterGenerator {
 
         if (options == null || options.testsToRun == null) {
             result.runOnly!.values = Object.keys(this.rulesIncluded).filter(
-                id => this.rulesIncluded[id].status === 'included',
+                id =>
+                    this.rulesIncluded[id].status === 'included' ||
+                    this.rulesIncluded[id].status === 'included-always',
             );
         } else {
-            result.runOnly!.values = options.testsToRun;
+            result.runOnly!.values = options.testsToRun.concat(
+                Object.keys(this.rulesIncluded).filter(
+                    id => this.rulesIncluded[id].status === 'included-always',
+                ),
+            );
         }
 
         return result;

diff --git a/...ests/unit/tests/DetailsView/components/__snapshots__/iframe-skipped-warning.test.tsx.snap b/...ests/unit/tests/DetailsView/components/__snapshots__/iframe-skipped-warning.test.tsx.snap
@@ -0,0 +1,14 @@
+// Jest Snapshot v1, https://goo.gl/fbAQLP
+
+exports[`IframeSkippedWarning render 1`] = `
+<div
+  data-automation-id="iframe-skipped-warning-container"
+>
+  There are iframes in the target page that are non-responsive. These frames have been skipped and are not included in results. If you get this warning unexpectedly in a page where you expect the iframes to be responsive, please
+  <NewTabLink
+    href="https://github.com/microsoft/accessibility-insights-web/issues/6347"
+  >
+    let us know.
+  </NewTabLink>
+</div>
+`;
diff --git a/src/tests/unit/tests/DetailsView/components/iframe-skipped-warning.test.tsx b/src/tests/unit/tests/DetailsView/components/iframe-skipped-warning.test.tsx
@@ -0,0 +1,12 @@
+// Copyright (c) Microsoft Corporation. All rights reserved.
+// Licensed under the MIT License.
+import { IframeSkippedWarning } from 'DetailsView/components/iframe-skipped-warning';
+import { shallow } from 'enzyme';
+import * as React from 'react';
+
+describe('IframeSkippedWarning', () => {
+    test('render', () => {
+        const wrapper = shallow(<IframeSkippedWarning />);
+        expect(wrapper.getElement()).toMatchSnapshot();
+    });
+});
diff --git a/src/tests/unit/tests/DetailsView/components/scan-incomplete-warning.test.tsx b/src/tests/unit/tests/DetailsView/components/scan-incomplete-warning.test.tsx
@@ -26,6 +26,7 @@ describe('ScanIncompleteWarning', () => {
             NamedFC<ScanIncompleteWarningMessageBarProps>('test', _ => null);
         warningConfiguration = {
             'missing-required-cross-origin-permissions': scanIncompleteWarningStub,
+            'frame-skipped': scanIncompleteWarningStub,
         };
     });
 

diff --git a/src/tests/unit/tests/injected/analyzers/base-analyzer.test.ts b/src/tests/unit/tests/injected/analyzers/base-analyzer.test.ts
@@ -51,7 +51,7 @@ describe('BaseAnalyzer', () => {
         };
 
         scanIncompleteWarningDetectorMock
-            .setup(idm => idm.detectScanIncompleteWarnings())
+            .setup(idm => idm.detectScanIncompleteWarnings(undefined))
             .returns(() => scanIncompleteWarnings);
 
         sendMessageMock

diff --git a/src/tests/unit/tests/injected/analyzers/batched-rule-analyzer.test.ts b/src/tests/unit/tests/injected/analyzers/batched-rule-analyzer.test.ts
@@ -65,7 +65,7 @@ describe('BatchedRuleAnalyzer', () => {
             .returns(() => scopingState)
             .verifiable();
         scanIncompleteWarningDetectorMock
-            .setup(idm => idm.detectScanIncompleteWarnings())
+            .setup(idm => idm.detectScanIncompleteWarnings(It.isAny()))
             .returns(() => []);
     });
 

diff --git a/src/tests/unit/tests/injected/analyzers/notification-text-creator.test.ts b/src/tests/unit/tests/injected/analyzers/notification-text-creator.test.ts
@@ -42,7 +42,7 @@ describe('NotificationTextCreator', () => {
         'generates text for needs review with results: $unifiedResults and warnings: $testScanWarnings',
         ({ unifiedResults, testScanWarnings, expectedText }) => {
             scanIncompleteWarningDetectorMock
-                .setup(m => m.detectScanIncompleteWarnings())
+                .setup(m => m.detectScanIncompleteWarnings(null))
                 .returns(() => testScanWarnings);
 
             expect(testSubject.needsReviewText(unifiedResults)).toEqual(expectedText);
@@ -64,7 +64,7 @@ describe('NotificationTextCreator', () => {
         'generates null for automated checks with $unifiedResults and $testScanWarnings', // automated checks uses notifications generated from visualizationMessages.Common.ScanCompleted
         ({ unifiedResults, testScanWarnings }) => {
             scanIncompleteWarningDetectorMock
-                .setup(m => m.detectScanIncompleteWarnings())
+                .setup(m => m.detectScanIncompleteWarnings(null))
                 .returns(testScanWarnings);
 
             expect(testSubject.automatedChecksText(unifiedResults)).toEqual(null);

diff --git a/src/tests/unit/tests/injected/analyzers/rule-analyzer.test.ts b/src/tests/unit/tests/injected/analyzers/rule-analyzer.test.ts
@@ -74,7 +74,7 @@ describe('RuleAnalyzer', () => {
             .returns(() => scopingState)
             .verifiable();
         scanIncompleteWarningDetectorMock
-            .setup(idm => idm.detectScanIncompleteWarnings())
+            .setup(idm => idm.detectScanIncompleteWarnings(It.isAny()))
             .returns(() => []);
     });
 

diff --git a/src/tests/unit/tests/injected/analyzers/tab-stops-analyzer.test.ts b/src/tests/unit/tests/injected/analyzers/tab-stops-analyzer.test.ts
@@ -64,7 +64,7 @@ describe('TabStopsAnalyzer', () => {
         tabStopsRequirementResultProcessorMock = Mock.ofType<TabStopsRequirementResultProcessor>();
 
         scanIncompleteWarningDetectorMock
-            .setup(idm => idm.detectScanIncompleteWarnings())
+            .setup(idm => idm.detectScanIncompleteWarnings(It.isAny()))
             .returns(() => []);
 
         testSubject = new TabStopsAnalyzer(

diff --git a/src/tests/unit/tests/injected/analyzers/unified-result-sender.test.ts b/src/tests/unit/tests/injected/analyzers/unified-result-sender.test.ts
@@ -111,7 +111,7 @@ describe('sendConvertedResults', () => {
                         .setup(m => m(inputResults))
                         .returns(val => unifiedRules);
                     scanIncompleteWarningDetectorMock
-                        .setup(m => m.detectScanIncompleteWarnings())
+                        .setup(m => m.detectScanIncompleteWarnings(inputResults))
                         .returns(() => warnings);
                     filterNeedsReviewResultsMock
                         .setup(m => m(axeInputResults))

diff --git a/src/tests/unit/tests/injected/scan-incomplete-warning-detector.test.ts b/src/tests/unit/tests/injected/scan-incomplete-warning-detector.test.ts
@@ -4,6 +4,7 @@ import { BaseStore } from 'common/base-store';
 import { PermissionsStateStoreData } from 'common/types/store-data/permissions-state-store-data';
 import { IframeDetector } from 'injected/iframe-detector';
 import { ScanIncompleteWarningDetector } from 'injected/scan-incomplete-warning-detector';
+import { ScanResults } from 'scanner/iruleresults';
 import { IMock, Mock } from 'typemoq';
 
 describe('ScanIncompleteWarningDetector', () => {
@@ -35,7 +36,33 @@ describe('ScanIncompleteWarningDetector', () => {
                 .setup(m => m.getState())
                 .returns(() => ({ hasAllUrlAndFilePermissions }));
 
-            expect(testSubject.detectScanIncompleteWarnings()).toStrictEqual(expectedResults);
+            expect(testSubject.detectScanIncompleteWarnings(null)).toStrictEqual(expectedResults);
+        },
+    );
+
+    it('should not detect frames skipped warning when it detects permissions warning ', () => {
+        iframeDetectorMock.setup(m => m.hasIframes()).returns(() => true);
+        permissionsStateStoreMock
+            .setup(m => m.getState())
+            .returns(() => ({ hasAllUrlAndFilePermissions: false }));
+        const results = { framesSkipped: true } as ScanResults;
+        expect(testSubject.detectScanIncompleteWarnings(results)).toStrictEqual([
+            'missing-required-cross-origin-permissions',
+        ]);
+    });
+
+    it('should detect no frames skipped if results are null', () => {
+        expect(testSubject.detectScanIncompleteWarnings(null)).toStrictEqual([]);
+    });
+
+    it.each([true, false])(
+        'should detect frames skipped correctly if results frames skipped is %s',
+        (resultsFramesSkipped: boolean) => {
+            const results = { framesSkipped: resultsFramesSkipped } as ScanResults;
+
+            expect(testSubject.detectScanIncompleteWarnings(results)).toStrictEqual(
+                resultsFramesSkipped ? ['frame-skipped'] : [],
+            );
         },
     );
 });
diff --git a/src/tests/unit/tests/scanner/__snapshots__/get-rule-inclusions.test.ts.snap b/src/tests/unit/tests/scanner/__snapshots__/get-rule-inclusions.test.ts.snap
@@ -142,8 +142,8 @@ exports[`getRuleInclusions matches snapshotted list of production rules 1`] = `
     "status": "included",
   },
   "frame-tested": {
-    "reason": "rule is tagged best-practice",
-    "status": "excluded",
+    "reason": "Tests for unresponsive frames, enables iframe skipped warning",
+    "status": "included-always",
   },
   "frame-title": {
     "status": "included",

diff --git a/src/tests/unit/tests/scanner/result-decorator.test.ts b/src/tests/unit/tests/scanner/result-decorator.test.ts
@@ -88,6 +88,7 @@ describe('ResultDecorator', () => {
                 timestamp: 100,
                 targetPageTitle: 'test title',
                 targetPageUrl: 'https://test_url',
+                framesSkipped: false,
             };
 
             messageDecoratorMock
@@ -168,6 +169,7 @@ describe('ResultDecorator', () => {
                 timestamp: 100,
                 targetPageTitle: 'test title',
                 targetPageUrl: 'https://test_url',
+                framesSkipped: false,
             };
 
             tagToLinkMapperMock
@@ -240,6 +242,7 @@ describe('ResultDecorator', () => {
                 timestamp: 100,
                 targetPageTitle: 'test title',
                 targetPageUrl: 'https://test_url',
+                framesSkipped: false,
             };
 
             instanceStub.nodes = [];
@@ -271,4 +274,43 @@ describe('ResultDecorator', () => {
             messageDecoratorMock.verifyAll();
         });
     });
+
+    describe('decorateResults: with incomplete results', () => {
+        it('should set framesSkipped', () => {
+            const incompleteInstance = {
+                id: 'frame-tested',
+                nodes: [],
+                description: null,
+                tags: ['tag2'],
+            };
+            const guidanceLinkStub = {} as any;
+            const tagToLinkMapper = () => [guidanceLinkStub];
+            const emptyResultsStub = {
+                passes: [],
+                violations: [],
+                inapplicable: [],
+                incomplete: [],
+                timestamp: 100,
+                targetPageTitle: 'test title',
+                targetPageUrl: 'https://test_url',
+                framesSkipped: true,
+            };
+            messageDecoratorMock.setup(mdm => mdm.decorateResultWithMessages(It.isAny()));
+            ruleProcessorMock
+                .setup(m => m.suppressChecksByMessages(It.isAny(), It.isAny()))
+                .returns(result => {
+                    return null;
+                });
+            const testSubject = new ResultDecorator(
+                documentUtilsMock.object,
+                messageDecoratorMock.object,
+                getHelpUrlMock.object,
+                tagToLinkMapper,
+                ruleProcessorMock.object,
+            );
+            nonEmptyResultStub.incomplete = [incompleteInstance];
+            const decoratedResult = testSubject.decorateResults(nonEmptyResultStub);
+            expect(decoratedResult).toEqual(emptyResultsStub);
+        });
+    });
 });