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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Escape key closes multiple components #395

Open
stephan281094 opened this issue Nov 29, 2019 · 8 comments 路 May be fixed by #968
Open

Escape key closes multiple components #395

stephan281094 opened this issue Nov 29, 2019 · 8 comments 路 May be fixed by #968
Assignees
Labels
Help Wanted Extra attention is needed Resolution: Stale Type: Enhancement General improvements or suggestions

Comments

@stephan281094
Copy link

馃悰 Bug report

Current Behavior

When you have an open combobox inside of an open dialog and you hit the escape key, it closes both the combobox and the dialog.

Screenshot 2019-11-29 at 17 14 30

Expected behavior

Instead of closing both the combobox and the dialog, it should only close the combobox when hitting the escape key.

Reproducible example

https://codesandbox.io/embed/reach-ui-template-7uyon

Suggested solution(s)

Maybe use a global stack that keeps track of all dismissible UI elements that listen to "escape". Every time you hit "escape" it could call pop() to only call the onDismiss of the "deepest" UI element.

Additional context

Perhaps the same issue occurs with other combinations of UI elements.

Your environment

Software Name/Version(s)
@reach/combobox 0.1.1
@reach/dialog 0.2.9
React 16.10.1
Browser Google Chrome 78.0.3904.108
Assistive tech none
Node 11.15.0
npm/yarn Yarn 1.17.3
Operating System MacOS Catalina 10.15.1
@chaance chaance added the Type: Enhancement General improvements or suggestions label Dec 1, 2019
@stephan281094
Copy link
Author

stephan281094 commented Jan 18, 2020

After playing around with the code, I managed to get this fixed by adding either event.stopPropagation() or event.preventDefault() to the combobox implementation:

      case "Escape": {
        if (state !== IDLE) {
          transition(ESCAPE);
+         event.stopPropagation();
        }
        break;
      }

Do you see any problems with this fix? If not, I'd like to create a pull request for this.

@stale stale bot removed the Resolution: Stale label Jan 18, 2020
@stephan281094
Copy link
Author

Another solution would be to export ComboboxContext. That way we can use it with useContext, to then check for the IDLE state ourselves, allowing us to add event.stopPropagation(). Please let me know which solution is preferred.

@stephan281094
Copy link
Author

It would be nice to get this fixed. I'd happily create a pull request for either solution. Could you let me know which one is preferred?

@stale stale bot removed the Resolution: Stale label Feb 21, 2020
@chaance chaance self-assigned this Mar 11, 2020
@chaance
Copy link
Member

chaance commented Mar 11, 2020

Sorry @stephan281094, this is on my radar but I just haven't had time to think much about the problem to figure if your proposal is the right approach. This is an issue with many components when used in conjunction with one another, and there's also some ambiguity as far as WCAG is concerned. I'll need to do a little more research on the accessibility impact before making a decision here.

@CodingDive
Copy link
Contributor

Hey @chancestrickland have you had some time to think about the issue at hand?

The expected behavior of @stephan281094 sounds very reasonable to me.

Instead of closing both the combobox and the dialog, it should only close the combobox when hitting the escape key.

Same as this potential technical solution although this could require library consumers to render a ReachContext in their root so that we can manage multiple dismissable elements. Alternatively, keep track only within the context of each Dialog (would cover Dialog and its popover children).

Maybe use a global stack that keeps track of all dismissible UI elements that listen to "escape". Every time you hit "escape" it could call pop() to only call the onDismiss of the "deepest" UI element.

@chaance
Copy link
Member

chaance commented May 20, 2020

@CodingDive Yes, the expected behavior as described is correct. What I haven't had time to vet out is the suggested implementation. I think event.stopPropagation() is probably fine, so a PR here is welcome! I'll just need to play around with it a bit to be sure.

@frontsideair
Copy link

I also think event.stopPropagation() is the correct way to go, will open a PR soon so this will be fixed.

@frontsideair
Copy link

As noted in the PR, the suggested fix doesn't make the tests pass. Is the test wrong somehow or is it the fix itself? I'll also look into it a bit more.

@chaance chaance added Help Wanted Extra attention is needed and removed Status: Investigation labels Aug 3, 2021
@stale stale bot closed this as completed May 1, 2022
@bennetstankidis bennetstankidis linked a pull request Sep 7, 2022 that will close this issue
16 tasks
@chaance chaance reopened this Oct 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Help Wanted Extra attention is needed Resolution: Stale Type: Enhancement General improvements or suggestions
Projects
None yet
4 participants