Skip to content
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: add banner for unresponsive iframes #6330

Merged
merged 5 commits into from
Jan 17, 2023
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions src/DetailsView/components/iframe-skipped-warning.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good starting point, but I'm not sure this is actionable enough. What do we want a user who sees this warning to do about it, exactly? Perhaps we should include something like "If you see this warning consistently for a page where it isn't expected, please file an issue."? @ferBonnin , what are your thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or perhaps it would be more manageable to create a single known tracking issue and link to it, asking folks to comment on it with the site in question.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either of those options sound reasonable to me! I'll defer to @ferBonnin's judgement for specific wording/ how to manage issues.

Copy link
Contributor

@ferBonnin ferBonnin Jan 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's create a tracking issue and let users comment on it, the barrier is lower for the user.
What are the cases where the user would get this message unexpectedly? if there aren't iframes?

for the message, how about:
If you get this warning in a page (due to reason to get it unexpectedly), please [let us know](link to the issue).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are the cases where the user would get this message unexpectedly? if there aren't iframes?

Users shouldn't get this message when there aren't iframes on the page, but they might get it on pages where they expect the iframes to be responsive but they're not, for example, if the iframes are responding but very slowly.

for the message, how about: If you get this warning in a page , please [let us know](link to the issue).

Sounds good to me! I assume that would be in addition to the existing text?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure I follow completely: in which cases would we want the user to let us know / file a bug?

Sounds good to me! I assume that would be in addition to the existing text?

edited my message because I was using <> and the text between them didn't show 🙂
Yes, that would be in addition to the text " There are iframes in the target page ..."

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if Dan has other thoughts, but IMO I think it would be interesting to get some example target pages where a user thinks that the iframes should be responsive but for some reason we aren't able to communicate with them (and as a result show this error). So it would be valuable for a user to file an issue if they are seeing this warning unexpectedly and think that the iframes should be/ are responsive.

In this case, should we be a little more specific in the wording and include something like If you get this warning unexpectedly in a page where you expect the iframes to be responsive, please [let us know](link to the issue).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I created a tracking issue to link to here: #6347, please feel free to edit it directly if I missed anything!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That message looks great to me!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! I think the most common case will actually be that the person running the tool won't really know a lot of details about the iframes causing problems in the page, but I think that message should cover that case okay. Thanks for the update!

and are not included in results.
</div>
));
3 changes: 3 additions & 0 deletions src/DetailsView/components/warning-configuration.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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,
};
2 changes: 1 addition & 1 deletion src/common/types/store-data/scan-incomplete-warnings.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
2 changes: 1 addition & 1 deletion src/injected/analyzers/base-analyzer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ export class BaseAnalyzer implements Analyzer {
scanResult: originalAxeResult,
testType: config.testType,
scanIncompleteWarnings:
this.scanIncompleteWarningDetector.detectScanIncompleteWarnings(),
this.scanIncompleteWarningDetector.detectScanIncompleteWarnings(originalAxeResult),
};

return {
Expand Down
2 changes: 1 addition & 1 deletion src/injected/analyzers/notification-text-creator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.';
}
Expand Down
2 changes: 1 addition & 1 deletion src/injected/analyzers/unified-result-sender.ts
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ export class UnifiedResultSender {
notificationMessage: TextGenerator,
): UnifiedScanCompletedPayload => {
const scanIncompleteWarnings =
this.scanIncompleteWarningDetector.detectScanIncompleteWarnings();
this.scanIncompleteWarningDetector.detectScanIncompleteWarnings(results);

let telemetry: ScanIncompleteWarningsTelemetryData = null;

Expand Down
9 changes: 8 additions & 1 deletion src/injected/scan-incomplete-warning-detector.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -11,14 +12,20 @@ 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');
}

if (results && results.framesSkipped) {
warnings.push('frame-skipped');
dbjorge marked this conversation as resolved.
Show resolved Hide resolved
}

return warnings;
};
}
1 change: 1 addition & 0 deletions src/scanner/iruleresults.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@ export interface ScanResults {
timestamp: string;
targetPageUrl: string;
targetPageTitle: string;
framesSkipped?: boolean;
}

export interface RuleDecorations {
Expand Down
1 change: 1 addition & 0 deletions src/scanner/result-decorator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
8 changes: 4 additions & 4 deletions src/scanner/scan-parameter-generator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,11 +20,11 @@ export class ScanParameterGenerator {
};

if (options == null || options.testsToRun == null) {
result.runOnly!.values = Object.keys(this.rulesIncluded).filter(
id => this.rulesIncluded[id].status === 'included',
);
result.runOnly!.values = Object.keys(this.rulesIncluded)
.filter(id => this.rulesIncluded[id].status === 'included')
.concat('frame-tested');
dbjorge marked this conversation as resolved.
Show resolved Hide resolved
} else {
result.runOnly!.values = options.testsToRun;
result.runOnly!.values = options.testsToRun.concat('frame-tested');
}

return result;
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
// 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.
</div>
`;
Original file line number Diff line number Diff line change
@@ -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();
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ describe('ScanIncompleteWarning', () => {
NamedFC<ScanIncompleteWarningMessageBarProps>('test', _ => null);
warningConfiguration = {
'missing-required-cross-origin-permissions': scanIncompleteWarningStub,
'frame-skipped': scanIncompleteWarningStub,
};
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ describe('BaseAnalyzer', () => {
};

scanIncompleteWarningDetectorMock
.setup(idm => idm.detectScanIncompleteWarnings())
.setup(idm => idm.detectScanIncompleteWarnings(undefined))
.returns(() => scanIncompleteWarnings);

sendMessageMock
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ describe('BatchedRuleAnalyzer', () => {
.returns(() => scopingState)
.verifiable();
scanIncompleteWarningDetectorMock
.setup(idm => idm.detectScanIncompleteWarnings())
.setup(idm => idm.detectScanIncompleteWarnings(It.isAny()))
.returns(() => []);
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ describe('RuleAnalyzer', () => {
.returns(() => scopingState)
.verifiable();
scanIncompleteWarningDetectorMock
.setup(idm => idm.detectScanIncompleteWarnings())
.setup(idm => idm.detectScanIncompleteWarnings(It.isAny()))
.returns(() => []);
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Expand Down Expand Up @@ -35,7 +36,22 @@ describe('ScanIncompleteWarningDetector', () => {
.setup(m => m.getState())
.returns(() => ({ hasAllUrlAndFilePermissions }));

expect(testSubject.detectScanIncompleteWarnings()).toStrictEqual(expectedResults);
expect(testSubject.detectScanIncompleteWarnings(null)).toStrictEqual(expectedResults);
},
);

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'] : [],
);
},
);
});
42 changes: 42 additions & 0 deletions src/tests/unit/tests/scanner/result-decorator.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ describe('ResultDecorator', () => {
timestamp: 100,
targetPageTitle: 'test title',
targetPageUrl: 'https://test_url',
framesSkipped: false,
};

messageDecoratorMock
Expand Down Expand Up @@ -168,6 +169,7 @@ describe('ResultDecorator', () => {
timestamp: 100,
targetPageTitle: 'test title',
targetPageUrl: 'https://test_url',
framesSkipped: false,
};

tagToLinkMapperMock
Expand Down Expand Up @@ -240,6 +242,7 @@ describe('ResultDecorator', () => {
timestamp: 100,
targetPageTitle: 'test title',
targetPageUrl: 'https://test_url',
framesSkipped: false,
};

instanceStub.nodes = [];
Expand Down Expand Up @@ -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);
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ describe('ScanParameterGenerator', () => {
restoreScroll: true,
runOnly: {
type: 'rule',
values: ['rule-a', 'rule-b'],
values: ['rule-a', 'rule-b', 'frame-tested'],
},
pingWaitTime: FRAME_COMMUNICATION_TIMEOUT_MS,
};
Expand All @@ -48,7 +48,7 @@ describe('ScanParameterGenerator', () => {
restoreScroll: true,
runOnly: {
type: 'rule',
values: ['rule-a', 'rule-b'],
values: ['rule-a', 'rule-b', 'frame-tested'],
},
pingWaitTime: FRAME_COMMUNICATION_TIMEOUT_MS,
};
Expand All @@ -64,7 +64,7 @@ describe('ScanParameterGenerator', () => {
restoreScroll: true,
runOnly: {
type: 'rule',
values: ['ruleA', 'ruleB'],
values: ['ruleA', 'ruleB', 'frame-tested'],
},
pingWaitTime: FRAME_COMMUNICATION_TIMEOUT_MS,
};
Expand Down