reach / reach-ui

The Accessible Foundation for React Apps and Design Systems
https://reacttraining.com/reach-ui/
MIT License
5.97k stars 559 forks source link

[combobox] Binding `value` undesirely expands combobox #755

Open bentolor opened 3 years ago

bentolor commented 3 years ago

πŸ› Bug report

Thanks @chaance for this beautifuly library. We are trying to use it in a Form editor page and have an issue with the combobox

Current Behavior

When we set the current value, the combobox displays the expand state including popup on some instances. This even occurs if it is not visible (i.e. being in a collapsed section). Regardless of that, it makes it unusable, as it overlays/hides many other fields in the form.

We have 321 comboboxes in the page and (the same) 10~15 comboxes reveal this behavior.

This is how we use it in React:

import { faChevronDown } from '@fortawesome/free-solid-svg-icons'
import { FontAwesomeIcon } from '@fortawesome/react-fontawesome'
import {
  Combobox,
  ComboboxInput,
  ComboboxList,
  ComboboxOption,
  ComboboxPopover,
} from '@reach/combobox'
import '@reach/combobox/styles.css'
import React from 'react'
import Attribute from './Attribute'
import Delete from './shared/Delete'

/**
 * @param {{
 *  label: string
 *  description: string
 *  options: string[]
 *  defaultValue?(): string
 *  required?: boolean
 *  deletable?: boolean
 *  validationErrors: import('../../../../shared/validationTypes').ValidationError[]
 *  dataPath: string
 *  value: unknown
 *  onUpdate({}): void
 * }} props
 */
export default function EnumAttribute({
  options,
  required = true,
  deletable,
  ...props
}) {
  return (
    <Attribute {...props}>
      {({ onChange, onDelete }) => (
        <div className="max-w-md flex">
          <div className="w-full">
            <Combobox
              className="w-full"
              openOnFocus
              onSelect={(item) => {
                onChange(item)
              }}
            >
              <label className="block w-full flex">
                <ComboboxInput
                  value={/** @type {string} */ (props.value)}
                  className="border border-gray-400 py-1 px-2 w-full shadow-inner rounded-l"
                  selectOnClick
                  required={required}
                  onChange={(e) => {
                    onChange(e.target.value)
                  }}
                />
                <div className="flex items-center justify-center w-8 text-xs border border-gray-400 rounded-r bg-white hover:bg-gray-200 cursor-pointer">
                  <FontAwesomeIcon icon={faChevronDown} />
                </div>
              </label>
              <ComboboxPopover>
                <ComboboxList persistSelection>
                  {options.map((option, index) => (
                    <ComboboxOption key={index} value={option} />
                  ))}
                </ComboboxList>
              </ComboboxPopover>
            </Combobox>
          </div>
          {deletable ? (
            <Delete
              doDelete={() => {
                onDelete()
              }}
            />
          ) : null}
        </div>
      )}
    </Attribute>
  )
}

This is one example of the popup code of one of the initially expanded Combobox. It misses the hidden="" as present in the other <reach-portal instances.

<reach-portal><div data-reach-popover="" data-reach-combobox-popover="" data-state="suggesting" tabindex="-1" style="position: absolute; width: 0px; left: 0px; top: 0px;"><ul role="listbox" data-reach-combobox-list="" id="listbox--3"><li aria-selected="false" role="option" data-reach-combobox-option="" id="95844769" tabindex="-1"><span data-reach-combobox-option-text="" data-suggested-value="true">draft</span></li><li aria-selected="false" role="option" data-reach-combobox-option="" id="97436022" tabindex="-1"><span data-reach-combobox-option-text="" data-user-value="true">final</span></li><li aria-selected="false" role="option" data-reach-combobox-option="" id="1958062848" tabindex="-1"><span data-reach-combobox-option-text="" data-suggested-value="true">interim</span></li></ul></div></reach-portal>

This is the html node of the affected combobox

<div class="w-full" data-reach-combobox="" data-state="suggesting"><label class="block w-full flex"><input aria-autocomplete="both" aria-controls="listbox--3" aria-expanded="true" aria-haspopup="listbox" role="combobox" class="border border-gray-400 py-1 px-2 w-full shadow-inner rounded-l" required="" data-reach-combobox-input="" data-state="suggesting" value="final"><div class="flex items-center justify-center w-8 text-xs border border-gray-400 rounded-r bg-white hover:bg-gray-200 cursor-pointer"><svg aria-hidden="true" focusable="false" data-prefix="fas" data-icon="chevron-down" class="svg-inline--fa fa-chevron-down fa-w-14 " role="img" xmlns="http://www.w3.org/2000/svg" viewBox="0 0 448 512"><path fill="currentColor" d="M207.029 381.476L12.686 187.132c-9.373-9.373-9.373-24.569 0-33.941l22.667-22.667c9.357-9.357 24.522-9.375 33.901-.04L224 284.505l154.745-154.021c9.379-9.335 24.544-9.317 33.901.04l22.667 22.667c9.373 9.373 9.373 24.569 0 33.941L240.971 381.476c-9.373 9.372-24.569 9.372-33.942 0z"></path></svg></div></label></div>

Expected behavior

The combobox should not expand itself unless by user interaction

Reproducible example

Sorry – I have no minimal reproducible at hand, as it seems only to happen in specific constellations. I hope the issue might be obvious/simple enough by the upper code example.

Suggested solution(s)

I don't know. Any clue what might cause this behaviour? Is it by design? Is it a bug? Is there an easy workaround?

Your environment

Software Name(s) Version
Reach Package @reach/combobox 0.13.0
React react 17.0.1
Browser Firefox 86.0
Assistive tech ? ?
Node node v14.16
npm/yarn npm 6.14
Operating System Ubuntu 20.04
bentolor commented 3 years ago

@chaance I updated the description with a specific example. Does anything from the HTML code look phishy?

More info: This appears in a high-load szenario. So we build a very large form which even sometimes triggers the Browser "webpage not reacting"-Warning. It seems to work in situations, where the overall initial layout generation process is faster!

indiesquidge commented 3 years ago

@bentolor Looking at just the Combobox part of your snippet above, I don't see any issues with its construction, but it's difficult to debug static example code, especially when there is more code than Reach playing a part, and you're saying it's context-specific.

Would you mind including creating a minimal reusable example with the CodeSandbox Template so that we can isolate the issue to Reach to better solve your problem?

geekus commented 3 years ago

Don't mean to hijack your thread @bentolor but what you describes sounds familiar and brings to mind an issue I'm struggling with myself and considered opening an issue for. And since you don't have an isolated example, I post this to try to help with that. So please confirm if this replicates your problem or not: https://codesandbox.io/s/reach-combobox-forked-lxxuj?file=/src/App.js

The issue is that changing the value in the input field programmatically by changing the variable passed to the value prop of input field opens the popover with suggestions. Preferrably the field would get the new value and the popover would not open. The popover should only open when the field is activated by the user by clicking/focusing on it, or typing in the field if openOnFocus is false.

bentolor commented 3 years ago

Thanks @indiesquidge and @geekus for your kind feedback!

Yes, @geekus – glad that you jumped in! That looks exactly like our issue! I only wonder, because the issue is not appearing in all of our comboboxes. Maybe due to different points in time, when the value actually is set?

We are already in the bugfixing cycle and the deadline is pressing. Therefore I'll fail to deliver a reproducer. The final (simple) editor will be open-source, so I'll be able to provide the showcase, soon. But I'm pretty confident that @geekus already pinpointed the issue.

Any hint for a quick'n'dirty workaround? Otherwise we'll probably have to go to replace it. :-(

geekus commented 3 years ago

Yes, from my experience it seems like if the value is set before the combobox or input is rendered, and then not changed, it will not open. But if changed after first render it opens the popover.

karlwills commented 3 years ago

@bentolor I was just wondering if you managed a workaround for this at all? I'm having a similar issue and was wondering if there was a solution you ran with?

bentolor commented 3 years ago

@karlwills Due to time constraints and the fact, the the handling inside the Reach Combobox is managed by a state machine and according to our quick research the place where a fix would be needed no longer can distinguish between "user triggered change" and "programatically triggered change" we reckoned that there won't be a easy 3-liner fix.

Therefore we dropped reach-combobox in favour of material-ui project.

chaance commented 3 years ago

This was addressed in #783, but I actually need to revert that as it introduced some other regressions that I unfortunately didn't discover until after I merged it. I will work on a new fix for this next week.

tdurand commented 2 years ago

Any workaround to this, I tried to trigger manually .blur() on the input to close the popover then selecting a value, but it does not work ?

EDIT: I downgraded to 0.16.3 for now which does not have this bug