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: Make failure highlight more visible #6164

Merged
merged 9 commits into from
Nov 22, 2022

Conversation

segamiken
Copy link
Contributor

@segamiken segamiken commented Nov 3, 2022

Details

Motivation

close #5610

Context

Design team suggested trying multiple border colors with red in the center and white surrounding this.

I implemented this based on one of the design team's suggestions listed in the issue.

This will make failure highlight more visible, especially for a dark background as in the following picture. However, for pages with white or other background colors, it is not very effective.

screenshots/GIFs to description above

Dark Background

Before After
スクリーンショット 2022-11-03 20 00 00 スクリーンショット 2022-11-03 19 58 34

White Background

Before After
スクリーンショット 2022-11-03 20 14 26 スクリーンショット 2022-11-03 20 11 11

Pull request checklist

  • Addresses an existing issue: [Usability] Explore making failure instance highlight more visible #5610
  • Ran yarn null:autoadd
  • 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

@segamiken
Copy link
Contributor Author

@microsoft-github-policy-service agree

@segamiken segamiken marked this pull request as ready for review November 3, 2022 11:19
@segamiken segamiken requested a review from a team as a code owner November 3, 2022 11:19
@dbjorge
Copy link
Contributor

dbjorge commented Nov 16, 2022

Hi @segamiken ,

Thanks very much for the contribution! I think this is definitely on the right track visually, @ferBonnin and I have discussed and agree that the new UX you've created is a big improvement over the current highlight boxes.

I noticed while testing your changes locally that the new boxes look like they render a pixel or two off, and also that they appear to render the inner white border overtop the inside of the element being highlighted; we think we'd prefer to keep all 3 layers of the highlight box outline on the outside of the element if possible. I've been experimenting locally with an updated version of your changes that use a slightly different CSS technique to achieve that vs what you've done here, with a white box-shadow behind a colored outline instead of using a pseudo-element - it seems promising, but the experiment I have locally needs a bit more work before I send it out.

I just wanted to send a status update and acknowledge your contribution - we appreciate your PR, it's just taking us a bit longer than usual to review!

@segamiken
Copy link
Contributor Author

Hi @dbjorge!
Thank you for sharing current status!

I noticed while testing your changes locally that the new boxes look like they render a pixel or two off,

Oh, sorry and thank you for finding this!

it seems promising, but the experiment I have locally needs a bit more work before I send it out.

I got it!
I'm waiting for you to share it with me when you're done with your experiment!

@dbjorge
Copy link
Contributor

dbjorge commented Nov 17, 2022

I've pushed an update that swaps to the box-shadow + outline version. It updates all the box drawers and their labels. It pushes the determination of box width to CSS to reduce the amount of per-element overhead in style attributes. It touches a bunch of files to change some DrawerConfiguration types from using borderColor to outlineColor. I also made some related refactorings while patching up the tests:

  • The HighlightBoxDrawer previously allowed for no formatter to be passed and contained a "default drawer configuration" for that case. This was never used except for tests; I've removed it and updated drawer.test.ts accordingly
  • Removed an obsolete LandmarkValue type (not used anywhere and referenced borderColor)

I also fixed up some issues specific to the landmark visualizer; per discussion with @ferBonnin, I included the same white border there but reduced the width of the dashed part from 3px to 2px. I also fixed an issue I noticed with high-contrast-mode rendering of the landmark visualizer's dashed highlight boxes (previously, we used HC colors for the outer box, but not the text label box; this fixes it to be consistent).

For reviewers, I recommend including the Deque Mars test page among your smoke tests; it has a lot of failures against non-white backgrounds that are good test cases.

Screenshots of new behavior:

ad-hoc automated checks

ad-hoc headings

ad-hoc landmarks

Tab stops is not changed by this update and would need to be addressed in a separate PR:
tab stops

In Windows HC mode:

automated checks visualizer in Windows high contrast Aquatic

landmarks visualizer in Windows high contrast Aquatic

@dbjorge dbjorge added the pr: second review required Author has requested an additional review. label Nov 17, 2022
outline-width: 1px !important;
outline-style: solid !important;
outline-offset: 1px !important;
box-shadow: 0 0 0 3px white !important;
Copy link
Contributor

Choose a reason for hiding this comment

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

Opinion: I find 1px outline over 3px shadow makes the red outline not stand out as much as I'm used to, especially around images.
image
I think 2px outline over 4px shadow is a little clearer:
image
Will totally defer to @ferBonnin here.

Copy link
Contributor

@ferBonnin ferBonnin Nov 18, 2022

Choose a reason for hiding this comment

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

I see what you mean @peterdur specially for white backgrounds; I agree that 2px makes it more clearer but it also uses more space. I don't feel strongly about one way or another so if we prefer it, lets go with 2px with 4px shadow 🙂

Copy link
Contributor

@peterdur peterdur left a comment

Choose a reason for hiding this comment

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

Implementation looks good, built and ran locally. Just one question about whether we want the outline to be a little stronger.

@dbjorge dbjorge merged commit b34426b into microsoft:main Nov 22, 2022
@dbjorge
Copy link
Contributor

dbjorge commented Nov 22, 2022

Thanks again for your contribution, @segamiken ! This change will be included in the next Accessibility Insights for Web release.

@segamiken
Copy link
Contributor Author

@dbjorge
Thank you for your review and refactoring!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: second review required Author has requested an additional review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Usability] Explore making failure instance highlight more visible
4 participants