From 4e08610bf7321fcc7e44dfce269af9d6944c5b0b Mon Sep 17 00:00:00 2001 From: Serhii Kulykov Date: Tue, 24 Sep 2024 16:21:52 +0300 Subject: [PATCH] fix!: prevent focusout when closing combo-box on outside click (#7846) --- .../combo-box/src/vaadin-combo-box-light.js | 1 - .../combo-box/src/vaadin-combo-box-mixin.js | 6 +- .../src/vaadin-combo-box-overlay-mixin.js | 15 +++++ packages/combo-box/src/vaadin-combo-box.js | 1 - .../src/vaadin-lit-combo-box-light.js | 1 - .../combo-box/src/vaadin-lit-combo-box.js | 1 - .../combo-box/test/interactions.common.js | 62 +++++++++---------- packages/date-time-picker/test/basic.test.js | 43 ++++++++----- .../grid-pro-custom-editor.test.js | 38 +++++++++++- 9 files changed, 109 insertions(+), 59 deletions(-) diff --git a/packages/combo-box/src/vaadin-combo-box-light.js b/packages/combo-box/src/vaadin-combo-box-light.js index 9a3144ba71..db39e5b313 100644 --- a/packages/combo-box/src/vaadin-combo-box-light.js +++ b/packages/combo-box/src/vaadin-combo-box-light.js @@ -84,7 +84,6 @@ class ComboBoxLight extends ComboBoxLightMixin(ThemableMixin(PolymerElement)) { theme$="[[_theme]]" position-target="[[inputElement]]" no-vertical-overlap - restore-focus-node="[[inputElement]]" > `; } diff --git a/packages/combo-box/src/vaadin-combo-box-mixin.js b/packages/combo-box/src/vaadin-combo-box-mixin.js index d0250d2c5b..f01a788ded 100644 --- a/packages/combo-box/src/vaadin-combo-box-mixin.js +++ b/packages/combo-box/src/vaadin-combo-box-mixin.js @@ -619,8 +619,6 @@ export const ComboBoxMixin = (subclass) => this.inputElement.focus(); } } - - this._overlayElement.restoreFocusOnClose = true; } else { this._onClosed(); } @@ -719,9 +717,7 @@ export const ComboBoxMixin = (subclass) => _onKeyDown(e) { super._onKeyDown(e); - if (e.key === 'Tab') { - this._overlayElement.restoreFocusOnClose = false; - } else if (e.key === 'ArrowDown') { + if (e.key === 'ArrowDown') { this._onArrowDown(); // Prevent caret from moving diff --git a/packages/combo-box/src/vaadin-combo-box-overlay-mixin.js b/packages/combo-box/src/vaadin-combo-box-overlay-mixin.js index 47c3c1e820..defe881658 100644 --- a/packages/combo-box/src/vaadin-combo-box-overlay-mixin.js +++ b/packages/combo-box/src/vaadin-combo-box-overlay-mixin.js @@ -3,6 +3,7 @@ * Copyright (c) 2015 - 2024 Vaadin Ltd. * This program is available under Apache License Version 2.0, available at https://vaadin.com/license/ */ +import { isElementFocusable } from '@vaadin/a11y-base/src/focus-utils.js'; import { PositionMixin } from '@vaadin/overlay/src/vaadin-overlay-position-mixin.js'; /** @@ -46,6 +47,20 @@ export const ComboBoxOverlayMixin = (superClass) => return !eventPath.includes(this.positionTarget) && !eventPath.includes(this); } + /** + * @protected + * @override + */ + _mouseDownListener(event) { + super._mouseDownListener(event); + + // Prevent global mousedown event to avoid losing focus on outside click, + // unless the clicked element is also focusable (e.g. in date-time-picker). + if (this._shouldCloseOnOutsideClick(event) && !isElementFocusable(event.composedPath()[0])) { + event.preventDefault(); + } + } + /** @protected */ _updateOverlayWidth() { const propPrefix = this.localName; diff --git a/packages/combo-box/src/vaadin-combo-box.js b/packages/combo-box/src/vaadin-combo-box.js index db9184d1af..23a3d861fa 100644 --- a/packages/combo-box/src/vaadin-combo-box.js +++ b/packages/combo-box/src/vaadin-combo-box.js @@ -206,7 +206,6 @@ class ComboBox extends ComboBoxDataProviderMixin( theme$="[[_theme]]" position-target="[[_positionTarget]]" no-vertical-overlap - restore-focus-node="[[inputElement]]" > diff --git a/packages/combo-box/src/vaadin-lit-combo-box-light.js b/packages/combo-box/src/vaadin-lit-combo-box-light.js index 35afdb52b5..248281dfe1 100644 --- a/packages/combo-box/src/vaadin-lit-combo-box-light.js +++ b/packages/combo-box/src/vaadin-lit-combo-box-light.js @@ -46,7 +46,6 @@ class ComboBoxLight extends ComboBoxLightMixin(ThemableMixin(PolylitMixin(LitEle ?loading="${this.loading}" theme="${ifDefined(this._theme)}" .positionTarget="${this.inputElement}" - .restoreFocusNode="${this.inputElement}" no-vertical-overlap > `; diff --git a/packages/combo-box/src/vaadin-lit-combo-box.js b/packages/combo-box/src/vaadin-lit-combo-box.js index 9d5c386113..91be31c4fe 100644 --- a/packages/combo-box/src/vaadin-lit-combo-box.js +++ b/packages/combo-box/src/vaadin-lit-combo-box.js @@ -106,7 +106,6 @@ class ComboBox extends ComboBoxDataProviderMixin( ?loading="${this.loading}" theme="${ifDefined(this._theme)}" .positionTarget="${this._positionTarget}" - .restoreFocusNode="${this.inputElement}" no-vertical-overlap > diff --git a/packages/combo-box/test/interactions.common.js b/packages/combo-box/test/interactions.common.js index 3b80d68381..b587b31136 100644 --- a/packages/combo-box/test/interactions.common.js +++ b/packages/combo-box/test/interactions.common.js @@ -13,7 +13,7 @@ import { tap, touchstart, } from '@vaadin/testing-helpers'; -import { sendKeys } from '@web/test-runner-commands'; +import { resetMouse, sendKeys, sendMouse } from '@web/test-runner-commands'; import sinon from 'sinon'; import { isTouch } from '@vaadin/component-base/src/browser-utils.js'; import { getFirstItem, setInputValue } from './helpers.js'; @@ -161,30 +161,6 @@ describe('interactions', () => { expect(event.defaultPrevented).to.be.true; }); - it('should restore focus to the input on outside click', async () => { - comboBox.focus(); - comboBox.open(); - outsideClick(); - await aTimeout(0); - expect(document.activeElement).to.equal(input); - }); - - it('should restore focus to the input on toggle button click', async () => { - comboBox.focus(); - comboBox.open(); - comboBox._toggleElement.click(); - await aTimeout(0); - expect(document.activeElement).to.equal(input); - }); - - it('should focus the input on outside click if not focused before opening', async () => { - expect(document.activeElement).to.equal(document.body); - comboBox.open(); - outsideClick(); - await aTimeout(0); - expect(document.activeElement).to.equal(input); - }); - // NOTE: WebKit incorrectly detects touch environment // See https://github.com/vaadin/web-components/issues/257 (isTouch ? it.skip : it)('should focus the input on opening if not focused', () => { @@ -225,14 +201,33 @@ describe('interactions', () => { expect(comboBox.hasAttribute('focus-ring')).to.be.true; }); - it('should not keep focus-ring attribute after closing with outside click', () => { - comboBox.focus(); - comboBox.setAttribute('focus-ring', ''); - comboBox.open(); - outsideClick(); - expect(comboBox.hasAttribute('focus-ring')).to.be.false; - // FIXME: see https://github.com/vaadin/web-components/issues/4148 - // expect(comboBox.hasAttribute('focus-ring')).to.be.true; + describe('close on click', () => { + afterEach(async () => { + await resetMouse(); + }); + + it('should restore focus to the input on outside click', async () => { + comboBox.focus(); + comboBox.open(); + await sendMouse({ type: 'click', position: [200, 200] }); + expect(document.activeElement).to.equal(input); + }); + + it('should keep focus in the input on toggle button click', async () => { + comboBox.focus(); + comboBox.open(); + const rect = comboBox._toggleElement.getBoundingClientRect(); + await sendMouse({ type: 'click', position: [rect.x, rect.y] }); + expect(document.activeElement).to.equal(input); + }); + + it('should keep focus-ring attribute after closing with outside click', async () => { + comboBox.focus(); + comboBox.setAttribute('focus-ring', ''); + comboBox.open(); + await sendMouse({ type: 'click', position: [200, 200] }); + expect(comboBox.hasAttribute('focus-ring')).to.be.true; + }); }); }); @@ -251,6 +246,7 @@ describe('interactions', () => { }); it('should re-enable virtual keyboard on blur', async () => { + comboBox.focus(); comboBox.open(); comboBox.close(); await aTimeout(0); diff --git a/packages/date-time-picker/test/basic.test.js b/packages/date-time-picker/test/basic.test.js index 469e25e4d8..02abdc15a2 100644 --- a/packages/date-time-picker/test/basic.test.js +++ b/packages/date-time-picker/test/basic.test.js @@ -1,6 +1,6 @@ import { expect } from '@vaadin/chai-plugins'; import { aTimeout, fixtureSync, focusin, focusout, nextFrame, nextRender, outsideClick } from '@vaadin/testing-helpers'; -import { sendKeys } from '@web/test-runner-commands'; +import { resetMouse, sendKeys, sendMouse } from '@web/test-runner-commands'; import sinon from 'sinon'; import '../src/vaadin-date-time-picker.js'; import { changeInputValue } from './helpers.js'; @@ -41,12 +41,23 @@ describe('Basic features', () => { let datePicker; let timePicker; + async function click(element) { + const rect = element.inputElement.getBoundingClientRect(); + const x = Math.floor(rect.x + rect.width / 2); + const y = Math.floor(rect.y + rect.height / 2); + await sendMouse({ type: 'click', position: [x, y] }); + } + beforeEach(() => { dateTimePicker = fixtureSync(''); datePicker = getDatePicker(dateTimePicker); timePicker = getTimePicker(dateTimePicker); }); + afterEach(async () => { + await resetMouse(); + }); + it('should have default value', () => { expect(dateTimePicker.value).to.equal(''); }); @@ -189,13 +200,11 @@ describe('Basic features', () => { describe('date-picker focused', () => { it('should remove focused attribute on time-picker click', async () => { - datePicker.focus(); - datePicker.click(); + await click(datePicker); await nextRender(); expect(datePicker.hasAttribute('focused')).to.be.true; - timePicker.focus(); - timePicker.click(); + await click(timePicker); expect(datePicker.hasAttribute('focused')).to.be.false; }); @@ -207,37 +216,39 @@ describe('Basic features', () => { await nextRender(); expect(datePicker.hasAttribute('focus-ring')).to.be.true; - timePicker.focus(); - timePicker.click(); + await click(timePicker); expect(datePicker.hasAttribute('focus-ring')).to.be.false; }); }); describe('time-picker focused', () => { + beforeEach(() => { + // Disable auto-open to make tests more reliable by only moving + // focus on mousedown (and not the date-picker overlay opening). + dateTimePicker.autoOpenDisabled = true; + }); + it('should remove focused attribute on date-picker click', async () => { - timePicker.focus(); - timePicker.click(); + await click(timePicker); + // Open the overlay with the keyboard + await sendKeys({ press: 'ArrowDown' }); await nextRender(); expect(timePicker.hasAttribute('focused')).to.be.true; - datePicker.focus(); - datePicker.click(); - await nextRender(); + await click(datePicker); expect(timePicker.hasAttribute('focused')).to.be.false; }); it('should remove focus-ring attribute on date-picker click', async () => { // Focus the time-picker with the keyboard - await sendKeys({ press: 'Tab' }); + datePicker.focus(); await sendKeys({ press: 'Tab' }); // Open the overlay with the keyboard await sendKeys({ press: 'ArrowDown' }); await nextRender(); expect(timePicker.hasAttribute('focus-ring')).to.be.true; - datePicker.focus(); - datePicker.click(); - await nextRender(); + await click(datePicker); expect(timePicker.hasAttribute('focus-ring')).to.be.false; }); }); diff --git a/test/integration/grid-pro-custom-editor.test.js b/test/integration/grid-pro-custom-editor.test.js index fef9e8c240..59ec1b8b34 100644 --- a/test/integration/grid-pro-custom-editor.test.js +++ b/test/integration/grid-pro-custom-editor.test.js @@ -1,6 +1,6 @@ import { expect } from '@vaadin/chai-plugins'; import { fixtureSync, nextRender } from '@vaadin/testing-helpers'; -import { sendKeys } from '@web/test-runner-commands'; +import { resetMouse, sendKeys, sendMouse } from '@web/test-runner-commands'; import '@vaadin/combo-box'; import '@vaadin/date-picker'; import '@vaadin/grid-pro'; @@ -57,6 +57,10 @@ describe('grid-pro custom editor', () => { await nextRender(); }); + afterEach(async () => { + await resetMouse(); + }); + describe('date-picker', () => { it('should apply the updated date to the cell when exiting on Tab', async () => { const cell = getContainerCell(grid.$.items, 0, 2); @@ -107,6 +111,22 @@ describe('grid-pro custom editor', () => { expect(cell._content.textContent).to.equal('active'); }); + + it('should not stop editing and update value when closing on outside click', async () => { + const cell = getContainerCell(grid.$.items, 0, 3); + cell.focus(); + + await sendKeys({ press: 'Enter' }); + + await sendKeys({ press: 'ArrowDown' }); + await sendKeys({ type: 'active' }); + + await sendMouse({ type: 'click', position: [10, 10] }); + + const editor = cell._content.querySelector('vaadin-combo-box'); + expect(editor).to.be.ok; + expect(editor.value).to.equal('active'); + }); }); describe('time-picker', () => { @@ -133,5 +153,21 @@ describe('grid-pro custom editor', () => { expect(cell._content.textContent).to.equal('10:00'); }); + + it('should not stop editing and update value when closing on outside click', async () => { + const cell = getContainerCell(grid.$.items, 0, 4); + cell.focus(); + + await sendKeys({ press: 'Enter' }); + + await sendKeys({ press: 'ArrowDown' }); + await sendKeys({ type: '10:00' }); + + await sendMouse({ type: 'click', position: [10, 10] }); + + const editor = cell._content.querySelector('vaadin-time-picker'); + expect(editor).to.be.ok; + expect(editor.value).to.equal('10:00'); + }); }); });