From a9ee082660ad1608508c9be0f01f0117097bb919 Mon Sep 17 00:00:00 2001 From: Stacey Gammon Date: Wed, 17 May 2017 09:48:26 -0400 Subject: [PATCH] Fix modals in react (#11714) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix focused button not being set Need to do it in a timeout because ng-react hasn’t loaded the react code yet, so it can’t find the buttons. * add a test that would have caught the issue, and also change the default focused button for the listing delete page since it’s a destructive action. * Add a comment * Push focusing of default button into react code * Push ESC key handling into react as well * Remove ESC tests in angular form They don’t pass, I’m guessing because of the way it’s triggered, but these test cases should now be handled in jest. Also confirmed it works as expected with the modal. * Address code review comments * Use autoFocus --- .../dashboard/listing/dashboard_listing.html | 3 + .../dashboard/listing/dashboard_listing.js | 4 +- .../public/modals/__tests__/confirm_modal.js | 34 ------- src/ui/public/modals/confirm_modal.html | 1 + src/ui/public/modals/confirm_modal.js | 22 +---- src/ui/public/modals/index.js | 2 + .../apps/dashboard/_dashboard_listing.js | 69 ++++++++++++++ test/functional/apps/dashboard/index.js | 1 + test/functional/page_objects/common_page.js | 4 + .../functional/page_objects/dashboard_page.js | 23 ++++- .../components/modal/confirm_modal.js | 25 ++++- .../components/modal/confirm_modal.test.js | 91 ++++++++++++++++--- ui_framework/services/index.js | 1 + ui_framework/services/key_codes.js | 1 + 14 files changed, 211 insertions(+), 70 deletions(-) create mode 100644 test/functional/apps/dashboard/_dashboard_listing.js create mode 100644 ui_framework/services/key_codes.js diff --git a/src/core_plugins/kibana/public/dashboard/listing/dashboard_listing.html b/src/core_plugins/kibana/public/dashboard/listing/dashboard_listing.html index 1179263c1858..b5c36852ff1b 100644 --- a/src/core_plugins/kibana/public/dashboard/listing/dashboard_listing.html +++ b/src/core_plugins/kibana/public/dashboard/listing/dashboard_listing.html @@ -43,6 +43,7 @@ ng-if="listingController.getSelectedItemsCount() > 0" tooltip="Delete selected dashboards" tooltip-append-to-body="true" + data-test-subj="deleteSelectedDashboards" > @@ -100,6 +101,7 @@
@@ -155,6 +157,7 @@ diff --git a/src/core_plugins/kibana/public/dashboard/listing/dashboard_listing.js b/src/core_plugins/kibana/public/dashboard/listing/dashboard_listing.js index ec90a9f0cec5..c9b7094b7613 100644 --- a/src/core_plugins/kibana/public/dashboard/listing/dashboard_listing.js +++ b/src/core_plugins/kibana/public/dashboard/listing/dashboard_listing.js @@ -3,6 +3,7 @@ import 'ui/pager_control'; import 'ui/pager'; import { DashboardConstants, createDashboardEditUrl } from '../dashboard_constants'; import { SortableProperties } from 'ui_framework/services'; +import { ConfirmationButtonTypes } from 'ui/modals'; export function DashboardListingController($injector, $scope) { const $filter = $injector.get('$filter'); @@ -125,7 +126,8 @@ export function DashboardListingController($injector, $scope) { 'Are you sure you want to delete the selected dashboards? This action is irreversible!', { confirmButtonText: 'Delete', - onConfirm: doDelete + onConfirm: doDelete, + defaultFocusedButton: ConfirmationButtonTypes.CANCEL }); }; diff --git a/src/ui/public/modals/__tests__/confirm_modal.js b/src/ui/public/modals/__tests__/confirm_modal.js index 163e3f3598b4..28683a4407ac 100644 --- a/src/ui/public/modals/__tests__/confirm_modal.js +++ b/src/ui/public/modals/__tests__/confirm_modal.js @@ -114,39 +114,5 @@ describe('ui/modals/confirm_modal', function () { expect(confirmCallback.called).to.be(true); expect(cancelCallback.called).to.be(false); }); - - it('onKeyDown detects ESC and calls onCancel', function () { - const confirmModalOptions = { - confirmButtonText: 'bye', - onConfirm: confirmCallback, - onCancel: cancelCallback, - title: 'hi' - }; - - 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 () { - const confirmModalOptions = { - confirmButtonText: 'bye', - onConfirm: confirmCallback, - onCancel: cancelCallback, - title: 'hi' - }; - - confirmModal('hi', confirmModalOptions); - - const e = angular.element.Event('keydown'); // eslint-disable-line new-cap - e.keyCode = 50; - angular.element(document.body).trigger(e); - - expect(cancelCallback.called).to.be(false); - }); }); }); diff --git a/src/ui/public/modals/confirm_modal.html b/src/ui/public/modals/confirm_modal.html index 9b4044189ab7..05b3cc1c5969 100644 --- a/src/ui/public/modals/confirm_modal.html +++ b/src/ui/public/modals/confirm_modal.html @@ -6,4 +6,5 @@ cancel-button-text="cancelButtonText" message="message" title="title" + default-focused-button="defaultFocusedButton" > diff --git a/src/ui/public/modals/confirm_modal.js b/src/ui/public/modals/confirm_modal.js index 6e9048803b76..3e64631e2933 100644 --- a/src/ui/public/modals/confirm_modal.js +++ b/src/ui/public/modals/confirm_modal.js @@ -6,9 +6,11 @@ import { ModalOverlay } from './modal_overlay'; const module = uiModules.get('kibana'); +import { CONFIRM_BUTTON, CANCEL_BUTTON } from 'ui_framework/components/modal/confirm_modal'; + export const ConfirmationButtonTypes = { - CONFIRM: 'Confirm', - CANCEL: 'Cancel' + CONFIRM: CONFIRM_BUTTON, + CANCEL: CANCEL_BUTTON }; /** @@ -48,6 +50,7 @@ module.factory('confirmModal', function ($rootScope, $compile) { const confirmScope = $rootScope.$new(); confirmScope.message = message; + confirmScope.defaultFocusedButton = options.defaultFocusedButton; confirmScope.confirmButtonText = options.confirmButtonText; confirmScope.cancelButtonText = options.cancelButtonText; confirmScope.title = options.title; @@ -67,21 +70,6 @@ module.factory('confirmModal', function ($rootScope, $compile) { function showModal(confirmScope) { const modalInstance = $compile(template)(confirmScope); modalPopover = new ModalOverlay(modalInstance); - angular.element(document.body).on('keydown', (event) => { - if (event.keyCode === 27) { - confirmScope.onCancel(); - } - }); - - switch (options.defaultFocusedButton) { - case ConfirmationButtonTypes.CONFIRM: - modalInstance.find('[data-test-subj=confirmModalConfirmButton]').focus(); - break; - case ConfirmationButtonTypes.CANCEL: - modalInstance.find('[data-test-subj=confirmModalCancelButton]').focus(); - break; - default: - } } if (modalPopover) { diff --git a/src/ui/public/modals/index.js b/src/ui/public/modals/index.js index cba0dd8c9ea0..8052ced3b514 100644 --- a/src/ui/public/modals/index.js +++ b/src/ui/public/modals/index.js @@ -1,2 +1,4 @@ import './confirm_modal'; import './confirm_modal_promise'; + +export { ConfirmationButtonTypes } from './confirm_modal'; diff --git a/test/functional/apps/dashboard/_dashboard_listing.js b/test/functional/apps/dashboard/_dashboard_listing.js new file mode 100644 index 000000000000..0bdfc80ce5d0 --- /dev/null +++ b/test/functional/apps/dashboard/_dashboard_listing.js @@ -0,0 +1,69 @@ +import expect from 'expect.js'; + +export default function ({ getPageObjects }) { + const PageObjects = getPageObjects(['dashboard', 'header', 'common']); + + describe('dashboard listing page', function describeIndexTests() { + const dashboardName = 'Dashboard Listing Test'; + + before(async function () { + await PageObjects.dashboard.initTests(); + }); + + describe('create prompt', async () => { + it('appears when there are no dashboards', async function () { + const promptExists = await PageObjects.dashboard.getCreateDashboardPromptExists(); + expect(promptExists).to.be(true); + }); + + it('creates a new dashboard', async function () { + await PageObjects.dashboard.clickCreateDashboardPrompt(); + await PageObjects.dashboard.saveDashboard(dashboardName); + await PageObjects.header.clickToastOK(); + + await PageObjects.dashboard.gotoDashboardLandingPage(); + const countOfDashboards = await PageObjects.dashboard.getDashboardCountWithName(dashboardName); + expect(countOfDashboards).to.equal(1); + }); + + it('is not shown when there is a dashboard', async function () { + const promptExists = await PageObjects.dashboard.getCreateDashboardPromptExists(); + expect(promptExists).to.be(false); + }); + + it('is not shown when there are no dashboards shown during a search', async function () { + const countOfDashboards = await PageObjects.dashboard.getDashboardCountWithName('gobeldeguck'); + expect(countOfDashboards).to.equal(0); + + const promptExists = await PageObjects.dashboard.getCreateDashboardPromptExists(); + expect(promptExists).to.be(false); + }); + }); + + describe('delete', async function () { + it('default confirm action is cancel', async function () { + await PageObjects.dashboard.searchForDashboardWithName(''); + await PageObjects.dashboard.clickListItemCheckbox(); + await PageObjects.dashboard.clickDeleteSelectedDashboards(); + + await PageObjects.common.pressEnterKey(); + + const isConfirmOpen = await PageObjects.common.isConfirmModalOpen(); + expect(isConfirmOpen).to.be(false); + + const countOfDashboards = await PageObjects.dashboard.getDashboardCountWithName(dashboardName); + expect(countOfDashboards).to.equal(1); + }); + + it('succeeds on confirmation press', async function () { + await PageObjects.dashboard.clickListItemCheckbox(); + await PageObjects.dashboard.clickDeleteSelectedDashboards(); + + await PageObjects.common.clickConfirmOnModal(); + + const countOfDashboards = await PageObjects.dashboard.getDashboardCountWithName(dashboardName); + expect(countOfDashboards).to.equal(0); + }); + }); + }); +} diff --git a/test/functional/apps/dashboard/index.js b/test/functional/apps/dashboard/index.js index e254d0a26f5a..4ddc32663d2f 100644 --- a/test/functional/apps/dashboard/index.js +++ b/test/functional/apps/dashboard/index.js @@ -8,5 +8,6 @@ export default function ({ getService, loadTestFile }) { loadTestFile(require.resolve('./_dashboard')); loadTestFile(require.resolve('./_dashboard_save')); loadTestFile(require.resolve('./_dashboard_time')); + loadTestFile(require.resolve('./_dashboard_listing')); }); } diff --git a/test/functional/page_objects/common_page.js b/test/functional/page_objects/common_page.js index ae2f921c4eb9..edf6726a8907 100644 --- a/test/functional/page_objects/common_page.js +++ b/test/functional/page_objects/common_page.js @@ -229,6 +229,10 @@ export function CommonPageProvider({ getService, getPageObjects }) { await this.ensureModalOverlayHidden(); } + async pressEnterKey() { + await remote.pressKeys('\uE007'); + } + async clickCancelOnModal() { log.debug('Clicking modal cancel'); await testSubjects.click('confirmModalCancelButton'); diff --git a/test/functional/page_objects/dashboard_page.js b/test/functional/page_objects/dashboard_page.js index 9daa2aca701a..490739628fcf 100644 --- a/test/functional/page_objects/dashboard_page.js +++ b/test/functional/page_objects/dashboard_page.js @@ -93,6 +93,22 @@ export function DashboardPageProvider({ getService, getPageObjects }) { return testSubjects.click('newDashboardLink'); } + async clickCreateDashboardPrompt() { + await retry.try(() => testSubjects.click('createDashboardPromptButton')); + } + + async getCreateDashboardPromptExists() { + return await testSubjects.exists('createDashboardPromptButton'); + } + + async clickListItemCheckbox() { + await testSubjects.click('dashboardListItemCheckbox'); + } + + async clickDeleteSelectedDashboards() { + await testSubjects.click('deleteSelectedDashboards'); + } + clickAddVisualization() { return testSubjects.click('dashboardAddPanelButton'); } @@ -135,9 +151,9 @@ export function DashboardPageProvider({ getService, getPageObjects }) { filterVizNames(vizName) { return retry.try(() => getRemote() - .findByCssSelector('input[placeholder="Visualizations Filter..."]') - .click() - .pressKeys(vizName)); + .findByCssSelector('input[placeholder="Visualizations Filter..."]') + .click() + .pressKeys(vizName)); } clickVizNameLink(vizName) { @@ -242,6 +258,7 @@ export function DashboardPageProvider({ getService, getPageObjects }) { await retry.try(async () => { const searchFilter = await testSubjects.find('searchFilter'); + await searchFilter.clearValue(); await searchFilter.click(); // Note: this replacement of - to space is to preserve original logic but I'm not sure why or if it's needed. await searchFilter.type(dashName.replace('-',' ')); diff --git a/ui_framework/components/modal/confirm_modal.js b/ui_framework/components/modal/confirm_modal.js index ad715fc52b94..806db8f10d99 100644 --- a/ui_framework/components/modal/confirm_modal.js +++ b/ui_framework/components/modal/confirm_modal.js @@ -8,6 +8,15 @@ import { KuiModalHeaderTitle } from './modal_header_title'; import { KuiModalBody } from './modal_body'; import { KuiModalBodyText } from './modal_body_text'; import { KuiButton } from '../index'; +import { ESC_KEY_CODE } from '../../services'; + +export const CONFIRM_BUTTON = 'confirm'; +export const CANCEL_BUTTON = 'cancel'; + +const CONFIRM_MODAL_BUTTONS = [ + CONFIRM_BUTTON, + CANCEL_BUTTON, +]; export function KuiConfirmModal({ message, @@ -17,7 +26,17 @@ export function KuiConfirmModal({ cancelButtonText, confirmButtonText, className, - ...rest }) { + defaultFocusedButton, + ...rest, + }) { + + const onKeyDown = (event) => { + // Treat the 'esc' key as a cancel indicator. + if (event.keyCode === ESC_KEY_CODE) { + onCancel(); + } + }; + const ariaLabel = rest['aria-label']; const dataTestSubj = rest['data-test-subj']; return ( @@ -26,6 +45,7 @@ export function KuiConfirmModal({ data-tests-subj={ dataTestSubj } aria-label={ ariaLabel } className={ className } + onKeyDown={ onKeyDown } > { title ? @@ -45,6 +65,7 @@ export function KuiConfirmModal({ @@ -52,6 +73,7 @@ export function KuiConfirmModal({ @@ -72,4 +94,5 @@ KuiConfirmModal.propTypes = { dataTestSubj: PropTypes.string, ariaLabel: PropTypes.string, className: PropTypes.string, + defaultFocusedButton: PropTypes.oneOf(CONFIRM_MODAL_BUTTONS) }; diff --git a/ui_framework/components/modal/confirm_modal.test.js b/ui_framework/components/modal/confirm_modal.test.js index fe3314bf66a9..216074d7ab2b 100644 --- a/ui_framework/components/modal/confirm_modal.test.js +++ b/ui_framework/components/modal/confirm_modal.test.js @@ -5,7 +5,7 @@ import { mount, render } from 'enzyme'; import { requiredProps } from '../../test/required_props'; import { - KuiConfirmModal, + CANCEL_BUTTON, CONFIRM_BUTTON, KuiConfirmModal, } from './confirm_modal'; let onConfirm; @@ -44,18 +44,81 @@ test('onConfirm', () => { sinon.assert.notCalled(onCancel); }); -test('onCancel', () => { - const component = mount(); - component.find('[data-test-subj="confirmModalCancelButton"]').simulate('click'); - sinon.assert.notCalled(onConfirm); - sinon.assert.calledOnce(onCancel); +describe('onCancel', () => { + test('triggerd by click', () => { + const component = mount(); + component.find('[data-test-subj="confirmModalCancelButton"]').simulate('click'); + sinon.assert.notCalled(onConfirm); + sinon.assert.calledOnce(onCancel); + }); + + test('triggered by esc key', () => { + const component = mount(); + component.simulate('keydown', { keyCode: 27 }); + sinon.assert.notCalled(onConfirm); + sinon.assert.calledOnce(onCancel); + }); +}); + +describe('defaultFocusedButton', () => { + test('is cancel', () => { + const component = mount(); + const button = component.find('[data-test-subj="confirmModalCancelButton"]').getDOMNode(); + expect(document.activeElement).toEqual(button); + }); + + test('is confirm', () => { + const component = mount(); + const button = component.find('[data-test-subj="confirmModalConfirmButton"]').getDOMNode(); + expect(document.activeElement).toEqual(button); + }); + + test('when not given focuses on the confirm button', () => { + const component = mount(); + const button = component.find('[data-test-subj="confirmModalConfirmButton"]').getDOMNode(); + expect(document.activeElement).toEqual(button); + }); }); diff --git a/ui_framework/services/index.js b/ui_framework/services/index.js index 287383f7e057..06f62c9687d7 100644 --- a/ui_framework/services/index.js +++ b/ui_framework/services/index.js @@ -1 +1,2 @@ export { SortableProperties } from './sort'; +export { ESC_KEY_CODE } from './key_codes'; diff --git a/ui_framework/services/key_codes.js b/ui_framework/services/key_codes.js new file mode 100644 index 000000000000..0dfedd711f7e --- /dev/null +++ b/ui_framework/services/key_codes.js @@ -0,0 +1 @@ +export const ESC_KEY_CODE = 27;