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

Consider making Shiny's default binding inputs less inclusive #2956

Closed
sigmapi opened this issue Jun 30, 2020 · 3 comments · Fixed by #3861 · May be fixed by #2994
Closed

Consider making Shiny's default binding inputs less inclusive #2956

sigmapi opened this issue Jun 30, 2020 · 3 comments · Fixed by #3861 · May be fixed by #2994
Assignees
Labels
edition Involves changing default behavior and therefore would make sense for editions

Comments

@sigmapi
Copy link

sigmapi commented Jun 30, 2020

While I'm developing custom Shiny inputs, I've noticed that some Shiny's default binding inputs are too inclusive, as in find method they search for every input element in the page:
For example, look at checkboxInputBinding find method:

  find: function(scope) {
    return $(scope).find('input[type="checkbox"]');
  },

It tries to register a binding for every checkbox input element.
The behavior is similar for textInputBinding, numberInputBinding, passwordInputBinding, selectInputBinding, textareaInputBinding

On the other hand there are other input bindings that look for a custom class, for example radioInputBinding:

  find: function(scope) {
    return $(scope).find('.shiny-input-radiogroup');
  },

I believe that this is the proper way to implement the other bindings above, always using a custom class.

Of course you could argue that we could create an input binding that takes over these elements, with each own find implementation and Shiny will not create additional input.

But the problems occur when we try to create a custom Shiny input, where the html element that controls the Shiny input, is ancestor of the html element that Shiny takes control over.
For example consider a custom checkbox input binding:

<div class="custom-checkbox-input" id="scales">
  <input type="checkbox" id="scalesChk"
         checked>
  <label for="scalesChk">Scales</label>
</div>

While my custom input binding will operate on the outer div element, Shiny will create an additional input or the checkbox input.

Similar situation occurs when using a custom control that uses a text input for example.

Let's look at textInputBinding find method:

  find: function(scope) {
    var $inputs = $(scope).find('input[type="text"], input[type="search"], input[type="url"], input[type="email"]');
    // selectize.js 0.12.4 inserts a hidden text input with an
    // id that ends in '-selectized'. The .not() selector below
    // is to prevent textInputBinding from accidentally picking up
    // this hidden element as a shiny input (#2396)
    return $inputs.not('input[type="text"][id$="-selectized"]');
  },

Here you can see the problematic situation I described.
Selectize.js inserts a hidden text input and before the fix, Shiny was creating an additional input.
That was fixed internally, but this solution is far from elegant and it does not deal with external/custom Shiny inputs that face the same problem.

All this would be fixed if a custom class is added to these problematic inputs so that they are less inclusive.
I could make a PR if you are interested.

@jcheng5
Copy link
Member

jcheng5 commented Jul 1, 2020

Thanks for the feedback. I agree with this. The earliest conception of Shiny involved app authors writing raw HTML directly, and in an effort to make that HTML as concise as possible, I made it so that explicit classes were not required. Given how Shiny turned out, this was the wrong choice but we've left this behavior for backward compatibility reasons.

Going forward I could imagine a couple of ways to improve the situation without breaking backward compatibility.

  1. Add a special classname that stops an element (or a whole subtree of the document) from being bound. This could either apply only to these basic inputs, or alternatively, could apply to ALL input/output bindings. Obviously this would be opt-in.
  2. Put in an option that turns off binding of these inputs unless they have more explicit classnames; turn the option on in this year's edition of Shiny. (An "edition" is like a basket of default behaviors, that you explicitly opt into in your Shiny app's source code. See testthat 3e r-lib/testthat#1027)

@jcheng5 jcheng5 added the edition Involves changing default behavior and therefore would make sense for editions label Jul 1, 2020
@sigmapi
Copy link
Author

sigmapi commented Jul 1, 2020

Thanks, as a reference my current workaround (after many attempts) is this:

  • In the initialize method of my custom input binding, I define the property 'data-input-id' in the input element that I want to exclude, with value equal to the id of the custom input binding. In that way Shiny.bindInputs will reject it as it will be already bound.
    That normally would be the attribute 'data-input-id' but bug InputBinding.getId tries to read element's property 'data-input-id', instead of attribute #2955 forces me to use the property definition.
    But when that will be fixed, using the attribute, the following html would create just one input binding:
<div class="custom-checkbox-input" id="scales">
  <input type="checkbox" id="scalesChk" data-input-id="scales"
         checked>
  <label for="scalesChk">Scales</label>
</div>

That's similar to your first proposal. To that context, alternatively of a class, it could be just a simple flag attribute, like data-shiny-ignore, where its presence would exclude the binding:

<div class="custom-checkbox-input" id="scales">
  <input type="checkbox" id="scalesChk" data-shiny-ignore
         checked>
  <label for="scalesChk">Scales</label>
</div>

That approach would require minimal changes and only to the Javascript code of the input bindings (or perhaps just in bindInputs).

On the other hand your second proposal feels closer to the correct approach: using custom classes.
But I'm not sure how the edition check will work with Javascript code. Is that doable?

@zsigmas
Copy link

zsigmas commented Mar 14, 2023

I am having a similar issue, in which I am creating several select tags inside a single custom input. The selects are created in the initialize method (according to data-init-choices) and the values are gathered in the getValue method. See below both pieces of code.

Therefore, I was wondering if there is any official support for the data-input-id (thanks a lot @sigmapi for the method! 👍 ) approach as the PR solving this issue seems to be stalled. Just as an additional workaround, at least in my particular case, if the select tag does not contain id or name it seems to be ignored by shiny when binding. Then an alternative attribute, e.g., data-name can be used instead of id and name.

Thanks a lot for your help!


$(function() {
  // Input binding
  let myInputBinding = new Shiny.InputBinding();
  
  $.extend(myInputBinding, {
    initialize: function(el) {
      console.log("Initializing")
      let choices = JSON.parse($(el).attr("data-init_choices"));
      let ns = $(el).attr("id")
      for(let k in choices){
	    let subel = $(`<select name=${k} data-input-id = ${ns}></select>`).append()
	    let v = choices[k]
	    v.map((x)=>$(subel).append(`<option value = ${x}>${x}</option>`))
	    $(el).append(subel)
}
    },
    find: function(scope) {
      return $(scope).find('.hier_select');
    },
    getValue: function(el) {
      let selection = {}
      $(el).children("select").each(function(x){
        selection[$(this).attr("name")] = $(this).val()
      })
      return JSON.stringify(selection)
    },
    getType: function(el) {
    return "hier_select.listOutput";
    },
    subscribe: function(el, callback) {
      $(el).on("change.hier_select select", function(event){
        callback();
      })
    }
  });
  Shiny.inputBindings.register(myInputBinding, "my_inputs.hier_select");
});
<div class="form-group shiny-input-container">
  <div id="my_input" class="hier_select shiny-bound-input" data-init_choices="{'A':['a'],'B':['b','h','j'],'C':['c']}">
    <select name="A" data-input-id="my_input">
      <option value="a">a</option>
    </select>
    <select name="B" data-input-id="my_input">
      <option value="b">b</option>
      <option value="h">h</option>
      <option value="j">j</option>
    </select>
    <select name="C" data-input-id="my_input">
     <option value="c">c</option>
    </select>
  </div>
</div>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
edition Involves changing default behavior and therefore would make sense for editions
Projects
None yet
4 participants