Reach-ui: Escape key closes multiple components

[stephan281094]

🐛 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.

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 |

[stephan281094]

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.

[stephan281094]

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]

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?

[chaance]

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]

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]

@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]

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

[frontsideair]

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.