From 2683a730807fa4fcf3ce44a9f4e3294c8e060147 Mon Sep 17 00:00:00 2001 From: Sciurus7 Date: Fri, 14 Apr 2023 13:39:05 +0200 Subject: [PATCH 1/3] fix(form-core) sync autofocus to focusable node --- .changeset/silly-shirts-promise.md | 5 +++ .../ui/components/form-core/src/FocusMixin.js | 31 +++++++++++++++++ .../form-core/test/FocusMixin.test.js | 33 +++++++++++++++++++ .../form-core/types/FocusMixinTypes.ts | 6 ++++ 4 files changed, 75 insertions(+) create mode 100644 .changeset/silly-shirts-promise.md diff --git a/.changeset/silly-shirts-promise.md b/.changeset/silly-shirts-promise.md new file mode 100644 index 000000000..9cb5fb8da --- /dev/null +++ b/.changeset/silly-shirts-promise.md @@ -0,0 +1,5 @@ +--- +'@lion/ui': patch +--- + +[FocusMixin] now syncs autofocus between host and the focusable node. diff --git a/packages/ui/components/form-core/src/FocusMixin.js b/packages/ui/components/form-core/src/FocusMixin.js index 7490e54ec..54aaf735d 100644 --- a/packages/ui/components/form-core/src/FocusMixin.js +++ b/packages/ui/components/form-core/src/FocusMixin.js @@ -27,6 +27,7 @@ const FocusMixinImplementation = superclass => return { focused: { type: Boolean, reflect: true }, focusedVisible: { type: Boolean, reflect: true, attribute: 'focused-visible' }, + autofocus: { type: Boolean, reflect: true }, // Required in Lit to observe autofocus }; } @@ -52,6 +53,7 @@ const FocusMixinImplementation = superclass => connectedCallback() { super.connectedCallback(); this.__registerEventsForFocusMixin(); + this.__syncAutofocusToFocusableElement(); } disconnectedCallback() { @@ -59,6 +61,35 @@ const FocusMixinImplementation = superclass => this.__teardownEventsForFocusMixin(); } + /** + * Gets called when an attribute is changed. + * @param {String} name + * @param {String | null} _old + * @param {String | null} value + * @protected + */ + attributeChangedCallback(name, _old, value) { + super.attributeChangedCallback(name, _old, value); + if (name === 'autofocus') { + this.__syncAutofocusToFocusableElement(); + } + } + + /** + * @private + */ + __syncAutofocusToFocusableElement() { + if (!this._focusableNode) { + return; + } + + if (this.hasAttribute('autofocus')) { + this._focusableNode.setAttribute('autofocus', ''); + } else { + this._focusableNode.removeAttribute('autofocus'); + } + } + /** * Calls `focus()` on focusable element within */ diff --git a/packages/ui/components/form-core/test/FocusMixin.test.js b/packages/ui/components/form-core/test/FocusMixin.test.js index c5c5edb01..3638e55f8 100644 --- a/packages/ui/components/form-core/test/FocusMixin.test.js +++ b/packages/ui/components/form-core/test/FocusMixin.test.js @@ -323,6 +323,39 @@ describe('FocusMixin', () => { spy4.restore(); restoreMock4(); }); + + it('has mirrors syncs autofocus on the focusable element when autofocus changes', async () => { + const el = /** @type {Focusable} */ ( + await fixture(html` + <${tag}> + `) + ); + + // @ts-ignore [allow-protected] in test + const { _focusableNode } = el; + + el.setAttribute('autofocus', ''); + await el.updateComplete; + expect(_focusableNode.hasAttribute('autofocus')).to.be.true; + + el.removeAttribute('autofocus'); + await el.updateComplete; + expect(_focusableNode.hasAttribute('autofocus')).not.to.be.true; + }); + + it('has mirrors syncs autofocus on the focusable element when autofocus was set on render', async () => { + const el = /** @type {Focusable} */ ( + await fixture(html` + <${tag} autofocus> + `) + ); + + // @ts-ignore [allow-protected] in test + const { _focusableNode } = el; + + expect(el.hasAttribute('autofocus')).to.be.true; + expect(_focusableNode.hasAttribute('autofocus')).to.be.true; + }); }); }); }); diff --git a/packages/ui/components/form-core/types/FocusMixinTypes.ts b/packages/ui/components/form-core/types/FocusMixinTypes.ts index 3f3445c9e..11d79aa36 100644 --- a/packages/ui/components/form-core/types/FocusMixinTypes.ts +++ b/packages/ui/components/form-core/types/FocusMixinTypes.ts @@ -25,6 +25,12 @@ export declare class FocusHost { */ blur(): void; + /** + * Synchronizes property values when attributes change. + * @category attributes + */ + attributeChangedCallback(name: string, _old: string | null, value: string | null): void; + /** * The focusable element: * could be an input, textarea, select, button or any other element with tabindex > -1 From 251de2c84ecb80cd69edfd7e1005e28b9656217d Mon Sep 17 00:00:00 2001 From: Sciurus7 Date: Mon, 17 Apr 2023 16:42:48 +0200 Subject: [PATCH 2/3] fix(form-core) use updated over attributeChangedCallback --- packages/ui/components/form-core/src/FocusMixin.js | 13 +++++-------- .../components/form-core/types/FocusMixinTypes.ts | 6 ------ 2 files changed, 5 insertions(+), 14 deletions(-) diff --git a/packages/ui/components/form-core/src/FocusMixin.js b/packages/ui/components/form-core/src/FocusMixin.js index 54aaf735d..a1102cdaf 100644 --- a/packages/ui/components/form-core/src/FocusMixin.js +++ b/packages/ui/components/form-core/src/FocusMixin.js @@ -48,6 +48,7 @@ const FocusMixinImplementation = superclass => * @type {boolean} */ this.focusedVisible = false; + this.autofocus = false; } connectedCallback() { @@ -62,15 +63,11 @@ const FocusMixinImplementation = superclass => } /** - * Gets called when an attribute is changed. - * @param {String} name - * @param {String | null} _old - * @param {String | null} value - * @protected + * @param {import('lit').PropertyValues } changedProperties */ - attributeChangedCallback(name, _old, value) { - super.attributeChangedCallback(name, _old, value); - if (name === 'autofocus') { + updated(changedProperties) { + super.updated(changedProperties); + if (changedProperties.has('autofocus')) { this.__syncAutofocusToFocusableElement(); } } diff --git a/packages/ui/components/form-core/types/FocusMixinTypes.ts b/packages/ui/components/form-core/types/FocusMixinTypes.ts index 11d79aa36..3f3445c9e 100644 --- a/packages/ui/components/form-core/types/FocusMixinTypes.ts +++ b/packages/ui/components/form-core/types/FocusMixinTypes.ts @@ -25,12 +25,6 @@ export declare class FocusHost { */ blur(): void; - /** - * Synchronizes property values when attributes change. - * @category attributes - */ - attributeChangedCallback(name: string, _old: string | null, value: string | null): void; - /** * The focusable element: * could be an input, textarea, select, button or any other element with tabindex > -1 From 56b3d334f6a2d3d1789d6823af4bdba8ace02bf2 Mon Sep 17 00:00:00 2001 From: gerjanvangeest Date: Tue, 16 May 2023 12:01:18 +0200 Subject: [PATCH 3/3] chore(dialog): add focus tests --- .../dialog/test/lion-dialog.test.js | 56 +++++++++++++++++++ .../test/dialog-integrations.test.js | 18 ++++++ 2 files changed, 74 insertions(+) diff --git a/packages/ui/components/dialog/test/lion-dialog.test.js b/packages/ui/components/dialog/test/lion-dialog.test.js index 389eaf62d..97b81a22d 100644 --- a/packages/ui/components/dialog/test/lion-dialog.test.js +++ b/packages/ui/components/dialog/test/lion-dialog.test.js @@ -1,3 +1,4 @@ +/* eslint-disable lit-a11y/no-autofocus */ import { expect, fixture as _fixture, html, unsafeStatic } from '@open-wc/testing'; import { runOverlayMixinSuite } from '../../overlays/test-suites/OverlayMixin.suite.js'; import '@lion/ui/define/lion-dialog.js'; @@ -72,4 +73,59 @@ describe('lion-dialog', () => { expect(nestedDialogEl.opened).to.be.true; }); }); + + describe('focus', () => { + it('sets focus on contentSlot by default', async () => { + const el = await fixture(html` + + +
+ + +
+ + `); + // @ts-expect-error [allow-protected-in-tests] + const invokerNode = el._overlayInvokerNode; + invokerNode.focus(); + invokerNode.click(); + const contentNode = el.querySelector('[slot="content"]'); + expect(document.activeElement).to.equal(contentNode); + }); + + it('sets focus on autofocused element', async () => { + const el = await fixture(html` + + +
+ + +
+ + `); + // @ts-expect-error [allow-protected-in-tests] + const invokerNode = el._overlayInvokerNode; + invokerNode.focus(); + invokerNode.click(); + const input = el.querySelector('input'); + expect(document.activeElement).to.equal(input); + }); + + it('with trapsKeyboardFocus set to false the focus stays on the invoker', async () => { + const el = /** @type {LionDialog} */ await fixture(html` + + +
+ + +
+ + `); + // @ts-expect-error [allow-protected-in-tests] + const invokerNode = el._overlayInvokerNode; + invokerNode.focus(); + invokerNode.click(); + expect(document.activeElement).to.equal(invokerNode); + }); + }); }); diff --git a/packages/ui/components/form-integrations/test/dialog-integrations.test.js b/packages/ui/components/form-integrations/test/dialog-integrations.test.js index a2be44e07..393da46d6 100644 --- a/packages/ui/components/form-integrations/test/dialog-integrations.test.js +++ b/packages/ui/components/form-integrations/test/dialog-integrations.test.js @@ -1,3 +1,4 @@ +/* eslint-disable lit-a11y/no-autofocus */ import { expect, fixture } from '@open-wc/testing'; import { html } from 'lit'; import { getAllTagNames } from './helpers/helpers.js'; @@ -70,4 +71,21 @@ describe('Form inside dialog Integrations', () => { 'lion-textarea', ]); }); + + it('sets focus on first focusable element with autofocus', async () => { + const el = /** @type {LionDialog} */ await fixture(html` + + +
+ + +
+ + `); + // @ts-expect-error [allow-protected-in-tests] + el._overlayInvokerNode.click(); + const lionInput = el.querySelector('[name="input"]'); + // @ts-expect-error [allow-protected-in-tests] + expect(document.activeElement).to.equal(lionInput._focusableNode); + }); });