fix: copy issue details button on insecure origin target page

[kennyzhen13]

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: General error message for Copy failure details on non-HTTPS and non-localhost webpages #3803 (comment). Example screenshot is shown below:
   

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:
   

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

• Addresses an existing issue: General error message for Copy failure details on non-HTTPS and non-localhost webpages #3803
• Ran yarn fastpass
• Added/updated relevant unit test(s) (and ran yarn test)
• Verified code coverage for the changes made. Check coverage report at: <rootDir>/test-results/unit/coverage
• PR title AND final merge commit title both start with a semantic tag (fix:, chore:, feat(feature-name):, refactor:). See CONTRIBUTING.md.
• (UI changes only) Added screenshots/GIFs to description above
• (UI changes only) Verified usability with NVDA/JAWS

[ghost]

All CLA requirements met.

[dbjorge]

I felt a little confused by the name onClickCopyIssueDetailsButtonHelpMessage when I read it initially; based on the name, I expected it to be something that would get called when the button's help message was clicked.

Perhaps it would be more clear to just keep the single existing onClickCopyIssueDetailsButton prop, and give it handler which does both the existing this.props.deps.targetPageActionMessageCreator.copyIssueDetailsClicked() and the new setState({ showInsecureOriginPageMessage: ... }) operations? I think this would be more consistent with how DetailsDialogHandler.inspectButtonClickHandler works.

[dbjorge]

You've done a good job of using a consistent pattern with what the existing shouldShowInspectButtonMessage is doing, but unfortunately I think the existing code has set a bad example for you to follow. I think it would be better to pass this.state.showInsecureOriginPageMessage as a value here, rather than passing a prop which is a function that returns that value.

React chooses whether to re-render a child component as part of re-rendering a parent component based on whether the props passed to the child have changed. However, by passing a prop of the form () => something, React is going to see the prop changing with every parent render (since the parent is creating a new lambda function to pass on every render). That's not ideal; it would be better to pass the value from the state as the prop, so that React can detect whether the state value has changed from render-to-render, and can avoid re-rendering the child component unnecessarily if that value hasn't changed.

[dbjorge]

Changing this won't have a direct impact on its own without changing the existing issues in the similar props; if any single prop has this issue, the whole child component will have this issue. It's not necessary to fix the old code as part of this PR; it would probably be better to do that in its own separate PR. But as you've seen, it's really easy for new code in the future to copy from what old code is doing, so I think it would be nice to improve this for the new code even in the absence of fixing the old code.

[dbjorge]

Thanks very much for the contribution! I've left a few comments inline, but this looks great overall, especially the thorough unit test updates.

[kennyzhen13]

@dbjorge thank you for the review! I will address the comments. What is your recommendation regarding the screen reader behavior with the toast message (described under Context in the PR description)?

[dbjorge]

@kennyzhen13 I definitely think we should improve the screen reader behavior, but I lean towards creating a separate issue and PR for it; it sounds like the issues are mostly-present already in the similar messages, we'll want to fix them holistically and probably need some design review work to figure out the right flow. Would you mind opening a new issue to track it so we can make sure it doesn't get lost?

[kennyzhen13]

@dbjorge I have addressed the comments in my newest commit. I have also created a new bug #4211 for the screen reader behavior as discussed. Looking forward to your input.

[dbjorge]

Thanks for the update! Code-wise, this looks good to me.

User experience-wise, though, I think we'll probably want slightly different wording than "first open the details page" - the term "details page" is what we call the combined fastpass/assessment page internally, but we don't use that term in anything user-facing. @ferBonnin / @juchampa , do you have specific wording you'd like to see for the error text here to indicate that the user needs to open the FastPass/Assessment page?

[ferBonnin]

User experience-wise, though, I think we'll probably want slightly different wording than "first open the details page" - the term "details page" is what we call the combined fastpass/assessment page internally, but we don't use that term in anything user-facing. @ferBonnin / @juchampa , do you have specific wording you'd like to see for the error text here to indicate that the user needs to open the FastPass/Assessment page?

In our documentation, we simply refer to the details view as the "Accessibility Insights for Web page" we could try this?

[kennyzhen13]

In our documentation, we simply refer to the details view as the "Accessibility Insights for Web page" we could try this?

I can go ahead and change the text to "To copy failure details, first open the Accessibility Insights for Web page."

[dbjorge]

Looks good to me! Thanks for the contribution!