From f4471e1ba9f40c5d27e35650424626ce935d78a6 Mon Sep 17 00:00:00 2001 From: amandaesmith3 Date: Thu, 31 Mar 2022 14:00:15 -0500 Subject: [PATCH 1/9] fix(menu): preserve scroll position when focusing on open --- core/src/components/menu/menu.tsx | 35 ++++++++++++++++++++----------- 1 file changed, 23 insertions(+), 12 deletions(-) diff --git a/core/src/components/menu/menu.tsx b/core/src/components/menu/menu.tsx index b44dfad9da6..0a1472518ae 100644 --- a/core/src/components/menu/menu.tsx +++ b/core/src/components/menu/menu.tsx @@ -5,7 +5,7 @@ import { getIonMode } from '../../global/ionic-global'; import { Animation, Gesture, GestureDetail, MenuChangeEventDetail, MenuI, Side } from '../../interface'; import { getTimeGivenProgression } from '../../utils/animation/cubic-bezier'; import { GESTURE_CONTROLLER } from '../../utils/gesture'; -import { Attributes, assert, clamp, inheritAttributes, isEndSide as isEnd } from '../../utils/helpers'; +import { Attributes, assert, clamp, inheritAttributes, isEndSide as isEnd, raf } from '../../utils/helpers'; import { menuController } from '../../utils/menu-controller'; import { getOverlay } from '../../utils/overlays'; @@ -396,6 +396,28 @@ export class Menu implements ComponentInterface, MenuI { } this.beforeAnimation(shouldOpen); + + if(shouldOpen) { + // focus menu content for screen readers, preserving scroll position + if (this.menuInnerEl) { + const innerContent = this.el.querySelector('ion-content'); + const contentScrollEl = await innerContent?.getScrollElement(); + const scrollPos = contentScrollEl?.scrollTop; + this.focusFirstDescendant(); + if (scrollPos !== undefined) { + raf(() => { + innerContent?.scrollToPoint(0, scrollPos); + }); + } + } + + // setup focus trapping + document.addEventListener('focus', this.handleFocus, true); + } else { + // undo focus trapping so multiple menus don't collide + document.removeEventListener('focus', this.handleFocus, true); + } + await this.loadAnimation(); await this.startAnimation(shouldOpen, animated); this.afterAnimation(shouldOpen); @@ -620,14 +642,6 @@ export class Menu implements ComponentInterface, MenuI { // emit open event this.ionDidOpen.emit(); - - // focus menu content for screen readers - if (this.menuInnerEl) { - this.focusFirstDescendant(); - } - - // setup focus trapping - document.addEventListener('focus', this.handleFocus, true); } else { // remove css classes and unhide content from screen readers this.el.classList.remove(SHOW_MENU); @@ -658,9 +672,6 @@ export class Menu implements ComponentInterface, MenuI { // emit close event this.ionDidClose.emit(); - - // undo focus trapping so multiple menus don't collide - document.removeEventListener('focus', this.handleFocus, true); } } From 99f58c1d4194eb49db0a23d3e3865edb2d78229b Mon Sep 17 00:00:00 2001 From: amandaesmith3 Date: Thu, 31 Mar 2022 14:00:46 -0500 Subject: [PATCH 2/9] test(menu): add more content to basic menu so it can scroll --- core/src/components/menu/test/basic/index.html | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/core/src/components/menu/test/basic/index.html b/core/src/components/menu/test/basic/index.html index 021a2f8a53e..94e69ac8281 100644 --- a/core/src/components/menu/test/basic/index.html +++ b/core/src/components/menu/test/basic/index.html @@ -40,6 +40,21 @@ Menu Item Menu Item Menu Item + Menu Item + Menu Item + Menu Item + Menu Item + Menu Item + Menu Item + Menu Item + Menu Item + Menu Item + Menu Item + Menu Item + Menu Item + Menu Item + Menu Item + Menu Item From 75dffa0e1466710dbeba95f454bcb36e592aef9f Mon Sep 17 00:00:00 2001 From: amandaesmith3 Date: Fri, 1 Apr 2022 10:46:11 -0500 Subject: [PATCH 3/9] test(menu): add scroll position test --- core/src/components/menu/test/basic/e2e.ts | 24 ++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/core/src/components/menu/test/basic/e2e.ts b/core/src/components/menu/test/basic/e2e.ts index e2feb526174..a290cdf99cf 100644 --- a/core/src/components/menu/test/basic/e2e.ts +++ b/core/src/components/menu/test/basic/e2e.ts @@ -34,6 +34,30 @@ test('menu: focus trap', async () => { expect(activeElID).toEqual('start-menu-button'); }); +test('menu: preserve scroll position', async () => { + const page = await newE2EPage({ url: '/src/components/menu/test/basic?ionic:_testing=true' }); + + await page.click('#open-first'); + const menu = await page.find('#start-menu'); + await menu.waitForVisible(); + + await page.$eval('#start-menu ion-content', (menuContentEl: any) => { + return menuContentEl.scrollToPoint(0, 200); + }); + + await menu.callMethod('close'); + + await page.click('#open-first'); + await menu.waitForVisible(); + + const scrollTop = await page.$eval('#start-menu ion-content', async (menuContentEl: any) => { + const contentScrollEl = await menuContentEl.getScrollElement(); + return contentScrollEl.scrollTop; + }); + + expect(scrollTop).toEqual(200); +}); + /** * RTL Tests */ From 47b4b4980b7ef6808641d68eb9b7dcf05bc75315 Mon Sep 17 00:00:00 2001 From: amandaesmith3 Date: Fri, 1 Apr 2022 10:48:36 -0500 Subject: [PATCH 4/9] chore(): lint --- core/src/components/menu/menu.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/components/menu/menu.tsx b/core/src/components/menu/menu.tsx index 0a1472518ae..c191224e1b8 100644 --- a/core/src/components/menu/menu.tsx +++ b/core/src/components/menu/menu.tsx @@ -397,7 +397,7 @@ export class Menu implements ComponentInterface, MenuI { this.beforeAnimation(shouldOpen); - if(shouldOpen) { + if (shouldOpen) { // focus menu content for screen readers, preserving scroll position if (this.menuInnerEl) { const innerContent = this.el.querySelector('ion-content'); From b955ce056dbbeede76b0cacb1975de06b91b4c74 Mon Sep 17 00:00:00 2001 From: amandaesmith3 Date: Fri, 1 Apr 2022 12:58:03 -0500 Subject: [PATCH 5/9] chore(): lint --- core/src/components/menu/test/basic/e2e.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/components/menu/test/basic/e2e.ts b/core/src/components/menu/test/basic/e2e.ts index a290cdf99cf..fcec67ff6ae 100644 --- a/core/src/components/menu/test/basic/e2e.ts +++ b/core/src/components/menu/test/basic/e2e.ts @@ -49,7 +49,7 @@ test('menu: preserve scroll position', async () => { await page.click('#open-first'); await menu.waitForVisible(); - + const scrollTop = await page.$eval('#start-menu ion-content', async (menuContentEl: any) => { const contentScrollEl = await menuContentEl.getScrollElement(); return contentScrollEl.scrollTop; From 20fd27f775920c472dc41734743a6d93cd8747a7 Mon Sep 17 00:00:00 2001 From: amandaesmith3 Date: Wed, 6 Apr 2022 09:54:57 -0500 Subject: [PATCH 6/9] fix(menu): revert changes and focus host el instead --- core/src/components/menu/menu.tsx | 34 +++++++++++-------------------- 1 file changed, 12 insertions(+), 22 deletions(-) diff --git a/core/src/components/menu/menu.tsx b/core/src/components/menu/menu.tsx index c191224e1b8..ad7e4ea59ef 100644 --- a/core/src/components/menu/menu.tsx +++ b/core/src/components/menu/menu.tsx @@ -5,7 +5,7 @@ import { getIonMode } from '../../global/ionic-global'; import { Animation, Gesture, GestureDetail, MenuChangeEventDetail, MenuI, Side } from '../../interface'; import { getTimeGivenProgression } from '../../utils/animation/cubic-bezier'; import { GESTURE_CONTROLLER } from '../../utils/gesture'; -import { Attributes, assert, clamp, inheritAttributes, isEndSide as isEnd, raf } from '../../utils/helpers'; +import { Attributes, assert, clamp, inheritAttributes, isEndSide as isEnd } from '../../utils/helpers'; import { menuController } from '../../utils/menu-controller'; import { getOverlay } from '../../utils/overlays'; @@ -397,27 +397,6 @@ export class Menu implements ComponentInterface, MenuI { this.beforeAnimation(shouldOpen); - if (shouldOpen) { - // focus menu content for screen readers, preserving scroll position - if (this.menuInnerEl) { - const innerContent = this.el.querySelector('ion-content'); - const contentScrollEl = await innerContent?.getScrollElement(); - const scrollPos = contentScrollEl?.scrollTop; - this.focusFirstDescendant(); - if (scrollPos !== undefined) { - raf(() => { - innerContent?.scrollToPoint(0, scrollPos); - }); - } - } - - // setup focus trapping - document.addEventListener('focus', this.handleFocus, true); - } else { - // undo focus trapping so multiple menus don't collide - document.removeEventListener('focus', this.handleFocus, true); - } - await this.loadAnimation(); await this.startAnimation(shouldOpen, animated); this.afterAnimation(shouldOpen); @@ -642,6 +621,14 @@ export class Menu implements ComponentInterface, MenuI { // emit open event this.ionDidOpen.emit(); + + /** + * Set up focus trapping. Focus the host element to start with + * instead of the first descendant to avoid the scroll position + * jumping to that element right away. + */ + this.el.focus(); + document.addEventListener('focus', this.handleFocus, true); } else { // remove css classes and unhide content from screen readers this.el.classList.remove(SHOW_MENU); @@ -672,6 +659,9 @@ export class Menu implements ComponentInterface, MenuI { // emit close event this.ionDidClose.emit(); + + // undo focus trapping so multiple menus don't collide + document.removeEventListener('focus', this.handleFocus, true); } } From 239bdbf600515c33e0f083ca66294152c5825958 Mon Sep 17 00:00:00 2001 From: amandaesmith3 Date: Wed, 6 Apr 2022 10:06:40 -0500 Subject: [PATCH 7/9] test(menu): update focus trap tests to match new behavior --- core/src/components/menu/test/basic/e2e.ts | 5 +++++ core/src/components/menu/test/focus-trap/e2e.ts | 4 ++-- core/src/components/menu/test/focus-trap/index.html | 2 +- 3 files changed, 8 insertions(+), 3 deletions(-) diff --git a/core/src/components/menu/test/basic/e2e.ts b/core/src/components/menu/test/basic/e2e.ts index fcec67ff6ae..cc00ee0b4aa 100644 --- a/core/src/components/menu/test/basic/e2e.ts +++ b/core/src/components/menu/test/basic/e2e.ts @@ -27,8 +27,13 @@ test('menu: focus trap', async () => { await menu.waitForVisible(); let activeElID = await getActiveElementID(page); + expect(activeElID).toEqual('start-menu'); + + await page.keyboard.press('Tab'); + activeElID = await getActiveElementID(page); expect(activeElID).toEqual('start-menu-button'); + // do it again to make sure focus stays inside menu await page.keyboard.press('Tab'); activeElID = await getActiveElementID(page); expect(activeElID).toEqual('start-menu-button'); diff --git a/core/src/components/menu/test/focus-trap/e2e.ts b/core/src/components/menu/test/focus-trap/e2e.ts index 3d9cf8935f0..a7faaf96d31 100644 --- a/core/src/components/menu/test/focus-trap/e2e.ts +++ b/core/src/components/menu/test/focus-trap/e2e.ts @@ -18,7 +18,7 @@ test('menu: focus trap with overlays', async () => { await menu.callMethod('open'); await ionDidOpen.next(); - expect(await getActiveElementID(page)).toEqual('open-modal-button'); + expect(await getActiveElementID(page)).toEqual('menu'); const openModal = await page.find('#open-modal-button'); await openModal.click(); @@ -45,7 +45,7 @@ test('menu: focus trap with content inside overlays', async () => { await menu.callMethod('open'); await ionDidOpen.next(); - expect(await getActiveElementID(page)).toEqual('open-modal-button'); + expect(await getActiveElementID(page)).toEqual('menu'); const openModal = await page.find('#open-modal-button'); await openModal.click(); diff --git a/core/src/components/menu/test/focus-trap/index.html b/core/src/components/menu/test/focus-trap/index.html index 3f0aeb82d87..c0b223a256d 100644 --- a/core/src/components/menu/test/focus-trap/index.html +++ b/core/src/components/menu/test/focus-trap/index.html @@ -11,7 +11,7 @@ - + Menu From cac363df8f55b3fa1722b52d5fe9a561df7dbdd4 Mon Sep 17 00:00:00 2001 From: amandaesmith3 Date: Fri, 8 Apr 2022 13:39:56 -0500 Subject: [PATCH 8/9] fix(menu): only move focus if menu isn't already focused --- core/src/components/menu/menu.tsx | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/core/src/components/menu/menu.tsx b/core/src/components/menu/menu.tsx index 5863c2602bb..a639ec4b3a5 100644 --- a/core/src/components/menu/menu.tsx +++ b/core/src/components/menu/menu.tsx @@ -621,11 +621,16 @@ export class Menu implements ComponentInterface, MenuI { this.ionDidOpen.emit(); /** - * Set up focus trapping. Focus the host element to start with - * instead of the first descendant to avoid the scroll position - * jumping to that element right away. + * Move focus to the menu to prepare focus trapping, as long as + * it isn't already focused. Use the host element instead of the + * first descendant to avoid the scroll position jumping around. */ - this.el.focus(); + const focusedMenu = document.activeElement?.closest('ion-menu'); + if(!focusedMenu || focusedMenu !== this.el) { + this.el.focus(); + } + + // start focus trapping document.addEventListener('focus', this.handleFocus, true); } else { // remove css classes and unhide content from screen readers From 03c62a13f0e571873248850477ee2fdbce43651d Mon Sep 17 00:00:00 2001 From: Amanda Smith <90629384+amandaesmith3@users.noreply.github.com> Date: Fri, 8 Apr 2022 15:27:22 -0500 Subject: [PATCH 9/9] Update core/src/components/menu/menu.tsx Co-authored-by: Liam DeBeasi --- core/src/components/menu/menu.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/components/menu/menu.tsx b/core/src/components/menu/menu.tsx index a639ec4b3a5..c30108dcf57 100644 --- a/core/src/components/menu/menu.tsx +++ b/core/src/components/menu/menu.tsx @@ -626,7 +626,7 @@ export class Menu implements ComponentInterface, MenuI { * first descendant to avoid the scroll position jumping around. */ const focusedMenu = document.activeElement?.closest('ion-menu'); - if(!focusedMenu || focusedMenu !== this.el) { + if (focusedMenu !== this.el) { this.el.focus(); }