Fix multiselect in ComboBox

[jakubsob]

Changes

Closes #160

How to test

library(shiny)
library(shiny.fluent)

shinyApp(
  ui = fluentPage(
    ComboBox.shinyInput(
      inputId = "multiselect",
      multiSelect = TRUE,
      options = list(
        list(key = "A", text = "A"),
        list(key = "B", text = "B"),
        list(key = "C", text = "C")
      )
    ),
    ComboBox.shinyInput(
      inputId = "singleselect",
      multiSelect = FALSE,
      options = list(
        list(key = "A", text = "A"),
        list(key = "B", text = "B"),
        list(key = "C", text = "C")
      )
    )
  ),
  server = function(input, output, session) {

  }
)

[kamilzyla]

It works, thanks! 😁 🎉

Please run yarn webpack and commit the compiled JS.

[kamilzyla]

The code in this if branch is very similar to the code in Dropdown below. Perhaps we can make it a function? (Arguments: props.options, option; return newValue).

[jakubsob]

Additional test app to also test Dropdown

library(shiny)
library(shiny.fluent)

options <- list(
  list(key = "A", text = "Option A"),
  list(key = "B", text = "Option B"),
  list(key = "C", text = "Option C")
)

ui <- function(id) {
  ns <- NS(id)
  div(
    ComboBox.shinyInput(
      ns("combo"),
      options = options,
      multiSelect = TRUE,
      value = "A"
    ),
    textOutput(ns("comboValue")),
    Dropdown.shinyInput(
      ns("dropdown"),
      value = "A",
      options = options,
      multiSelect = TRUE
    ),
    textOutput(ns("dropdownValue"))
  )
}

server <- function(id) {
  moduleServer(id, function(input, output, session) {
    output$comboValue <- renderText({
      dput(input$combo)
    })
    output$dropdownValue <- renderText({
      input$dropdown
    })
  })
}

[kamilzyla]

It's been a long time! 😅 Thanks for the changes 💙 I did a bit more testing and discovered some issues. I used the following app for testing:

library(shiny)
library(shiny.fluent)

options <- list(
  list(key = "A", text = "Apples"),
  list(key = "B", text = "Bananas"),
  list(key = "C", text = "Cherries")
)

ui <- div(
  textOutput("comboValue"),
  ComboBox.shinyInput("combo",
    options = options,
    allowFreeform = TRUE
  ),
  textOutput("dropdownValue"),
  Dropdown.shinyInput("dropdown",
    options = options
    # multiSelect = TRUE
  )
)

server <- function(input, output, session) {
  output$comboValue <- renderText(paste("ComboBox:", deparse(input$combo)))
  output$dropdownValue <- renderText(paste("Dropdown:", deparse(input$dropdown)))
}

shinyApp(ui, server)

Running yarn webpack shows changes

Did you build and commit the latest JS? I did my remaining testing with the changes I got after running yarn webpack.

Initial value is not properly registered

Steps to reproduce:

1. Click apples in the ComboBox. The checkbox does not check, but the value becomes "A".
2. Click apples again. The checkbox checks and the value becomes c("A", "A").

Expected behavior: The checkbox checks upon the first click and value becomes "A". The second click unchecks the checkbox and the value becomes NULL again.

There are some variations to this issue:

1. Clicking apples and then bananas will lead to only bananas being checked, but the value will be c("A", "B").
2. Passing value = “A” seems to do the same thing as step (1), i.e. the behavior is as if apples were clicked.

Freeform input doesn’t work with multiselect

Steps to reproduce:

1. Uncomment the allowFreeform = TRUE argument.
2. Type any text into the ComboBox and press enter.
3. The value remains NULL. Additional quirk: the text inside the ComboBox disappears when it loses focus and reappears when it is focused again.

Expected behavior: Good question. To get a better idea of a reasonable API, let’s first see how the underlying component works with freeform & multiselect:

ComboBox(
  options = options,
  allowFreeform = TRUE,
  multiSelect = TRUE
)

I’m not sure what would be the best way to represent a selection of choices, where some come from the predefined options and some are entered with freeform. One possibility would be to have c(text = “Whatever”, key = “a”, text = “you want”), but I see many alternatives.

For now I would raise an error in R if allowFreeform is used together with multiSelect, and create a separate issue for this use case.

[erikrenz88]

Still seeing the issue from #160? Wondering if this has been paused?

[averissimo]

It looks good overall. I've tested multiple scenarios and couldn't find a problem.

Great job

I do have some comments about tests:

• Could you re-add the tests that were removed?
  • at least running with default arguments (as diff below does)
• Add a test to make sure deselecting works
  • code is also included below

note: it seems that most tests are running sequentially, which might be problematic :-\

diff --git a/js/cypress/integration/e2e-test/fluentComponents.js b/js/cypress/integration/e2e-test/fluentComponents.js
index a7bb51b..0e5715d 100644
--- a/js/cypress/integration/e2e-test/fluentComponents.js
+++ b/js/cypress/integration/e2e-test/fluentComponents.js
@@ -183,6 +183,29 @@ function dropdownChangeTest() {
   cy.get('#fluentInputs-dropdownValue').contains('Value: C');
 }
 
+function dropdownMultiselectDefaultTest(value = 'Option A, Option C', output = 'Value: A Value: C') {
+  cy.get('#fluentInputs-dropdownMultiselect').within(() => {
+    cy.get('#fluentInputs-dropdownMultiselect-option').should('contain', `${value}`);
+  });
+  cy.get('#fluentInputs-dropdownMultiselectValue').should('contain', `${output}`);
+}
+
+function dropdownMultiselectChangeTest() {
+  cy.get('#fluentInputs-dropdownMultiselect-option').click();
+  cy.get('#fluentInputs-dropdownMultiselect-list0').parent().click();
+  cy.get('#fluentInputs-dropdownMultiselect-list1').parent().click();
+  cy.get('#fluentInputs-dropdownMultiselect-option').should('contain', 'Option C, Option B');
+  cy.get('#fluentInputs-dropdownMultiselectValue').contains('Value: C Value: B');
+}
+
+function dropdownMultiselectDeselectTest() {
+  cy.get('#fluentInputs-dropdownMultiselect-option').click();
+  cy.get('#fluentInputs-dropdownMultiselect-list0').parent().click();
+  cy.get('#fluentInputs-dropdownMultiselect-list2').parent().click();
+  cy.get('#fluentInputs-dropdownMultiselect-option').invoke('text').should("be.empty");
+  cy.get('#fluentInputs-dropdownMultiselectValue').invoke('text').should("be.empty");
+}
+
 function datePickerDefaultTest(date = 'Thu Jun 25 2020', dttm = '2020-06-25T12:00:00.000Z') {
   cy.get('#fluentInputs-datePicker-label').should('have.attr', 'value', date);
   cy.get('#fluentInputs-datePickerValue').should('contain', `Value: ${dttm}`);
@@ -335,6 +358,10 @@ describe('ComboBox.shinyInput()', () => {
 });
 
 describe('Dropdown.shinyInput()', () => {
+  beforeEach(() => {
+    cy.reload();
+  })
+
   it('setting default values works', () => {
     dropdownDefaultTest();
   });
@@ -342,6 +369,18 @@ describe('Dropdown.shinyInput()', () => {
   it('value change works', () => {
     dropdownChangeTest();
   });
+
+  it('setting default values for multiSelect works', () => {
+    dropdownMultiselectDefaultTest();
+  });
+
+  it('updating multiSelect options and values works', () => {
+    dropdownMultiselectChangeTest();
+  });
+
+  it('updating multiSelect by deselecting all options and values works', () => {
+    dropdownMultiselectDeselectTest();
+  });
 });
 
 describe('DatePicker.shinyInput()', () => {

[averissimo]

Why did you remove these tests? They work as intended

[jakubsob]

Well, I started observing some intermittency in those tests, which I don't quite understand. It seems that now it takes a bit longer to spin up the test app and that's why some fail because components are not yet rendered. For this multiselect when I run it interactively everything works fine, but on the whole Cypress run it tends to fail -- I have big troubles debugging it.

I'd prefer to refactor those tests to be more isolated, as all components being put in one big app seems to be the root cause of this intermittency. But for now I'm a bit stuck with this test, I'll give it one more try

[jakubsob]

I'm confused @averissimo… Why it passed now when if didn't on multiple runs previously?

I restored multiselect tests, but since it required some changes in shiny.react, here is a PR that enables this fix, PTAL.

[averissimo]

I agree with refactoring. But that's out of scope for this PR 😁

The main issue I found was that it was running the test sequentially and keeping the state between tests. (hence the beforeEach(() => cy.reload()) I had on my diff.

I've approved the Appsilon/shiny.react#75

[averissimo]

I like the solution!! I have 4 comments:

• I think the inst/examples/ComboBox.R example needs a small fix (if the list(text = "...") is not supported. (see diff below)
  • Are we dropping that support for defaultValue = list(text = "...")
• Should the default value be appended to the list of options?
• The allowFreeForm = TRUE is not updating the props.options as new values are added
  • edit: Which is weird as if we log the calls to ComboBox the props.options are updated (see screenshot below)

diff --git a/inst/examples/ComboBox.R b/inst/examples/ComboBox.R
index d9212d3..fe3a181 100644
--- a/inst/examples/ComboBox.R
+++ b/inst/examples/ComboBox.R
@@ -10,7 +10,7 @@ options <- list(
 ui <- function(id) {
   ns <- NS(id)
   div(
-    ComboBox.shinyInput(ns("combo"), value = list(text = "some text"),
+    ComboBox.shinyInput(ns("combo"), value = "some text",
       options = options, allowFreeform = TRUE
     ),
     textOutput(ns("comboValue"))
@@ -20,7 +20,7 @@ ui <- function(id) {
 server <- function(id) {
   moduleServer(id, function(input, output, session) {
     output$comboValue <- renderText({
-      sprintf("Value: %s", input$combo$text)
+      sprintf("Value: %s", input$combo)
     })
   })
 }

Screenshot (see the list of options showing in the browser)

[averissimo]

Should we keep the list(text = "some text") parameters?

Suggested change

	selectedKey: value,
	text: getSelectedOptionText(options, value, props.multiSelectDelimiter || ', ') || value,
	selectedKey: value?.key || value?.text || value,
	text: getSelectedOptionText(options, value, props.multiSelectDelimiter || ', ') || value?.text || value,

[averissimo]

@jakubsob Can you replicate this bug? Where it takes the "Option A" as the key starts with "A"

inst/examples/ComboBox.R

Screencast.from.2023-11-13.14-01-10.webm