Skip to content

Commit

Permalink
fix!: prevent focusout when closing combo-box on outside click (#7846) (
Browse files Browse the repository at this point in the history
#7859)

Co-authored-by: Serhii Kulykov <[email protected]>
  • Loading branch information
vaadin-bot and web-padawan authored Sep 24, 2024
1 parent 7a1399e commit 4da3920
Show file tree
Hide file tree
Showing 9 changed files with 109 additions and 59 deletions.
1 change: 0 additions & 1 deletion packages/combo-box/src/vaadin-combo-box-light.js
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,6 @@ class ComboBoxLight extends ComboBoxLightMixin(ThemableMixin(PolymerElement)) {
theme$="[[_theme]]"
position-target="[[inputElement]]"
no-vertical-overlap
restore-focus-node="[[inputElement]]"
></vaadin-combo-box-overlay>
`;
}
Expand Down
6 changes: 1 addition & 5 deletions packages/combo-box/src/vaadin-combo-box-mixin.js
Original file line number Diff line number Diff line change
Expand Up @@ -619,8 +619,6 @@ export const ComboBoxMixin = (subclass) =>
this.inputElement.focus();
}
}

this._overlayElement.restoreFocusOnClose = true;
} else {
this._onClosed();
}
Expand Down Expand Up @@ -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
Expand Down
15 changes: 15 additions & 0 deletions packages/combo-box/src/vaadin-combo-box-overlay-mixin.js
Original file line number Diff line number Diff line change
Expand Up @@ -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';

/**
Expand Down Expand Up @@ -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;
Expand Down
1 change: 0 additions & 1 deletion packages/combo-box/src/vaadin-combo-box.js
Original file line number Diff line number Diff line change
Expand Up @@ -206,7 +206,6 @@ class ComboBox extends ComboBoxDataProviderMixin(
theme$="[[_theme]]"
position-target="[[_positionTarget]]"
no-vertical-overlap
restore-focus-node="[[inputElement]]"
></vaadin-combo-box-overlay>
<slot name="tooltip"></slot>
Expand Down
1 change: 0 additions & 1 deletion packages/combo-box/src/vaadin-lit-combo-box-light.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
></vaadin-combo-box-overlay>
`;
Expand Down
1 change: 0 additions & 1 deletion packages/combo-box/src/vaadin-lit-combo-box.js
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,6 @@ class ComboBox extends ComboBoxDataProviderMixin(
?loading="${this.loading}"
theme="${ifDefined(this._theme)}"
.positionTarget="${this._positionTarget}"
.restoreFocusNode="${this.inputElement}"
no-vertical-overlap
></vaadin-combo-box-overlay>
Expand Down
62 changes: 29 additions & 33 deletions packages/combo-box/test/interactions.common.js
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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', () => {
Expand Down Expand Up @@ -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;
});
});
});

Expand All @@ -251,6 +246,7 @@ describe('interactions', () => {
});

it('should re-enable virtual keyboard on blur', async () => {
comboBox.focus();
comboBox.open();
comboBox.close();
await aTimeout(0);
Expand Down
43 changes: 27 additions & 16 deletions packages/date-time-picker/test/basic.test.js
Original file line number Diff line number Diff line change
@@ -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';
Expand Down Expand Up @@ -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('<vaadin-date-time-picker></vaadin-date-time-picker>');
datePicker = getDatePicker(dateTimePicker);
timePicker = getTimePicker(dateTimePicker);
});

afterEach(async () => {
await resetMouse();
});

it('should have default value', () => {
expect(dateTimePicker.value).to.equal('');
});
Expand Down Expand Up @@ -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;
});

Expand All @@ -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;
});
});
Expand Down
38 changes: 37 additions & 1 deletion test/integration/grid-pro-custom-editor.test.js
Original file line number Diff line number Diff line change
@@ -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';
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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', () => {
Expand All @@ -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');
});
});
});

0 comments on commit 4da3920

Please sign in to comment.