Terra-core: [terra-form-select] Combobox Variant Renders Options Inconsistently

[emilyrohrbough]

Bug Report

Description

Combo box variant displays an empty object selection when using the space-bar to display the space options. This spacing persists with hover styles when space is removed.

Reason I think it's a bug:

1. This selection 'space' option doesn't render the same height of the other select options.
2. It doesn't show the add option (+ ) indication to show that it should be added to the list.
3. Steps 7-10 results in an empty option that receives hover styles. The initial open options does not have this spacing.

Steps to Reproduce

1. Navigate to https://engineering.cerner.com/terra-core/#/raw/tests/terra-form-select/form-select/uncontrolled-combobox

2. Hit tab to focus to the select input

3. Hit space to open select options
   

   screenshot

   
   
   

4. Click middle of page to remove select focus
   

   screenshot

   
   
   

5. Hit tab to move focus to the select input

6. Hit down arrow key to open select options. See the empty, select option that is select. Its because it thinks it's a space.

screenshot

1. Hit backspace key to remove the space.

screenshot

1. Click middle of page to remove focus.

2. Hit tab to return focus to input.

3. Hit down arrow or space bar to see an empty option (or gap before options) that receives hover styles.

screenshot

Expected Behavior

No empty selection option when no option was selected.

Environment

• Component Name and Version: v5.15.0
• Browser Name and Version: Chrome

[emilyrohrbough]

I am only seeing this on the combo-box variant.

[bjankord]

Talked about trimming the input value to ensure no trailing spaces are entered.

[nramamurth]

@bjankord

Talked about trimming the input value to ensure no trailing spaces are entered.

By this, did you mean that we wouldn't allow input to start with a space? I however did a high-level initial investigation and found out that DropDownMenu's children (Line 145) consisted of an additional Option element whose display was " " during Step 6 described above.

https://github.com/cerner/terra-core/blob/349fefe8f89f4166473b33015f27d746b909aa45/packages/terra-form-select/src/_Menu.jsx#L145-L146

So, I then updated the filtering logic to something like below and it seems to have worked. I have no clue about the other repercussions of this change but is this something you had in mind or is there a different way where we could introduce this check much earlier?

/**
 * Filters the object content by the search criteria.
 * @param {ReactNode} object - The node being filtered.
 * @param {string} searchValue - The value being searched for.
 * @param {function|undefined} optionFilter - An optional custom filter.
 * @return {array} - An array of filtered content.
*/
static filter(object, searchValue, optionFilter) {
    return React.Children.toArray(object).reduce((accumulator, option) => {
-     if (option.type.isOption && MenuUtil.filterOption(option, searchValue, optionFilter)) {
+     if (option.type.isOption && option.props.display.trim().length > 0 && MenuUtil.filterOption(option, searchValue, optionFilter)) {
        accumulator.push(option);
      } else if (option.type.isOptGroup) {
        const children = MenuUtil.filter(option.props.children, searchValue, optionFilter);
        // Ignore groups that do not contain any filtered options.
        if (children.length > 0) {
          accumulator.push(React.cloneElement(option, {}, children));
        }
      }
      return accumulator;
    }, []);
  }

[StephenEsser]

@nramamurth

The code above looks like it filters out already added options, but doesn't prevent them from being added. This would likely cause strange behavior as the component could have an infinite number of invalid options getting added that are never visible.

We'll want to prevent invalid options (options consisting of only spaces) from being added as freetext. The first place I would investigate adding the change would be within shouldAddOptionOnBlur.

https://github.com/cerner/terra-core/blob/master/packages/terra-form-select/src/_FrameUtil.js#L98

[nramamurth]

@StephenEsser

I see. Thank you for the direction and the first condition on Line 103 ^[1] evaluates to true for "combobox" variant and it looks like maybe the fix will have to just be to extract out the second condition that checks for input with trailing spaces.

[1] https://github.com/cerner/terra-core/blob/master/packages/terra-form-select/src/_FrameUtil.js#L103

[nramamurth]

@StephenEsser

1. I believe that the fix to the original issue is in shouldAddOptionOnBlur - to not add blank string as an option.
2. However, I think that the fix for the issue that you discovered is outside of shouldAddOptionOnBlur. Please do correct me if I am wrong.
3. Do we maintain an internal state somewhere that holds the display value that probably needs to be cleared out when the search value is a blank/empty string for combo-box variant?
4. What does this line achieve upon blur? Shouldn't it set the search value to be a blank string and not the previously selected option upon re-render? Is it because hasSearchChanged is being set to false in [1] and upon re-render it shows the display according to the logic [2]?

[1] https://github.com/cerner/terra-core/blob/master/packages/terra-form-select/src/_Frame.jsx#L258
[2] https://github.com/cerner/terra-core/blob/master/packages/terra-form-select/src/_Frame.jsx#L243

[nramamurth]

@StephenEsser

I was playing around whether Frame needs to maintain a local state [1] to clear out the search similar to how Menu does. What are your thoughts on this?

[1] https://github.com/cerner/terra-core/compare/proto-invalid-options-form-select?expand=1

@bjankord

[StephenEsser]

@nramamurth

I don't recommend maintaining an additional internal state within the component to avoid making the behavior of this component more complicated.

This line resets the input field so that when the dropdown closes the search input reverts back to a cleared state. The searchValue is only associated with the dropdowns filtered results. We do not want to display the search value without a list of associated results.

For the scope of this issue I think we should implement a solution similar to this: https://github.com/cerner/terra-core/commit/de1f76829eaf724f0f3bbab1fd886ac935fec541

This returns an empty string instead of a blank string for freetext. The component is already design to appropriately handle an empty string. This will allow a user to clear a search on blur if there is no valid text within the input and will prevent invalid blank options from being added.

[nramamurth]

@StephenEsser

This line resets the input field so that when the dropdown closes the search input reverts back to a cleared state.

What do you mean by "cleared state" here?

Thank you for proposing a solution and it does work but I am a bit confused. IMO, this line looks a bit flaky. The existing logic in shouldAddOptionOnBlur() translates to something like "add the option (irrespective of whether it is valid/invalid) if the variant is combobox (in addition to other conditions satisfying)". I think that the validation that we are intending to do here should probably be done inside this method.

It is confusing to me that we are now having shouldAddOptionOnBlur return true for invalid input and then adding a second layer of validation later to prevent it (?). Is it okay to do so or am I overthinking at this point?

I slightly modified your proposal so that shouldAddOptionOnBlur returns false for invalid input - https://github.com/cerner/terra-core/compare/proto-invalid-options-form-select?expand=1 Any thoughts?

[StephenEsser]

What do you mean by "cleared state" here?

The cleared state is the state in which the select returns when the dropdown closes. The text inside the search input is either accepted or rejected when the dropdown closes or the select loses focus. Either way the "search" text must be cleared. The value of the component can be updated, but the search query returns to an empty string.

This existing line of code is allowing any combobox value to be accepted, regardless of length, but for tag variants we reject if the value was empty.

(variant !== Variants.TAG || searchValue.trim().length > 0)

It is roughly equivalent to the following.

if (variant === Variants.combobox) {
  return true;
} else if (variant === Variants.TAG && searchValue.length > 0) {
  return true;
}

We cannot reject blank strings inside of shouldAddOptionOnBlur for the combobox otherwise we end up with the behavior where empty strings revert the field back to a previously selected value.

The behavior the Combobox should have:
(On Blur the Combobox returns and updates the value to an empty string)

The behavior that will happen if we reject blank strings in shouldAddOptionOnBlur:

( On Blur the Combobox reverts back to Green )

Empty strings must be allowed in shouldAddOptionOnBlur in order to clear the value, but we do not want to create blank strings within the options dropdown.

For your proposed solution the result seems to be roughly the same. However, shouldAddOptionOnBlur returns false but we add an option anyway, I think this is also confusing and duplicates some of the search changed logic that is already handled within shouldAddOptionOnBlur.

For me it makes sense that when a blank string is entered as input we should still add an option on blur, that option just happens to be an empty string. The shouldAddOptionOnBlur isn't necessarily solely for input validation, it's just a yes or no that when focus is lost we need to add an option.

I would choose whichever of the two options you think makes the most sense, as long as the behavior matches what we are expecting and does not add any additional state management into the component. I recommend also adding some comment documentation that explains the logic path for sending back an empty string.

[nramamurth]

@StephenEsser

Awesome! I am sorry about a lot of to and fro but I appreciate the detailed explanation. I shall go ahead and open a PR. Thank you.