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.

Sign up for GitHub

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

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

Merged
merged 5 commits into from
Mar 1, 2017
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4333032
#10099 adds keyboard handling to close confirm_modal on ESC
dpenny52 Feb 19, 2017
dccafec
remove resetSpyCounts, use beforeEach
w33ble Feb 27, 2017
e1b31c6
Merge pull request #1 from w33ble/fix/modal-tests-beforeeach
dpenny52 Feb 28, 2017
6761493
#10099 change from .bind to .on and remove unnecessary onKeyDown call
dpenny52 Feb 28, 2017
13366a5
#10099 move ESC handling to confirm_modal.js
dpenny52 Mar 1, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 43 additions & 0 deletions src/ui/public/modals/__tests__/confirm_modal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -182,5 +182,48 @@ describe('ui/modals/confirm_modal', function () {
expect(confirmCallback.called).to.be(false);
expect(cancelCallback.called).to.be(true);
});

it('onKeyDown detects ESC and calls onCancel', function () {
resetSpyCounts();
const confirmModalOptions = {
confirmButtonText: 'bye',
onConfirm: confirmCallback,
onCancel: cancelCallback,
onClose: closeCallback,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

title: 'hi',
showClose: false
};

confirmModal('hi', confirmModalOptions);

const e = angular.element.Event('keydown'); // eslint-disable-line new-cap
e.keyCode = 27;
angular.element(document.body).trigger(e);

expect(cancelCallback.called).to.be(true);
});

it('onKeyDown ignores keys other than ESC', function () {
resetSpyCounts();
const confirmModalOptions = {
confirmButtonText: 'bye',
onConfirm: confirmCallback,
onCancel: cancelCallback,
onClose: closeCallback,
title: 'hi',
showClose: false
};

confirmModal('hi', confirmModalOptions);

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

e.keyCode = Math.floor((Math.random() * 100) + 1);
}
angular.element(document.body).trigger(e);

expect(cancelCallback.called).to.be(false);
});
});
});
1 change: 0 additions & 1 deletion src/ui/public/modals/confirm_modal.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
<div class="kuiModalHeader__title" data-test-subj="confirmModalTitleText">
{{title}}
</div>

<div
ng-if="showClose"
class="kuiModalHeaderCloseButton kuiIcon fa-times"
Expand Down
2 changes: 1 addition & 1 deletion src/ui/public/modals/confirm_modal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ module.factory('confirmModal', function ($rootScope, $compile) {
};

const modalInstance = $compile(template)(confirmScope);
modalPopover = new ModalOverlay(modalInstance);
modalPopover = new ModalOverlay(modalInstance, confirmScope);
modalInstance.find('[data-test-subj=confirmModalConfirmButton]').focus();

function destroy() {
Expand Down
2 changes: 1 addition & 1 deletion src/ui/public/modals/modal_overlay.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
<div class="kuiModalOverlay"></div>
<div class="kuiModalOverlay" ng-keydown="onKeyDown($event)"></div>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

11 changes: 10 additions & 1 deletion src/ui/public/modals/modal_overlay.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,16 +5,25 @@ 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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@dpenny52 dpenny52 Feb 28, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

this.overlayElement = angular.element(modalOverlayTemplate);
this.overlayElement.append(modalElement);

const onKeyDown = (event) => {
if(event.keyCode === 27) {
scope.onCancel();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@w33ble w33ble Feb 28, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@w33ble w33ble Mar 1, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
};

angular.element(document.body).bind('keydown', onKeyDown);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use .on here instead of .bind

angular.element(document.body).append(this.overlayElement);
}

/**
* Removes the overlay and modal from the dom.
*/
destroy() {
angular.element(document.body).off('keydown');
this.overlayElement.remove();
}
}