Remove prescriptive rendering of FASTSelect and adds placeholder content

[brianchristopherbrady]

Pull Request

📖 Description

This PR aims to remove the prescriptive rendering of the FASTSelect as well as adding support for placeholder content.

Added Placeholder Attribute and Functionality:

1. Implemented the placeholder attribute to display a default placeholder option when no selection is made.
2. Updated the component logic to handle the placeholder attribute and select the placeholder option if no other option is selected.
3. Added tests to verify the proper rendering and behavior of the placeholder functionality.

Removed Prescriptive Rendering when Multiple Attribute and Size Attribute are Present:

1. Removed the prescriptive rendering behavior for the select component when the multiple attribute or size attribute is present.
2. Updated the component to dynamically adjust the rendering based on the presence of these attributes.
3. When the multiple attribute is present, the select component allows multiple options to be selected.
4. When the size attribute is set, the component adjusts its rendering to display a specified number of visible options.
5. When the number of options is less than the specified size attribute value, the size token is set to the number of options.
6. Added tests to validate the rendering and behavior of the select component with the multiple and size attributes.

Introduce Listbox Mode:

1. Added a new attribute called listbox-mode to render the select component as a listbox only, without the control portion.
2. When listbox-mode is enabled, the select component displays only the listbox portion, allowing selection from available options.
3. Modified the component's rendering logic to support the listbox-mode attribute and exclude the control portion when enabled.
4. Added tests to ensure proper rendering and behavior of the select component in listbox mode.

Modified Keypress and Click Handlers:

1. Updated the keypress and click handlers to account for the changes introduced by the placeholder, multiple, size, and listbox-mode enhancements.
2. Ensured that key events and click events are properly handled based on the component's current state and attributes.
3. Refactored the handlers to improve code clarity and maintainability.

Added Tests:

1. Included a comprehensive set of tests to cover the new functionality, rendering variations, and event handling introduced by the changes.
2. Verified the correct rendering, behavior, and interaction of the select component in different scenarios.

🎫 Issues

Remove Prescriptive Nature of the Select

👩‍💻 Reviewer Notes

📑 Test Plan

All current tests continue to pass. Added additional tests for coverage of additional features.

✅ Checklist

General

• I have included a change request file using $ yarn change
• I have added tests for my changes.
• I have tested my changes.
• I have updated the project documentation to reflect my changes.
• I have read the CONTRIBUTING documentation and followed the standards for this project.

Component-specific

• I have added a new component
• I have modified an existing component
• I have updated the definition file
• I have updated the configuration file

Screenshots

FASTSelect multiple

FASTSelect listbox-mode

FASTSelect size

[chrisdholt]

Thanks for the PR @brianchristopherbrady - there’s a lot of changes here so it’ll take a bit to dig in and sift through this. I’m going to also ping @radium-v as he put a ton of work into ensuring these were accessible and worked as one might expect similar to a select (and combobox) and his feedback would be valuable here.

[radium-v]

Holding for review

[radium-v]

For placeholder, I'd like to see a solution that more closely matches how it's handled with the native <select> - adding a first option that's disabled and hidden. This could be solved by making it a conditional <option> as part of the template. Currently, this could theoretically work with a slotted <fast-option> but the hidden attribute would actually exempt it from the options list, so it gets bypassed.

• Add placeholder and placeholderOption as properties to the Select class:

  @attr
  placeholder: string;
  
  @observable
  public placeholderOption: HTMLOptionElement | null = null;

• Add a conditional template partial In select.template.ts, before the slottedOptions slot (line 63):

  ${when(
      x => x.placeholder,
      html<T>`
          <option disabled hidden ${ref("placeholderOption")}>
              ${x => x.placeholder}
          </option>
      `
  )}

• The return for the displayValue getter should optionally return the text from the placeholderOption:

    public get displayValue(): string {
        Observable.track(this, "displayValue");
  -    return this.firstSelectedOption?.text ?? "";
  +    return this.firstSelectedOption?.text ?? this.placeholderOption?.text ?? "";
    }

• The conditional for setDefaultSelectedOption() should set the selectedIndex to -1 if there's a placeholder:

  protected setDefaultSelectedOption(): void {
      const options: FASTListboxOption[] =
          this.options ??
          Array.from(this.children).filter(FASTListbox.slottedOptionFilter);
  
      const selectedIndex = options?.findIndex(
          el => el.hasAttribute("selected") || el.selected || el.value === this.value
      );
  
  -    if (selectedIndex !== -1) {
  +    if (selectedIndex !== -1 || this.placeholder !== "") {
          this.selectedIndex = selectedIndex;
          return;
      }
  
      this.selectedIndex = 0;
  }

• updateDisplayValue() would now need to wait until the component is connected:

  private updateDisplayValue(): void {
  -    if (this.collapsible) {
  +    if (this.$fastController.isConnected && this.collapsible) {
              Observable.notify(this, "displayValue");
          }
      }
  }

All of this would enable the ability to set a placeholder attribute with very little additional logic.

<fast-select placeholder="Select an option">
    <!-- options -->
</fast-select>

[brianchristopherbrady]

For placeholder, I'd like to see a solution that more closely matches how it's handled with the native <select> - adding a first option that's disabled and hidden. This could be solved by making it a conditional <option> as part of the template. Currently, this could theoretically work with a slotted <fast-option> but the hidden attribute would actually exempt it from the options list, so it gets bypassed.

• Add placeholder and placeholderOption as properties to the Select class:

  @attr
  placeholder: string;
  
  @observable
  public placeholderOption: HTMLOptionElement | null = null;

• Add a conditional template partial In select.template.ts, before the slottedOptions slot (line 63):

  ${when(
      x => x.placeholder,
      html<T>`
          <option disabled hidden ${ref("placeholderOption")}>
              ${x => x.placeholder}
          </option>
      `
  )}

• The return for the displayValue getter should optionally return the text from the placeholderOption:

    public get displayValue(): string {
        Observable.track(this, "displayValue");
  -    return this.firstSelectedOption?.text ?? "";
  +    return this.firstSelectedOption?.text ?? this.placeholderOption?.text ?? "";
    }

• The conditional for setDefaultSelectedOption() should set the selectedIndex to -1 if there's a placeholder:

  protected setDefaultSelectedOption(): void {
      const options: FASTListboxOption[] =
          this.options ??
          Array.from(this.children).filter(FASTListbox.slottedOptionFilter);
  
      const selectedIndex = options?.findIndex(
          el => el.hasAttribute("selected") || el.selected || el.value === this.value
      );
  
  -    if (selectedIndex !== -1) {
  +    if (selectedIndex !== -1 || this.placeholder !== "") {
          this.selectedIndex = selectedIndex;
          return;
      }
  
      this.selectedIndex = 0;
  }

• updateDisplayValue() would now need to wait until the component is connected:

  private updateDisplayValue(): void {
  -    if (this.collapsible) {
  +    if (this.$fastController.isConnected && this.collapsible) {
              Observable.notify(this, "displayValue");
          }
      }
  }

All of this would enable the ability to set a placeholder attribute with very little additional logic.

<fast-select placeholder="Select an option">
    <!-- options -->
</fast-select>

@radium-v
Great suggestion. Made the changes.

[MaiLeeTR]

@chrisdholt @radium-v
Great PR, @brianchristopherbrady!

Are there any updates on when these changes are expected to go in?

[brianchristopherbrady]

@chrisdholt @radium-v Great PR, @brianchristopherbrady!

Are there any updates on when these changes are expected to go in?

@MaiLeeTR Just waiting on some approvals!

[rinaok]

@chrisdholt @radium-v @brianchristopherbrady
Do you have any idea when it will be merged? We are waiting for this change.

[brianchristopherbrady]

@rinaok Hi! I have dedicated time for this task now so just a matter of getting follow up reviews from people. I've pinged the discord server and it's getting some attention now.

No definitive answer but I would say no later then next week.

[chrisdholt]

No definitive answer but I would say no later then next week.

Just for awareness, we can't ensure this unless we've validated this meets all requirements, does not regress a11y, etc. Appreciate the PR - but timing is dependent on that being done and getting approval for the implementation.

[MaiLeeTR]

@brianchristopherbrady - Just checking in again to see if there are any updates to gauge if we should customize a solution on our end instead.

[bheston]

I'm sorry I missed the issue you opened for this, or I would have provided this feedback then.

The main concern here is complicated by the history of poorly designed and outdated native elements. One of the motivations of the Foundation components was to augment native elements to make them more convenient, but to maintain the same interface as their native counterpart. This is obviously a long-term view, and one that is countered by the standards evolving another direction, like adding selectlist instead of updating select. Again, history.

I agree with the addition of the placeholder support as it makes handling that closer to other input type components. As someone who is guilty of large PRs that make multiple changes, I'd like to see that part split out and I think it would be easier to get in.

I understand the intent behind the listbox mode, but it makes the template and the base class more prescriptive than it already is. Following my comments at the start, features like size only complicate the usefulness and usability of the set of components implemented as Select, Listbox, and Picker (and potentially Comboxbox). A Select with size is logically the same as a Listbox. And size is not the way anyone wants to set the height of an element anymore. Not to mention it's been hard to style because it requires knowing how tall items are going to be when they render.

From a usability perspective, a multiple select collapsible (listbox mode) Select component is a less usable variation of Picker. If someone is allowed to pick multiple items from a list, they should be able to easily find and clear that selection. The chips in the Picker allow exactly that in a modern way that I argue is stronger than a comma-delimited string. In your example screenshot the content breaks down after more than two items are selected. The default Picker component could use some styling work, but the infrastructure is sound, and the experience is more usable.

If we consider a change to this model, I propose simplifying the current components and removing the size attribute altogether:

• A Select is for a droplist selection of a single value only
• A Listbox is for a scrollable single or multi selection (although it should also have checkboxes for a11y)
• A Picker is for a droplist selection of multiple values

It's possible I'm missing the intent here, so please help me to consider the use case that supports bundling these different modes into the Select component.

[brianchristopherbrady]

I'm sorry I missed the issue you opened for this, or I would have provided this feedback then.

The main concern here is complicated by the history of poorly designed and outdated native elements. One of the motivations of the Foundation components was to augment native elements to make them more convenient, but to maintain the same interface as their native counterpart. This is obviously a long-term view, and one that is countered by the standards evolving another direction, like adding selectlist instead of updating select. Again, history.

I agree with the addition of the placeholder support as it makes handling that closer to other input type components. As someone who is guilty of large PRs that make multiple changes, I'd like to see that part split out and I think it would be easier to get in.

I understand the intent behind the listbox mode, but it makes the template and the base class more prescriptive than it already is. Following my comments at the start, features like size only complicate the usefulness and usability of the set of components implemented as Select, Listbox, and Picker (and potentially Comboxbox). A Select with size is logically the same as a Listbox. And size is not the way anyone wants to set the height of an element anymore. Not to mention it's been hard to style because it requires knowing how tall items are going to be when they render.

From a usability perspective, a multiple select collapsible (listbox mode) Select component is a less usable variation of Picker. If someone is allowed to pick multiple items from a list, they should be able to easily find and clear that selection. The chips in the Picker allow exactly that in a modern way that I argue is stronger than a comma-delimited string. In your example screenshot the content breaks down after more than two items are selected. The default Picker component could use some styling work, but the infrastructure is sound, and the experience is more usable.

If we consider a change to this model, I propose simplifying the current components and removing the size attribute altogether:

* A `Select` is for a droplist selection of a single value only

* A `Listbox` is for a scrollable single or multi selection (although it should also have checkboxes for a11y)

* A `Picker` is for a droplist selection of multiple values

It's possible I'm missing the intent here, so please help me to consider the use case that supports bundling these different modes into the Select component.

@bheston

Thanks for taking a look! I've gone ahead and opened up a PR for the placeholder feature.

I need to touch base the fluent team to find out the use case for the multiple selection.

Adds select placeholder feat #6854

[janechu]

Closing this, due to #6951 we are only putting in critical fixes or features.