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

Work around a selectize bug (re-fixes #3966) #4142

Merged
merged 7 commits into from
Dec 6, 2024

Conversation

dvg-p4
Copy link
Contributor

@dvg-p4 dvg-p4 commented Oct 10, 2024

Calling selectize.clear() first works around selectize/selectize.js#2146: As of selectize.js >= v0.13.1, .clearOptions() clears the selection, but does NOT remove the previously-selected options. So unless we do a manual .clear() first, the current selection(s) will remain as (deselected) options.

Closes #3966. (Note that #3967 attempted to fix this bug, but erred slightly in putting the .clear() after the .clearOptions() instead of before.)

Reprex

Minimal reprex app

library(shiny)

ui <- fluidPage(
  fluidRow(
    column(width = 6,
      selectizeInput("sel", label = "Selectize:", choices = NULL,
      options = list(placeholder = "Make a selection...")),
      actionButton("set_lower", "Set options to a,b,c; selected = 'b'"),
      actionButton("set_upper", "Set options to A,B,C,D,E; selected = NULL"),
      actionButton("set_numbr", "Set options to 1,2,3; selected = ''"),
    ),
    column(width = 6,
      textOutput("out")
    )
  )
)

server <- function(input, output, session) {
  observeEvent(input$set_lower, {
    updateSelectizeInput(session, "sel", choices = letters[1:3], selected = "b", server = T)
  })
  observeEvent(input$set_upper, {
    updateSelectizeInput(session, "sel", choices = LETTERS[1:5], server = T)
  })
  observeEvent(input$set_numbr, {
    updateSelectizeInput(session, "sel", choices = 1:3, selected = "", server = T)
  })
  # Parrot the current selectize values
  output$out <- renderText({print(input$sel); paste(c("Selected values:", input$sel), collapse = " ")})
}

shinyApp(ui = ui, server = server)

Bigger reprex with a full matrix of possible select(ize) input types

library(shiny)

ui <- fluidPage(
  fluidRow(
    column(width = 4,
      selectInput("sel1", selectize = F, label = "Select:", choices = NULL),
      textOutput("out1"),
      br(),
      selectInput("msel1", selectize = F, label = "Multi-select:", choices = NULL, multiple = T, size = 2),
      textOutput("mout1"),
      br(),
    ),
    column(width = 4,
      selectizeInput("sel2", label = "Client-side selectize:", choices = NULL),
      textOutput("out2"),
      br(),
      selectizeInput("msel2", label = "Client-side multi-selectize:", choices = NULL, multiple = TRUE),
      textOutput("mout2"),
      br(),
    ),
    column(width = 4,
      selectizeInput("sel3", label = "Server-side selectize:", choices = NULL),
      textOutput("out3"),
      br(),
      selectizeInput("msel3", label = "Server-side multi-selectize:", choices = NULL, multiple = TRUE),
      textOutput("mout3"),
      br(),
    ),
  ),
  fluidRow(
    column(width = 8,
      actionButton("set_lower", "Set options to a,b,c; selected = 'b'"),
      actionButton("set_upper", "Set options to A,B,C,D,E; selected = NULL"),
      actionButton("set_numbr", "Set options to 1,2,3; selected = ''"),
    )
  ),
)

server <- function(input, output, session) {
  observeEvent(input$set_lower, {
    updateSelectInput(session, "sel1", choices = letters[1:5], selected = "b")
    updateSelectizeInput(session, "sel2", choices = letters[1:5], selected = "b", server = F)
    updateSelectizeInput(session, "sel3", choices = letters[1:5], selected = "b", server = T)
    updateSelectInput(session, "msel1", choices = letters[1:5], selected = "b")
    updateSelectizeInput(session, "msel2", choices = letters[1:5], selected = "b", server = F)
    updateSelectizeInput(session, "msel3", choices = letters[1:5], selected = "b", server = T)
  })
  observeEvent(input$set_upper, {
    updateSelectInput(session, "sel1", choices = LETTERS[1:5])
    updateSelectizeInput(session, "sel2", choices = LETTERS[1:5], server = F)
    updateSelectizeInput(session, "sel3", choices = LETTERS[1:5], server = T)
    updateSelectInput(session, "msel1", choices = LETTERS[1:5])
    updateSelectizeInput(session, "msel2", choices = LETTERS[1:5], server = F)
    updateSelectizeInput(session, "msel3", choices = LETTERS[1:5], server = T)
  })
  observeEvent(input$set_numbr, {
    updateSelectInput(session, "sel1", choices = 1:5, selected = "")
    updateSelectizeInput(session, "sel2", choices = 1:5, selected = "", server = F)
    updateSelectizeInput(session, "sel3", choices = 1:5, selected = "", server = T)
    updateSelectInput(session, "msel1", choices = 1:5, selected = "")
    updateSelectizeInput(session, "msel2", choices = 1:5, selected = "", server = F)
    updateSelectizeInput(session, "msel3", choices = 1:5, selected = "", server = T)
  })

  # Parrot the current selectize values
  output$out1 <- renderText({print(input$sel1); paste(c("Selected values:", input$sel1), collapse = " ")})
  output$out2 <- renderText({print(input$sel2); paste(c("Selected values:", input$sel2), collapse = " ")})
  output$out3 <- renderText({print(input$sel3); paste(c("Selected values:", input$sel3), collapse = " ")})
  output$mout1 <- renderText({print(input$msel1); paste(c("Selected values:", input$msel1), collapse = " ")})
  output$mout2 <- renderText({print(input$msel2); paste(c("Selected values:", input$msel2), collapse = " ")})
  output$mout3 <- renderText({print(input$msel3); paste(c("Selected values:", input$msel3), collapse = " ")})
}

shinyApp(ui = ui, server = server)

  1. Click any of the three buttons
  2. Select some options (or just leave 'b' selected if you clicked the first button)
  3. Click either of the other two buttons
  4. Examine the available options of the selectize

Current behavior (shiny 1.8.0 - 1.9.1 or current main)

For the client-side select(ize) inputs, the list of options will now only contain the new options.

For the server-side selectizes, the list of options will additionally contain whatever option(s) were previously selected, though it will now be deselected (regardless of what was passed as the selected parameter).

Desired behavior (this PR or shiny ≤ 1.7.5.1)

The server-side selectize behaves like the client-side selectize: currently-selected options are cleared and removed when updateSelectizeInput() is called with new choices.

Edge case to consider

An app might call updateSelectizeInput() with a set of choices that includes the current selection. Arguably, it might make sense in that case to not clear the current selection, leaving it selected if a new selected was not provided. However, this would be a change from past and current shiny behavior. Further discussion in comments below.

Timeline of bug

Tl;dr: `selectize.js` tried to implement two incompatible feature requests

  • Oct 2014: Someone files selectize.js #593 requesting that .clearOptions() should not call .clear()
  • Jun 2015: Someone files selectize.js #832 requesting that .clearOptions() take a silent parameter and pass it through to .clear()
  • Jun 2018: Selectize v0.12.5 implements the request not to clear the current selection with clearOptions, and notes it in the changelog. The docs remain ambiguous. The change consists of two parts:
    • Removing the .clear() call at the end of the function
    • Changes the logic in the middle of the function to only delete from the option list options that are not currently selected
  • Feb 2021: Selectize v0.13.1 attempts to implement the request to pass a silent parameter through to .clear(), and clarify the docs (erroneously specifying that currently-selected items are removed.) This change appears to have been made without accounting for the previous change in v0.12.5, since it:
    • Adds back the clear() call at the end of the function
    • Leaves the logic that only removes nonselected options
    • This introduces the current buggy behavior of selectize.clearOptions(), in which any items currently selected will be deselected, but not removed.
  • Nov 2023: Shiny updates its selectize dependency from v0.12.4 to v0.15.2 (across all the changes previously mentioned) in release v1.8.0
  • Mar 2024: fix(updateSelectizeInput): Clear current value before update if selected and sever = TRUE #3967 attempts to work around the new behavior by adding an additional selectize.clear() when a new input is specified, under the impression that selectize's behavior is intentional. Unfortunately the .clear() is done after the .clearOptions(), so it's redundant with the aforementioned .clear() that's already being done inside clearOptions.
    • This would have worked if clearOptions behaved as documented in the selectize v0.12.5 changelog, but that changelog lacks an entry for v0.13.1 with the further semi-intentional breaking change

@dvg-p4
Copy link
Contributor Author

dvg-p4 commented Oct 10, 2024

Might take a crack at extending this PR to fix some of my other selectize pet peeves...not sure I'll have the time though

@Roleren
Copy link

Roleren commented Oct 10, 2024

I think a lot of people want this feature, I personally don't have enough js experience to help fix this. I could try to reach out to some js friends if you do not have time to do it ? :)

@@ -187,7 +186,7 @@ class SelectInputBinding extends InputBinding {
callback(res);
if (!loaded) {
if (hasDefinedProperty(data, "value")) {
selectize.setValue(data.value as any);
selectize.setValue(data.value);
Copy link
Contributor Author

@dvg-p4 dvg-p4 Oct 10, 2024

Choose a reason for hiding this comment

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

I was trying to fix typescript "errors" here and accidentally committed to this branch--turns out it was actually just an issue with my vscode settings (see #4143) but I think this change is good anyways, since setValue expects a string and data.value is guaranteed to be a string at this point.

Copy link
Member

Choose a reason for hiding this comment

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

I added the as any back in to reduce the lines changed in this diff.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair enough! Probably doesn't make any actual difference once it's all transpiled to javascript and minimized.

@dvg-p4
Copy link
Contributor Author

dvg-p4 commented Oct 10, 2024

This PR should to be good to go as-is, it restores the behavior of shiny pre-1.8.0. Depending when the shiny maintainers can get around to reviewing (and whether workarounds that don't require patching selectize.js are possible) I'd like to take a crack at fixing #3400, #3732, and #3755, either in this PR or in a followup one.

@gadenbuie
Copy link
Member

This PR should to be good to go as-is, it restores the behavior of shiny pre-1.8.0.

Could you take a little more time to describe the issue, including a reprex, and to summarize how this behavior has changed over Shiny versions? The description in #3966 is helpful but nuanced and includes several threads; it'd make reviewing this PR easier to have a clear and concise summary of the problem being addressed.

I'd like to take a crack at fixing #3400, #3732, and #3755, either in this PR or in a followup one.

We'd certainly appreciate the PRs! Please try to keep PRs small and focused to a single change. Smaller PRs are generally easier to review and more likely to be merged.

@dvg-p4
Copy link
Contributor Author

dvg-p4 commented Oct 10, 2024

I was initially thinking it would be pointless to try to preserve the current selection, hence my removing the conditional logic, but it might actually be possible by just re-adding all available options (which should be all and only those previously selected) after the clearOptions. Will add that to this PR later today.

@gadenbuie
Copy link
Member

Thanks for adding the timeline of changes in selectize and the reprex. I don't want to overfocus on selectize's behavior, though. I think it's more important that Shiny's behavior is correct or consistent or at least defensible.

To that end, it would be very helpful if you could add a description of the undesired behavior that the reprex app demonstrates. Then explain how this PR changes that behavior.

...attempts work around the new behavior by adding an additional selectize.clear() when a new input is specified, under the impression that selectize's behavior is intentional

My memory of that PR is that this change had less to do with selectize's behavior and more to do with the fact that when a new input is specified it's safe to call selectize.clear() (safe as in clearly correct and without any downsides). I remember server = TRUE being an important component of this decision and that there's a asymmetrical information at play: when server = TRUE the client doesn't know everything that the server does.

@dvg-p4
Copy link
Contributor Author

dvg-p4 commented Oct 10, 2024

I remember server = TRUE being an important component of this decision

Correct, this codepath is only reachable when server = TRUE, it's gated by hasDefinedProperty(data, "url"). server = FALSE does something entirely different (specifically, just a one-line this.setValue(el, data.value)).

To that end, it would be very helpful if you could add a description of the undesired behavior that the reprex app demonstrates. Then explain how this PR changes that behavior.

Will do later today.

@dvg-p4
Copy link
Contributor Author

dvg-p4 commented Oct 14, 2024

@gadenbuie I've updated the description up top with a bigger reprex that compares the behaviors of all 6 possible select(ize) types, and the requested description of the current behavior vs. desired behavior, which accidentally kind of turned into a bit of a ramble as I changed my mind halfway through writing it. (will probably edit down tomorrow, I should go to sleep now)

@dvg-p4
Copy link
Contributor Author

dvg-p4 commented Oct 14, 2024

Took the rambly section out of the description, I'll put it down here for posterity.

Tentatively proposed more subtle behavior

Leave the current selection when passing new options if selected is left NULL. (Clearing can still be accomplished by passing an empty string or character(0).)

This seems more in line with what shiny #3967 and selectize v0.12.5 were trying to accomplish. This would be most useful when trying to update the options to a superset of the current options, while leaving the user's current selection untouched. (This can be sort-of accomplished with server-side logic, but won't work robustly if the user is actively changing their selection when the server is trying to update the options.)

On second thought though, that sort of flow is probably already handled in most cases just by using server-side selectize in the first place. And it would either introduce a further discrepancy between server- and client-side selectizes; or introduce a breaking change to client-side updateSelectizeInput(). Let me take a look across the issues tomorrow morning and see if anyone's actually asking for this sort of thing, and across our code to see if this would even be helpful to me.

I finished up a further commit implementing this, but at this point I'm thinking I probably won't push it to this PR. Saving as a separate branch...here (diff with this PR branch) for posterity.

@gadenbuie
Copy link
Member

Leave the current selection when passing new options if selected is left NULL.

I'm surprised this isn't the current behavior already, but on further investigation neither updateRadioButtons() nor updateCheckboxGroup() work this way either. They have similar behavior to the current selectize, which I think can be summarized as "updating choices re-initializes the selection unless selected is explicitly provided".

Thanks for the reprex, it was very helpful! I updated it a little bit as I investigated (here's my version on shinylive).

Reprex app
library(shiny)
library(bslib)

ui <- page_sidebar(
  title = "PR 4142",
  fillable = FALSE,
  sidebar = sidebar(
    actionButton("set_lower", "Set options to a,b,c; selected = 'b'"),
    actionButton("set_upper", "Set options to A,B,C,D,E; selected = NULL"),
    actionButton("set_upper_sel", "Set options to A,B,C,D,E,F; selected = 'C'"),
    actionButton("set_numbr", "Set options to 1,2,3; selected = ''"),
    input_switch("server_side", "Server Side", TRUE)
  ),
  selectizeInput(
    "sel",
    label = "Selectize:",
    choices = NULL,
    options = list(placeholder = "Make a selection...")
  ),
  div(
    verbatimTextOutput("debug"),
    actionButton("clear", "Clear log")
  ),
  hr(),
  checkboxGroupInput("chk", "Checkbox Group Input", inline = TRUE),
  radioButtons("rad", "Radio Buttons", choices = "", inline = TRUE),
)

server <- function(input, output, session) {
  observeEvent(input$set_lower, {
    updateSelectizeInput(session, "sel", choices = letters[1:3], selected = "b", server = input$server_side)
    updateCheckboxGroupInput(session, "chk", choices = letters[1:3], selected = "b")
    updateRadioButtons(session, "rad", choices = letters[1:3], selected = "b")
  })

  observeEvent(input$set_upper, {
    updateSelectizeInput(session, "sel", choices = LETTERS[1:5], server = input$server_side)
    updateCheckboxGroupInput(session, "chk", choices = LETTERS[1:5])
    updateRadioButtons(session, "rad", choices = LETTERS[1:5])
  })

  observeEvent(input$set_upper_sel, {
    updateSelectizeInput(session, "sel", choices = LETTERS[1:6], selected = "C", server = input$server_side)
    updateCheckboxGroupInput(session, "chk", choices = LETTERS[1:6], selected = "C")
    updateRadioButtons(session, "rad", choices = LETTERS[1:6], selected = "C")
  })

  observeEvent(input$set_numbr, {
    updateSelectizeInput(session, "sel", choices = 1:3, selected = "", server = input$server_side)
    updateCheckboxGroupInput(session, "chk", choices = 1:3, selected = "")
    updateRadioButtons(session, "rad", choices = 1:3, selected = "")
  })

  changes <- reactiveVal()

  observeEvent(input$clear, changes(NULL))

  observeEvent(input$sel, {
    withr::local_envvar(list(NO_COLOR = TRUE))
    x <- cli::format_inline("Selected values: {.val {input$sel}}")
    message(x)
    changes(c(changes(), x))
  })

  observeEvent(input$server_side, {
    changes(c(changes(), paste("Server side:", input$server_side)))
  })

  output$debug <- renderPrint({
    req(changes())
    cat(changes(), sep = "\n")
  })
}

shinyApp(ui = ui, server = server)

Given that selected = NULL doesn't attempt to retain the current selection, I think it's very reasonable for updateSelectizeInput() to clear and completely forget the current selection when provided new choices when server = TRUE. In other words, if the options are initially c("a", "b", "c") with "b" selected and the input is updated to c("A", "B", ..., "E"), then "b" should not appear in the choices in the UI. If "b" is a valid choice it will eventually show up in the UI.

To reproduce this scenario in the reprex app:

  1. Click the first button, observe "b" is selected.
  2. Click the second button, observe "b" is not selected but is still a choice.

I'd be happy with this PR solving this specific problem.


There's a secondary problem I've noticed in this reprex, which is that updating the choices of a selectize input with server = TRUE temporarily and very briefly sets the value of the input to "" before the new value is set.

  1. Click the first button, "b" is selected.
  2. Click the second button, after which "A" is selected.

Clicking the second button should invoke only one input update, from "b" to "A". However, there's an intermediate "" value sent:

Selected values: "b"
Selected values: ""
Selected values: "A"

I'm not sure where that's being introduced or if it'd be better to solve in a second PR or if it'd be easy and logical to fix at the same time as the core issue in this PR.

@dvg-p4
Copy link
Contributor Author

dvg-p4 commented Oct 15, 2024

There's a secondary problem I've noticed in this reprex, which is that updating the choices of a selectize input with server = TRUE temporarily and very briefly sets the value of the input to "" before the new value is set.

Definitely a preexisting issue :-) #3755

Looks like multiple-selectizes send spurious NULLs, and single-selectizes send spurious empty strings.

@gadenbuie
Copy link
Member

Definitely a preexisting issue :-) #3755

Oh right 🤦 Thanks for linking back to your open issue! Let's definitely tackle that separately.

@dvg-p4
Copy link
Contributor Author

dvg-p4 commented Oct 24, 2024

Given that selected = NULL doesn't attempt to retain the current selection, I think it's very reasonable for updateSelectizeInput() to clear and completely forget the current selection [...] I'd be happy with this PR solving this specific problem.

That is in fact the behavior this PR currently implements, I'll leave any further fixes or changes for separate PRs.

@gadenbuie gadenbuie added this to the Next Release milestone Nov 13, 2024
@jcheng5 jcheng5 requested a review from gadenbuie December 6, 2024 18:19
@gadenbuie
Copy link
Member

Here are screen recordings showing the problem being solved by this PR:

Problem

When choices are changed for server=TRUE selectize, the selected value is retained as a choice.

Kapture.2024-12-06.at.14.44.37.mp4

This PR

Kapture.2024-12-06.at.14.44.04.mp4

Prior behavior in v1.7.5.1

Kapture.2024-12-06.at.14.43.16.mp4

Copy link
Member

@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 @dvg-p4!

@gadenbuie gadenbuie merged commit b6bcfc8 into rstudio:main Dec 6, 2024
11 of 12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants