ramnathv / htmlwidgets

HTML Widgets for R
http://htmlwidgets.org
Other
792 stars 205 forks source link

Add "aria-labelledby" attribute #469

Open dmurdoch opened 1 year ago

dmurdoch commented 1 year ago

... to default widget_html() output.

This will allow knitr to put fig_alt or fig_cap text on widgets as alternate text for accessibility purposes.

The text is added unconditionally because I can't think of any likely bad effects it could have. If used outside of knitr or with aknitr version prior to 1.42.12 it should have no effect unless there happens to be an HTML element with id equal to the widget id plus suffix "-aria", in which case that element will be the source of the alt text. That seems very unlikely, and mostly harmless.

Fixes #465 .

jcheng5 commented 1 year ago

Thanks, LGTM. @gtritchie, any thoughts?

gtritchie commented 1 year ago

@jcheng5

It is difficult to say without knowing the full context and seeing representative samples of it in use.

Generally, sprinkling in aria attributes is likely to make accessibility worse unless it's done very intentionally and in a way that complies with the strict guidelines of their usage. I can't tell from this tiny bit of code if that's the case.

First, of course, the id being referenced must exist, and that element must have accessible text. The id must be unique on the page, otherwise, it's a WCAG 2.1 violation, but probably cannot enforce that at this low level of the code. As the author says, if the id doesn't exist, it will just be ignored, but this will cause accessibility auditing tools to flag it as an issue.

Next, aria-labelledby is applied to a span. That isn't valid usage and may not work with some (possibly most) screen readers unless that span is explicitly given a valid interactive component role. More on that https://dequeuniversity.com/rules/axe/4.7/aria-allowed-attr?application=AxeChrome.

Another problem is if the "widget" is authored correctly such that it has its own accessible label using standard HTML (strongly preferred to using aria), that will be overridden by the text in the element referenced by aria-labelledby, rendering the inherent label invisible (at least as a label) to a screen reader user. Of course that can be worked around by passing in an id that doesn't exist, but this will generate noise during accessibility audits as I mentioned above. Not the end of the world but not ideal.

And so on.

Again, this may be totally fine, but there's no way to know just looking at this code.

dmurdoch commented 1 year ago

@gtritchie, thanks for the comments. I was unaware of the issues you mention.

The intention of the change was to get widgets to have alt text when used within documents produced by knitr. The issue there is that knitr allows fig.alt to specify alternate text, but that happens after the code for the widget is produced, so that kind of alt text can't possibly be output by htmlwidgets code, nor can the widget necessarily know if alt text will be applied later.

With a recently merged patch to knitr, it (i.e. knitr) now looks for aria-labelledby in the first HTML tag in the widget output, and if it is found, will generate an appropriate target. The rgl package is using this in its vignettes, but it needs the devel version of knitr to get the desired alt text; you can see output with default alt text in this vignette.

I'd be happy to modify the PR, but first I'd like to clarify one thing. You said "Another problem is if the "widget" is authored correctly such that it has its own accessible label using standard HTML (strongly preferred to using aria), that will be overridden by the text in the element referenced by aria-labelledby". Just to be sure: if the standard HTML is embedded within a div containing the aria-labelledby attribute, will the outer setting override the nested one?

gtritchie commented 1 year ago

I'm actually on vacation until Tuesday. I just tried that page you linked to using VoiceOver on iPhone and that plot isn't correctly labeled and is invisible to a screen reader user. Not sure if that was intended of an example of something that works.

Adding accessibility stuff to a web page without testing it via screen readers is equivalent to changing css without loading the page in a web browser to see how it looks. Sadly that's the norm in the industry and is why almost all web pages are inaccessible. It is depressing.

gtritchie commented 1 year ago

Sorry, that wasn't aimed at you, I'm just generally frustrated about the state of web accessibility.

dmurdoch commented 1 year ago

It was supposed to be something that works, though the text isn't the intended text -- that needs an unreleased version of knitr to be set properly. It should have text "3D plot" on all of the rgl plots.

For testing, I was using the "Inspect Accessibility Properties" popup in Firefox. I thought it looked okay there, but I guess I need better tests. I don't normally use a screen reader, but I'll see if I can figure VoiceOver out.

If I can't, could I bother you again next week?

gtritchie commented 1 year ago

If I can't, could I bother you again next week?

Absolutely, and sorry again for the cranky-sounding words.

dmurdoch commented 1 year ago

I've made things conditional now. ARIA support will be requested if the option htmlwidgets.USE_ARIA is TRUE and the widget has a use_aria argument. The widget_html.default function gains that argument and adds the aria-labelledby attribute to a div if requested, never to a span.

@gtrichie, you said VoiceOver didn't work on https://dmurdoch.github.io/rgl/dev/articles/rgl.html . I turned it on, and when I asked it to read the page, I heard "3D plot" at that spot. (That's the default alt text that rgl uses when knitr doesn't support this stuff.) Could you check again, and let me know what's wrong in more detail if it still fails?

dmurdoch commented 1 year ago

@gtritchie, sorry for the typo in your id in the previous comment.

gtritchie commented 1 year ago

VoiceOver still doesn't announce it on Safari on iOS. Easy to fix, though. Add an image role to the canvas, e.g.: <canvas role="img" ...>. Not all screen readers / browsers automatically give the canvas element a meaningful semantic role.

<canvas>, in general, is tricky to make accessible. Giving it the image role will at least make it announce consistently; I made that tweak and confirmed it announces on iOS, and also checked JAWS 2023, NVDA, and Narrator on Windows.

Other accessibility considerations in the future would include keyboard support; a keyboard-only user (e.g. mobility issues preventing use of pointing devices) cannot currently tab to that plot and rotate it, for example. If a keyboard interface was added, then the role="img" would not be appropriate. But that's a different kettle of... whatever is irritating to keep in a kettle.

Anyway, this looks good. It would be ideal to add that role="img" to the canvas, but I don't think that's part of this PR.

Thanks for considering accessibility!