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

Clicking the trigger button when the menu is open does not close the menu on Safari and Firefox #832

Closed
matthewsecrist opened this issue Nov 22, 2019 · 8 comments · Fixed by #837 or #907

Comments

@matthewsecrist
Copy link

  • downshift version: 3.4.2
  • node version: n/a
  • npm (or yarn) version: n/a

Relevant code or config
https://codesandbox.io/s/53qfj (the example in the repo)

What happened:

  • Open the above link in either Safari or Firefox
  • Click the trigger button, see the menu open
  • Click the trigger button again, the menu does not close, but rather re-opens
  • On Chrome, the menu does close as expected.

Problem description:
Clicking the trigger button when the menu is open does not close the menu on Safari and Firefox

Suggested solution:
Unsure if this is expected behavior with those two browsers or if this is something that should be addressed.

@silviuaavram
Copy link
Collaborator

we need to look into this.

@silviuaavram
Copy link
Collaborator

relatedTarget from blur event is not the button in this case, as it is in Chrome. Will probably need to augment the check to work for Safari and Firefox, like here facebook/react#2011 (comment)

@silviuaavram
Copy link
Collaborator

    if (
      event.relatedTarget !== toggleButtonRef.current &&
      // https://github.com/downshift-js/downshift/issues/832 - workaround for Firefox.
      !(
        event.nativeEvent &&
        (toggleButtonRef.current === event.nativeEvent.explicitOriginalTarget ||
          toggleButtonRef.current.contains(
            event.nativeEvent.explicitOriginalTarget,
          ))
      )
    ) {

This should fix Firefox. I could not find something for Safari so far.

@silviuaavram
Copy link
Collaborator

Reopen it for Safari.

@anilanar
Copy link

anilanar commented Dec 8, 2019

#837 breaks useSelect because there's no guarantee that toggleButton exists. Downshift and useSelect can be used without trigger buttons.

In particular, nextElement.contains(event.nativeEvent.explicitOriginalTarget), in which nextElement is toggleButtonRef.current which can be null when a menu is blurred.

@silviuaavram
Copy link
Collaborator

useSelect cannot be used without toggleButton as it's a <select> not a <combobox>. But yes we should add more relevant errors like please make sure you used getToggleButtonProps inside the hook.

@silviuaavram silviuaavram reopened this Dec 11, 2019
silviuaavram added a commit that referenced this issue Feb 10, 2020
chore: refactor and improve hooks code and tests

Contains refactoring, test improvements, bug fixes.

BREAKING CHANGE: Removed `FunctionClearKeysSoFar` from state change types and TS typings. In useSelect, once the timeout for keeping the character keys in memory has expired, `FunctionSetInputValue` will be used. We are doing this as we are renaming `keysSoFar` with `inputValue`.

To migrate to the new change, simply check for `FunctionSetInputValue` with empty string as `inputValue` instead of checking for `FunctionClearKeysSoFar` in `stateReducer`.

BREAKING CHANGE: Both `getA11yStatusMessage` and `getA11ySelectionMessage` will be called with the same props as the `getA11yStatusMessage` in `<Downshift>`, apart from `previousResultCount`. In the TS typings it's now marked as optional, and all functions have the same interface definition, `A11yStatusMessageOptions`.

To migrate to the new changes, in `useSelect` and `useCombobox`, if you used `items` as parameters in any of the a11y message functions, now you should use `resultCount` as probably you only needed `items.length` from it anyway.

Also typings `UseSelectA11yMessageOptions` and `UseComboboxA11yMessageOptions` have been removed. Use `A11yStatusMessageOptions` instead.

Code Changes: tests have been enhanced by using `user input` from `RTL` and they now look better and cleaner. Also covered more use cases better. Code has been refactored as well, and bundle size slightly reduced.

Functional Improvement: better focus management for both `useSelect` and `useCombobox`.

Fixes #832.
Closes #892
Closes #891
Closes #873
@silviuaavram
Copy link
Collaborator

🎉 This issue has been resolved in version 5.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@mskelton
Copy link

This appears to be broken again in version 5.4.5 of Downshift. I'm noticing the issue both in my code as well as the example on the useSelect docs: https://www.downshift-js.com/use-select. This is only breaking for me on Firefox, Safari seems to be fine.

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