-
Notifications
You must be signed in to change notification settings - Fork 208
Add "aria-labelledby" attribute #469
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
base: master
Are you sure you want to change the base?
Conversation
default widget_html() output.
|
Thanks, LGTM. @gtritchie, any thoughts? |
|
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 Next, 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 And so on. Again, this may be totally fine, but there's no way to know just looking at this code. |
|
@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 With a recently merged patch to 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? |
|
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. |
|
Sorry, that wasn't aimed at you, I'm just generally frustrated about the state of web accessibility. |
|
It was supposed to be something that works, though the text isn't the intended text -- that needs an unreleased version of 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? |
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.
|
I've made things conditional now. ARIA support will be requested if the option @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 |
|
@gtritchie, sorry for the typo in your id in the previous comment. |
|
VoiceOver still doesn't announce it on Safari on iOS. Easy to fix, though. Add an image role to the canvas, e.g.:
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 Anyway, this looks good. It would be ideal to add that Thanks for considering accessibility! |
There was a problem hiding this 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.
| do_use_aria <- function(knitrOptions) { | ||
| getOption("htmlwidgets.USE_ARIA") %||% | ||
| ("fig.alt" %in% names(knitrOptions) && | ||
| requireNamespace("knitr") && | ||
| packageVersion("knitr") >= "1.42.12") | ||
| } |
There was a problem hiding this comment.
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.
| 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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 inhtmlwidgetscode, 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.
| } | ||
|
|
||
| widget_html <- function(name, package, id, style, class, inline = FALSE, ...) { | ||
| widget_html <- function(name, package, id, style, class, inline = FALSE, use_aria = FALSE, ...) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
| 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.
| 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) { |
Co-authored-by: Carson Sievert <[email protected]>
... to default widget_html() output.
This will allow knitr to put
fig_altorfig_captext 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
knitror with aknitrversion prior to 1.42.12 it should have no effect unless there happens to be an HTML element withidequal 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 .