Skip to content

Commit

Permalink
fix: focus input element when overlay is opened (vaadin#5966) (vaadin…
Browse files Browse the repository at this point in the history
  • Loading branch information
bwajtr authored Jun 16, 2023
1 parent 6c5a358 commit e608d52
Show file tree
Hide file tree
Showing 17 changed files with 94 additions and 7 deletions.
16 changes: 16 additions & 0 deletions packages/combo-box/src/vaadin-combo-box-light.js
Original file line number Diff line number Diff line change
Expand Up @@ -201,6 +201,22 @@ class ComboBoxLight extends ComboBoxDataProviderMixin(ComboBoxMixin(ValidateMixi
this._clear();
}
}

/**
* @protected
* @override
*/
_onFocusout(event) {
const isBlurringControlButtons = event.target === this._toggleElement || event.target === this.clearElement;
const isFocusingInputElement = event.relatedTarget && event.relatedTarget === this._nativeInput;

// Prevent closing the overlay when moving focus from clear or toggle buttons to the internal input
if (isBlurringControlButtons && isFocusingInputElement) {
return;
}

super._onFocusout(event);
}
}

customElements.define(ComboBoxLight.is, ComboBoxLight);
Expand Down
4 changes: 3 additions & 1 deletion packages/combo-box/src/vaadin-combo-box-mixin.js
Original file line number Diff line number Diff line change
Expand Up @@ -544,7 +544,9 @@ export const ComboBoxMixin = (subclass) =>
// For touch devices, we don't want to popup virtual keyboard
// unless input element is explicitly focused by the user.
if (!this._isInputFocused() && !isTouch) {
this.focus();
if (this.inputElement) {
this.inputElement.focus();
}
}

this.$.overlay.restoreFocusOnClose = true;
Expand Down
11 changes: 10 additions & 1 deletion packages/combo-box/test/combo-box-light.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import './not-animated-styles.js';
import '../vaadin-combo-box-light.js';
import '@polymer/polymer/lib/elements/dom-repeat.js';
import { html, PolymerElement } from '@polymer/polymer/polymer-element.js';
import { isTouch } from '@vaadin/component-base/src/browser-utils.js';
import { getFirstItem } from './helpers.js';

class MyInput extends PolymerElement {
Expand Down Expand Up @@ -281,10 +282,11 @@ describe('custom buttons', () => {
});

describe('toggle-button', () => {
let toggleButton;
let toggleButton, inputElement;

beforeEach(() => {
toggleButton = comboBox.querySelector('.toggle-button');
inputElement = comboBox.querySelector('input');
});

it('should toggle overlay by clicking toggle element', () => {
Expand Down Expand Up @@ -329,6 +331,13 @@ describe('custom buttons', () => {

expect(comboBox.opened).to.be.true;
});

// WebKit returns true for isTouch in the test envirnoment. This test fails when isTouch == true, which is a correct behavior
(isTouch ? it.skip : it)('should focus input element on toggle button click', () => {
click(toggleButton);
expect(comboBox.opened).to.be.true;
expect(document.activeElement).to.equal(inputElement);
});
});

describe('clear-button', () => {
Expand Down
16 changes: 16 additions & 0 deletions packages/multi-select-combo-box/test/basic.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { sendKeys } from '@web/test-runner-commands';
import sinon from 'sinon';
import './not-animated-styles.js';
import '../vaadin-multi-select-combo-box.js';
import { isTouch } from '@vaadin/component-base/src/browser-utils.js';

describe('basic', () => {
let comboBox, internal, inputElement;
Expand Down Expand Up @@ -225,6 +226,21 @@ describe('basic', () => {
});
});

describe('toggle button', () => {
let toggleButton;

beforeEach(() => {
toggleButton = comboBox.$.toggleButton;
});

// WebKit returns true for isTouch in the test envirnoment. This test fails when isTouch == true, which is a correct behavior
(isTouch ? it.skip : it)('should focus input element on toggle button click', () => {
toggleButton.click();
expect(internal.opened).to.be.true;
expect(document.activeElement).to.equal(inputElement);
});
});

describe('chips', () => {
const getChips = (combo) => combo.shadowRoot.querySelectorAll('[part~="chip"]');

Expand Down
1 change: 1 addition & 0 deletions packages/multi-select-combo-box/test/visual/common.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ registerStyles(
'vaadin-multi-select-combo-box',
css`
:host([focused][focus-ring]) ::slotted(input),
:host([focused][has-value]) ::slotted(input),
:host([focused][opened]) ::slotted(input) {
caret-color: transparent !important;
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { fixtureSync } from '@vaadin/testing-helpers/dist/fixture.js';
import { fixtureSync, mousedown } from '@vaadin/testing-helpers';
import { sendKeys } from '@web/test-runner-commands';
import { visualDiff } from '@web/test-runner-visual-regression';
import '../common.js';
Expand All @@ -16,6 +16,13 @@ describe('multi-select-combo-box', () => {
element.items = ['Apple', 'Banana', 'Lemon', 'Pear'];
});

afterEach(() => {
// After tests which use sendKeys() the focus-utils.js -> isKeyboardActive is set to true.
// Click once here on body to reset it so other tests are not affected by it.
// An unwanted focus-ring would be shown in other tests otherwise.
mousedown(document.body);
});

it('basic', async () => {
await visualDiff(div, 'basic');
});
Expand Down
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { fixtureSync } from '@vaadin/testing-helpers/dist/fixture.js';
import { fixtureSync, mousedown } from '@vaadin/testing-helpers';
import { sendKeys } from '@web/test-runner-commands';
import { visualDiff } from '@web/test-runner-visual-regression';
import '../common.js';
Expand All @@ -16,6 +16,13 @@ describe('multi-select-combo-box', () => {
element.items = ['Apple', 'Banana', 'Lemon', 'Pear'];
});

afterEach(() => {
// After tests which use sendKeys() the focus-utils.js -> isKeyboardActive is set to true.
// Click once here on body to reset it so other tests are not affected by it.
// An unwanted focus-ring would be shown in other tests otherwise.
mousedown(document.body);
});

it('basic', async () => {
await visualDiff(div, 'basic');
});
Expand Down
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Original file line number Diff line number Diff line change
Expand Up @@ -318,7 +318,10 @@ snapshots["vaadin-time-picker host name"] =
/* end snapshot vaadin-time-picker host name */

snapshots["vaadin-time-picker host opened default"] =
`<vaadin-time-picker opened="">
`<vaadin-time-picker
focused=""
opened=""
>
<label
for="input-vaadin-time-picker-4"
id="label-vaadin-time-picker-0"
Expand Down
16 changes: 16 additions & 0 deletions packages/time-picker/test/time-picker.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { sendKeys } from '@web/test-runner-commands';
import sinon from 'sinon';
import './not-animated-styles.js';
import '../vaadin-time-picker.js';
import { isTouch } from '@vaadin/component-base/src/browser-utils.js';
import { outsideClick, setInputValue } from './helpers.js';

describe('time-picker', () => {
Expand Down Expand Up @@ -278,6 +279,21 @@ describe('time-picker', () => {
});
});

describe('toggle button', () => {
let toggleButton;

beforeEach(() => {
toggleButton = timePicker.$.toggleButton;
});

// WebKit returns true for isTouch in the test envirnoment. This test fails when isTouch == true, which is a correct behavior
(isTouch ? it.skip : it)('should focus input element on toggle button click', () => {
toggleButton.click();
expect(comboBox.opened).to.be.true;
expect(document.activeElement).to.equal(inputElement);
});
});

describe('autoselect', () => {
it('should set autoselect to false by default', () => {
expect(timePicker.autoselect).to.be.false;
Expand Down
7 changes: 6 additions & 1 deletion packages/time-picker/test/visual/lumo/time-picker.test.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { fixtureSync } from '@vaadin/testing-helpers/dist/fixture.js';
import { fixtureSync, mousedown } from '@vaadin/testing-helpers';
import { sendKeys } from '@web/test-runner-commands';
import { visualDiff } from '@web/test-runner-visual-regression';
import '../common.js';
Expand All @@ -22,6 +22,11 @@ describe('time-picker', () => {
await sendKeys({ press: 'Tab' });

await visualDiff(div, 'focus-ring');

// At this moment focus-utils.js -> isKeyboardActive is true.
// Click once here to reset it so other tests are not affected by it.
// A focus-ring would be shown in other tests otherwise.
mousedown(document.body);
});

it('disabled', async () => {
Expand Down
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { fixtureSync } from '@vaadin/testing-helpers/dist/fixture.js';
import { fixtureSync, mousedown } from '@vaadin/testing-helpers';
import { sendKeys } from '@web/test-runner-commands';
import { visualDiff } from '@web/test-runner-visual-regression';
import '../common.js';
Expand All @@ -22,6 +22,11 @@ describe('time-picker', () => {
await sendKeys({ press: 'Tab' });

await visualDiff(div, 'focus-ring');

// At this moment focus-utils.js -> isKeyboardActive is true.
// Click once here to reset it so other tests are not affected by it.
// A focus-ring would be shown in other tests otherwise.
mousedown(document.body);
});

it('disabled', async () => {
Expand Down

0 comments on commit e608d52

Please sign in to comment.