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

'shiny-bound-input' class automatically added to custom Shiny input html improperly #2779

Closed
leonawicz opened this issue Mar 4, 2020 · 11 comments · Fixed by #3861
Closed
Assignees

Comments

@leonawicz
Copy link

leonawicz commented Mar 4, 2020

Hi, I first asked this in this RStudio community forum post since I do not think it is necessarily a bug but rather by design- but it does have problematic unanticipated consequences. It doesn't feel like a feature request either. But it was suggested that I file an issue here.

For web accessibility reasons the custom Shiny input widgets I create need to meet certain requirements. Specifically, I have cases where I need to have unique id attributes for <input> tags within a larger Shiny input HTML block.

For example,

<fieldset id="myid" class="my-input-checkboxgrp">
  <legend>mylabel</legend>
  <ul class="my-options-group">
    <li class="my-option-item">
      <input type="checkbox" id="my-checkboxgroup-A" name="my-checkboxgroup-myid" value="A"></input>
      <label for="my-checkboxgroup-A">A</label>
    </li>
    <li class="my-option-item">
      <input type="checkbox" id="my-checkboxgroup-B" name="my-checkboxgroup-myid" value="B"></input>
      <label for="my-checkboxgroup-B">B</label>
    </li>
  </ul>
</fieldset>

The inputId of the Shiny widget is myid in the fieldset tag. Notice each individual checkbox within has an id. The problem seems to be that as soon as I give an input tag an id attribute, Shiny appends the shiny-bound-input class to it. So when the above widget is rendered in the browser by Shiny, it not only adds that class to the top level alongside myid, but also to each individual checkbox within.

So a custom Shiny input like above with five options will end up with class="shiny-bound-input" appended in six places. I need a way to prevent these additional class additions. It doesn't happen if I remove their id attributes, but I need to have the ids.

Is there a recommended way I can prevent or override this class appending behavior? The explicit and unambiguous id pairing is critical.

@jcheng5
Copy link
Member

jcheng5 commented Mar 4, 2020

Hmmm... perhaps Shiny should not try to bind inputs that are inside of other inputs? (The same cannot be said for outputs, which do have important recursive cases)

Oh, or at the very least, have a way for input bindings to opt into “protecting” their contents from further binding.

@leonawicz
Copy link
Author

I suppose it's possible other users could unknowingly be relying on the former? Though I'm not sure exactly how or why that would be the case. Maybe the latter is a safer option, but perhaps either is fine? This issue is something I am looking to resolve soon. If there is any way I can help with a PR for either approach let me know.

@jooyoungseo
Copy link

@leonawicz, would you mind verifying if you still have this issue?

I cannot reproduce this issue on my end. It would be greatly appreciated if you could share your reproducible R code with us.

Reproducible Code

library(shiny)

ui <- fluidPage(HTML('<fieldset id="myid" class="my-input-checkboxgrp">
  <legend>mylabel</legend>
  <ul class="my-options-group">
    <li class="my-option-item">
      <input type="checkbox" id="my-checkboxgroup-A" name="my-checkboxgroup-myid" value="A"></input>
      <label for="my-checkboxgroup-A">A</label>
    </li>
    <li class="my-option-item">
      <input type="checkbox" id="my-checkboxgroup-B" name="my-checkboxgroup-myid" value="B"></input>
      <label for="my-checkboxgroup-B">B</label>
    </li>
  </ul>
</fieldset>'))

server <- function(input, output) {

}

shinyApp(ui = ui, server = server)
#> 
#> Listening on http://127.0.0.1:3355

xfun::session_info("shiny")
#> R version 4.0.2 (2020-06-22)
#> Platform: x86_64-w64-mingw32/x64 (64-bit)
#> Running under: Windows 10 x64 (build 19041)
#> 
#> Locale:
#>   LC_COLLATE=English_United States.1252 
#>   LC_CTYPE=English_United States.1252   
#>   LC_MONETARY=English_United States.1252
#>   LC_NUMERIC=C                          
#>   LC_TIME=English_United States.1252    
#> system code page: 949
#> 
#> Package version:
#>   base64enc_0.1.3   BH_1.72.0.3       commonmark_1.7    crayon_1.3.4     
#>   digest_0.6.25     fastmap_1.0.1     glue_1.4.1        graphics_4.0.2   
#>   grDevices_4.0.2   htmltools_0.5.0   httpuv_1.5.4      jsonlite_1.7.0   
#>   later_1.1.0.1     magrittr_1.5      methods_4.0.2     mime_0.9         
#>   promises_1.1.1    R6_2.4.1          Rcpp_1.0.5        rlang_0.4.7      
#>   shiny_1.5.0.9001  sourcetools_0.1.7 stats_4.0.2       tools_4.0.2      
#>   utils_4.0.2       withr_2.2.0       xtable_1.8.4

Created on 2020-08-05 by the reprex package (v0.3.0.9001)

Rendering Results

<!DOCTYPE html>
<html>
<head>
  <meta http-equiv="Content-Type" content="text/html; charset=utf-8"/>
  <script type="application/shiny-singletons"></script>
  <script type="application/html-dependencies">json2[2014.02.04];jquery[3.5.1];shiny[1.5.0.9001];bootstrap[3.4.1]</script>
<script src="shared/json2-min.js"></script>
<script src="shared/jquery.min.js"></script>
<link href="shared/shiny.css" rel="stylesheet" />
<script src="shared/shiny.min.js"></script>
<meta name="viewport" content="width=device-width, initial-scale=1" />
<link href="shared/bootstrap/css/bootstrap.min.css" rel="stylesheet" />
<link href="shared/bootstrap/accessibility/css/bootstrap-accessibility.css" rel="stylesheet" />
<script src="shared/bootstrap/js/bootstrap.min.js"></script>
<script src="shared/bootstrap/accessibility/js/bootstrap-accessibility.min.js"></script>
</head>
<body>
  <div class="container-fluid"><fieldset id="myid" class="my-input-checkboxgrp">
  <legend>mylabel</legend>
  <ul class="my-options-group">
    <li class="my-option-item">
      <input type="checkbox" id="my-checkboxgroup-A" name="my-checkboxgroup-myid" value="A"></input>
      <label for="my-checkboxgroup-A">A</label>
    </li>
    <li class="my-option-item">
      <input type="checkbox" id="my-checkboxgroup-B" name="my-checkboxgroup-myid" value="B"></input>
      <label for="my-checkboxgroup-B">B</label>
    </li>
  </ul>
</fieldset></div>
</body>
</html>

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

I still get the original problem. While the browser page source shows the original HTML, when I inspect the elements in the browser console I can see how class="shiny-bound-input" is added to every checkbox within the widget.

In a realistic use case a Shiny input function generates this HTML and it has an accompanying Shiny input binding in javascript for my-input-checkboxgrp. This allows the whole widget (at the top level) to be reactive, and the input binding methods work on the internal parts. Something else, however, seems to be making each inner checkbox input separately reactive, as described above but I cannot figure out what. This happens even with your example where there it is just HTML, no input function or input binding involved.

@jcheng5
Copy link
Member

jcheng5 commented Aug 6, 2020

@leonawicz I have an idea of how to fix it but can you also tell me why the additional class causes a problem? There shouldn't be any style associated with the class.

@jcheng5
Copy link
Member

jcheng5 commented Aug 6, 2020

See also #2956 (comment)

@jcheng5
Copy link
Member

jcheng5 commented Aug 6, 2020

First crack at a fix in #2994

@jooyoungseo jooyoungseo assigned jcheng5 and unassigned jooyoungseo Aug 6, 2020
@jooyoungseo
Copy link

@leonawicz, please feel free to let me know if you need accessibility test. I am a screen reader user. I did not experience any noticeable challenges against screen readers when I tested the example code above. Since every each label and input is tied using id value, I am not quite sure what you meant by accessibility issue caused by the additional shiny class.

@leonawicz
Copy link
Author

leonawicz commented Aug 6, 2020

@jooyoungseo At my workplace we are creating our own Shiny inputs that meet various accessibility requirements we have. That is the motivation for widgets like in this example, but no, the additional injection of shiny-bound-input classes on internal elements does not cause an accessibility issue. It causes other app design and management issues.

On that topic, @jcheng5 , it becomes necessary at a minimum to make sure any internal reactive input id attributes are unique, not just within the widget but between multiple instances of this custom widget in an app, or other widgets. If options appear in multiple widgets and have the same ID, and Shiny is making them reactive, that can cause behavioral issues with the inputs. This means when people develop new widgets like this, they have to know they will need to do something like prepend the options IDs with the unique ID of the container, to ensure all the accidentally reactive inputs are unique. It seems like too much to expect the app author to be aware of.

It also causes inconveniences with things like bookmarking app state. If you put several checkbox groups in an app for example, suddenly there are a large number of reactive IDs being tracked that shouldn't be reactive. This can appear in a parameterized URL for example. Of course, these can be explicitly ignored by the bookmarking code, but this quickly becomes tedious and error prone providing a long list of reactive inputs, which kind of requires the person making an app and using a widget to have a level of familiarity with the internals of the widget the author had, and then to make the ad hoc accommodation uniquely in every app.

Thanks for that reference. I'll take a look at the initial fix as soon as I can! :)

@leonawicz
Copy link
Author

@jooyoungseo If you would like to test some of our widgets with a screen reader, I would greatly appreciate that! I could always reach out to you when we have a good example app containing each of our Shiny inputs. We are trying to meet stronger accessibility guidelines with checkboxes, radio buttons, etc. We've created a handful of typical inputs so far. We're trying to adhere to best practices but we also want to know users' actual experiences.

@jcheng5
Copy link
Member

jcheng5 commented Aug 6, 2020

@leonawicz Thanks for that explanation, that totally makes sense. I would add that HTML already forbids multiple elements having the same id within a single document (though like everything else HTML-related, browsers are totally lenient); if you can't guarantee uniqueness between unrelated checkboxes on the same page, consider using name.

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

Successfully merging a pull request may close this issue.

4 participants