From eb620ed089c9f4192fed011f5c4f19689ef197e9 Mon Sep 17 00:00:00 2001 From: web-padawan Date: Thu, 23 May 2024 11:38:35 +0300 Subject: [PATCH 1/4] refactor: restore focus for popover click and focus triggers --- packages/popover/src/vaadin-popover.js | 37 +++++- packages/popover/test/a11y.test.js | 163 +++++++++++++++++++++++++ packages/popover/test/helpers.js | 10 ++ packages/popover/test/trigger.test.js | 10 +- 4 files changed, 209 insertions(+), 11 deletions(-) create mode 100644 packages/popover/test/a11y.test.js create mode 100644 packages/popover/test/helpers.js diff --git a/packages/popover/src/vaadin-popover.js b/packages/popover/src/vaadin-popover.js index 301c08e62a..fc80b90d07 100644 --- a/packages/popover/src/vaadin-popover.js +++ b/packages/popover/src/vaadin-popover.js @@ -127,6 +127,13 @@ class Popover extends PopoverPositionMixin( type: Boolean, value: false, }, + + /** @private */ + __shouldRestoreFocus: { + type: Boolean, + value: false, + sync: true, + }, }; } @@ -165,9 +172,11 @@ class Popover extends PopoverPositionMixin( @focusin="${this.__onOverlayFocusIn}" @focusout="${this.__onOverlayFocusOut}" @opened-changed="${this.__onOpenedChanged}" + .restoreFocusOnClose="${this.__shouldRestoreFocus}" .restoreFocusNode="${this.target}" @vaadin-overlay-escape-press="${this.__onEscapePress}" @vaadin-overlay-outside-click="${this.__onOutsideClick}" + @vaadin-overlay-closed="${this.__onOverlayClosed}" > `; } @@ -257,6 +266,9 @@ class Popover extends PopoverPositionMixin( /** @private */ __onTargetClick() { if (this.__hasTrigger('click')) { + if (!this.opened) { + this.__shouldRestoreFocus = true; + } this.opened = !this.opened; } } @@ -268,6 +280,11 @@ class Popover extends PopoverPositionMixin( event.stopPropagation(); this.opened = false; } + + // Prevent restoring focus after target blur on Tab key + if (event.key === 'Tab' && this.__shouldRestoreFocus) { + this.__shouldRestoreFocus = false; + } } /** @private */ @@ -282,7 +299,11 @@ class Popover extends PopoverPositionMixin( return; } - this.opened = true; + // Prevent overlay re-opening when restoring focus on close. + if (!this.__shouldRestoreFocus) { + this.__shouldRestoreFocus = true; + this.opened = true; + } } } @@ -299,7 +320,8 @@ class Popover extends PopoverPositionMixin( __onTargetMouseEnter() { this.__hoverInside = true; - if (this.__hasTrigger('hover')) { + if (this.__hasTrigger('hover') && !this.opened) { + this.__shouldRestoreFocus = false; this.opened = true; } } @@ -372,6 +394,17 @@ class Popover extends PopoverPositionMixin( this.opened = event.detail.value; } + /** @private */ + __onOverlayClosed() { + // Reset restoring focus state after a timeout to make sure focus was restored + // and then allow re-opening overlay on re-focusing target with focus trigger. + if (this.__shouldRestoreFocus) { + setTimeout(() => { + this.__shouldRestoreFocus = false; + }); + } + } + /** * Close the popover if `noCloseOnEsc` isn't set to true. * @private diff --git a/packages/popover/test/a11y.test.js b/packages/popover/test/a11y.test.js new file mode 100644 index 0000000000..fd2da7013c --- /dev/null +++ b/packages/popover/test/a11y.test.js @@ -0,0 +1,163 @@ +import { expect } from '@esm-bundle/chai'; +import { esc, fixtureSync, focusout, nextRender, nextUpdate, outsideClick, tab } from '@vaadin/testing-helpers'; +import sinon from 'sinon'; +import './not-animated-styles.js'; +import '../vaadin-popover.js'; +import { mouseenter, mouseleave } from './helpers.js'; + +describe('a11y', () => { + let popover, target, overlay; + + beforeEach(async () => { + popover = fixtureSync(''); + target = fixtureSync(''); + popover.target = target; + popover.renderer = (root) => { + if (!root.firstChild) { + root.appendChild(document.createElement('input')); + } + }; + await nextRender(); + overlay = popover.shadowRoot.querySelector('vaadin-popover-overlay'); + }); + + describe('focus restoration', () => { + describe('focus trigger', () => { + beforeEach(async () => { + popover.trigger = ['focus']; + await nextUpdate(popover); + + target.focus(); + await nextRender(); + }); + + it('should restore focus on Esc with trigger set to focus', async () => { + const focusSpy = sinon.spy(target, 'focus'); + overlay.$.overlay.focus(); + esc(overlay.$.overlay); + await nextRender(); + + expect(focusSpy).to.be.calledOnce; + }); + + it('should not restore focus on Tab with trigger set to focus', async () => { + const focusSpy = sinon.spy(target, 'focus'); + overlay.$.overlay.focus(); + tab(target); + focusout(target); + await nextRender(); + + expect(focusSpy).to.not.be.called; + }); + + it('should not re-open when restoring focus on Esc with trigger set to focus', async () => { + overlay.$.overlay.focus(); + esc(overlay.$.overlay); + await nextRender(); + + expect(popover.opened).to.be.false; + }); + + it('should not re-open when restoring focus on outside click with trigger set to focus', async () => { + overlay.$.overlay.focus(); + outsideClick(); + await nextRender(); + + expect(popover.opened).to.be.false; + }); + + it('should re-open when re-focusing after closing on outside click with trigger set to focus', async () => { + overlay.$.overlay.focus(); + outsideClick(); + await nextRender(); + + target.blur(); + target.focus(); + await nextRender(); + + expect(popover.opened).to.be.true; + }); + }); + + describe('click trigger', () => { + beforeEach(async () => { + popover.trigger = ['click']; + await nextUpdate(popover); + + target.click(); + await nextRender(); + }); + + it('should restore focus on Esc with trigger set to click', async () => { + const focusSpy = sinon.spy(target, 'focus'); + overlay.$.overlay.focus(); + esc(overlay.$.overlay); + await nextRender(); + + expect(focusSpy).to.be.calledOnce; + }); + + it('should restore focus on outside click with trigger set to click', async () => { + const focusSpy = sinon.spy(target, 'focus'); + outsideClick(); + await nextRender(); + + expect(focusSpy).to.be.calledOnce; + }); + }); + + describe('hover trigger', () => { + it('should not restore focus on Esc with trigger set to hover', async () => { + popover.trigger = ['hover']; + await nextUpdate(popover); + + mouseenter(target); + await nextRender(); + + const focusSpy = sinon.spy(target, 'focus'); + overlay.$.overlay.focus(); + esc(overlay.$.overlay); + await nextRender(); + + expect(focusSpy).to.not.be.called; + }); + }); + + describe('hover and focus trigger', () => { + beforeEach(async () => { + popover.trigger = ['hover', 'focus']; + await nextUpdate(popover); + + target.focus(); + await nextRender(); + }); + + it('should not restore focus when re-opening on hover after being restored', async () => { + outsideClick(); + await nextRender(); + + // Re-open overlay + mouseenter(target); + await nextRender(); + + const focusSpy = sinon.spy(target, 'focus'); + overlay.$.overlay.focus(); + esc(overlay.$.overlay); + await nextRender(); + + expect(focusSpy).to.not.be.called; + }); + + it('should restore focus when hover occurs after opening on focus', async () => { + mouseenter(target); + + const focusSpy = sinon.spy(target, 'focus'); + overlay.$.overlay.focus(); + esc(overlay.$.overlay); + await nextRender(); + + expect(focusSpy).to.be.calledOnce; + }); + }); + }); +}); diff --git a/packages/popover/test/helpers.js b/packages/popover/test/helpers.js new file mode 100644 index 0000000000..9e32d3b8d7 --- /dev/null +++ b/packages/popover/test/helpers.js @@ -0,0 +1,10 @@ +import { fire } from '@vaadin/testing-helpers'; + +export function mouseenter(target) { + fire(target, 'mouseenter'); +} + +export function mouseleave(target, relatedTarget) { + const eventProps = relatedTarget ? { relatedTarget } : {}; + fire(target, 'mouseleave', undefined, eventProps); +} diff --git a/packages/popover/test/trigger.test.js b/packages/popover/test/trigger.test.js index 950c775947..91c82cc091 100644 --- a/packages/popover/test/trigger.test.js +++ b/packages/popover/test/trigger.test.js @@ -11,19 +11,11 @@ import { } from '@vaadin/testing-helpers'; import './not-animated-styles.js'; import '../vaadin-popover.js'; +import { mouseenter, mouseleave } from './helpers.js'; describe('trigger', () => { let popover, target, overlay; - function mouseenter(target) { - fire(target, 'mouseenter'); - } - - function mouseleave(target, relatedTarget) { - const eventProps = relatedTarget ? { relatedTarget } : {}; - fire(target, 'mouseleave', undefined, eventProps); - } - beforeEach(async () => { popover = fixtureSync(''); target = fixtureSync(''); From 93e705b5e46f963f98ef7a339a407b23d7793936 Mon Sep 17 00:00:00 2001 From: web-padawan Date: Thu, 23 May 2024 13:29:51 +0300 Subject: [PATCH 2/4] fix: set pointer-events: auto on the modal popover target --- packages/popover/src/vaadin-popover.js | 10 +++++++++- packages/popover/test/a11y.test.js | 27 +++++++++++++++++++++++++- 2 files changed, 35 insertions(+), 2 deletions(-) diff --git a/packages/popover/src/vaadin-popover.js b/packages/popover/src/vaadin-popover.js index fc80b90d07..f7f1291980 100644 --- a/packages/popover/src/vaadin-popover.js +++ b/packages/popover/src/vaadin-popover.js @@ -321,7 +321,10 @@ class Popover extends PopoverPositionMixin( this.__hoverInside = true; if (this.__hasTrigger('hover') && !this.opened) { - this.__shouldRestoreFocus = false; + // Prevent closing due to `pointer-events: none` set on body. + if (this.modal) { + this.target.style.pointerEvents = 'auto'; + } this.opened = true; } } @@ -403,6 +406,11 @@ class Popover extends PopoverPositionMixin( this.__shouldRestoreFocus = false; }); } + + // Restore pointer-events set when opening on hover. + if (this.modal && this.target.style.pointerEvents) { + this.target.style.pointerEvents = ''; + } } /** diff --git a/packages/popover/test/a11y.test.js b/packages/popover/test/a11y.test.js index fd2da7013c..0ac5eda48b 100644 --- a/packages/popover/test/a11y.test.js +++ b/packages/popover/test/a11y.test.js @@ -107,10 +107,12 @@ describe('a11y', () => { }); describe('hover trigger', () => { - it('should not restore focus on Esc with trigger set to hover', async () => { + beforeEach(async () => { popover.trigger = ['hover']; await nextUpdate(popover); + }); + it('should not restore focus on Esc with trigger set to hover', async () => { mouseenter(target); await nextRender(); @@ -121,6 +123,29 @@ describe('a11y', () => { expect(focusSpy).to.not.be.called; }); + + it('should set pointer-events: auto on the target when opened if modal', async () => { + popover.modal = true; + await nextUpdate(popover); + + mouseenter(target); + await nextRender(); + + expect(target.style.pointerEvents).to.equal('auto'); + }); + + it('should remove pointer-events on the target when closed if modal', async () => { + popover.modal = true; + await nextUpdate(popover); + + mouseenter(target); + await nextRender(); + + mouseleave(target); + await nextRender(); + + expect(target.style.pointerEvents).to.equal(''); + }); }); describe('hover and focus trigger', () => { From 11c6ba1eaeef91f8c81d458fc4a773ecca55ed6e Mon Sep 17 00:00:00 2001 From: web-padawan Date: Thu, 23 May 2024 14:23:55 +0300 Subject: [PATCH 3/4] fix: restore focus after tabbing to overlay content --- packages/popover/src/vaadin-popover.js | 6 ++++++ packages/popover/test/a11y.test.js | 22 ++++++++++++++++++++++ 2 files changed, 28 insertions(+) diff --git a/packages/popover/src/vaadin-popover.js b/packages/popover/src/vaadin-popover.js index f7f1291980..2a0d934de5 100644 --- a/packages/popover/src/vaadin-popover.js +++ b/packages/popover/src/vaadin-popover.js @@ -341,6 +341,12 @@ class Popover extends PopoverPositionMixin( /** @private */ __onOverlayFocusIn() { this.__focusInside = true; + + // When using Tab to move focus, restoring focus is reset. However, if pressing Tab + // causes focus to be moved inside the overlay, we should restore focus on close. + if ((this.__hasTrigger('focus') || this.__hasTrigger('click')) && !this.__shouldRestoreFocus) { + this.__shouldRestoreFocus = true; + } } /** @private */ diff --git a/packages/popover/test/a11y.test.js b/packages/popover/test/a11y.test.js index 0ac5eda48b..7bfc9c4252 100644 --- a/packages/popover/test/a11y.test.js +++ b/packages/popover/test/a11y.test.js @@ -50,6 +50,17 @@ describe('a11y', () => { expect(focusSpy).to.not.be.called; }); + it('should restore focus on close after Tab to overlay with trigger set to focus', async () => { + const focusSpy = sinon.spy(target, 'focus'); + tab(target); + focusout(target, overlay); + overlay.$.overlay.focus(); + esc(overlay.$.overlay); + await nextRender(); + + expect(focusSpy).to.be.calledOnce; + }); + it('should not re-open when restoring focus on Esc with trigger set to focus', async () => { overlay.$.overlay.focus(); esc(overlay.$.overlay); @@ -104,6 +115,17 @@ describe('a11y', () => { expect(focusSpy).to.be.calledOnce; }); + + it('should restore focus on close after Tab to overlay with trigger set to click', async () => { + const focusSpy = sinon.spy(target, 'focus'); + tab(target); + focusout(target, overlay); + overlay.$.overlay.focus(); + esc(overlay.$.overlay); + await nextRender(); + + expect(focusSpy).to.be.calledOnce; + }); }); describe('hover trigger', () => { From 2c7de006bf88ab4c9aabd66c27f1826ef3ae7371 Mon Sep 17 00:00:00 2001 From: Serhii Kulykov Date: Thu, 23 May 2024 16:17:31 +0300 Subject: [PATCH 4/4] Update packages/popover/src/vaadin-popover.js Co-authored-by: Sergey Vinogradov --- packages/popover/src/vaadin-popover.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/popover/src/vaadin-popover.js b/packages/popover/src/vaadin-popover.js index 2a0d934de5..70da333f49 100644 --- a/packages/popover/src/vaadin-popover.js +++ b/packages/popover/src/vaadin-popover.js @@ -344,7 +344,7 @@ class Popover extends PopoverPositionMixin( // When using Tab to move focus, restoring focus is reset. However, if pressing Tab // causes focus to be moved inside the overlay, we should restore focus on close. - if ((this.__hasTrigger('focus') || this.__hasTrigger('click')) && !this.__shouldRestoreFocus) { + if (this.__hasTrigger('focus') || this.__hasTrigger('click')) { this.__shouldRestoreFocus = true; } }