From 5b8d655f104cd8fe30246ee5d5bf9cc1e66bc4e5 Mon Sep 17 00:00:00 2001 From: Danny Moerkerke Date: Thu, 13 Apr 2023 15:04:23 +0200 Subject: [PATCH] Fix/dialog scroll to top ios (#1957) * fix: possible fix for iOS focus issue WIP * chore: added test for elementToFocusAfterHide WIP * chore: dialog tests for elementToFocusAFterHide * chore: added test to assert element specified in `elementToFocusAfterHide` config key of lion-dialog is scrolled into the viewport * chore: - added test to assert that element specified in dialog config key `elementToFocusAfterHide` is not focused when the dialog is closed if the user deliberately moved focus to another element while the dialog was open - added changeset * chore: - removed unneeded button - renamed this.__activeElement to this.__activeElementRightBeforeHide in OverlayController.js - set this.__activeElementRightBeforeHide to this.contentNode.getRootNode().activeElement instead of document.activeElement * chore: moved test to assert if element specified in dialog config key elementToFocusAfterHide is in viewport when dialog is closed to OverlayController.test.js --------- Co-authored-by: Thijs Louisse --- .changeset/brown-radios-boil.md | 5 ++++ .../overlays/src/OverlayController.js | 26 ++++++++++++++----- .../overlays/test/OverlayController.test.js | 16 +++++++++++- 3 files changed, 40 insertions(+), 7 deletions(-) create mode 100644 .changeset/brown-radios-boil.md diff --git a/.changeset/brown-radios-boil.md b/.changeset/brown-radios-boil.md new file mode 100644 index 000000000..2976a9762 --- /dev/null +++ b/.changeset/brown-radios-boil.md @@ -0,0 +1,5 @@ +--- +'@lion/ui': patch +--- + +OverlayController: fixed check to determine if native dialog is supported, fixed check to determine if user has moved focus while dialog is open, added test to assert if element specified in dialog config key `elementToFocusAfterHide` is in viewport when dialog is closed diff --git a/packages/ui/components/overlays/src/OverlayController.js b/packages/ui/components/overlays/src/OverlayController.js index 0614b1275..ec69981f8 100644 --- a/packages/ui/components/overlays/src/OverlayController.js +++ b/packages/ui/components/overlays/src/OverlayController.js @@ -124,6 +124,9 @@ export class OverlayController extends EventTarget { /** @private */ this.__sharedConfig = config; + /** @private */ + this.__activeElementRightBeforeHide = null; + /** @type {OverlayConfig} */ this.config = {}; @@ -728,7 +731,7 @@ export class OverlayController extends EventTarget { const event = new CustomEvent('before-show', { cancelable: true }); this.dispatchEvent(event); if (!event.defaultPrevented) { - if (this.__wrappingDialogNode instanceof HTMLDialogElement) { + if ('HTMLDialogElement' in window && this.__wrappingDialogNode instanceof HTMLDialogElement) { this.__wrappingDialogNode.open = true; } // @ts-ignore @@ -833,6 +836,14 @@ export class OverlayController extends EventTarget { this._hideResolve = resolve; }); + // save the current activeElement so we know if the user set focus to another element than the invoker of the dialog + // while the dialog was open. + // We need this in the _restoreFocus method to determine if we should focus this.elementToFocusAfterHide when the + // dialog is closed or keep focus on the element that the user deliberately gave focus + this.__activeElementRightBeforeHide = /** @type {ShadowRoot} */ ( + this.contentNode.getRootNode() + ).activeElement; + if (this.manager) { this.manager.hide(this); } @@ -850,9 +861,10 @@ export class OverlayController extends EventTarget { contentNode: this.contentNode, }); - if (this.__wrappingDialogNode instanceof HTMLDialogElement) { + if ('HTMLDialogElement' in window && this.__wrappingDialogNode instanceof HTMLDialogElement) { this.__wrappingDialogNode.close(); } + // @ts-ignore this.__wrappingDialogNode.style.display = 'none'; this._handleFeatures({ phase: 'hide' }); @@ -912,18 +924,20 @@ export class OverlayController extends EventTarget { /** @protected */ _restoreFocus() { // We only are allowed to move focus if we (still) 'own' the active element. - // Otherwise we assume the 'outside world' has purposefully taken over - const { activeElement } = /** @type {ShadowRoot} */ (this.contentNode.getRootNode()); + // Otherwise, we assume the 'outside world' has purposefully taken over const weStillOwnActiveElement = - activeElement instanceof HTMLElement && this.contentNode.contains(activeElement); + this.__activeElementRightBeforeHide instanceof HTMLElement && + this.contentNode.contains(this.__activeElementRightBeforeHide); + if (!weStillOwnActiveElement) { return; } if (this.elementToFocusAfterHide) { this.elementToFocusAfterHide.focus(); + this.elementToFocusAfterHide.scrollIntoView({ block: 'center' }); } else { - activeElement.blur(); + /** @type {HTMLElement} */ (this.__activeElementRightBeforeHide).blur(); } } diff --git a/packages/ui/components/overlays/test/OverlayController.test.js b/packages/ui/components/overlays/test/OverlayController.test.js index d4b565a30..277efc8d1 100644 --- a/packages/ui/components/overlays/test/OverlayController.test.js +++ b/packages/ui/components/overlays/test/OverlayController.test.js @@ -46,6 +46,19 @@ function getProtectedMembers(overlayControllerEl) { }; } +/** + * @param {HTMLElement} element + * */ +const isInViewport = element => { + const rect = element.getBoundingClientRect(); + return ( + rect.top >= 0 && + rect.left >= 0 && + rect.bottom <= (window.innerHeight || document.documentElement.clientHeight) && + rect.right <= (window.innerWidth || document.documentElement.clientWidth) + ); +}; + const withGlobalTestConfig = () => /** @type {OverlayConfig} */ ({ placementMode: 'global', @@ -905,9 +918,10 @@ describe('OverlayController', () => { await ctrl.hide(); expect(document.activeElement).to.equal(input); + expect(isInViewport(input)).to.be.true; }); - it('supports elementToFocusAfterHide option when shadowRoot involved involved', async () => { + it('supports elementToFocusAfterHide option when shadowRoot involved', async () => { const input = /** @type {HTMLElement} */ (await fixture('')); const contentNode = /** @type {HTMLElement} */ ( await fixture('
')