-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Conversation
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 |
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); |
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 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.
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 added the as any
back in to reduce the lines changed in this diff.
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.
Fair enough! Probably doesn't make any actual difference once it's all transpiled to javascript and minimized.
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 |
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.
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. |
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. |
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.
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 |
Correct, this codepath is only reachable when
Will do later today. |
@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) |
Took the rambly section out of the description, I'll put it down here for posterity. Tentatively proposed more subtle behaviorLeave the current selection when passing new options if 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 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. |
I'm surprised this isn't the current behavior already, but on further investigation neither Thanks for the reprex, it was very helpful! I updated it a little bit as I investigated (here's my version on shinylive). Reprex applibrary(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 To reproduce this scenario in the reprex app:
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
Clicking the second button should invoke only one input update, from
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. |
Definitely a preexisting issue :-) #3755 Looks like multiple-selectizes send spurious NULLs, and single-selectizes send spurious empty strings. |
Oh right 🤦 Thanks for linking back to your open issue! Let's definitely tackle that separately. |
That is in fact the behavior this PR currently implements, I'll leave any further fixes or changes for separate PRs. |
Here are screen recordings showing the problem being solved by this PR: ProblemWhen Kapture.2024-12-06.at.14.44.37.mp4This PRKapture.2024-12-06.at.14.44.04.mp4Prior behavior in v1.7.5.1Kapture.2024-12-06.at.14.43.16.mp4 |
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 @dvg-p4!
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
Bigger reprex with a full matrix of possible select(ize) input types
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 newchoices
.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 newselected
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
.clearOptions()
should not call.clear()
.clearOptions()
take a silent parameter and pass it through to.clear()
.clear()
call at the end of the functionsilent
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:clear()
call at the end of the functionselectize.clearOptions()
, in which any items currently selected will be deselected, but not removed.selected
andsever = TRUE
#3967 attempts to work around the new behavior by adding an additionalselectize.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 insideclearOptions
.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