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: #3803 (comment). 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: `<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 Co-authored-by: Kenny Zhen <[email protected]>
microsoft · May 12, 2021 · c4b66bf · c4b66bf
1 parent 00c13ba
commit c4b66bf
Show file tree

Hide file tree

Showing 13 changed files with 400 additions and 35 deletions.
diff --git 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<any>) => void;
+    hasSecureTargetPage: boolean;
 };
 
 export class CopyIssueDetailsButton extends React.Component<CopyIssueDetailsButtonProps> {
@@ -52,8 +53,13 @@ export class CopyIssueDetailsButton extends React.Component<CopyIssueDetailsButt
                 this.getIssueDetailsText(this.props.issueDetailsData),
             );
         } catch (error) {
-            toast.show('Failed to copy failure details. Please try again.');
-            return;
+            if (this.props.hasSecureTargetPage) {
+                toast.show('Failed to copy failure details. Please try again.');
+                return;
+            } else {
+                toast.show('Failed to copy failure details.');
+                return;
+            }
         }
         toast.show('Failure details copied.');
     };

diff --git a/src/common/window-utils.ts b/src/common/window-utils.ts
@@ -72,6 +72,10 @@ export class WindowUtils {
         return window.navigator.platform;
     }
 
+    public isSecureOrigin(): boolean {
+        return window.isSecureContext;
+    }
+
     public getRandomValueArray(length: number): Uint8Array {
         return window.crypto.getRandomValues(new Uint8Array(length));
     }

diff --git a/src/injected/components/command-bar.tsx b/src/injected/components/command-bar.tsx
@@ -39,6 +39,8 @@ export type CommandBarProps = {
     userConfigurationStoreData: UserConfigurationStoreData;
     shouldShowInspectButtonMessage: () => boolean;
     devToolsShortcut: string;
+    hasSecureTargetPage: boolean;
+    shouldShowInsecureOriginPageMessage: boolean;
 };
 
 export const CommandBar = NamedFC<CommandBarProps>('CommandBar', props => {
@@ -67,16 +69,23 @@ export const CommandBar = NamedFC<CommandBarProps>('CommandBar', props => {
 
         return (
             <>
-                <CopyIssueDetailsButton
-                    deps={props.deps}
-                    issueDetailsData={issueData}
-                    onClick={props.onClickCopyIssueDetailsButton}
-                />
+                {renderCopyIssueDetailsButton(issueData)}
                 {renderFileIssueButton(issueData)}
             </>
         );
     };
 
+    const renderCopyIssueDetailsButton = (issueData: CreateIssueDetailsTextData): JSX.Element => {
+        return (
+            <CopyIssueDetailsButton
+                deps={props.deps}
+                issueDetailsData={issueData}
+                onClick={props.onClickCopyIssueDetailsButton}
+                hasSecureTargetPage={props.hasSecureTargetPage}
+            />
+        );
+    };
+
     const renderFileIssueButton = (issueData: CreateIssueDetailsTextData): JSX.Element => {
         return (
             <IssueFilingButton
@@ -98,11 +107,22 @@ export const CommandBar = NamedFC<CommandBarProps>('CommandBar', props => {
         }
     };
 
+    const renderCopyIssueDetailsMessage = (): JSX.Element => {
+        if (props.shouldShowInsecureOriginPageMessage) {
+            return (
+                <div role="alert" className="copy-issue-details-button-help">
+                    To copy failure details, first open the Accessibility Insights for Web page.
+                </div>
+            );
+        }
+    };
+
     return (
         <div className="insights-dialog-target-button-container">
             {renderInspectButton()}
             {renderIssueButtons()}
             {renderInspectMessage()}
+            {renderCopyIssueDetailsMessage()}
         </div>
     );
 });
diff --git 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<DetailsDialogProps, DetailsDialogState> {
@@ -65,6 +66,8 @@ export class DetailsDialog extends React.Component<DetailsDialogProps, DetailsDi
     public isBackButtonDisabled: () => 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<DetailsDialogProps, DetailsDi
         this.isInspectButtonDisabled = () => {
             return this.props.dialogHandler.isInspectButtonDisabled(this);
         };
-
+        this.onClickCopyIssueDetailsButton = (ev: React.MouseEvent<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<DetailsDialogProps, DetailsDi
             // eslint-disable-next-line react/no-unused-state
             showInspectMessage: false,
             userConfigurationStoreData: props.userConfigStore.getState(),
+            // eslint-disable-next-line react/no-unused-state
+            showInsecureOriginPageMessage: false,
         };
     }
 
@@ -161,12 +171,13 @@ export class DetailsDialog extends React.Component<DetailsDialogProps, DetailsDi
             deps: this.props.deps,
             devToolsShortcut: this.props.devToolsShortcut,
             failedRules: this.props.failedRules,
-            onClickCopyIssueDetailsButton: this.props.deps.targetPageActionMessageCreator
-                .copyIssueDetailsClicked,
+            onClickCopyIssueDetailsButton: this.onClickCopyIssueDetailsButton,
             onClickInspectButton: this.onClickInspectButton,
             shouldShowInspectButtonMessage: () =>
                 this.props.dialogHandler.shouldShowInspectButtonMessage(this),
             userConfigurationStoreData: this.state.userConfigurationStoreData,
+            hasSecureTargetPage: this.props.dialogHandler.isTargetPageOriginSecure(),
+            shouldShowInsecureOriginPageMessage: this.shouldShowInsecureOriginPageMessage(),
         };
 
         return <CommandBar {...props} />;

diff --git 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<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
@@ -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
@@ -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
@@ -49,34 +49,78 @@ describe('CopyIssueDetailsButtonTest', () => {
             },
             issueDetailsData: {} as CreateIssueDetailsTextData,
             onClick: onClickMock.object,
+            hasSecureTargetPage: true,
         };
     });
 
     test('render', () => {
         const result = Enzyme.shallow(<CopyIssueDetailsButton {...props} />);
         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(<CopyIssueDetailsButton {...props} />);
+            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(<CopyIssueDetailsButton {...props} />);
-        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(<CopyIssueDetailsButton {...props} />);
+            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(<CopyIssueDetailsButton {...props} />);
+            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 {