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

Add "aria-labelledby" attribute #469

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

dmurdoch
Copy link

... 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 .

default widget_html() output.
@jcheng5
Copy link
Collaborator

jcheng5 commented May 25, 2023

Thanks, LGTM. @gtritchie, any thoughts?

@gtritchie
Copy link

gtritchie commented May 25, 2023

@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
Copy link
Author

@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?

@dmurdoch dmurdoch marked this pull request as draft May 25, 2023 16:24
@gtritchie
Copy link

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
Copy link

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

@dmurdoch
Copy link
Author

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
Copy link

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

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

…ption if present, otherwise in recent version of `knitr` with `fig.alt` in the chunk options.
@dmurdoch dmurdoch marked this pull request as ready for review May 31, 2023 12:16
@dmurdoch
Copy link
Author

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
Copy link
Author

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

@gtritchie
Copy link

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!

Copy link
Collaborator

@gadenbuie gadenbuie left a comment

Choose a reason for hiding this comment

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

Thanks for this PR @dmurdoch! I've been following along at a distance while you were working out the general approach. I have just a couple comments that are more focused on the code and implementation in htmlwidgets.

R/htmlwidgets.R Show resolved Hide resolved
R/htmlwidgets.R Outdated
Comment on lines 170 to 175
do_use_aria <- function(knitrOptions) {
getOption("htmlwidgets.USE_ARIA") %||%
("fig.alt" %in% names(knitrOptions) &&
requireNamespace("knitr") &&
packageVersion("knitr") >= "1.42.12")
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is entirely a stylistic nit, but I'd rather break the tests into individual lines and more clearly call out the return value associated with the results of each test.

Suggested change
do_use_aria <- function(knitrOptions) {
getOption("htmlwidgets.USE_ARIA") %||%
("fig.alt" %in% names(knitrOptions) &&
requireNamespace("knitr") &&
packageVersion("knitr") >= "1.42.12")
}
do_use_aria <- function(knitrOptions = NULL) {
opt <- getOption("htmlwidgets.use_aria")
if (!is.null(opt)) return(opt)
if (!requireNamespace("knitr", quietly = TRUE)) return(FALSE)
if (packageVersion("knitr") <= "1.42.12") return(FALSE)
"fig.alt" %in% names(knitrOptions)
}

Refactoring in this way brings up a question: does it make sense for this function to return TRUE if fig.alt isn't part of knitrOptions? Similarly, does it make sense to force use_aria even if we have an outdated version of knitr?

Copy link
Author

Choose a reason for hiding this comment

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

The %||% operator is used fairly frequently in htmlwidgets code, so I think it's reasonable to use it here.

knitr will use fig.cap as the alt label if fig.alt isn't specified but the widget asks for alt text by including the aria-labelledby attribute. This shouldn't be the default, but I think users should be able to ask for it.

Similarly, an htmlwidget might be used in some context other than knitr, so I think it should be possible to force the attribute.

Copy link
Collaborator

@gadenbuie gadenbuie Jun 20, 2023

Choose a reason for hiding this comment

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

Thanks for the answers; I can see how forcing use_aria could be useful. In addition to the global R option, we could also consult a knitr chunk option, e.g. use_aria, which would more easily allow users to set the option at the chunk or document level.

The %||% operator is used fairly frequently in htmlwidgets code, so I think it's reasonable to use it here.

My comment wasn't so much about %||% as the entire expression. htmlwidgets is maintained by a reasonably large number of people and it's worth the tradeoff to be as readable as possible. I'm much more confident that future contributors will understand and correctly update the refactored version than if all of the tests are performed in a single expression. For example, it's much easier to correctly introduce a new check, e.g. for a use_aria chunk option, in the more verbose version than in the single expression version.

@@ -257,18 +264,22 @@ lookup_widget_html_method <- function(name, package) {
list(fn = widget_html.default, name = "widget_html.default", legacy = FALSE)
}

widget_html <- function(name, package, id, style, class, inline = FALSE, ...) {
widget_html <- function(name, package, id, style, class, inline = FALSE, use_aria = FALSE, ...) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it make sense for use_aria to follow the htmlwidgets.use_aria option?

Copy link
Author

Choose a reason for hiding this comment

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

I don't think so. It might make sense to use some test like the do_use_aria test, but at this point we can't see the knitr options.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Well, the knitr options aren't strictly necessary because the global R option is checked before the knitr options. So it'd be reasonable to do either

Suggested change
widget_html <- function(name, package, id, style, class, inline = FALSE, use_aria = FALSE, ...) {
widget_html <- function(name, package, id, style, class, inline = FALSE, ..., use_aria = getOption("htmlwidgets.USE_ARIA", FALSE)) {

or to default to use_aria = NULL and then internally call do_use_aria()

  args$use_aria <- use_aria %||% do_use_aria()

In either case, I'd prefer to add use_aria after the ..., since we're extending the original function signature.

Suggested change
widget_html <- function(name, package, id, style, class, inline = FALSE, use_aria = FALSE, ...) {
widget_html <- function(name, package, id, style, class, inline = FALSE, ..., use_aria = FALSE) {

R/htmlwidgets.R Outdated Show resolved Hide resolved
Co-authored-by: Carson Sievert <cpsievert1@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Accessibility support
5 participants