From 02e5d773319031cee3652ee6682d84ec6a95e342 Mon Sep 17 00:00:00 2001 From: Hein Haraldson Berg Date: Mon, 4 Oct 2021 23:14:45 +0200 Subject: [PATCH 1/7] Make it possible to opt out of focus handling --- src/HashLink.jsx | 41 ++++++++++++++++++++++------------------- 1 file changed, 22 insertions(+), 19 deletions(-) diff --git a/src/HashLink.jsx b/src/HashLink.jsx index 6b5d8b6..d59ae26 100644 --- a/src/HashLink.jsx +++ b/src/HashLink.jsx @@ -25,7 +25,7 @@ function isInteractiveElement(element) { ); } -function getElAndScroll() { +function getElAndScroll(handleFocus) { let element = null; if (hashFragment === '#') { // use document.body instead of document.documentElement because of a bug in smoothscroll-polyfill in safari @@ -47,19 +47,21 @@ function getElAndScroll() { if (element !== null) { scrollFunction(element); - // update focus to where the page is scrolled to - // unfortunately this doesn't work in safari (desktop and iOS) when blur() is called - let originalTabIndex = element.getAttribute('tabindex'); - if (originalTabIndex === null && !isInteractiveElement(element)) { - element.setAttribute('tabindex', -1); - } - element.focus({ preventScroll: true }); - if (originalTabIndex === null && !isInteractiveElement(element)) { - // for some reason calling blur() in safari resets the focus region to where it was previously, - // if blur() is not called it works in safari, but then are stuck with default focus styles - // on an element that otherwise might never had focus styles applied, so not an option - element.blur(); - element.removeAttribute('tabindex'); + if (handleFocus) { + // update focus to where the page is scrolled to + // unfortunately this doesn't work in safari (desktop and iOS) when blur() is called + let originalTabIndex = element.getAttribute('tabindex'); + if (originalTabIndex === null && !isInteractiveElement(element)) { + element.setAttribute('tabindex', -1); + } + element.focus({ preventScroll: true }); + if (originalTabIndex === null && !isInteractiveElement(element)) { + // for some reason calling blur() in safari resets the focus region to where it was previously, + // if blur() is not called it works in safari, but then are stuck with default focus styles + // on an element that otherwise might never had focus styles applied, so not an option + element.blur(); + element.removeAttribute('tabindex'); + } } reset(); @@ -68,12 +70,12 @@ function getElAndScroll() { return false; } -function hashLinkScroll(timeout) { +function hashLinkScroll(timeout, handleFocus) { // Push onto callback queue so it runs after the DOM is updated window.setTimeout(() => { - if (getElAndScroll() === false) { + if (getElAndScroll(handleFocus) === false) { if (observer === null) { - observer = new MutationObserver(getElAndScroll); + observer = new MutationObserver(() => getElAndScroll(handleFocus)); } observer.observe(document, { attributes: true, @@ -125,10 +127,10 @@ export function genericHashLink(As) { props.smooth ? el.scrollIntoView({ behavior: 'smooth' }) : el.scrollIntoView()); - hashLinkScroll(props.timeout); + hashLinkScroll(props.timeout, props.handleFocus); } } - const { scroll, smooth, timeout, elementId, ...filteredProps } = props; + const { scroll, smooth, timeout, elementId, handleFocus, ...filteredProps } = props; return ( {props.children} @@ -152,6 +154,7 @@ if (process.env.NODE_ENV !== 'production') { timeout: PropTypes.number, elementId: PropTypes.string, to: PropTypes.oneOfType([PropTypes.string, PropTypes.object]), + handleFocus: PropTypes.bool, }; HashLink.propTypes = propTypes; From f40ca674c471c18ce8d4720a0d77ebf89a88cef9 Mon Sep 17 00:00:00 2001 From: Hein Haraldson Berg Date: Mon, 4 Oct 2021 23:17:32 +0200 Subject: [PATCH 2/7] Regard contenteditables as interactive elements --- src/HashLink.jsx | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/HashLink.jsx b/src/HashLink.jsx index d59ae26..1ad6d6e 100644 --- a/src/HashLink.jsx +++ b/src/HashLink.jsx @@ -21,7 +21,8 @@ function isInteractiveElement(element) { const linkTags = ['A', 'AREA']; return ( (formTags.includes(element.tagName) && !element.hasAttribute('disabled')) || - (linkTags.includes(element.tagName) && element.hasAttribute('href')) + (linkTags.includes(element.tagName) && element.hasAttribute('href')) || + element.isContentEditable ); } From 996945b5f8cb78b15c1101d2e424da673edca714 Mon Sep 17 00:00:00 2001 From: Hein Haraldson Berg Date: Mon, 4 Oct 2021 23:34:32 +0200 Subject: [PATCH 3/7] Document the option --- README.md | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index 86b06a3..8b7a4ea 100644 --- a/README.md +++ b/README.md @@ -152,7 +152,7 @@ const MyComponent = () => ( ## Focus Management -`react-router-hash-link` attempts to recreate the native browser focusing behavior as closely as possible. +`react-router-hash-link` attempts to recreate the native browser focusing behavior as closely as possible The browser native behavior when clicking a hash link is: @@ -165,3 +165,7 @@ To recreate this `react-router-hash-link` does the following: - For focusable elements, it calls `element.focus()` and leaves focus on the target element. Note that you may find it useful to leave focus on non-interactive elements (by adding a `tabindex` of `-1`) to augment the navigation action with a visual focus indicator. + +### `handleFocus: boolean` + +- Optionally disable the focus handling From 7eabaa8c8a5b8c87ce126e740ab4694d4308d397 Mon Sep 17 00:00:00 2001 From: Hein Haraldson Berg Date: Mon, 4 Oct 2021 23:41:15 +0200 Subject: [PATCH 4/7] Revert unintended removal of a period --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 8b7a4ea..f76a257 100644 --- a/README.md +++ b/README.md @@ -152,7 +152,7 @@ const MyComponent = () => ( ## Focus Management -`react-router-hash-link` attempts to recreate the native browser focusing behavior as closely as possible +`react-router-hash-link` attempts to recreate the native browser focusing behavior as closely as possible. The browser native behavior when clicking a hash link is: From ca95a761fbae966647f09968acdc52e82dacc105 Mon Sep 17 00:00:00 2001 From: Hein Haraldson Berg Date: Thu, 7 Oct 2021 14:48:17 +0200 Subject: [PATCH 5/7] Flip the logic to match the desired default value `handleFocus` (needed to be set to `true` to retain current focus behavior, a breaking change) -> `preventFocusHandling` (needs to be explicitly set to `true` to opt out of current focus behavior, non-breaking). --- src/HashLink.jsx | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/HashLink.jsx b/src/HashLink.jsx index 1ad6d6e..6444963 100644 --- a/src/HashLink.jsx +++ b/src/HashLink.jsx @@ -26,7 +26,7 @@ function isInteractiveElement(element) { ); } -function getElAndScroll(handleFocus) { +function getElAndScroll(preventFocusHandling) { let element = null; if (hashFragment === '#') { // use document.body instead of document.documentElement because of a bug in smoothscroll-polyfill in safari @@ -48,7 +48,7 @@ function getElAndScroll(handleFocus) { if (element !== null) { scrollFunction(element); - if (handleFocus) { + if (!preventFocusHandling) { // update focus to where the page is scrolled to // unfortunately this doesn't work in safari (desktop and iOS) when blur() is called let originalTabIndex = element.getAttribute('tabindex'); @@ -71,12 +71,12 @@ function getElAndScroll(handleFocus) { return false; } -function hashLinkScroll(timeout, handleFocus) { +function hashLinkScroll(timeout, preventFocusHandling) { // Push onto callback queue so it runs after the DOM is updated window.setTimeout(() => { - if (getElAndScroll(handleFocus) === false) { + if (getElAndScroll(preventFocusHandling) === false) { if (observer === null) { - observer = new MutationObserver(() => getElAndScroll(handleFocus)); + observer = new MutationObserver(() => getElAndScroll(preventFocusHandling)); } observer.observe(document, { attributes: true, @@ -128,10 +128,10 @@ export function genericHashLink(As) { props.smooth ? el.scrollIntoView({ behavior: 'smooth' }) : el.scrollIntoView()); - hashLinkScroll(props.timeout, props.handleFocus); + hashLinkScroll(props.timeout, props.preventFocusHandling); } } - const { scroll, smooth, timeout, elementId, handleFocus, ...filteredProps } = props; + const { scroll, smooth, timeout, elementId, preventFocusHandling, ...filteredProps } = props; return ( {props.children} @@ -155,7 +155,7 @@ if (process.env.NODE_ENV !== 'production') { timeout: PropTypes.number, elementId: PropTypes.string, to: PropTypes.oneOfType([PropTypes.string, PropTypes.object]), - handleFocus: PropTypes.bool, + preventFocusHandling: PropTypes.bool, }; HashLink.propTypes = propTypes; From 73c610751f7f39c1a730abc5cbf37b012d609ccb Mon Sep 17 00:00:00 2001 From: Hein Haraldson Berg Date: Thu, 7 Oct 2021 14:49:04 +0200 Subject: [PATCH 6/7] Update docs to reflect flipped logic --- README.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index f76a257..b4ff426 100644 --- a/README.md +++ b/README.md @@ -166,6 +166,6 @@ To recreate this `react-router-hash-link` does the following: Note that you may find it useful to leave focus on non-interactive elements (by adding a `tabindex` of `-1`) to augment the navigation action with a visual focus indicator. -### `handleFocus: boolean` +### `preventFocusHandling: boolean` -- Optionally disable the focus handling +- Optionally prevent the default focus handling From 784b3507c00556fd3dc877c3c367378ed3cef42f Mon Sep 17 00:00:00 2001 From: Hein Haraldson Berg Date: Thu, 7 Oct 2021 18:18:46 +0200 Subject: [PATCH 7/7] Code review fixes --- src/HashLink.jsx | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/src/HashLink.jsx b/src/HashLink.jsx index 6444963..bcd7a6c 100644 --- a/src/HashLink.jsx +++ b/src/HashLink.jsx @@ -6,9 +6,11 @@ let hashFragment = ''; let observer = null; let asyncTimerId = null; let scrollFunction = null; +let preventFocusHandling = false; function reset() { hashFragment = ''; + preventFocusHandling = false; if (observer !== null) observer.disconnect(); if (asyncTimerId !== null) { window.clearTimeout(asyncTimerId); @@ -26,7 +28,7 @@ function isInteractiveElement(element) { ); } -function getElAndScroll(preventFocusHandling) { +function getElAndScroll() { let element = null; if (hashFragment === '#') { // use document.body instead of document.documentElement because of a bug in smoothscroll-polyfill in safari @@ -71,12 +73,12 @@ function getElAndScroll(preventFocusHandling) { return false; } -function hashLinkScroll(timeout, preventFocusHandling) { +function hashLinkScroll(timeout) { // Push onto callback queue so it runs after the DOM is updated window.setTimeout(() => { - if (getElAndScroll(preventFocusHandling) === false) { + if (getElAndScroll() === false) { if (observer === null) { - observer = new MutationObserver(() => getElAndScroll(preventFocusHandling)); + observer = new MutationObserver(getElAndScroll); } observer.observe(document, { attributes: true, @@ -112,6 +114,7 @@ export function genericHashLink(As) { function handleClick(e) { reset(); hashFragment = props.elementId ? `#${props.elementId}` : linkHash; + preventFocusHandling = !!props.preventFocusHandling; if (props.onClick) props.onClick(e); if ( hashFragment !== '' && @@ -128,7 +131,7 @@ export function genericHashLink(As) { props.smooth ? el.scrollIntoView({ behavior: 'smooth' }) : el.scrollIntoView()); - hashLinkScroll(props.timeout, props.preventFocusHandling); + hashLinkScroll(props.timeout); } } const { scroll, smooth, timeout, elementId, preventFocusHandling, ...filteredProps } = props;