From 078436bea445daad7bdbb411101630c809a56574 Mon Sep 17 00:00:00 2001 From: Liam DeBeasi Date: Fri, 15 Dec 2023 12:00:44 -0500 Subject: [PATCH 01/25] Add POC --- core/src/components/nav/test/basic/index.html | 1 - .../components/title/test/basic/index.html | 2 +- core/src/components/title/title.tsx | 12 ++++ core/src/css/core.scss | 10 +++ core/src/utils/config.ts | 7 ++ core/src/utils/transition/index.ts | 69 +++++++++++++++++++ 6 files changed, 99 insertions(+), 2 deletions(-) diff --git a/core/src/components/nav/test/basic/index.html b/core/src/components/nav/test/basic/index.html index 65d59daa379..a534fa5d526 100644 --- a/core/src/components/nav/test/basic/index.html +++ b/core/src/components/nav/test/basic/index.html @@ -45,7 +45,6 @@

Page One

-

Page Two

Go to Page Three diff --git a/core/src/components/title/test/basic/index.html b/core/src/components/title/test/basic/index.html index 0b3257888b0..36ccfdd4c9a 100644 --- a/core/src/components/title/test/basic/index.html +++ b/core/src/components/title/test/basic/index.html @@ -37,7 +37,7 @@ - Title + Title Subtitle diff --git a/core/src/components/title/title.tsx b/core/src/components/title/title.tsx index 82a108071ff..1f7b126bfad 100644 --- a/core/src/components/title/title.tsx +++ b/core/src/components/title/title.tsx @@ -56,11 +56,23 @@ export class ToolbarTitle implements ComponentInterface { } render() { + const { el } = this; const mode = getIonMode(this); const size = this.getSize(); + const page = el.closest('.ion-page'); + const hasHeading = page?.querySelector('h1, [role="heading"][aria-level="1"]'); + + /** + * The first `ion-title` on the page is considered + * the heading. + */ + const isHeading = hasHeading === null && page?.querySelector('ion-title') === el; + return ( any; } +type FocusManagerPriority = 'content' | 'heading' | 'banner'; + export const setupConfig = (config: IonicConfig) => { const win = window as any; const Ionic = win.Ionic; diff --git a/core/src/utils/transition/index.ts b/core/src/utils/transition/index.ts index 5600c5cf3fa..cdde895e195 100644 --- a/core/src/utils/transition/index.ts +++ b/core/src/utils/transition/index.ts @@ -36,10 +36,22 @@ export const transition = (opts: TransitionOptions): Promise = }); }; +const LAST_FOCUS = 'ion-last-focus'; const beforeTransition = (opts: TransitionOptions) => { const enteringEl = opts.enteringEl; const leavingEl = opts.leavingEl; + /** + * When going back to a previously visited page + * focus should typically be moved back to the + * element that was last focused when the user + * was on this view. + */ + const activeEl = document.activeElement; + if (activeEl !== null && leavingEl?.contains(activeEl)) { + activeEl.setAttribute(LAST_FOCUS, 'true'); + } + setZIndex(enteringEl, leavingEl, opts.direction); if (opts.showGoBack) { @@ -71,6 +83,18 @@ const runTransition = async (opts: TransitionOptions): Promise return ani; }; +/** + * Moves focus to a specified element. + * Note that we do not remove the tabindex + * because that can result in an unintentional + * blur. Non-focusables can't be focused, so the + * body will get focused again. + */ +const moveFocus = (el: HTMLElement) => { + el.tabIndex = -1; + el.focus(); +} + const afterTransition = (opts: TransitionOptions) => { const enteringEl = opts.enteringEl; const leavingEl = opts.leavingEl; @@ -80,6 +104,51 @@ const afterTransition = (opts: TransitionOptions) => { leavingEl.classList.remove('ion-page-invisible'); leavingEl.style.removeProperty('pointer-events'); } + + if (!enteringEl.contains(document.activeElement)) { + /** + * When going back to a previously visited view + * focus should always be moved back to the element + * that the user was last focused on when they were on this view. + */ + const lastFocus = enteringEl.querySelector(`[${LAST_FOCUS}]`); + if (lastFocus) { + moveFocus(lastFocus); + return; + } + + /** + * If no last focus exists then we should prefer to focus + * level one headings. We do not prioritize the header yet + * because the header can have non-title elements such as + * a back button which is not necessarily helpful + * to focus first. + */ + const headingOne = enteringEl.querySelector('h1, [role="heading"][aria-level="1"]'); + if (headingOne) { + moveFocus(headingOne); + return; + } + + /** + * If no level one heading exists then we should at least focus the + * header so focus starts at the top of the page. + */ + const header = enteringEl.querySelector('header, [role="banner"]'); + if (header) { + moveFocus(header); + return; + } + + /** + * If there is no header then focus the page + * so focus at least moves to the correct view. + * The browser will then determine where within the + * page to move focus to. + */ + moveFocus(enteringEl); + } + }; const getAnimationBuilder = async (opts: TransitionOptions): Promise => { From 7ad8f64e9906c544855664d24ae5e9425459ffd5 Mon Sep 17 00:00:00 2001 From: Liam DeBeasi Date: Fri, 15 Dec 2023 13:38:44 -0500 Subject: [PATCH 02/25] remove test code --- core/src/components/title/title.tsx | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/core/src/components/title/title.tsx b/core/src/components/title/title.tsx index 1f7b126bfad..8af6e285f3d 100644 --- a/core/src/components/title/title.tsx +++ b/core/src/components/title/title.tsx @@ -59,20 +59,8 @@ export class ToolbarTitle implements ComponentInterface { const { el } = this; const mode = getIonMode(this); const size = this.getSize(); - - const page = el.closest('.ion-page'); - const hasHeading = page?.querySelector('h1, [role="heading"][aria-level="1"]'); - - /** - * The first `ion-title` on the page is considered - * the heading. - */ - const isHeading = hasHeading === null && page?.querySelector('ion-title') === el; - return ( Date: Fri, 15 Dec 2023 13:38:57 -0500 Subject: [PATCH 03/25] lint --- core/src/components/title/title.tsx | 1 - core/src/utils/transition/index.ts | 3 +-- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/core/src/components/title/title.tsx b/core/src/components/title/title.tsx index 8af6e285f3d..5713ea17685 100644 --- a/core/src/components/title/title.tsx +++ b/core/src/components/title/title.tsx @@ -56,7 +56,6 @@ export class ToolbarTitle implements ComponentInterface { } render() { - const { el } = this; const mode = getIonMode(this); const size = this.getSize(); return ( diff --git a/core/src/utils/transition/index.ts b/core/src/utils/transition/index.ts index cdde895e195..c031da8c9cc 100644 --- a/core/src/utils/transition/index.ts +++ b/core/src/utils/transition/index.ts @@ -93,7 +93,7 @@ const runTransition = async (opts: TransitionOptions): Promise const moveFocus = (el: HTMLElement) => { el.tabIndex = -1; el.focus(); -} +}; const afterTransition = (opts: TransitionOptions) => { const enteringEl = opts.enteringEl; @@ -148,7 +148,6 @@ const afterTransition = (opts: TransitionOptions) => { */ moveFocus(enteringEl); } - }; const getAnimationBuilder = async (opts: TransitionOptions): Promise => { From 444524785e0f2dcd32a740b08de9afd05ae2207e Mon Sep 17 00:00:00 2001 From: Liam DeBeasi Date: Fri, 15 Dec 2023 13:47:46 -0500 Subject: [PATCH 04/25] integrate with config --- core/src/utils/transition/index.ts | 80 ++++++++++++++++++++---------- 1 file changed, 54 insertions(+), 26 deletions(-) diff --git a/core/src/utils/transition/index.ts b/core/src/utils/transition/index.ts index c031da8c9cc..48d9809e7bb 100644 --- a/core/src/utils/transition/index.ts +++ b/core/src/utils/transition/index.ts @@ -1,4 +1,5 @@ import { Build, writeTask } from '@stencil/core'; +import { printIonWarning } from '@utils/logging'; import { LIFECYCLE_DID_ENTER, @@ -7,12 +8,12 @@ import { LIFECYCLE_WILL_LEAVE, } from '../../components/nav/constants'; import type { NavOptions, NavDirection } from '../../components/nav/nav-interface'; +import { config } from '../../global/config'; import type { Animation, AnimationBuilder } from '../animation/animation-interface'; import { raf } from '../helpers'; const iosTransitionAnimation = () => import('./ios.transition'); const mdTransitionAnimation = () => import('./md.transition'); - // TODO(FW-2832): types export const transition = (opts: TransitionOptions): Promise => { @@ -40,6 +41,7 @@ const LAST_FOCUS = 'ion-last-focus'; const beforeTransition = (opts: TransitionOptions) => { const enteringEl = opts.enteringEl; const leavingEl = opts.leavingEl; + const focusManagerEnabled = config.get('experimentalFocusManagerPriority', false); /** * When going back to a previously visited page @@ -47,9 +49,11 @@ const beforeTransition = (opts: TransitionOptions) => { * element that was last focused when the user * was on this view. */ - const activeEl = document.activeElement; - if (activeEl !== null && leavingEl?.contains(activeEl)) { - activeEl.setAttribute(LAST_FOCUS, 'true'); + if (focusManagerEnabled) { + const activeEl = document.activeElement; + if (activeEl !== null && leavingEl?.contains(activeEl)) { + activeEl.setAttribute(LAST_FOCUS, 'true'); + } } setZIndex(enteringEl, leavingEl, opts.direction); @@ -93,6 +97,7 @@ const runTransition = async (opts: TransitionOptions): Promise const moveFocus = (el: HTMLElement) => { el.tabIndex = -1; el.focus(); + console.log('focus',el) }; const afterTransition = (opts: TransitionOptions) => { @@ -105,7 +110,8 @@ const afterTransition = (opts: TransitionOptions) => { leavingEl.style.removeProperty('pointer-events'); } - if (!enteringEl.contains(document.activeElement)) { + const focusManagerPriorities = config.get('experimentalFocusManagerPriority', false); + if (Array.isArray(focusManagerPriorities) && !enteringEl.contains(document.activeElement)) { /** * When going back to a previously visited view * focus should always be moved back to the element @@ -117,27 +123,49 @@ const afterTransition = (opts: TransitionOptions) => { return; } - /** - * If no last focus exists then we should prefer to focus - * level one headings. We do not prioritize the header yet - * because the header can have non-title elements such as - * a back button which is not necessarily helpful - * to focus first. - */ - const headingOne = enteringEl.querySelector('h1, [role="heading"][aria-level="1"]'); - if (headingOne) { - moveFocus(headingOne); - return; - } - - /** - * If no level one heading exists then we should at least focus the - * header so focus starts at the top of the page. - */ - const header = enteringEl.querySelector('header, [role="banner"]'); - if (header) { - moveFocus(header); - return; + for (const priority of focusManagerPriorities) { + console.log(priority) + switch (priority) { + case 'content': + /** + * If no level one heading exists then we should at least focus the + * header so focus starts at the top of the page. + */ + const content = enteringEl.querySelector('main, [role="main"]'); + if (content) { + moveFocus(content); + return; + } + break; + case 'heading': + /** + * If no last focus exists then we should prefer to focus + * level one headings. We do not prioritize the header yet + * because the header can have non-title elements such as + * a back button which is not necessarily helpful + * to focus first. + */ + const headingOne = enteringEl.querySelector('h1, [role="heading"][aria-level="1"]'); + if (headingOne) { + moveFocus(headingOne); + return; + } + break; + case 'banner': + /** + * If no level one heading exists then we should at least focus the + * header so focus starts at the top of the page. + */ + const header = enteringEl.querySelector('header, [role="banner"]'); + if (header) { + moveFocus(header); + return; + } + break; + default: + printIonWarning(`Unrecognized focus manager priority value ${priority}`); + break; + } } /** From 40c01eb295bcca459e511f0c776d39a9edd65b87 Mon Sep 17 00:00:00 2001 From: Liam DeBeasi Date: Fri, 15 Dec 2023 13:49:07 -0500 Subject: [PATCH 05/25] TEMP content fix --- core/src/components/content/content.tsx | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/core/src/components/content/content.tsx b/core/src/components/content/content.tsx index 41b58376099..d2a4193b6e5 100644 --- a/core/src/components/content/content.tsx +++ b/core/src/components/content/content.tsx @@ -428,12 +428,12 @@ export class Content implements ComponentInterface { const mode = getIonMode(this); const forceOverscroll = this.shouldForceOverscroll(); const transitionShadow = mode === 'ios'; - const TagType = isMainContent ? 'main' : ('div' as any); this.resize(); return (
(this.backgroundContentEl = el)} id="background-content" part="background">
- (this.scrollEl = scrollEl!)} + ref={(scrollEl) => (this.scrollEl = scrollEl)} onScroll={this.scrollEvents ? (ev: UIEvent) => this.onScroll(ev) : undefined} part="scroll" > - +
{transitionShadow ? (
From a40ee3ad8bda5c340da875b65bd6729f7b5bde94 Mon Sep 17 00:00:00 2001 From: Liam DeBeasi Date: Fri, 15 Dec 2023 13:49:56 -0500 Subject: [PATCH 06/25] clean up --- core/src/components/nav/test/basic/index.html | 6 ++++++ core/src/utils/transition/index.ts | 1 - 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/core/src/components/nav/test/basic/index.html b/core/src/components/nav/test/basic/index.html index a534fa5d526..26b7b508846 100644 --- a/core/src/components/nav/test/basic/index.html +++ b/core/src/components/nav/test/basic/index.html @@ -74,6 +74,12 @@

Page Three

customElements.define('page-one', PageOne); customElements.define('page-two', PageTwo); customElements.define('page-three', PageThree); + + window.Ionic = { + config: { + experimentalFocusManagerPriority: ['heading', 'content'] + } + } diff --git a/core/src/utils/transition/index.ts b/core/src/utils/transition/index.ts index 48d9809e7bb..25b811ca47b 100644 --- a/core/src/utils/transition/index.ts +++ b/core/src/utils/transition/index.ts @@ -124,7 +124,6 @@ const afterTransition = (opts: TransitionOptions) => { } for (const priority of focusManagerPriorities) { - console.log(priority) switch (priority) { case 'content': /** From 1c07d3ae5184e139878a4e05a426e92ba97208db Mon Sep 17 00:00:00 2001 From: Liam DeBeasi Date: Tue, 16 Jan 2024 11:20:02 -0500 Subject: [PATCH 07/25] test --- core/src/components/nav/test/basic/index.html | 12 +++++++++--- core/src/utils/transition/index.ts | 17 +++++++++++++---- 2 files changed, 22 insertions(+), 7 deletions(-) diff --git a/core/src/components/nav/test/basic/index.html b/core/src/components/nav/test/basic/index.html index 26b7b508846..7169c697d33 100644 --- a/core/src/components/nav/test/basic/index.html +++ b/core/src/components/nav/test/basic/index.html @@ -16,7 +16,7 @@ class PageOne extends HTMLElement { connectedCallback() { this.innerHTML = ` - + @@ -24,11 +24,17 @@ Page One - -

Page One

+ + + + Page One + + +
Go to Page Two +
`; } diff --git a/core/src/utils/transition/index.ts b/core/src/utils/transition/index.ts index 25b811ca47b..079e1dedb4d 100644 --- a/core/src/utils/transition/index.ts +++ b/core/src/utils/transition/index.ts @@ -100,6 +100,14 @@ const moveFocus = (el: HTMLElement) => { console.log('focus',el) }; +/** + * Elements that are hidden using `display: none` should + * not be focused even if they are present in the DOM. + */ +const isVisible = (el: HTMLElement) => { + return el.offsetParent !== null; +} + const afterTransition = (opts: TransitionOptions) => { const enteringEl = opts.enteringEl; const leavingEl = opts.leavingEl; @@ -118,12 +126,13 @@ const afterTransition = (opts: TransitionOptions) => { * that the user was last focused on when they were on this view. */ const lastFocus = enteringEl.querySelector(`[${LAST_FOCUS}]`); - if (lastFocus) { + if (lastFocus && isVisible(lastFocus)) { moveFocus(lastFocus); return; } for (const priority of focusManagerPriorities) { + console.log('priority',priority) switch (priority) { case 'content': /** @@ -131,7 +140,7 @@ const afterTransition = (opts: TransitionOptions) => { * header so focus starts at the top of the page. */ const content = enteringEl.querySelector('main, [role="main"]'); - if (content) { + if (content && isVisible(content)) { moveFocus(content); return; } @@ -145,7 +154,7 @@ const afterTransition = (opts: TransitionOptions) => { * to focus first. */ const headingOne = enteringEl.querySelector('h1, [role="heading"][aria-level="1"]'); - if (headingOne) { + if (headingOne && isVisible(headingOne)) { moveFocus(headingOne); return; } @@ -156,7 +165,7 @@ const afterTransition = (opts: TransitionOptions) => { * header so focus starts at the top of the page. */ const header = enteringEl.querySelector('header, [role="banner"]'); - if (header) { + if (header && isVisible(header)) { moveFocus(header); return; } From 847c82b3fda15be64688e7359929af6b3aea82e7 Mon Sep 17 00:00:00 2001 From: Liam DeBeasi Date: Tue, 16 Jan 2024 14:01:56 -0500 Subject: [PATCH 08/25] fix: wait for page to render --- core/src/utils/transition/index.ts | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/core/src/utils/transition/index.ts b/core/src/utils/transition/index.ts index 079e1dedb4d..ec85e6e997f 100644 --- a/core/src/utils/transition/index.ts +++ b/core/src/utils/transition/index.ts @@ -132,7 +132,6 @@ const afterTransition = (opts: TransitionOptions) => { } for (const priority of focusManagerPriorities) { - console.log('priority',priority) switch (priority) { case 'content': /** @@ -229,8 +228,17 @@ const animation = async (animationBuilder: AnimationBuilder, opts: TransitionOpt const noAnimation = async (opts: TransitionOptions): Promise => { const enteringEl = opts.enteringEl; const leavingEl = opts.leavingEl; + const focusManagerEnabled = config.get('experimentalFocusManagerPriority', false); - await waitForReady(opts, false); + /** + * If the focus manager is enabled then we + * need to wait for Ionic components to be rendered + * otherwise the correct component to focus may not + * be focused because it is still hidden. + * However, if the manager is not enabled + * then there's no need to wait since there will be no animation. + */ + await waitForReady(opts, focusManagerEnabled); fireWillEvents(enteringEl, leavingEl); fireDidEvents(enteringEl, leavingEl); From 8fa67763f9d81c92bf1927e1b9d4cf2363d7476c Mon Sep 17 00:00:00 2001 From: Liam DeBeasi Date: Tue, 16 Jan 2024 14:02:34 -0500 Subject: [PATCH 09/25] remove log --- core/src/utils/transition/index.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/core/src/utils/transition/index.ts b/core/src/utils/transition/index.ts index ec85e6e997f..3e716d0d1a9 100644 --- a/core/src/utils/transition/index.ts +++ b/core/src/utils/transition/index.ts @@ -97,7 +97,6 @@ const runTransition = async (opts: TransitionOptions): Promise const moveFocus = (el: HTMLElement) => { el.tabIndex = -1; el.focus(); - console.log('focus',el) }; /** From 1009126d441c83ba15cc110181000ca5de42812c Mon Sep 17 00:00:00 2001 From: Liam DeBeasi Date: Thu, 11 Apr 2024 13:39:03 -0400 Subject: [PATCH 10/25] update comment --- core/src/css/core.scss | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/core/src/css/core.scss b/core/src/css/core.scss index 8f6f6366a49..3e57a04e458 100644 --- a/core/src/css/core.scss +++ b/core/src/css/core.scss @@ -395,10 +395,8 @@ ion-input input::-webkit-date-and-time-value { } /** - * When moving focus on page transitions - * we call .focus() on an element which - * can add an undesired outline ring. - * This CSS removes the outline ring. + * When moving focus on page transitions we call .focus() on an element which can + * add an undesired outline ring. This CSS removes the outline ring. */ [ion-last-focus] { outline: none; From 5d853774207333a7d87df6104e8e0a60996283cd Mon Sep 17 00:00:00 2001 From: Liam DeBeasi Date: Thu, 11 Apr 2024 13:41:47 -0400 Subject: [PATCH 11/25] comments and lint --- core/src/utils/transition/index.ts | 43 ++++++++++++------------------ 1 file changed, 17 insertions(+), 26 deletions(-) diff --git a/core/src/utils/transition/index.ts b/core/src/utils/transition/index.ts index 3e716d0d1a9..4b50fad0be3 100644 --- a/core/src/utils/transition/index.ts +++ b/core/src/utils/transition/index.ts @@ -44,10 +44,8 @@ const beforeTransition = (opts: TransitionOptions) => { const focusManagerEnabled = config.get('experimentalFocusManagerPriority', false); /** - * When going back to a previously visited page - * focus should typically be moved back to the - * element that was last focused when the user - * was on this view. + * When going back to a previously visited page focus should typically be moved + * back to the element that was last focused when the user was on this view. */ if (focusManagerEnabled) { const activeEl = document.activeElement; @@ -88,11 +86,9 @@ const runTransition = async (opts: TransitionOptions): Promise }; /** - * Moves focus to a specified element. - * Note that we do not remove the tabindex - * because that can result in an unintentional - * blur. Non-focusables can't be focused, so the - * body will get focused again. + * Moves focus to a specified element. Note that we do not remove the tabindex + * because that can result in an unintentional blur. Non-focusables can't be + * focused, so the body will get focused again. */ const moveFocus = (el: HTMLElement) => { el.tabIndex = -1; @@ -105,7 +101,7 @@ const moveFocus = (el: HTMLElement) => { */ const isVisible = (el: HTMLElement) => { return el.offsetParent !== null; -} +}; const afterTransition = (opts: TransitionOptions) => { const enteringEl = opts.enteringEl; @@ -120,9 +116,8 @@ const afterTransition = (opts: TransitionOptions) => { const focusManagerPriorities = config.get('experimentalFocusManagerPriority', false); if (Array.isArray(focusManagerPriorities) && !enteringEl.contains(document.activeElement)) { /** - * When going back to a previously visited view - * focus should always be moved back to the element - * that the user was last focused on when they were on this view. + * When going back to a previously visited view focus should always be moved back + * to the element that the user was last focused on when they were on this view. */ const lastFocus = enteringEl.querySelector(`[${LAST_FOCUS}]`); if (lastFocus && isVisible(lastFocus)) { @@ -134,8 +129,8 @@ const afterTransition = (opts: TransitionOptions) => { switch (priority) { case 'content': /** - * If no level one heading exists then we should at least focus the - * header so focus starts at the top of the page. + * If no level one heading exists then we should at least focus the header so focus + * starts at the top of the page. */ const content = enteringEl.querySelector('main, [role="main"]'); if (content && isVisible(content)) { @@ -145,11 +140,9 @@ const afterTransition = (opts: TransitionOptions) => { break; case 'heading': /** - * If no last focus exists then we should prefer to focus - * level one headings. We do not prioritize the header yet - * because the header can have non-title elements such as - * a back button which is not necessarily helpful - * to focus first. + * If no last focus exists then we should prefer to focus level one headings. We do + * not prioritize the header yet because the header can have non-title elements + * such as a back button which is not necessarily helpful to focus first. */ const headingOne = enteringEl.querySelector('h1, [role="heading"][aria-level="1"]'); if (headingOne && isVisible(headingOne)) { @@ -159,8 +152,8 @@ const afterTransition = (opts: TransitionOptions) => { break; case 'banner': /** - * If no level one heading exists then we should at least focus the - * header so focus starts at the top of the page. + * If no level one heading exists then we should at least focus the header so focus + * starts at the top of the page. */ const header = enteringEl.querySelector('header, [role="banner"]'); if (header && isVisible(header)) { @@ -175,10 +168,8 @@ const afterTransition = (opts: TransitionOptions) => { } /** - * If there is no header then focus the page - * so focus at least moves to the correct view. - * The browser will then determine where within the - * page to move focus to. + * If there is no header then focus the page so focus at least moves to the correct + * view. The browser will then determine where within the page to move focus to. */ moveFocus(enteringEl); } From 92210d1b6787a6bd03a21ab08589ab31de441f0e Mon Sep 17 00:00:00 2001 From: Liam DeBeasi Date: Thu, 11 Apr 2024 13:48:33 -0400 Subject: [PATCH 12/25] more coments --- core/src/utils/transition/index.ts | 68 ++++++++++++++++-------------- 1 file changed, 36 insertions(+), 32 deletions(-) diff --git a/core/src/utils/transition/index.ts b/core/src/utils/transition/index.ts index 4b50fad0be3..0564932be44 100644 --- a/core/src/utils/transition/index.ts +++ b/core/src/utils/transition/index.ts @@ -85,6 +85,19 @@ const runTransition = async (opts: TransitionOptions): Promise return ani; }; +const afterTransition = (opts: TransitionOptions) => { + const enteringEl = opts.enteringEl; + const leavingEl = opts.leavingEl; + enteringEl.classList.remove('ion-page-invisible'); + enteringEl.style.removeProperty('pointer-events'); + if (leavingEl !== undefined) { + leavingEl.classList.remove('ion-page-invisible'); + leavingEl.style.removeProperty('pointer-events'); + } + + setViewFocus(enteringEl); +}; + /** * Moves focus to a specified element. Note that we do not remove the tabindex * because that can result in an unintentional blur. Non-focusables can't be @@ -103,59 +116,49 @@ const isVisible = (el: HTMLElement) => { return el.offsetParent !== null; }; -const afterTransition = (opts: TransitionOptions) => { - const enteringEl = opts.enteringEl; - const leavingEl = opts.leavingEl; - enteringEl.classList.remove('ion-page-invisible'); - enteringEl.style.removeProperty('pointer-events'); - if (leavingEl !== undefined) { - leavingEl.classList.remove('ion-page-invisible'); - leavingEl.style.removeProperty('pointer-events'); - } - +const setViewFocus = (referenceEl: HTMLElement) => { const focusManagerPriorities = config.get('experimentalFocusManagerPriority', false); - if (Array.isArray(focusManagerPriorities) && !enteringEl.contains(document.activeElement)) { + /** + * If the focused element is a descendant of the referenceEl then it's possible + * that the app developer manually moved focus, so we do not want to override that. + * This can happen with inputs the are focused when a view transitions in. + */ + if (Array.isArray(focusManagerPriorities) && !referenceEl.contains(document.activeElement)) { /** * When going back to a previously visited view focus should always be moved back * to the element that the user was last focused on when they were on this view. */ - const lastFocus = enteringEl.querySelector(`[${LAST_FOCUS}]`); + const lastFocus = referenceEl.querySelector(`[${LAST_FOCUS}]`); if (lastFocus && isVisible(lastFocus)) { moveFocus(lastFocus); return; } for (const priority of focusManagerPriorities) { + /** + * For each recognized case (excluding the default case) make sure to return + * so that the fallback focus behavior does not run. + * + * We intentionally query for specific roles/semantic elements so that the + * transition manager can work with both Ionic and non-Ionic UI components. + */ switch (priority) { case 'content': - /** - * If no level one heading exists then we should at least focus the header so focus - * starts at the top of the page. - */ - const content = enteringEl.querySelector('main, [role="main"]'); + const content = referenceEl.querySelector('main, [role="main"]'); if (content && isVisible(content)) { moveFocus(content); return; } break; case 'heading': - /** - * If no last focus exists then we should prefer to focus level one headings. We do - * not prioritize the header yet because the header can have non-title elements - * such as a back button which is not necessarily helpful to focus first. - */ - const headingOne = enteringEl.querySelector('h1, [role="heading"][aria-level="1"]'); + const headingOne = referenceEl.querySelector('h1, [role="heading"][aria-level="1"]'); if (headingOne && isVisible(headingOne)) { moveFocus(headingOne); return; } break; case 'banner': - /** - * If no level one heading exists then we should at least focus the header so focus - * starts at the top of the page. - */ - const header = enteringEl.querySelector('header, [role="banner"]'); + const header = referenceEl.querySelector('header, [role="banner"]'); if (header && isVisible(header)) { moveFocus(header); return; @@ -168,12 +171,13 @@ const afterTransition = (opts: TransitionOptions) => { } /** - * If there is no header then focus the page so focus at least moves to the correct - * view. The browser will then determine where within the page to move focus to. + * If there is nothing to focus then focus the page so focus at least moves to + * the correct view. The browser will then determine where within the page to + * move focus to. */ - moveFocus(enteringEl); + moveFocus(referenceEl); } -}; +} const getAnimationBuilder = async (opts: TransitionOptions): Promise => { if (!opts.leavingEl || !opts.animated || opts.duration === 0) { From 4e0e7d330c24f02d4d3b15d5c4c16c995854a533 Mon Sep 17 00:00:00 2001 From: Liam DeBeasi Date: Thu, 11 Apr 2024 13:49:01 -0400 Subject: [PATCH 13/25] rename --- core/src/utils/config.ts | 2 +- core/src/utils/transition/index.ts | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/core/src/utils/config.ts b/core/src/utils/config.ts index 125b22a1b73..9deccd9daef 100644 --- a/core/src/utils/config.ts +++ b/core/src/utils/config.ts @@ -207,7 +207,7 @@ export interface IonicConfig { /** * @experimental */ - experimentalFocusManagerPriority?: FocusManagerPriority[]; + focusManagerPriority?: FocusManagerPriority[]; /** * @experimental * If `true`, the [CloseWatcher API](https://github.com/WICG/close-watcher) will be used to handle diff --git a/core/src/utils/transition/index.ts b/core/src/utils/transition/index.ts index 0564932be44..16090e2a837 100644 --- a/core/src/utils/transition/index.ts +++ b/core/src/utils/transition/index.ts @@ -41,7 +41,7 @@ const LAST_FOCUS = 'ion-last-focus'; const beforeTransition = (opts: TransitionOptions) => { const enteringEl = opts.enteringEl; const leavingEl = opts.leavingEl; - const focusManagerEnabled = config.get('experimentalFocusManagerPriority', false); + const focusManagerEnabled = config.get('focusManagerPriority', false); /** * When going back to a previously visited page focus should typically be moved @@ -117,7 +117,7 @@ const isVisible = (el: HTMLElement) => { }; const setViewFocus = (referenceEl: HTMLElement) => { - const focusManagerPriorities = config.get('experimentalFocusManagerPriority', false); + const focusManagerPriorities = config.get('focusManagerPriority', false); /** * If the focused element is a descendant of the referenceEl then it's possible * that the app developer manually moved focus, so we do not want to override that. @@ -222,7 +222,7 @@ const animation = async (animationBuilder: AnimationBuilder, opts: TransitionOpt const noAnimation = async (opts: TransitionOptions): Promise => { const enteringEl = opts.enteringEl; const leavingEl = opts.leavingEl; - const focusManagerEnabled = config.get('experimentalFocusManagerPriority', false); + const focusManagerEnabled = config.get('focusManagerPriority', false); /** * If the focus manager is enabled then we From 10082dcca6742a99792c861df009843a139aa6ca Mon Sep 17 00:00:00 2001 From: Liam DeBeasi Date: Thu, 11 Apr 2024 13:50:10 -0400 Subject: [PATCH 14/25] comments --- core/src/utils/config.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/core/src/utils/config.ts b/core/src/utils/config.ts index 9deccd9daef..4d177e3c29e 100644 --- a/core/src/utils/config.ts +++ b/core/src/utils/config.ts @@ -206,6 +206,9 @@ export interface IonicConfig { /** * @experimental + * When defined, Ionic will move focus to the appropriate element after each + * page transition. This ensures that users relying on assistive technology + * are informed when a page transition happens. */ focusManagerPriority?: FocusManagerPriority[]; /** From b38499b9ca92c2fe0bfb3a76feadbf019f759097 Mon Sep 17 00:00:00 2001 From: Liam DeBeasi Date: Thu, 11 Apr 2024 13:51:08 -0400 Subject: [PATCH 15/25] clean up --- core/src/components/title/title.tsx | 1 + 1 file changed, 1 insertion(+) diff --git a/core/src/components/title/title.tsx b/core/src/components/title/title.tsx index 5713ea17685..82a108071ff 100644 --- a/core/src/components/title/title.tsx +++ b/core/src/components/title/title.tsx @@ -58,6 +58,7 @@ export class ToolbarTitle implements ComponentInterface { render() { const mode = getIonMode(this); const size = this.getSize(); + return ( Date: Thu, 11 Apr 2024 13:53:30 -0400 Subject: [PATCH 16/25] more clean up --- core/src/utils/transition/index.ts | 31 ++++++++++++++++++------------ 1 file changed, 19 insertions(+), 12 deletions(-) diff --git a/core/src/utils/transition/index.ts b/core/src/utils/transition/index.ts index 16090e2a837..36d5b72fa47 100644 --- a/core/src/utils/transition/index.ts +++ b/core/src/utils/transition/index.ts @@ -14,6 +14,7 @@ import { raf } from '../helpers'; const iosTransitionAnimation = () => import('./ios.transition'); const mdTransitionAnimation = () => import('./md.transition'); + // TODO(FW-2832): types export const transition = (opts: TransitionOptions): Promise => { @@ -37,22 +38,11 @@ export const transition = (opts: TransitionOptions): Promise = }); }; -const LAST_FOCUS = 'ion-last-focus'; const beforeTransition = (opts: TransitionOptions) => { const enteringEl = opts.enteringEl; const leavingEl = opts.leavingEl; - const focusManagerEnabled = config.get('focusManagerPriority', false); - /** - * When going back to a previously visited page focus should typically be moved - * back to the element that was last focused when the user was on this view. - */ - if (focusManagerEnabled) { - const activeEl = document.activeElement; - if (activeEl !== null && leavingEl?.contains(activeEl)) { - activeEl.setAttribute(LAST_FOCUS, 'true'); - } - } + saveViewFocus(leavingEl); setZIndex(enteringEl, leavingEl, opts.direction); @@ -116,6 +106,21 @@ const isVisible = (el: HTMLElement) => { return el.offsetParent !== null; }; +const saveViewFocus = (referenceEl?: HTMLElement) => { + const focusManagerEnabled = config.get('focusManagerPriority', false); + + /** + * When going back to a previously visited page focus should typically be moved + * back to the element that was last focused when the user was on this view. + */ + if (focusManagerEnabled) { + const activeEl = document.activeElement; + if (activeEl !== null && referenceEl?.contains(activeEl)) { + activeEl.setAttribute(LAST_FOCUS, 'true'); + } + } +} + const setViewFocus = (referenceEl: HTMLElement) => { const focusManagerPriorities = config.get('focusManagerPriority', false); /** @@ -396,3 +401,5 @@ export interface TransitionResult { hasCompleted: boolean; animation?: Animation; } + +const LAST_FOCUS = 'ion-last-focus'; From dc8190a85439ab69a5c41f45dfc1a263644d8f93 Mon Sep 17 00:00:00 2001 From: Liam DeBeasi Date: Thu, 11 Apr 2024 13:55:11 -0400 Subject: [PATCH 17/25] new line --- core/src/utils/config.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/core/src/utils/config.ts b/core/src/utils/config.ts index 4d177e3c29e..e38d43beb4e 100644 --- a/core/src/utils/config.ts +++ b/core/src/utils/config.ts @@ -211,6 +211,7 @@ export interface IonicConfig { * are informed when a page transition happens. */ focusManagerPriority?: FocusManagerPriority[]; + /** * @experimental * If `true`, the [CloseWatcher API](https://github.com/WICG/close-watcher) will be used to handle From 02dc77b908dffdb0348c64cb4acefae3092f6b03 Mon Sep 17 00:00:00 2001 From: Liam DeBeasi Date: Thu, 11 Apr 2024 14:04:10 -0400 Subject: [PATCH 18/25] refactor: use controller approach --- core/src/utils/focus-controller/index.ts | 120 +++++++++++++++++++++++ core/src/utils/transition/index.ts | 105 +------------------- core/tsconfig.json | 3 +- 3 files changed, 126 insertions(+), 102 deletions(-) create mode 100644 core/src/utils/focus-controller/index.ts diff --git a/core/src/utils/focus-controller/index.ts b/core/src/utils/focus-controller/index.ts new file mode 100644 index 00000000000..eadb24ff368 --- /dev/null +++ b/core/src/utils/focus-controller/index.ts @@ -0,0 +1,120 @@ +import { config } from '@global/config'; +import { printIonWarning } from '@utils/logging'; + +/** + * The focus controller allows us to manage focus within a view so assistive + * technologies can inform users of changes to the navigation state. Traditional + * native apps have a way of informing assistive technology about a navigation + * state change. Mobile browsers have this too, but only when doing a full page + * load. In a single page app we do not do that, so we need to build this + * integration ourselves. + */ +export const createFocusController = (): FocusController => { + /** + * Moves focus to a specified element. Note that we do not remove the tabindex + * because that can result in an unintentional blur. Non-focusables can't be + * focused, so the body will get focused again. + */ + const moveFocus = (el: HTMLElement) => { + el.tabIndex = -1; + el.focus(); + }; + + /** + * Elements that are hidden using `display: none` should + * not be focused even if they are present in the DOM. + */ + const isVisible = (el: HTMLElement) => { + return el.offsetParent !== null; + }; + + const saveViewFocus = (referenceEl?: HTMLElement) => { + const focusManagerEnabled = config.get('focusManagerPriority', false); + + /** + * When going back to a previously visited page focus should typically be moved + * back to the element that was last focused when the user was on this view. + */ + if (focusManagerEnabled) { + const activeEl = document.activeElement; + if (activeEl !== null && referenceEl?.contains(activeEl)) { + activeEl.setAttribute(LAST_FOCUS, 'true'); + } + } + }; + + const setViewFocus = (referenceEl: HTMLElement) => { + const focusManagerPriorities = config.get('focusManagerPriority', false); + /** + * If the focused element is a descendant of the referenceEl then it's possible + * that the app developer manually moved focus, so we do not want to override that. + * This can happen with inputs the are focused when a view transitions in. + */ + if (Array.isArray(focusManagerPriorities) && !referenceEl.contains(document.activeElement)) { + /** + * When going back to a previously visited view focus should always be moved back + * to the element that the user was last focused on when they were on this view. + */ + const lastFocus = referenceEl.querySelector(`[${LAST_FOCUS}]`); + if (lastFocus && isVisible(lastFocus)) { + moveFocus(lastFocus); + return; + } + + for (const priority of focusManagerPriorities) { + /** + * For each recognized case (excluding the default case) make sure to return + * so that the fallback focus behavior does not run. + * + * We intentionally query for specific roles/semantic elements so that the + * transition manager can work with both Ionic and non-Ionic UI components. + */ + switch (priority) { + case 'content': + const content = referenceEl.querySelector('main, [role="main"]'); + if (content && isVisible(content)) { + moveFocus(content); + return; + } + break; + case 'heading': + const headingOne = referenceEl.querySelector('h1, [role="heading"][aria-level="1"]'); + if (headingOne && isVisible(headingOne)) { + moveFocus(headingOne); + return; + } + break; + case 'banner': + const header = referenceEl.querySelector('header, [role="banner"]'); + if (header && isVisible(header)) { + moveFocus(header); + return; + } + break; + default: + printIonWarning(`Unrecognized focus manager priority value ${priority}`); + break; + } + } + + /** + * If there is nothing to focus then focus the page so focus at least moves to + * the correct view. The browser will then determine where within the page to + * move focus to. + */ + moveFocus(referenceEl); + } + }; + + return { + saveViewFocus, + setViewFocus, + }; +}; + +export type FocusController = { + saveViewFocus: (referenceEl?: HTMLElement) => void; + setViewFocus: (referenceEl: HTMLElement) => void; +}; + +const LAST_FOCUS = 'ion-last-focus'; diff --git a/core/src/utils/transition/index.ts b/core/src/utils/transition/index.ts index 36d5b72fa47..a543739a486 100644 --- a/core/src/utils/transition/index.ts +++ b/core/src/utils/transition/index.ts @@ -1,5 +1,4 @@ import { Build, writeTask } from '@stencil/core'; -import { printIonWarning } from '@utils/logging'; import { LIFECYCLE_DID_ENTER, @@ -10,10 +9,12 @@ import { import type { NavOptions, NavDirection } from '../../components/nav/nav-interface'; import { config } from '../../global/config'; import type { Animation, AnimationBuilder } from '../animation/animation-interface'; +import { createFocusController } from '../focus-controller'; import { raf } from '../helpers'; const iosTransitionAnimation = () => import('./ios.transition'); const mdTransitionAnimation = () => import('./md.transition'); +const focusController = createFocusController(); // TODO(FW-2832): types @@ -42,7 +43,7 @@ const beforeTransition = (opts: TransitionOptions) => { const enteringEl = opts.enteringEl; const leavingEl = opts.leavingEl; - saveViewFocus(leavingEl); + focusController.saveViewFocus(leavingEl); setZIndex(enteringEl, leavingEl, opts.direction); @@ -85,105 +86,9 @@ const afterTransition = (opts: TransitionOptions) => { leavingEl.style.removeProperty('pointer-events'); } - setViewFocus(enteringEl); + focusController.setViewFocus(enteringEl); }; -/** - * Moves focus to a specified element. Note that we do not remove the tabindex - * because that can result in an unintentional blur. Non-focusables can't be - * focused, so the body will get focused again. - */ -const moveFocus = (el: HTMLElement) => { - el.tabIndex = -1; - el.focus(); -}; - -/** - * Elements that are hidden using `display: none` should - * not be focused even if they are present in the DOM. - */ -const isVisible = (el: HTMLElement) => { - return el.offsetParent !== null; -}; - -const saveViewFocus = (referenceEl?: HTMLElement) => { - const focusManagerEnabled = config.get('focusManagerPriority', false); - - /** - * When going back to a previously visited page focus should typically be moved - * back to the element that was last focused when the user was on this view. - */ - if (focusManagerEnabled) { - const activeEl = document.activeElement; - if (activeEl !== null && referenceEl?.contains(activeEl)) { - activeEl.setAttribute(LAST_FOCUS, 'true'); - } - } -} - -const setViewFocus = (referenceEl: HTMLElement) => { - const focusManagerPriorities = config.get('focusManagerPriority', false); - /** - * If the focused element is a descendant of the referenceEl then it's possible - * that the app developer manually moved focus, so we do not want to override that. - * This can happen with inputs the are focused when a view transitions in. - */ - if (Array.isArray(focusManagerPriorities) && !referenceEl.contains(document.activeElement)) { - /** - * When going back to a previously visited view focus should always be moved back - * to the element that the user was last focused on when they were on this view. - */ - const lastFocus = referenceEl.querySelector(`[${LAST_FOCUS}]`); - if (lastFocus && isVisible(lastFocus)) { - moveFocus(lastFocus); - return; - } - - for (const priority of focusManagerPriorities) { - /** - * For each recognized case (excluding the default case) make sure to return - * so that the fallback focus behavior does not run. - * - * We intentionally query for specific roles/semantic elements so that the - * transition manager can work with both Ionic and non-Ionic UI components. - */ - switch (priority) { - case 'content': - const content = referenceEl.querySelector('main, [role="main"]'); - if (content && isVisible(content)) { - moveFocus(content); - return; - } - break; - case 'heading': - const headingOne = referenceEl.querySelector('h1, [role="heading"][aria-level="1"]'); - if (headingOne && isVisible(headingOne)) { - moveFocus(headingOne); - return; - } - break; - case 'banner': - const header = referenceEl.querySelector('header, [role="banner"]'); - if (header && isVisible(header)) { - moveFocus(header); - return; - } - break; - default: - printIonWarning(`Unrecognized focus manager priority value ${priority}`); - break; - } - } - - /** - * If there is nothing to focus then focus the page so focus at least moves to - * the correct view. The browser will then determine where within the page to - * move focus to. - */ - moveFocus(referenceEl); - } -} - const getAnimationBuilder = async (opts: TransitionOptions): Promise => { if (!opts.leavingEl || !opts.animated || opts.duration === 0) { return undefined; @@ -401,5 +306,3 @@ export interface TransitionResult { hasCompleted: boolean; animation?: Animation; } - -const LAST_FOCUS = 'ion-last-focus'; diff --git a/core/tsconfig.json b/core/tsconfig.json index 2874f056cc9..acdd4094006 100644 --- a/core/tsconfig.json +++ b/core/tsconfig.json @@ -31,7 +31,8 @@ "baseUrl": ".", "paths": { "@utils/*": ["src/utils/*"], - "@utils/test": ["src/utils/test/utils"] + "@utils/test": ["src/utils/test/utils"], + "@global/*": ["src/global/*"] } }, "include": [ From 12a9392de94a1a232ebeff13a537a928c1cb7f32 Mon Sep 17 00:00:00 2001 From: Liam DeBeasi Date: Thu, 25 Apr 2024 12:09:47 -0400 Subject: [PATCH 19/25] clean up --- core/src/utils/focus-controller/index.ts | 36 ++++++++++++------------ core/src/utils/transition/index.ts | 8 ++---- 2 files changed, 20 insertions(+), 24 deletions(-) diff --git a/core/src/utils/focus-controller/index.ts b/core/src/utils/focus-controller/index.ts index eadb24ff368..cd6a0453c33 100644 --- a/core/src/utils/focus-controller/index.ts +++ b/core/src/utils/focus-controller/index.ts @@ -1,6 +1,24 @@ import { config } from '@global/config'; import { printIonWarning } from '@utils/logging'; +/** + * Moves focus to a specified element. Note that we do not remove the tabindex + * because that can result in an unintentional blur. Non-focusables can't be + * focused, so the body will get focused again. + */ +const moveFocus = (el: HTMLElement) => { + el.tabIndex = -1; + el.focus(); +}; + +/** + * Elements that are hidden using `display: none` should not be focused even if + * they are present in the DOM. + */ +const isVisible = (el: HTMLElement) => { + return el.offsetParent !== null; +}; + /** * The focus controller allows us to manage focus within a view so assistive * technologies can inform users of changes to the navigation state. Traditional @@ -10,24 +28,6 @@ import { printIonWarning } from '@utils/logging'; * integration ourselves. */ export const createFocusController = (): FocusController => { - /** - * Moves focus to a specified element. Note that we do not remove the tabindex - * because that can result in an unintentional blur. Non-focusables can't be - * focused, so the body will get focused again. - */ - const moveFocus = (el: HTMLElement) => { - el.tabIndex = -1; - el.focus(); - }; - - /** - * Elements that are hidden using `display: none` should - * not be focused even if they are present in the DOM. - */ - const isVisible = (el: HTMLElement) => { - return el.offsetParent !== null; - }; - const saveViewFocus = (referenceEl?: HTMLElement) => { const focusManagerEnabled = config.get('focusManagerPriority', false); diff --git a/core/src/utils/transition/index.ts b/core/src/utils/transition/index.ts index a543739a486..970858ba82c 100644 --- a/core/src/utils/transition/index.ts +++ b/core/src/utils/transition/index.ts @@ -135,12 +135,8 @@ const noAnimation = async (opts: TransitionOptions): Promise = const focusManagerEnabled = config.get('focusManagerPriority', false); /** - * If the focus manager is enabled then we - * need to wait for Ionic components to be rendered - * otherwise the correct component to focus may not - * be focused because it is still hidden. - * However, if the manager is not enabled - * then there's no need to wait since there will be no animation. + * If the focus manager is enabled then we need to wait for Ionic components to be + * rendered otherwise the component to focus may not be focused because it is hidden. */ await waitForReady(opts, focusManagerEnabled); From 7ec10406af7a5178595d26dcf20526b2d81757f5 Mon Sep 17 00:00:00 2001 From: Liam DeBeasi Date: Thu, 25 Apr 2024 12:10:26 -0400 Subject: [PATCH 20/25] revert template changes --- core/src/components/nav/test/basic/index.html | 19 ++++--------------- .../components/title/test/basic/index.html | 2 +- 2 files changed, 5 insertions(+), 16 deletions(-) diff --git a/core/src/components/nav/test/basic/index.html b/core/src/components/nav/test/basic/index.html index 7169c697d33..65d59daa379 100644 --- a/core/src/components/nav/test/basic/index.html +++ b/core/src/components/nav/test/basic/index.html @@ -16,7 +16,7 @@ class PageOne extends HTMLElement { connectedCallback() { this.innerHTML = ` - + @@ -24,17 +24,11 @@ Page One - - - - Page One - - -
+ +

Page One

Go to Page Two -
`; } @@ -51,6 +45,7 @@ +

Page Two

Go to Page Three @@ -80,12 +75,6 @@

Page Three

customElements.define('page-one', PageOne); customElements.define('page-two', PageTwo); customElements.define('page-three', PageThree); - - window.Ionic = { - config: { - experimentalFocusManagerPriority: ['heading', 'content'] - } - } diff --git a/core/src/components/title/test/basic/index.html b/core/src/components/title/test/basic/index.html index 36ccfdd4c9a..0b3257888b0 100644 --- a/core/src/components/title/test/basic/index.html +++ b/core/src/components/title/test/basic/index.html @@ -37,7 +37,7 @@ - Title + Title Subtitle From 97dd187bd8d63c2fb3cdc530fedc30722342bbc5 Mon Sep 17 00:00:00 2001 From: Liam DeBeasi Date: Thu, 25 Apr 2024 12:47:36 -0400 Subject: [PATCH 21/25] add tests --- .../test/generic/focus-controller.e2e.ts | 63 +++++++++++ .../focus-controller/test/generic/index.html | 105 ++++++++++++++++++ .../test/ionic/focus-controller.e2e.ts | 63 +++++++++++ .../focus-controller/test/ionic/index.html | 104 +++++++++++++++++ 4 files changed, 335 insertions(+) create mode 100644 core/src/utils/focus-controller/test/generic/focus-controller.e2e.ts create mode 100644 core/src/utils/focus-controller/test/generic/index.html create mode 100644 core/src/utils/focus-controller/test/ionic/focus-controller.e2e.ts create mode 100644 core/src/utils/focus-controller/test/ionic/index.html diff --git a/core/src/utils/focus-controller/test/generic/focus-controller.e2e.ts b/core/src/utils/focus-controller/test/generic/focus-controller.e2e.ts new file mode 100644 index 00000000000..ae2771693f9 --- /dev/null +++ b/core/src/utils/focus-controller/test/generic/focus-controller.e2e.ts @@ -0,0 +1,63 @@ +import { expect } from '@playwright/test'; +import { configs, test } from '@utils/test/playwright'; + +configs({ modes: ['md'], directions: ['ltr'] }).forEach(({ title, config }) => { + test.describe(title('focus controller: generic components'), () => { + test.beforeEach(async ({ page }) => { + await page.goto('/src/utils/focus-controller/test/generic', config); + }); + test('should focus heading', async ({ page }) => { + const goToPageOneButton = page.locator('page-root button.page-one'); + const nav = page.locator('ion-nav'); + const ionNavDidChange = await (nav as any).spyOnEvent('ionNavDidChange'); + + // Focus heading on Page One + await goToPageOneButton.click(); + await ionNavDidChange.next(); + + const pageOneTitle = page.locator('page-one h1'); + await expect(pageOneTitle).toBeFocused(); + }); + + test('should focus banner', async ({ page }) => { + const goToPageThreeButton = page.locator('page-root button.page-three'); + const nav = page.locator('ion-nav'); + const ionNavDidChange = await (nav as any).spyOnEvent('ionNavDidChange'); + + const pageThreeHeader = page.locator('page-three header'); + await goToPageThreeButton.click(); + await ionNavDidChange.next(); + + await expect(pageThreeHeader).toBeFocused(); + }); + + test('should focus content', async ({ page }) => { + const goToPageTwoButton = page.locator('page-root button.page-two'); + const nav = page.locator('ion-nav'); + const ionNavDidChange = await (nav as any).spyOnEvent('ionNavDidChange'); + const pageTwoContent = page.locator('page-two main'); + + await goToPageTwoButton.click(); + await ionNavDidChange.next(); + + await expect(pageTwoContent).toBeFocused(); + }); + + test('should return focus when going back', async ({ page, browserName }) => { + test.skip(browserName === 'webkit', 'Desktop Safari does not consider buttons to be focusable'); + + const goToPageOneButton = page.locator('page-root button.page-one'); + const nav = page.locator('ion-nav'); + const ionNavDidChange = await (nav as any).spyOnEvent('ionNavDidChange'); + const pageOneBackButton = page.locator('page-one ion-back-button'); + + await goToPageOneButton.click(); + await ionNavDidChange.next(); + + await pageOneBackButton.click(); + await ionNavDidChange.next(); + + await expect(goToPageOneButton).toBeFocused(); + }); + }); +}); diff --git a/core/src/utils/focus-controller/test/generic/index.html b/core/src/utils/focus-controller/test/generic/index.html new file mode 100644 index 00000000000..c39a834d3c1 --- /dev/null +++ b/core/src/utils/focus-controller/test/generic/index.html @@ -0,0 +1,105 @@ + + + + + Focus Manager + + + + + + + + + + + + + + + + + + + + + diff --git a/core/src/utils/focus-controller/test/ionic/focus-controller.e2e.ts b/core/src/utils/focus-controller/test/ionic/focus-controller.e2e.ts new file mode 100644 index 00000000000..d31b1d2b89e --- /dev/null +++ b/core/src/utils/focus-controller/test/ionic/focus-controller.e2e.ts @@ -0,0 +1,63 @@ +import { expect } from '@playwright/test'; +import { configs, test } from '@utils/test/playwright'; + +configs({ modes: ['md'], directions: ['ltr'] }).forEach(({ title, config }) => { + test.describe(title('focus controller: ionic components'), () => { + test.beforeEach(async ({ page }) => { + await page.goto('/src/utils/focus-controller/test/ionic', config); + }); + test('should focus heading', async ({ page }) => { + const goToPageOneButton = page.locator('page-root ion-button.page-one'); + const nav = page.locator('ion-nav'); + const ionNavDidChange = await (nav as any).spyOnEvent('ionNavDidChange'); + + // Focus heading on Page One + await goToPageOneButton.click(); + await ionNavDidChange.next(); + + const pageOneTitle = page.locator('page-one ion-title'); + await expect(pageOneTitle).toBeFocused(); + }); + + test('should focus banner', async ({ page }) => { + const goToPageThreeButton = page.locator('page-root ion-button.page-three'); + const nav = page.locator('ion-nav'); + const ionNavDidChange = await (nav as any).spyOnEvent('ionNavDidChange'); + + const pageThreeHeader = page.locator('page-three ion-header'); + await goToPageThreeButton.click(); + await ionNavDidChange.next(); + + await expect(pageThreeHeader).toBeFocused(); + }); + + test('should focus content', async ({ page }) => { + const goToPageTwoButton = page.locator('page-root ion-button.page-two'); + const nav = page.locator('ion-nav'); + const ionNavDidChange = await (nav as any).spyOnEvent('ionNavDidChange'); + const pageTwoContent = page.locator('page-two ion-content'); + + await goToPageTwoButton.click(); + await ionNavDidChange.next(); + + await expect(pageTwoContent).toBeFocused(); + }); + + test('should return focus when going back', async ({ page, browserName }) => { + test.skip(browserName === 'webkit', 'Desktop Safari does not consider buttons to be focusable'); + + const goToPageOneButton = page.locator('page-root ion-button.page-one'); + const nav = page.locator('ion-nav'); + const ionNavDidChange = await (nav as any).spyOnEvent('ionNavDidChange'); + const pageOneBackButton = page.locator('page-one ion-back-button'); + + await goToPageOneButton.click(); + await ionNavDidChange.next(); + + await pageOneBackButton.click(); + await ionNavDidChange.next(); + + await expect(goToPageOneButton).toBeFocused(); + }); + }); +}); diff --git a/core/src/utils/focus-controller/test/ionic/index.html b/core/src/utils/focus-controller/test/ionic/index.html new file mode 100644 index 00000000000..e0afb2a6328 --- /dev/null +++ b/core/src/utils/focus-controller/test/ionic/index.html @@ -0,0 +1,104 @@ + + + + + Focus Manager + + + + + + + + + + + + + + + + + + + + + From 057a557d5f85124541fc4f27887c9c592ac2171a Mon Sep 17 00:00:00 2001 From: Liam DeBeasi Date: Thu, 25 Apr 2024 12:55:26 -0400 Subject: [PATCH 22/25] outline ring does not show when focusing --- core/src/css/core.scss | 11 ++++++++++- core/src/utils/focus-controller/index.ts | 3 +++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/core/src/css/core.scss b/core/src/css/core.scss index 3e57a04e458..e0d8f7fa415 100644 --- a/core/src/css/core.scss +++ b/core/src/css/core.scss @@ -397,8 +397,17 @@ ion-input input::-webkit-date-and-time-value { /** * When moving focus on page transitions we call .focus() on an element which can * add an undesired outline ring. This CSS removes the outline ring. + * We also remove the outline ring from elements that are actively being focused + * by the focus manager. We are intentionally selective about which elements this + * applies to so we do not accidentally override outlines set by the developer. */ -[ion-last-focus] { +[ion-last-focus], +header[tabindex="-1"]:focus, +[role="banner"][tabindex="-1"]:focus, +main[tabindex="-1"]:focus, +[role="main"][tabindex="-1"]:focus, +h1[tabindex="-1"]:focus, +[role="heading"][aria-level="1"][tabindex="-1"]:focus { outline: none; } diff --git a/core/src/utils/focus-controller/index.ts b/core/src/utils/focus-controller/index.ts index cd6a0453c33..762de335ee5 100644 --- a/core/src/utils/focus-controller/index.ts +++ b/core/src/utils/focus-controller/index.ts @@ -68,6 +68,9 @@ export const createFocusController = (): FocusController => { * * We intentionally query for specific roles/semantic elements so that the * transition manager can work with both Ionic and non-Ionic UI components. + * + * If new selectors are added, be sure to remove the outline ring by adding + * new selectors to rule in core.scss. */ switch (priority) { case 'content': From 2c84f632c6d5848aa6949e48660b31e346e81fa7 Mon Sep 17 00:00:00 2001 From: Liam DeBeasi Date: Mon, 29 Apr 2024 09:46:36 -0400 Subject: [PATCH 23/25] Update index.ts --- core/src/utils/transition/index.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/utils/transition/index.ts b/core/src/utils/transition/index.ts index 970858ba82c..1451abbabfd 100644 --- a/core/src/utils/transition/index.ts +++ b/core/src/utils/transition/index.ts @@ -7,7 +7,7 @@ import { LIFECYCLE_WILL_LEAVE, } from '../../components/nav/constants'; import type { NavOptions, NavDirection } from '../../components/nav/nav-interface'; -import { config } from '../../global/config'; +import { config } from '@global/config'; import type { Animation, AnimationBuilder } from '../animation/animation-interface'; import { createFocusController } from '../focus-controller'; import { raf } from '../helpers'; From 49681e5bad47bc385574bb2fff2d52043300c6f3 Mon Sep 17 00:00:00 2001 From: Liam DeBeasi Date: Mon, 29 Apr 2024 09:51:11 -0400 Subject: [PATCH 24/25] chore: lint --- core/src/utils/transition/index.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/utils/transition/index.ts b/core/src/utils/transition/index.ts index 1451abbabfd..69789bf862d 100644 --- a/core/src/utils/transition/index.ts +++ b/core/src/utils/transition/index.ts @@ -1,3 +1,4 @@ +import { config } from '@global/config'; import { Build, writeTask } from '@stencil/core'; import { @@ -7,7 +8,6 @@ import { LIFECYCLE_WILL_LEAVE, } from '../../components/nav/constants'; import type { NavOptions, NavDirection } from '../../components/nav/nav-interface'; -import { config } from '@global/config'; import type { Animation, AnimationBuilder } from '../animation/animation-interface'; import { createFocusController } from '../focus-controller'; import { raf } from '../helpers'; From 25b17436b1b73fc7bb0220bd2ed33a32e54420e1 Mon Sep 17 00:00:00 2001 From: Liam DeBeasi Date: Tue, 30 Apr 2024 13:58:29 -0400 Subject: [PATCH 25/25] fix types --- .../test/generic/focus-controller.e2e.ts | 9 +++++---- .../focus-controller/test/ionic/focus-controller.e2e.ts | 9 +++++---- 2 files changed, 10 insertions(+), 8 deletions(-) diff --git a/core/src/utils/focus-controller/test/generic/focus-controller.e2e.ts b/core/src/utils/focus-controller/test/generic/focus-controller.e2e.ts index ae2771693f9..dd10bd59905 100644 --- a/core/src/utils/focus-controller/test/generic/focus-controller.e2e.ts +++ b/core/src/utils/focus-controller/test/generic/focus-controller.e2e.ts @@ -1,5 +1,6 @@ import { expect } from '@playwright/test'; import { configs, test } from '@utils/test/playwright'; +import type { E2ELocator } from '@utils/test/playwright'; configs({ modes: ['md'], directions: ['ltr'] }).forEach(({ title, config }) => { test.describe(title('focus controller: generic components'), () => { @@ -8,7 +9,7 @@ configs({ modes: ['md'], directions: ['ltr'] }).forEach(({ title, config }) => { }); test('should focus heading', async ({ page }) => { const goToPageOneButton = page.locator('page-root button.page-one'); - const nav = page.locator('ion-nav'); + const nav = page.locator('ion-nav') as E2ELocator; const ionNavDidChange = await (nav as any).spyOnEvent('ionNavDidChange'); // Focus heading on Page One @@ -21,7 +22,7 @@ configs({ modes: ['md'], directions: ['ltr'] }).forEach(({ title, config }) => { test('should focus banner', async ({ page }) => { const goToPageThreeButton = page.locator('page-root button.page-three'); - const nav = page.locator('ion-nav'); + const nav = page.locator('ion-nav') as E2ELocator; const ionNavDidChange = await (nav as any).spyOnEvent('ionNavDidChange'); const pageThreeHeader = page.locator('page-three header'); @@ -33,7 +34,7 @@ configs({ modes: ['md'], directions: ['ltr'] }).forEach(({ title, config }) => { test('should focus content', async ({ page }) => { const goToPageTwoButton = page.locator('page-root button.page-two'); - const nav = page.locator('ion-nav'); + const nav = page.locator('ion-nav') as E2ELocator; const ionNavDidChange = await (nav as any).spyOnEvent('ionNavDidChange'); const pageTwoContent = page.locator('page-two main'); @@ -47,7 +48,7 @@ configs({ modes: ['md'], directions: ['ltr'] }).forEach(({ title, config }) => { test.skip(browserName === 'webkit', 'Desktop Safari does not consider buttons to be focusable'); const goToPageOneButton = page.locator('page-root button.page-one'); - const nav = page.locator('ion-nav'); + const nav = page.locator('ion-nav') as E2ELocator; const ionNavDidChange = await (nav as any).spyOnEvent('ionNavDidChange'); const pageOneBackButton = page.locator('page-one ion-back-button'); diff --git a/core/src/utils/focus-controller/test/ionic/focus-controller.e2e.ts b/core/src/utils/focus-controller/test/ionic/focus-controller.e2e.ts index d31b1d2b89e..8b576872b83 100644 --- a/core/src/utils/focus-controller/test/ionic/focus-controller.e2e.ts +++ b/core/src/utils/focus-controller/test/ionic/focus-controller.e2e.ts @@ -1,5 +1,6 @@ import { expect } from '@playwright/test'; import { configs, test } from '@utils/test/playwright'; +import type { E2ELocator } from '@utils/test/playwright'; configs({ modes: ['md'], directions: ['ltr'] }).forEach(({ title, config }) => { test.describe(title('focus controller: ionic components'), () => { @@ -8,7 +9,7 @@ configs({ modes: ['md'], directions: ['ltr'] }).forEach(({ title, config }) => { }); test('should focus heading', async ({ page }) => { const goToPageOneButton = page.locator('page-root ion-button.page-one'); - const nav = page.locator('ion-nav'); + const nav = page.locator('ion-nav') as E2ELocator; const ionNavDidChange = await (nav as any).spyOnEvent('ionNavDidChange'); // Focus heading on Page One @@ -21,7 +22,7 @@ configs({ modes: ['md'], directions: ['ltr'] }).forEach(({ title, config }) => { test('should focus banner', async ({ page }) => { const goToPageThreeButton = page.locator('page-root ion-button.page-three'); - const nav = page.locator('ion-nav'); + const nav = page.locator('ion-nav') as E2ELocator; const ionNavDidChange = await (nav as any).spyOnEvent('ionNavDidChange'); const pageThreeHeader = page.locator('page-three ion-header'); @@ -33,7 +34,7 @@ configs({ modes: ['md'], directions: ['ltr'] }).forEach(({ title, config }) => { test('should focus content', async ({ page }) => { const goToPageTwoButton = page.locator('page-root ion-button.page-two'); - const nav = page.locator('ion-nav'); + const nav = page.locator('ion-nav') as E2ELocator; const ionNavDidChange = await (nav as any).spyOnEvent('ionNavDidChange'); const pageTwoContent = page.locator('page-two ion-content'); @@ -47,7 +48,7 @@ configs({ modes: ['md'], directions: ['ltr'] }).forEach(({ title, config }) => { test.skip(browserName === 'webkit', 'Desktop Safari does not consider buttons to be focusable'); const goToPageOneButton = page.locator('page-root ion-button.page-one'); - const nav = page.locator('ion-nav'); + const nav = page.locator('ion-nav') as E2ELocator; const ionNavDidChange = await (nav as any).spyOnEvent('ionNavDidChange'); const pageOneBackButton = page.locator('page-one ion-back-button');