@microsoft-github-policy-service agree

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!

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!

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:

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

In Windows HC mode:

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

@dbjorge
Thank you for your review and refactoring!