#10099 adds keyboard handling to close confirm_modal on ESC #10455

dpenny52 · 2017-02-19T22:06:37Z

Closes #10099 by adding keyDown handling to document.body while the ConfirmModal is open, looking for ESC keyDown events (keyCode === 27) and calling the onCancel() method of the ConfirmModal when they are detected.

elasticmachine · 2017-02-19T22:06:38Z

Can one of the admins verify this patch?

dpenny52 · 2017-02-19T22:07:53Z

I just signed the CLA a minute ago, so maybe it takes a bit for it to register.

https://elasticsearch.na1.echosign.com/public/viewAgreement?tsid=CBFCIBAA3AAABLblqZhDkpOOr3IH4xwWvvmPHBsCb7pK1frNCgbyyzZpfw82NViqwrzft8EHUx7VN3sBAcBtdq0YxdJP80Q1v9UhrhYQH&

w33ble · 2017-02-27T21:39:43Z

src/ui/public/modals/__tests__/confirm_modal.js

+        confirmButtonText: 'bye',
+        onConfirm: confirmCallback,
+        onCancel: cancelCallback,
+        onClose: closeCallback,


You can remove the onClose lines in your tests, as well as the showClose setting. They aren't actually used in these tests anyway.

w33ble · 2017-02-27T21:43:30Z

src/ui/public/modals/__tests__/confirm_modal.js

+
+      const e = angular.element.Event('keydown'); // eslint-disable-line new-cap
+      e.keyCode = 27;
+      while(e.keyCode === 27) {


I don't think you need to worry about the random value here. Just set the keyCode to a value other than 27. Probably good to avoid 9 (tab) and 13 (enter) too. You could play it safe and just pick a letter, so any value between 48 and 90 is fine.

w33ble · 2017-02-27T21:46:45Z

src/ui/public/modals/modal_overlay.js

+      }
+    };
+
+    angular.element(document.body).bind('keydown', onKeyDown);


Please use .on here instead of .bind

w33ble · 2017-02-27T21:51:51Z

src/ui/public/modals/modal_overlay.html

@@ -1 +1 @@
-<div class="kuiModalOverlay"></div>
+<div class="kuiModalOverlay" ng-keydown="onKeyDown($event)"></div>


Since you're listening for the keypress event on document.body, you don't need this keydown directive here.

dpenny52 · 2017-02-27T21:58:25Z

Thanks for the comments, I will make these changes later on tonight or tomorrow.

w33ble · 2017-02-27T22:30:27Z

@dpenny52 you got it. Sorry it took me a while to review this, my local environment was being really goofy.

I also opened dpenny52#1 for you to check out. It's a small change to some of the test code that was already there, but it affects the couple tests you wrote as well.

Remove resetSpyCounts, use beforeEach

…wn call

dpenny52 · 2017-02-28T05:03:42Z

Updated with your PR + comments @w33ble

stacey-gammon · 2017-02-28T14:01:33Z

src/ui/public/modals/modal_overlay.js

+
+    angular.element(document.body).on('keydown', (event) => {
+      if(event.keyCode === 27) {
+        scope.onCancel();


This should be onClose, not onCancel (I know you have concerns @w33ble about the differences between them, and I am starting to see your side of things, since this is a subtle difference, and it caused confusion here).

onClose will be onCancel unless a caller specifically sent a separate function - onClose maps to the X, while onCancel maps to one of the buttons.

But if there is no X, then onCancel is the interaction we want.

And this is why I've been advocating for a new dialog type for when you want the X/close button. They're different things ;)

But this isn't really a concern for @dpenny52 to worry about. I think this PR should be handled with the intention of working with the "confirm" modal, and we can break out the "close" functionality and make a new modal later. To my knowledge, we don't use the yes/no/close interaction anywhere right now, so it's safe to pretend it doesn't exist yet.

Related: #10104 (comment) - your comment which advocates removing the Close option (and thus onClose) from the existing confirm modal.

Fair enough. Actually the whole reason for the onClose functionality may be going away, so we could rip that out (though I'd prefer to wait till after #10585 gets checked in just to be sure).

My modified feedback, in that case, is that I still think this logic should either go in confirm_modal, or be passed in as a function. No need for the modal overlay to have scope knowledge, and this could easily break if we change the name of the function inside confirm_modal.

If you both agree on that point I'd be happy to move that code down to the dialog level.

Yeah, I think I'm on board with that part. I think that, while you're right @dpenny52 about the ESC being a universal modal interaction, we probably want to handle it differently in each case (as outlined in the whole onClose/onCancel conversation here). For now, let's move it into the confirm modal, and we can worry about pushing that logic back up when we add other modal types.

stacey-gammon · 2017-02-28T14:02:23Z

src/ui/public/modals/modal_overlay.js

@@ -5,16 +5,23 @@ import modalOverlayTemplate from './modal_overlay.html';
 * Appends the modal to the dom on instantiation, and removes it when destroy is called.
 */
 export class ModalOverlay {
-  constructor(modalElement) {
+  constructor(modalElement, scope) {


I feel like this functionality belongs in confirm_modal.js, and I think we should only bind ESC key functionality if showClose is true in confirm_modal.

My concern is, we don't always show the X in the modal, and if we don't show the X then I think we should force the user to be explicit about their choice. Esc usually means, do nothing or dont make a choice right now which is what I see being mapped to the X icon functionality on a dialog. onCancel and onConfirm can be any functions, and the button text can also be anything, so it might not be a "noop" type of function.

I think we should only bind ESC key functionality if showClose is true in confirm_modal

I disagree. Keyboard navigation is key, and ESC means "no" on a confirmation dialog. If we're going to replace native functionality, we need to provide the same interactions in our replacement.

if we don't show the X then I think we should force the user to be explicit about their choice

We are. They can't do anything else until they deal with the confirm dialog. "no" is a valid response, and ESC allows the user to respond that way with from the keyboard, just like they could always do before.

The functionality we're trying to replicate here is that of window.confirm, and adding ESC handling gets us there.

I also agree that ESC traditionally maps to "Cancel" in basically any dialog that I can think of. It would be very confusing/annoying to me as a user if I was used to navigating using a keyboard, then opened a dialog with "showClose=false" and suddenly my normal means of navigation doesn't work anymore.

@stacey-gammon The reason I put this in the ModalOverlay rather than the ConfirmDialog itself was so that the functionality could be re-used for other dialog types when they are created. I figured that ESC->Cancel is universal enough that it would be re-used for any other dialog. If that's not desired then it is easy enough to move that code to the dialog itself.

dpenny52 · 2017-03-01T03:27:40Z

Updated with keyboard handling in confirm_modal.js

stacey-gammon

lgtm Thanks @dpenny52!

w33ble

Thanks @dpenny52 for the submission!

* #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 · 2017-03-01T22:41:09Z

5.x: 3264487
5.3: 6641bc1

elastic#10099 adds keyboard handling to close confirm_modal on ESC

4333032

w33ble assigned w33ble and stacey-gammon Feb 23, 2017

w33ble added the review label Feb 23, 2017

w33ble suggested changes Feb 27, 2017

View reviewed changes

remove resetSpyCounts, use beforeEach

dccafec

w33ble mentioned this pull request Feb 27, 2017

Remove resetSpyCounts, use beforeEach dpenny52/kibana#1

Merged

dpenny52 added 2 commits February 27, 2017 20:45

Merge pull request #1 from w33ble/fix/modal-tests-beforeeach

e1b31c6

Remove resetSpyCounts, use beforeEach

elastic#10099 change from .bind to .on and remove unnecessary onKeyDo…

6761493

…wn call

stacey-gammon suggested changes Feb 28, 2017

View reviewed changes

elastic#10099 move ESC handling to confirm_modal.js

13366a5

stacey-gammon approved these changes Mar 1, 2017

View reviewed changes

w33ble approved these changes Mar 1, 2017

View reviewed changes

w33ble merged commit cbe26ea into elastic:master Mar 1, 2017

w33ble added v5.2.2 v5.3.1 v6.0.0 v5.4.0 and removed v5.2.2 labels Mar 1, 2017

epixa added v5.3.0 and removed v5.3.1 labels Mar 3, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#10099 adds keyboard handling to close confirm_modal on ESC #10455

#10099 adds keyboard handling to close confirm_modal on ESC #10455

dpenny52 commented Feb 19, 2017

elasticmachine commented Feb 19, 2017

dpenny52 commented Feb 19, 2017 •

edited

Loading

w33ble Feb 27, 2017

w33ble Feb 27, 2017

w33ble Feb 27, 2017

w33ble Feb 27, 2017

dpenny52 commented Feb 27, 2017

w33ble commented Feb 27, 2017

dpenny52 commented Feb 28, 2017

stacey-gammon Feb 28, 2017

w33ble Feb 28, 2017 •

edited

Loading

w33ble Feb 28, 2017

stacey-gammon Feb 28, 2017

dpenny52 Feb 28, 2017

w33ble Mar 1, 2017 •

edited

Loading

stacey-gammon Feb 28, 2017

w33ble Feb 28, 2017

dpenny52 Feb 28, 2017 •

edited

Loading

dpenny52 commented Mar 1, 2017

stacey-gammon left a comment

w33ble left a comment

w33ble commented Mar 1, 2017

		@@ -1 +1 @@
		<div class="kuiModalOverlay"></div>
		<div class="kuiModalOverlay" ng-keydown="onKeyDown($event)"></div>

#10099 adds keyboard handling to close confirm_modal on ESC #10455

#10099 adds keyboard handling to close confirm_modal on ESC #10455

Conversation

dpenny52 commented Feb 19, 2017

elasticmachine commented Feb 19, 2017

dpenny52 commented Feb 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpenny52 commented Feb 27, 2017

w33ble commented Feb 27, 2017

dpenny52 commented Feb 28, 2017

Choose a reason for hiding this comment

w33ble Feb 28, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

w33ble Mar 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpenny52 Feb 28, 2017 • edited Loading

Choose a reason for hiding this comment

dpenny52 commented Mar 1, 2017

stacey-gammon left a comment

Choose a reason for hiding this comment

w33ble left a comment

Choose a reason for hiding this comment

w33ble commented Mar 1, 2017

dpenny52 commented Feb 19, 2017 •

edited

Loading

w33ble Feb 28, 2017 •

edited

Loading

w33ble Mar 1, 2017 •

edited

Loading

dpenny52 Feb 28, 2017 •

edited

Loading