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

<Combobox /> sets wrong values on click outside for non-primitive values and displayValue fallback #188

Open
rreckonerr opened this issue Nov 29, 2024 · 2 comments

Comments

@rreckonerr
Copy link
Contributor

rreckonerr commented Nov 29, 2024

When using the Combobox component with non-primitive option values, the component resets to an incorrect value (the null-ish displayValue in my case) when clicking outside after interacting with the options list.

Steps to Reproduce

  1. Render the Combobox component with a null value as the initial state.
  2. Select a new value from the options list.
  3. Trigger the options list again.
  4. Click outside of the component.

Expected behavior

The component should display the previously selected value.

Actual behavior

The component reverts to displaying the nullish display value.

Reproduction

Tests covering this scenario are included in PR.

It appears that the issue lies in how this._originalValue is being set in combobox.js. Specifically, it is currently set to this.inputValue which returns displayValue, whereas it might need to use optionComponent.args.value.

Adjusting this would also require modifying other interactions, such as handling keypress events in options.hbs, to ensure the selected option's value is passed consistently.


Could you confirm if this is a bug or a misuse of the component? If valid, I'm happy to collaborate on a fix.

@GavinJoyce
Copy link
Owner

Thanks for the issue.

Possibly related issue: #187

@rreckonerr
Copy link
Contributor Author

Currently, my workaround is

import { action } from '@ember/object';
import EmberHeadlessUiCombobox from 'ember-headlessui/components/combobox.js';
import { Keys } from 'ember-headlessui/utils/keyboard';

const ACTIVATE_FIRST = 1;

/**
 * This fixes the issue described in https://github.com/GavinJoyce/ember-headlessui/issues/188
 * There are two issues in the original implementation:
 * 1. When using the `Combobox` component with non-primitive option values, the component resets to an incorrect
 * value (the `null`-ish displayValue in my case) when clicking outside after interacting with the options list.
 * The fix for it is to set the `_originalValue` to the actual value passed to Option element when the option is selected.
 * 2. The fix above causes an issue with the keyboard navigation where the option can't be selected using the keyboard.
 * So I had to override the `handleKeyPress` method to set the `_originalValue` when the option is selected using the keyboard.
 * This fix is not a perfect one, but it works for my use case.
 *
 */
export default class extends EmberHeadlessUiCombobox {
  @action
  setSelectedOption(optionComponent, e) {
    let optionToCheck;
    let activeOption = this.activeOption;
    let firstOption = this.optionElements[0];

    if (activeOption) {
      optionToCheck = activeOption;
    } else if (firstOption) {
      optionToCheck = firstOption;
    } else {
      return;
    }

    if (optionToCheck?.element.hasAttribute('disabled')) {
      return;
    }

    if (e?.type === 'click') {
      if (!this.isMultiselectable) {
        this.closeCombobox();
      }
      this.inputElement?.focus();
    }

    // Fix for issue No. 1
    this._originalValue = optionComponent.args.value;
  }

  @action
  handleKeyPress(event) {
    if (
      event.key === Keys.Enter ||
      ((event.key === 'Space' || event.key === Keys.Space) && !this.isOpen)
    ) {
      this.activateBehaviour = ACTIVATE_FIRST;
      if (this.isOpen) {
        this.setActiveAsSelected();
        let option = this.options?.find(
          (o) => o.guid === this.activeOptionGuid
        );
        // Fix for issue No. 2
        this.setSelectedOption(option, event);
        this.isOpen = false;
        this.inputElement?.focus();
      } else {
        this.isOpen = true;
      }
    } else {
      this.isOpen = true;
    }
  }
}

I'll try to submit a fix in a separate PR soon

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants