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] Binding value undesirely expands combobox #755

Open
bentolor opened this issue Mar 2, 2021 · 9 comments · Fixed by #783
Open

[combobox] Binding value undesirely expands combobox #755

bentolor opened this issue Mar 2, 2021 · 9 comments · Fixed by #783
Labels
Status: In Progress Type: Bug Something isn't working

Comments

@bentolor
Copy link

bentolor commented Mar 2, 2021

🐛 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
@chaance chaance added Help Wanted Extra attention is needed Status: Unconfirmed Bug reports without a repro, not yet confirmed labels Mar 2, 2021
@bentolor
Copy link
Author

bentolor commented Mar 2, 2021

@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
Copy link
Collaborator

indiesquidge commented Mar 2, 2021

@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
Copy link

geekus commented Mar 3, 2021

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
Copy link
Author

bentolor commented Mar 4, 2021

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
Copy link

geekus commented Mar 4, 2021

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
Copy link

@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
Copy link
Author

@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 chaance added the Type: Bug Something isn't working label Mar 29, 2021
@chaance chaance changed the title Combobox: Binding value undesirely expands combobox [combobox] Binding value undesirely expands combobox Mar 30, 2021
@chaance chaance reopened this Oct 19, 2021
@chaance
Copy link
Member

chaance commented Oct 19, 2021

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.

chaance added a commit that referenced this issue Oct 19, 2021
This PR introduced regression in some cases where the popover doesn't
open until the user presses the down or up key after initial input.
See #755 for the related issue.
@chaance chaance added Status: In Progress and removed Help Wanted Extra attention is needed Status: Unconfirmed Bug reports without a repro, not yet confirmed labels Oct 19, 2021
@tdurand
Copy link

tdurand commented Mar 3, 2022

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: In Progress Type: Bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants
@tdurand @bentolor @chaance @indiesquidge @geekus @karlwills and others