From c4b66bfb6cc61b029ec6c7eeb77f1b55c38c1a1b Mon Sep 17 00:00:00 2001 From: Kenny Zhen Date: Wed, 12 May 2021 12:05:27 -0700 Subject: [PATCH] fix: copy issue details button on insecure origin target page (#4170) #### Details This PR improves the behavior of the copy issue details button of the detail dialog when a user is using AI-Web on an insecure target page. There are two main changes: 1. Upon failing to copy details due to insecure origin, the toast message now reads "Failed to copy failure details." as suggested: https://github.com/microsoft/accessibility-insights-web/issues/3803#issuecomment-758229250. Example screenshot is shown below: ![UpdatedToastMessage](https://user-images.githubusercontent.com/2243970/116487471-e51c1f00-a844-11eb-894b-e2f527b1d905.png "Toast message reads 'Failed to copy failure details.'") 2. In addition to (1), a help message also appears under the row of buttons that reads "To copy failure details, first open the details page." Example screenshot with each button's respective help message visible is shown below: ![AllHelpMessagesVisible](https://user-images.githubusercontent.com/2243970/116487472-e5b4b580-a844-11eb-9e41-4593870bc905.png "Detail dialog with all three help messages visible") ##### Motivation This address issue #3803 ##### Context When verifying the usability with screen reader software (Mac OSX VoiceOver), the help message is read aloud but the toast message is not. On subsequent clicks of the copy issue details button, the toast message is read aloud (since the help message stays visible after the first click). What should be the expected behavior? Alternative approaches: * When all help messages are visible, they are not ordered the same respective to the layout of the buttons (ex. copy issue details button is second in line, but its help message is last). This is because the help messages are rendered in different files. I considered separating the help messages into their own components, but did not proceed to avoid changing files that were unrelated to this issue. #### Pull request checklist - [x] Addresses an existing issue: #3803 - [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: `/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 Co-authored-by: Kenny Zhen --- .../components/copy-issue-details-button.tsx | 10 +- src/common/window-utils.ts | 4 + src/injected/components/command-bar.tsx | 30 +++++- src/injected/components/details-dialog.tsx | 17 ++- src/injected/details-dialog-handler.ts | 23 +++- src/injected/styles/injected.scss | 8 ++ src/injected/window-initializer.ts | 2 +- .../copy-issue-details-button.test.tsx | 76 ++++++++++--- .../__snapshots__/command-bar.test.tsx.snap | 102 ++++++++++++++++++ .../details-dialog.test.tsx.snap | 48 +++++++++ .../injected/components/command-bar.test.tsx | 19 ++++ .../components/details-dialog.test.tsx | 17 +-- .../injected/details-dialog-handler.test.ts | 79 +++++++++++++- 13 files changed, 400 insertions(+), 35 deletions(-) diff --git a/src/common/components/copy-issue-details-button.tsx b/src/common/components/copy-issue-details-button.tsx index cc63307ce55..66eac4126f0 100644 --- a/src/common/components/copy-issue-details-button.tsx +++ b/src/common/components/copy-issue-details-button.tsx @@ -20,6 +20,7 @@ export type CopyIssueDetailsButtonProps = { deps: CopyIssueDetailsButtonDeps; issueDetailsData: CreateIssueDetailsTextData; onClick: (clickEvent: React.MouseEvent) => void; + hasSecureTargetPage: boolean; }; export class CopyIssueDetailsButton extends React.Component { @@ -52,8 +53,13 @@ export class CopyIssueDetailsButton extends React.Component boolean; devToolsShortcut: string; + hasSecureTargetPage: boolean; + shouldShowInsecureOriginPageMessage: boolean; }; export const CommandBar = NamedFC('CommandBar', props => { @@ -67,16 +69,23 @@ export const CommandBar = NamedFC('CommandBar', props => { return ( <> - + {renderCopyIssueDetailsButton(issueData)} {renderFileIssueButton(issueData)} ); }; + const renderCopyIssueDetailsButton = (issueData: CreateIssueDetailsTextData): JSX.Element => { + return ( + + ); + }; + const renderFileIssueButton = (issueData: CreateIssueDetailsTextData): JSX.Element => { return ( ('CommandBar', props => { } }; + const renderCopyIssueDetailsMessage = (): JSX.Element => { + if (props.shouldShowInsecureOriginPageMessage) { + return ( +
+ To copy failure details, first open the Accessibility Insights for Web page. +
+ ); + } + }; + return (
{renderInspectButton()} {renderIssueButtons()} {renderInspectMessage()} + {renderCopyIssueDetailsMessage()}
); }); diff --git a/src/injected/components/details-dialog.tsx b/src/injected/components/details-dialog.tsx index 83a04f421d0..a0227482365 100644 --- a/src/injected/components/details-dialog.tsx +++ b/src/injected/components/details-dialog.tsx @@ -54,6 +54,7 @@ export interface DetailsDialogState { canInspect: boolean; userConfigurationStoreData: UserConfigurationStoreData; showInspectMessage: boolean; + showInsecureOriginPageMessage: boolean; } export class DetailsDialog extends React.Component { @@ -65,6 +66,8 @@ export class DetailsDialog extends React.Component boolean; public isNextButtonDisabled: () => boolean; public isInspectButtonDisabled: () => boolean; + public onClickCopyIssueDetailsButton: (ev) => void; + public shouldShowInsecureOriginPageMessage: () => boolean; constructor(props: DetailsDialogProps) { super(props); @@ -105,7 +108,12 @@ export class DetailsDialog extends React.Component { return this.props.dialogHandler.isInspectButtonDisabled(this); }; - + this.onClickCopyIssueDetailsButton = (ev: React.MouseEvent) => { + this.props.dialogHandler.copyIssueDetailsButtonClickHandler(this, ev); + }; + this.shouldShowInsecureOriginPageMessage = () => { + return this.props.dialogHandler.shouldShowInsecureOriginPageMessage(this); + }; this.state = { showDialog: true, currentRuleIndex: 0, @@ -114,6 +122,8 @@ export class DetailsDialog extends React.Component this.props.dialogHandler.shouldShowInspectButtonMessage(this), userConfigurationStoreData: this.state.userConfigurationStoreData, + hasSecureTargetPage: this.props.dialogHandler.isTargetPageOriginSecure(), + shouldShowInsecureOriginPageMessage: this.shouldShowInsecureOriginPageMessage(), }; return ; diff --git a/src/injected/details-dialog-handler.ts b/src/injected/details-dialog-handler.ts index 67b64efedc0..f04a7d507bd 100644 --- a/src/injected/details-dialog-handler.ts +++ b/src/injected/details-dialog-handler.ts @@ -2,13 +2,14 @@ // Licensed under the MIT License. import * as React from 'react'; import { HTMLElementUtils } from '../common/html-element-utils'; +import { WindowUtils } from '../common/window-utils'; import { DetailsDialog } from './components/details-dialog'; export class DetailsDialogHandler { private onDevToolChangedHandler: () => void; private onUserConfigChangedHandler: () => void; - constructor(private htmlElementUtils: HTMLElementUtils) {} + constructor(private htmlElementUtils: HTMLElementUtils, private windowUtils: WindowUtils) {} public backButtonClickHandler = (dialog: DetailsDialog): void => { const currentRuleIndex = dialog.state.currentRuleIndex; @@ -82,6 +83,26 @@ export class DetailsDialogHandler { } for this target`; }; + public isTargetPageOriginSecure = (): boolean => { + return this.windowUtils.isSecureOrigin(); + }; + + public copyIssueDetailsButtonClickHandler = ( + dialog: DetailsDialog, + event: React.MouseEvent, + ): void => { + dialog.props.deps.targetPageActionMessageCreator.copyIssueDetailsClicked(event); + if (!this.isTargetPageOriginSecure()) { + dialog.setState({ showInsecureOriginPageMessage: true }); + } else { + dialog.setState({ showInsecureOriginPageMessage: false }); + } + }; + + public shouldShowInsecureOriginPageMessage = (dialog: DetailsDialog): boolean => { + return dialog.state.showInsecureOriginPageMessage; + }; + public onLayoutDidMount = (): void => { const dialogContainer = this.htmlElementUtils.querySelector( '.insights-dialog-main-override', diff --git a/src/injected/styles/injected.scss b/src/injected/styles/injected.scss index 51b52b6433b..2ef847739b0 100644 --- a/src/injected/styles/injected.scss +++ b/src/injected/styles/injected.scss @@ -137,6 +137,7 @@ .insights-dialog-main-override .insights-dialog-inspect-disabled { margin-top: 4px !important; + margin-bottom: 8px !important; font-size: 12px !important; color: $negative-outcome !important; font-weight: $fontWeightSemiBold !important; @@ -150,6 +151,13 @@ font-weight: $fontWeightSemiBold; } + .insights-dialog-main-override .copy-issue-details-button-help { + margin-top: 4px !important; + font-size: 12px !important; + color: $negative-outcome !important; + font-weight: $fontWeightSemiBold !important; + } + .insights-dialog-main-override { .insights-dialog-title { margin-bottom: 16px !important; diff --git a/src/injected/window-initializer.ts b/src/injected/window-initializer.ts index 090ebfa9b87..46c4f1a5218 100644 --- a/src/injected/window-initializer.ts +++ b/src/injected/window-initializer.ts @@ -129,7 +129,7 @@ export class WindowInitializer { this.frameMessenger, this.browserAdapter, getRTL, - new DetailsDialogHandler(htmlElementUtils), + new DetailsDialogHandler(htmlElementUtils, this.windowUtils), ); this.drawingController = new DrawingController( this.frameMessenger, diff --git a/src/tests/unit/tests/common/components/copy-issue-details-button.test.tsx b/src/tests/unit/tests/common/components/copy-issue-details-button.test.tsx index cf313f3321f..a48b4cebda4 100644 --- a/src/tests/unit/tests/common/components/copy-issue-details-button.test.tsx +++ b/src/tests/unit/tests/common/components/copy-issue-details-button.test.tsx @@ -49,6 +49,7 @@ describe('CopyIssueDetailsButtonTest', () => { }, issueDetailsData: {} as CreateIssueDetailsTextData, onClick: onClickMock.object, + hasSecureTargetPage: true, }; }); @@ -56,27 +57,70 @@ describe('CopyIssueDetailsButtonTest', () => { const result = Enzyme.shallow(); expect(result.debug()).toMatchSnapshot(); }); + describe('toast message', () => { + test('render after click shows copy success message', async () => { + navigatorUtilsMock + .setup(navigatorUtils => navigatorUtils.copyToClipboard(issueDetailsText)) + .returns(() => { + return Promise.resolve(); + }) + .verifiable(Times.once()); - test('render after click shows toast', async () => { - navigatorUtilsMock - .setup(navigatorUtils => navigatorUtils.copyToClipboard(issueDetailsText)) - .returns(() => { - return Promise.resolve(); - }) - .verifiable(Times.once()); + const result = Enzyme.mount(); + const button = result.find(DefaultButton); + onClickMock.setup(m => m(It.isAny())).verifiable(Times.once()); + // tslint:disable-next-line: await-promise + await button.simulate('click'); - const result = Enzyme.mount(); - const button = result.find(DefaultButton); - onClickMock.setup(m => m(It.isAny())).verifiable(Times.once()); - // tslint:disable-next-line: await-promise - await button.simulate('click'); + const toast = result.find(Toast); - const toast = result.find(Toast); + expect(toast.state().toastVisible).toBe(true); + expect(toast.state().content).toBe('Failure details copied.'); - expect(toast.state().toastVisible).toBe(true); - expect(toast.state().content).toBe('Failure details copied.'); + verifyMocks(); + }); + test('render after click shows copy failure message', async () => { + navigatorUtilsMock + .setup(navigatorUtils => navigatorUtils.copyToClipboard(issueDetailsText)) + .returns(() => { + return Promise.reject(); + }) + .verifiable(Times.once()); - verifyMocks(); + const result = Enzyme.mount(); + const button = result.find(DefaultButton); + onClickMock.setup(m => m(It.isAny())).verifiable(Times.once()); + // tslint:disable-next-line: await-promise + await button.simulate('click'); + + const toast = result.find(Toast); + + expect(toast.state().toastVisible).toBe(true); + expect(toast.state().content).toBe('Failed to copy failure details. Please try again.'); + + verifyMocks(); + }); + test('shows copy failure message for insecure origin', async () => { + navigatorUtilsMock + .setup(navigatorUtils => navigatorUtils.copyToClipboard(issueDetailsText)) + .returns(() => { + return Promise.reject(); + }) + .verifiable(Times.once()); + props.hasSecureTargetPage = false; + const result = Enzyme.mount(); + const button = result.find(DefaultButton); + onClickMock.setup(m => m(It.isAny())).verifiable(Times.once()); + // tslint:disable-next-line: await-promise + await button.simulate('click'); + + const toast = result.find(Toast); + + expect(toast.state().toastVisible).toBe(true); + expect(toast.state().content).toBe('Failed to copy failure details.'); + + verifyMocks(); + }); }); function verifyMocks(): void { diff --git a/src/tests/unit/tests/injected/components/__snapshots__/command-bar.test.tsx.snap b/src/tests/unit/tests/injected/components/__snapshots__/command-bar.test.tsx.snap index 95f9c97cdb4..f44445f27a1 100644 --- a/src/tests/unit/tests/injected/components/__snapshots__/command-bar.test.tsx.snap +++ b/src/tests/unit/tests/injected/components/__snapshots__/command-bar.test.tsx.snap @@ -1,5 +1,107 @@ // Jest Snapshot v1, https://goo.gl/fbAQLP +exports[`CommandBar renders renders, shows copy button insecure origin message: false 1`] = ` +
+ + +
+ Inspect HTML +
+ + + + + +
+`; + +exports[`CommandBar renders renders, shows copy button insecure origin message: true 1`] = ` +
+ + +
+ Inspect HTML +
+ + + + + +
+ To copy failure details, first open the Accessibility Insights for Web page. +
+
+`; + exports[`CommandBar renders renders, shows inspect button message: false 1`] = `
{ failedRules: { 'RR-rule-id': ruleResult, }, + hasSecureTargetPage: undefined, + shouldShowInsecureOriginPageMessage: false, }; beforeAll(() => { @@ -65,6 +67,7 @@ describe('CommandBar', () => { describe('renders', () => { const showInspectButtonMessage = [true, false]; + const showCopyIssueDetailsHelpMessage = [true, false]; it.each(showInspectButtonMessage)('renders, shows inspect button message: %s', show => { const props = { @@ -78,6 +81,22 @@ describe('CommandBar', () => { expect(wrapper.getElement()).toMatchSnapshot(); axeConverterMock.verifyAll(); }); + + it.each(showCopyIssueDetailsHelpMessage)( + 'renders, shows copy button insecure origin message: %s', + show => { + const props = { + ...defaultCommandBarProps, + + shouldShowInsecureOriginPageMessage: show, + }; + + const wrapper = shallow(); + + expect(wrapper.getElement()).toMatchSnapshot(); + axeConverterMock.verifyAll(); + }, + ); }); describe('click handlers', () => { diff --git a/src/tests/unit/tests/injected/components/details-dialog.test.tsx b/src/tests/unit/tests/injected/components/details-dialog.test.tsx index a02119ca655..bbcf57822d8 100644 --- a/src/tests/unit/tests/injected/components/details-dialog.test.tsx +++ b/src/tests/unit/tests/injected/components/details-dialog.test.tsx @@ -12,7 +12,6 @@ import { DetailsDialogProps, } from '../../../../../injected/components/details-dialog'; import { DecoratedAxeNodeResult } from '../../../../../injected/scanner-utils'; -import { TargetPageActionMessageCreator } from '../../../../../injected/target-page-action-message-creator'; import { DictionaryStringTo } from '../../../../../types/common-types'; import { EventStubFactory } from '../../../common/event-stub-factory'; @@ -138,14 +137,18 @@ describe('DetailsDialog', () => { }); test('on click copy issue details button', () => { - const targetPageActionMessageCreatorMock = Mock.ofType(); + const clickHandlerMock = Mock.ofInstance( + (component: DetailsDialog, event: React.MouseEvent) => {}, + ); + + dialogDetailsHandlerMockObject.copyIssueDetailsButtonClickHandler = + clickHandlerMock.object; const deps = { ...defaultDetailsDialogDeps, browserAdapter: { getUrl: url => 'test-url', } as any, - targetPageActionMessageCreator: targetPageActionMessageCreatorMock.object, }; const props = { @@ -155,14 +158,14 @@ describe('DetailsDialog', () => { dialogHandler: dialogDetailsHandlerMockObject, }; - const wrapper = shallow(); + const wrapper = shallow(); const commandBar = wrapper.find(CommandBar); commandBar.prop('onClickCopyIssueDetailsButton')(eventStub); - targetPageActionMessageCreatorMock.verify( - creator => creator.copyIssueDetailsClicked(eventStub), + clickHandlerMock.verify( + handler => handler(wrapper.instance(), eventStub), Times.once(), ); }); @@ -242,6 +245,8 @@ describe('DetailsDialog', () => { getFailureInfo: () => 'Failure 1 of 1 for this target', componentDidMount: () => {}, shouldShowInspectButtonMessage: () => false, + isTargetPageOriginSecure: () => true, + shouldShowInsecureOriginPageMessage: () => false, }; }; }); diff --git a/src/tests/unit/tests/injected/details-dialog-handler.test.ts b/src/tests/unit/tests/injected/details-dialog-handler.test.ts index d0430e3a73a..08b38cc0246 100644 --- a/src/tests/unit/tests/injected/details-dialog-handler.test.ts +++ b/src/tests/unit/tests/injected/details-dialog-handler.test.ts @@ -1,5 +1,7 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. +import { WindowUtils } from 'common/window-utils'; +import { TargetPageActionMessageCreator } from 'injected/target-page-action-message-creator'; import { IMock, It, Mock, MockBehavior, Times } from 'typemoq'; import { BaseStore } from '../../../../common/base-store'; @@ -18,10 +20,12 @@ describe('DetailsDialogHandlerTest', () => { let detailsDialog: Element; let containerParent: Element; let detailsDialogMock: IMock; + let windowUtilsMock: IMock; beforeEach(() => { htmlElementUtilsMock = Mock.ofType(HTMLElementUtils); - testSubject = new DetailsDialogHandler(htmlElementUtilsMock.object); + windowUtilsMock = Mock.ofType(WindowUtils); + testSubject = new DetailsDialogHandler(htmlElementUtilsMock.object, windowUtilsMock.object); container = document.createElement('div'); detailsDialog = document.createElement('div'); container.appendChild(detailsDialog); @@ -174,6 +178,79 @@ describe('DetailsDialogHandlerTest', () => { devToolStoreMock.verifyAll(); }); + test.each([true, false])( + 'copyIssueDetailsButtonClickHandler when isTargetPageOriginSecure=%s', + isTargetPageOriginSecure => { + const targetPageActionMessageCreatorMock = Mock.ofType( + undefined, + MockBehavior.Strict, + ); + + const eventFactory = new EventStubFactory(); + const event = eventFactory.createMouseClickEvent(); + + testSubject.isTargetPageOriginSecure = () => isTargetPageOriginSecure; + + targetPageActionMessageCreatorMock + .setup(creator => creator.copyIssueDetailsClicked(It.isValue(event as any))) + .verifiable(Times.atLeastOnce()); + + detailsDialogMock + .setup(dialog => dialog.props) + .returns(() => { + return { + deps: { + targetPageActionMessageCreator: + targetPageActionMessageCreatorMock.object, + }, + } as any; + }) + .verifiable(Times.atLeastOnce()); + + detailsDialogMock + .setup(dialog => + dialog.setState( + It.isValue({ showInsecureOriginPageMessage: !isTargetPageOriginSecure }), + ), + ) + .verifiable(Times.once()); + + testSubject.copyIssueDetailsButtonClickHandler(detailsDialogMock.object, event as any); + + detailsDialogMock.verifyAll(); + targetPageActionMessageCreatorMock.verifyAll(); + }, + ); + + test.each([true, false])('isTargetPageOriginSecure=%s', isSecureOrigin => { + windowUtilsMock + .setup(w => w.isSecureOrigin()) + .returns(() => isSecureOrigin) + .verifiable(Times.once()); + + expect(testSubject.isTargetPageOriginSecure()).toEqual(isSecureOrigin); + + windowUtilsMock.verifyAll(); + }); + + test.each([true, false])('shouldShowInsecureOriginPageMessage=%s', show => { + detailsDialogMock + .setup(dialog => dialog.state) + .returns(() => { + return { + showInsecureOriginPageMessage: show, + } as any; + }) + .verifiable(Times.once()); + + const actualState = testSubject.shouldShowInsecureOriginPageMessage( + detailsDialogMock.object, + ); + expect(actualState).toEqual(show); + + detailsDialogMock.verifyAll(); + }); + test('showDialog', () => { detailsDialogMock .setup(dialog =>