1561334 - [meta] Review A11y for protection report

Status: RESOLVED FIXED

(Firefox :: Protections UI, enhancement, P1)

Description

[Erica Wright [:ewright]]

Ensure all of the report is usable/readable in all expected ways.

Comment 1

[Florens Verschelde :fvsch]

Attachment: about-protections-graph-highlight.png

Not a full review, but here are some issues.
Main problems I've found are keyboard navigation not working at all, color contrast issues, and coding information through color only.

Heading hierarchy

Currently using H2 and H3, could be H1 and H2 instead.

Link to about:settings#privacy

Currently coded as a paragraph with a click event handler.
We should probably use a link, going from:

<p id="protection-details" onClick="...">
  Protection Level is set to <span>Standard</span>
</p>

to:

<p id="protection-details">
  <a href="about:settings#privacy" target="_blank" title="Go to privacy settings">
    Protection Level is set to <strong>Standard</strong>
  </a>
</p>

or maybe:

<p id="protection-details">
  <a href="about:settings#privacy" target="_blank">
    Protection Level is set to <strong>Standard</strong>
    <span class="visually-hidden">(Go to privacy settings)</span>
  </a>
</p>

I think it's important to have extra information that explains what the link does. A title attribute might be useful for all users so I would go with that, if we think it will be rendered correctly as additional information in screen readers when the link is focused. We might want to combine both solutions too. This would need some testing in e.g. Windows + NVDA.

Legend tabs are not focusable

Tabs are implemented as labels with hidden radio inputs. Not sure that's the best implementation of a tab pattern. Maybe we should go with a JS implementation of an ARIA tab pattern, where you focus the whole tab group and use the arrow keys to select tabs?

If we keep the radio input implementation, instead of using display:none this could work:

#legend {
  overflow: hidden;
}
#legend input {
  position: absolute;
  left: -100px;
}
#legend label:hover,
#legend input:focus + label {
  background-color: ...;
}
#legend input:focus + label {
  outline: solid 2px;
  outline-offset: -2px;
}

Graph section highlight implemented on hover only

When hovering the legend tabs, we highlight the corresponding sections in the graphs. This should probably be implemented on focus as well.

Graph section highlight is not distinctive enough

Slightly darkening the bar colors might not be enough (not enough visual difference between both states). Instead maybe we should explore alternative designs, such as adding an outline to the highlighted bar or making the highlighted bar wider? See the attached screenshot, which uses:

.hover-crossSite .crossSite-bar {
  outline: solid 2px var(--crossSite-color-darker);
  outline-offset: -2px;
  width: calc(100% + 4px);
  margin-left: -2px;
}

Legend tabs text colors

None of the legend tab labels use an accessible color.
Using the DevTools Accessibility panel, I'm finding color contrasts between 1.66:1 (orange) to 3.34:1 (blue) and other values in-between. We're targetting a minimum contrast ratio of 4.5:1.

A quick win would be to use the darker color (e.g. var(--crossSite-color-darker)) instead of the graph's light color. Then we have 2 labels that pass the 4.5 threshold, and 3 that are still too low.

Another option is to use a neutral black/gray color for the label, and use another graphical element (like a bullet, maybe) next to the label.

Comment 2

[Johann Hofmann [:johannh]]

Thank you for digging into that, Florens. This is very helpful, though note that the protection report is still a bit WIP and things may change. Once it's a bit more stable, we plan to request a review from our a11y folks as well.

One note about "Linking to about:preferences#privacy", this is not as easy as it might seem, since that page is not linkable by web content (and the about:protections page is sandboxed like web content), thus we need to ask the parent to open it for us.

Or we could relax the non-linkable restrictions for pages in the about page process, though that seems a bit much effort to support this feature.

Comment 3

[James Teh [:Jamie]]

(In reply to Johann Hofmann [:johannh] from comment #2)

Thank you for digging into that, Florens.

Yes indeed; thank you!

This is very helpful, though note that the protection report is still a bit WIP and things may change. Once it's a bit more stable, we plan to request a review from our a11y folks as well.

I'd encourage you to discuss this as soon as you think the UX is relatively stable, rather than waiting until everything lands in Nightly. Retrofitting a11y is always much harder than if it were considered from early in the implementation.

One note about "Linking to about:preferences#privacy", this is not as easy as it might seem, since that page is not linkable by web content (and the about:protections page is sandboxed like web content), thus we need to ask the parent to open it for us.

Given the technical challenge, I think we can get the same effect for a11y semantics using ARIA:

<p id="protection-details" role="link" tabindex="0" onClick="..." title="Go to Privacy settings">
  Protection Level is set to <span>Standard</span>
</p>

(I agree the user, whether or not they use assistive technology, should have some idea of what this link does and the title attribute seems like a good approach.)

Comment 4

[Johann Hofmann [:johannh]]

(In reply to James Teh [:Jamie] from comment #3)

(In reply to Johann Hofmann [:johannh] from comment #2)

This is very helpful, though note that the protection report is still a bit WIP and things may change. Once it's a bit more stable, we plan to request a review from our a11y folks as well.

I'd encourage you to discuss this as soon as you think the UX is relatively stable, rather than waiting until everything lands in Nightly. Retrofitting a11y is always much harder than if it were considered from early in the implementation.

Ok, fair enough, we'll try to pull you in for review sooner.

One note about "Linking to about:preferences#privacy", this is not as easy as it might seem, since that page is not linkable by web content (and the about:protections page is sandboxed like web content), thus we need to ask the parent to open it for us.

Given the technical challenge, I think we can get the same effect for a11y semantics using ARIA:

<p id="protection-details" role="link" tabindex="0" onClick="..." title="Go to Privacy settings">
  Protection Level is set to <span>Standard</span>
</p>

(I agree the user, whether or not they use assistive technology, should have some idea of what this link does and the title attribute seems like a good approach.)

That seems like a good approach, thank you!

Comment 5

[Marco Zehe (:MarcoZ)]

Note that the graphs themselves are very difficult to comprehend with a screen reader right now. They are just a number of numbers stringed together more or less. Usually, an alternative method for graphs is to use a data table to put these items into a structured format. That could be visually hidden, and the graphis be aria-hidden so the unstructured data doesn't clutter up the virtual buffer.

Note that I am back from sick leave, and you can (and should) lean heavily on me to iterate over this. This is probably the most important Skyline feature, and we do want this to be right for accessibility.

Comment 6

[Johann Hofmann [:johannh]]

(In reply to Marco Zehe (:MarcoZ) from comment #5)

Note that the graphs themselves are very difficult to comprehend with a screen reader right now. They are just a number of numbers stringed together more or less. Usually, an alternative method for graphs is to use a data table to put these items into a structured format. That could be visually hidden, and the graphis be aria-hidden so the unstructured data doesn't clutter up the virtual buffer.

Note that I am back from sick leave, and you can (and should) lean heavily on me to iterate over this. This is probably the most important Skyline feature, and we do want this to be right for accessibility.

Thanks for offering to help! We're currently in the last phase of feature work and I think we would generally be very happy to collaborate. We can keep this bug around as a meta bug for the different things we'd like to improve, for example a hidden data table for the report. Since you seem to have a good idea about how this would work, would you be interested in implementing this yourself and get feedback from our team? Erica and Micah will be out for some time in August, so we appreciate any help we can get. Otherwise feel free to file bugs and we'll triage them.

Comment 7

[Marco Zehe (:MarcoZ)]

(In reply to Johann Hofmann [:johannh] from comment #6)

Thanks for offering to help! We're currently in the last phase of feature work and I think we would generally be very happy to collaborate. We can keep this bug around as a meta bug for the different things we'd like to improve, for example a hidden data table for the report. Since you seem to have a good idea about how this would work, would you be interested in implementing this yourself and get feedback from our team? Erica and Micah will be out for some time in August, so we appreciate any help we can get. Otherwise feel free to file bugs and we'll triage them.

No, I am no longer doing engineering work as such, doing the over-arching QA side of things and advising on solutions, keeping track of all the different bits and pieces to make sure Skyline ships with most, if not all, new features accessible. So the engineering work should be done by your team, with me offering to review, test, even try builds, etc. while iterating.

Comment 8

[Liz Henry (:lizzard) (relman/hg->git project)]

Is this review still in progress? Or is it basically done?

Comment 9

[Marco Zehe (:MarcoZ)]

The review is done, and the bugs were filed. Most have been fixed, but some are still pending, see the dependencies.

Comment 10

[Liz Henry (:lizzard) (relman/hg->git project)]

OK, looks like only one is waiting for a fix but it's rated P2 so it doesn't block the feature release.
I'll just mark this "fixed" for 70 in case you want to leave it open as a meta bug for future issues.