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

Add escape key handling to ConfirmModal #10099

Closed
w33ble opened this issue Jan 27, 2017 · 7 comments
Closed

Add escape key handling to ConfirmModal #10099

w33ble opened this issue Jan 27, 2017 · 7 comments
Labels
bug Fixes for quality problems that affect the customer experience good first issue low hanging fruit

Comments

@w33ble
Copy link
Contributor

w33ble commented Jan 27, 2017

Now that we're using our own custom model for confirmations, we've lost some of the keyboard use of native confirmations. This is a regression from previous confirmation dialog functionality.

We already focus the confirm button by default, so the user can just hit enter to confirm (though, that could change, see #10040), which is great.

We are still missing mapping the ESC key to cancel. A lot of users don't click on dialogs (myself included), preferring to use the keyboard instead. We should restore this useful keyboard interaction.

@w33ble w33ble added bug Fixes for quality problems that affect the customer experience good first issue low hanging fruit labels Jan 27, 2017
@stacey-gammon
Copy link
Contributor

If there is no X icon should we map ESC to onCancel? I feel like ESC should be mapped to onClose bc it's like choosing not to answer the question, and if showClose is false (no x icon) then I think we should force them to explicitly choose. That's my gut feel but I'm open to other ideas.

@w33ble
Copy link
Contributor Author

w33ble commented Jan 30, 2017

That's not how the native confirm dialog works. ESC is the same as cancel, because there is no close button. Confirm forces you to deal with the prompt. Our confirm should work the same as the native confirm. Please don't take my keyboard navigation away.

@dpenny52
Copy link
Contributor

I would like to take a stab at this as a first commit to the project. I've got working code already and just need to write tests.

One thing I could use some feedback on: it is easy enough to implement the ng-keydown event when the modal is focused, but if the user clicks outside of the modal buttons it becomes unfocused and the ng-keydown will not trigger anymore.

My suggestion would be, since the "Confirm" button starts out focused, call onCancel() in the ng-blur directive for the "Confirm" button. That way if the user clicks on the screen outside of the buttons, the dialog will close (I believe this is pretty standard for modal dialogs?). Feedback on this would be appreciated!

@w33ble

@dpenny52
Copy link
Contributor

Actually, I am realizing that my problem is that the ConfirmModal is passed as a child to the ModalOverlay, which does not have the ng-keydown directive. I am trying to figure out how to access the onKeyDown function of the ConfirmModal from the ModalOverlay, which I think will be the correct solution.

@dpenny52
Copy link
Contributor

dpenny52 commented Feb 19, 2017

Figured it out, but build is failing locally due to:

Running "licenses" task
Warning: Non-confirming licenses:
 [email protected]

[email protected] /Users/dpenny521/Documents/workspace/kibana
├─┬ [email protected]
│ └─┬ [email protected]
│   └─┬ [email protected]
│     └─┬ [email protected]
│       └─┬ [email protected]
│         └─┬ [email protected]
│           └── [email protected]
└─┬ [email protected]
  └─┬ [email protected]
    └─┬ [email protected]
      └─┬ [email protected]
        └── [email protected]

@tsullivan I notice you had the same error referenced in #9797 did you figure out a solution?

@w33ble
Copy link
Contributor Author

w33ble commented Feb 23, 2017

Hey @dpenny52, sorry I haven't gotten back to you sooner, I've been falling behind on my github notifications. Thanks for putting together a PR, I'll dig into that shortly.

@dpenny52
Copy link
Contributor

No problem @w33ble thanks for getting back

dpenny52 added a commit to dpenny52/kibana that referenced this issue Feb 28, 2017
dpenny52 added a commit to dpenny52/kibana that referenced this issue Mar 1, 2017
w33ble pushed a commit that referenced this issue Mar 1, 2017
* #10099 adds keyboard handling to close confirm_modal on ESC

* remove resetSpyCounts, use beforeEach

* #10099 change from .bind to .on and remove unnecessary onKeyDown call

* #10099 move ESC handling to confirm_modal.js
w33ble pushed a commit that referenced this issue Mar 1, 2017
* #10099 adds keyboard handling to close confirm_modal on ESC

* remove resetSpyCounts, use beforeEach

* #10099 change from .bind to .on and remove unnecessary onKeyDown call

* #10099 move ESC handling to confirm_modal.js
w33ble pushed a commit that referenced this issue Mar 1, 2017
* #10099 adds keyboard handling to close confirm_modal on ESC

* remove resetSpyCounts, use beforeEach

* #10099 change from .bind to .on and remove unnecessary onKeyDown call

* #10099 move ESC handling to confirm_modal.js
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fixes for quality problems that affect the customer experience good first issue low hanging fruit
Projects
None yet
Development

No branches or pull requests

3 participants