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 ALT to imageOutput/plotOutput #2494

Closed
wants to merge 9 commits into from

Conversation

trafficonese
Copy link

@trafficonese trafficonese commented Jun 14, 2019

Optional parameter to add alt attribute to img-tag in renderPlot and renderCachedPlot.

closes #612

Shiny-App to Test it

library(shiny)
ui <- fluidPage(
  fluidRow(
    column(4,
      sliderInput("obs", "Number of observations:",
                  min = 1, max = 1000, value = 500)
    ),
    column(8,
      plotOutput("distPlot"),
      plotOutput("distPlotALT"),
      plotOutput("plotCach"),
      plotOutput("plotCachALT")
    )
  )
)
server <- function(input, output, session) {
  output$distPlot <- renderPlot(alt="alternative text", {
    hist(rnorm(input$obs))
  })
  output$distPlotALT <- renderPlot({
    hist(rnorm(input$obs))
  })
  
  output$plotCach <- renderCachedPlot({
    Sys.sleep(2)  # Add an artificial delay
    seqn <- seq_len(input$obs)
    plot(mtcars$wt[seqn], mtcars$mpg[seqn],
         xlim = range(mtcars$wt), ylim = range(mtcars$mpg))
  },
  cacheKeyExpr = { list(input$obs) }
  )
  output$plotCachALT <- renderCachedPlot(alt="alternative text", {
    Sys.sleep(2)  # Add an artificial delay
    seqn <- seq_len(input$obs)
    plot(mtcars$wt[seqn], mtcars$mpg[seqn],
         xlim = range(mtcars$wt), ylim = range(mtcars$mpg))
  },
  cacheKeyExpr = { list(input$obs) }
  )
}
shinyApp(ui, server)

Optional parameter to add "alt" attribute to img-tag
Copy link
Collaborator

@wch wch left a comment

Choose a reason for hiding this comment

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

I think it makes more sense to put the alt argument on plotOutput and imageOutput, given that the alt text is not dynamic. Then it also does not need to be added to renderPlot, renderCachedPlot and renderImage.

NEWS.md Outdated Show resolved Hide resolved
R/render-cached-plot.R Outdated Show resolved Hide resolved
R/render-cached-plot.R Outdated Show resolved Hide resolved
@trafficonese
Copy link
Author

But in plotOutput / imageOutput I don't have access to the img tag just the surroundig div or?

I think it makes more sense to put the alt argument on plotOutput and imageOutput, given that the alt text is not dynamic. Then it also does not need to be added to renderPlot, renderCachedPlot and renderImage.

@wch
Copy link
Collaborator

wch commented Jun 19, 2019

Yes, this would require adding data to the div and then on the JavaScript side, adding the alt property to the img tag. That would be in srcjs/output_binding_image.js.

@trafficonese trafficonese changed the title Add ALT to renderPlot/renderCachedPlot Add ALT to imageOutput/plotOutput Jun 26, 2019
@CLAassistant
Copy link

CLAassistant commented Oct 2, 2019

CLA assistant check
All committers have signed the CLA.

@trafficonese
Copy link
Author

I'm having problems resolving the JS-checks.

I did run yarn clean and yarn build. I then rebuild the package and submitted the changes.
Is that the right procedure? Or should I not re-build the package after yarn build?

Another problem (on Windows 10) is that the build process errors with this message:

..\srcjs\_start.js
  9:20  error  Parsing error: Unterminated string constant

   7 |   var exports = window.Shiny = window.Shiny || {};
   8 |
>  9 |   exports.version = "1.4.0.9001
     |                     ^
  10 | ";  // Version number inserted by Grunt

I have to manually insert the version and then run build.

@trafficonese trafficonese requested a review from wch December 18, 2019 14:53
@leonawicz
Copy link

@wch I'd like to add that I was working on some wrapper functions to apply dynamic alt text on the server side and came across this issue and PR. I routinely get requests from people for dynamic alt text in Shiny apps. The static option is a bare minimum, but I think it makes more sense and is more critical to expose this attribute in the render* functions so it can be dynamic.

Technically, dynamic alt text can be achieved directly with renderImage() already, though contextually there is less reason to need dynamic alt text for images (but it's not difficult to find a need for this with a collection of images). For dynamic plots though, the same cannot be achieved with renderPlot(), which is the context where dynamic alt text is often very useful and important in Shiny apps.

shinyApp(
  ui = fluidPage(
    actionButton("btn1", "Update plots"),
    plotOutput("plot1", width = "400px"),
    imageOutput("img1")
  ),
  server = function(input, output, session){
    output$plot1 <- renderPlot({
      list(hist(rnorm(30)), alt = "Can't do this obviously.")
    })
    output$img1 <- renderImage({
      input$btn1
      x <- rnorm(30)
      alttext <- paste("Dynamic alt text. Mean(x):", round(mean(x), 3))
      outfile <- tempfile(fileext = ".png")
      png(outfile, width = 400, height = 400)
      hist(x)
      dev.off()
      list(src = outfile, contentType = "image/png", alt = alttext)
    })
  }
)

Something like an alt argument to renderPlot() and imagePlot() makes the most sense to me. Without this option, I would use observers and attach dynamic alt text separately to any number of plot ID tags in an app, but that seems like not a good use of resources. But the static text option in the *output() functions alone doesn't address a key accessibility need for dynamic content. It forces alt text to be less informative or complete than should be necessary for many plots. Any chance this might get turned around in shiny? Otherwise I can continue working on my own dynamic option separately.

@schloerke
Copy link
Collaborator

schloerke commented Mar 18, 2020

Ideas of things we could support...

@leonawicz @olyerickson Are there any other tag attributes that should also be dynamic? (Or only just alt for images?) Getting these to be dynamic would be good to have.

@schloerke schloerke added this to the 1.6 milestone Mar 18, 2020
@schloerke schloerke self-requested a review March 18, 2020 20:14
Copy link
Collaborator

@schloerke schloerke left a comment

Choose a reason for hiding this comment

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

Requesting Changes to block merging. The idea is good, but we should look to take more dynamic arguments from within renderPlot.

It might still be good to have the ... passed through in the plotOutput and imageOutput, but know that they could be overwritten by the result of a renderPlot execution.

// Set optional alt-attribute
var alttext = $el.attr("alt");
if (alttext && alttext !== "") {
img.setAttribute("alt", alttext);
Copy link
Collaborator

@schloerke schloerke Mar 18, 2020

Choose a reason for hiding this comment

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

Since alt exists, let's remove it from $el. (The alt attribute is only allowed for img tags)

@olyerickson
Copy link

olyerickson commented Mar 18, 2020

As I understand it, ultimately plots are embedded as img. Our objective is to make it easy to tag Shiny plots in a couple ways:

  • If the plot is stand-alone, specify the alt text for the plot
  • If the plot one of a group, use tags$div to specify role="img" and aria-label="The text" for the group

For details on acceptable "grouping" of img tags, see esp. https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/Role_Img

UPDATE: I see you are already are looking at the PSU guidelines; thanks!
https://accessibility.psu.edu/images/imageshtml/

@leonawicz
Copy link

@schloerke @olyerickson

longdesc is no longer supported

The other suggestions seem good to me if I understand correctly. arialabelledby is also helpful, but the user would want to pair that with an id elsewhere.

@jooyoungseo
Copy link

@leonawicz,

I was wondering if you were working on a separate PR for dynamic alt text.

If so, please kindly make a PR for review when you are ready.

@leonawicz
Copy link

@jooyoungseo My suggestion to accommodate server-side dynamic control rather than a static text option is still relevant, but no I am not working on this or other related PR.

@jooyoungseo
Copy link

Thanks very much for your response, @leonawicz.

@trafficonese, if you don't mind, I will do the rest of the house-cleaning stuff.

@itsozz
Copy link

itsozz commented Aug 14, 2020

Having recently read this thread in relation to improving the accessibility of my team's Shiny apps, we've published the following article on Medium: Alt text for dynamic plots in Shiny in case it is useful to the discussion.

It uses the technique @leonawicz mentions regarding observers and plot ids.

@jooyoungseo jooyoungseo self-assigned this Aug 14, 2020
@leonawicz
Copy link

@jooyoungseo @itsozz A colleague and I also have a package we've been working on here and there called shinyaccess. We haven't been able to work on it for a while but please feel free to check it out.

While I would love for this package to grow to offer as much accessibility and inclusive design as may be possible, I think what would be best for the community in the end is for something like this to be largely redundant with respect to a future version of the shiny package; perhaps for this to become an "opinionated design/alternative collection of Shiny widgets" package. But for now we just want to add as much accessibility features and considerations that we find lacking in the base shiny package. Maybe it's not feasible in the near term for the shiny package to accommodate everything in terms of accessibility on its own, given things like backward compatibility requirements etc., I really don't know.

@jooyoungseo
Copy link

Thanks very much for the great work, @leonawicz.

Ideally, we want to make Shiny accessible itself, and I have been working on each widget for accessibility. Some widgets need to be patched and tweaked around. For example, we have just made selectInput() accessible via #2993. The next target is sliderInput(), which we will have to patch IonRangeSlider.js for accessibility improvements. Please take a look at our NEWS.md for recent accessibility improvements. It would be greatly appreciated if you could contribute to this movement either via issues or PRs.

@itsozz
Copy link

itsozz commented Aug 17, 2020

@leonawicz, I'll have a look at shinyaccess as soon as I can, thanks for letting me know. Sounds like a very useful package.

@jooyoungseo, great to hear about the accessibility improvements being made to Shiny, thanks for the update. I briefly mentioned the contrast and font size issues with sliderInput() in my article. Would this be useful adding as a comment to PR #3003?

@jooyoungseo
Copy link

I briefly mentioned the contrast and font size issues with sliderInput() in my article. Would this be useful adding as a comment to PR #3003?

Yes, it would be greatly appreciated if you could add a comment to the PR.

@jooyoungseo jooyoungseo mentioned this pull request Aug 18, 2020
@jooyoungseo
Copy link

I am closing this PR in favor of another follow-up PR (#3006).

Thank you very much everyone for all the great input.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

request: pass ALT text to renderPlot
9 participants