Skip to content

Prevent Nav from Trapping while tabbing #427

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 11 commits into from
Sep 9, 2022
Merged

Prevent Nav from Trapping while tabbing #427

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
6f55b84
Initial work on setting tabindex to -1 deeply
hqhhuang Aug 30, 2022
407a90f
Refactor setting tabindex
hqhhuang Aug 31, 2022
5ea0443
Added test for tabindex
hqhhuang Sep 6, 2022
cc5c50d
Merge branch 'main' into nav-focus-trap
hqhhuang Sep 6, 2022
bc86684
Change function/variable name and revert unrelated changes
hqhhuang Sep 6, 2022
257f7e7
Remove `FocusTrap` from `NavBase`
hqhhuang Sep 6, 2022
80b8b59
Remove unnecessary async
hqhhuang Sep 6, 2022
0fa70b9
Revert change to `Asset.spec.js`
hqhhuang Sep 6, 2022
466d1d4
Fix header and simplify methods
hqhhuang Sep 7, 2022
8218e96
Fix header and revert whiteline change
hqhhuang Sep 7, 2022
e8ce464
simplify cacheAttribute function
hqhhuang Sep 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 3 additions & 8 deletions src/components/NavBase.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
<!--
This source file is part of the Swift.org open source project

Copyright (c) 2021 Apple Inc. and the Swift project authors
Copyright (c) 2021-2022 Apple Inc. and the Swift project authors
Licensed under Apache License v2.0 with Runtime Library Exception

See https://swift.org/LICENSE.txt for license information
Expand Down Expand Up @@ -75,7 +75,6 @@ import onIntersect from 'docc-render/mixins/onIntersect';
import NavMenuItems from 'docc-render/components/NavMenuItems.vue';
import BreakpointEmitter from 'docc-render/components/BreakpointEmitter.vue';

import FocusTrap from 'docc-render/utils/FocusTrap';
import scrollLock from 'docc-render/utils/scroll-lock';
import { baseNavStickyAnchorId, MenuLinkModifierClasses } from 'docc-render/constants/nav';
import { isBreakpointAbove } from 'docc-render/utils/breakpoints';
Expand Down Expand Up @@ -154,7 +153,6 @@ export default {
isTransitioning: false,
isSticking: false,
noBackgroundTransition: true,
focusTrapInstance: null,
currentBreakpoint: BreakpointName.large,
};
},
Expand Down Expand Up @@ -196,7 +194,6 @@ export default {
document.addEventListener('click', this.handleClickOutside);
this.handleFlashOnMount();
await this.$nextTick();
this.focusTrapInstance = new FocusTrap(this.$refs.wrapper);
},
beforeDestroy() {
window.removeEventListener('keydown', this.onEscape);
Expand All @@ -206,7 +203,6 @@ export default {
if (this.isOpen) {
this.toggleScrollLock(false);
}
this.focusTrapInstance.destroy();
},
methods: {
getIntersectionTargets() {
Expand Down Expand Up @@ -317,16 +313,15 @@ export default {
},
onExpand() {
this.$emit('open');
// lock focus
this.focusTrapInstance.start();
// hide sibling elements from VO
changeElementVOVisibility.hide(this.$refs.wrapper);
// focus on the toggle to prevent tabbing to links in the body
this.$refs.axToggle.focus();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why focusing on the axToggle here? should it be implicated focused if you clicked on it?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You click on another element, the axToggle is only when you focus navigate around :/

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is needed to retain focus on the toggle when user expands Nav with mouse, then starts tabbing.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we introduced a regression here....

},
onClose() {
this.$emit('close');
// stop the scroll lock
this.toggleScrollLock(false);
this.focusTrapInstance.stop();
changeElementVOVisibility.show(this.$refs.wrapper);
},
async handleFlashOnMount() {
Expand Down
85 changes: 61 additions & 24 deletions src/utils/changeElementVOVisibility.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,46 @@
/**
* This source file is part of the Swift.org open source project
*
* Copyright (c) 2021 Apple Inc. and the Swift project authors
* Copyright (c) 2021-2022 Apple Inc. and the Swift project authors
* Licensed under Apache License v2.0 with Runtime Library Exception
*
* See https://swift.org/LICENSE.txt for license information
* See https://swift.org/CONTRIBUTORS.txt for Swift project authors
*/
import TabManager from 'docc-render/utils/TabManager';

const OG_PREFIX = 'data-original-';
const ARIA = 'aria-hidden';
const TABINDEX = 'tabindex';

function cacheOriginalAttribute(element, prop) {
const attr = OG_PREFIX + prop;

// make sure that prop isn't cached already
if (element.getAttribute(attr)) return;

const originalValue = element.getAttribute(prop) || '';
element.setAttribute(attr, originalValue);
}

function retrieveOriginalAttribute(element, prop) {
const attr = OG_PREFIX + prop;

// return if attribute doesn't exist
if (!element.hasAttribute(attr)) return;
// get the cached property
const originalValue = element.getAttribute(attr);
// remove the prefixed attribute
element.removeAttribute(attr);

// if there is a value, set it back.
if (originalValue.length) {
element.setAttribute(prop, originalValue);
} else {
// otherwise remove the attribute entirely.
element.removeAttribute(prop);
}
}

/* eslint-disable no-cond-assign */
function iterateOverSiblings(el, callback) {
Expand All @@ -27,43 +61,46 @@ function iterateOverSiblings(el, callback) {
}
}

const PREFIX = 'data-original-';
const prop = 'aria-hidden';
const prefixedProperty = PREFIX + prop;

/**
* Hides an element from VO
* @param {HTMLElement} element
*/
const hideElement = (element) => {
let originalValue = element.getAttribute(prefixedProperty);
if (!originalValue) {
// store the prop temporarily, to retrieve later.
originalValue = element.getAttribute(prop) || '';
element.setAttribute(prefixedProperty, originalValue);
// set original value for prefixed properties and tabindex
// store the prop temporarily, to retrieve later.
cacheOriginalAttribute(element, ARIA);
cacheOriginalAttribute(element, TABINDEX);

// hide the component from VO
element.setAttribute(ARIA, 'true');

// hide the component from tabbing
element.setAttribute(TABINDEX, '-1');
// make sure element's tabbable children are hidden as well
const tabbables = TabManager.getTabbableElements(element);
let i = tabbables.length - 1;
while (i >= 0) {
cacheOriginalAttribute(tabbables[i], TABINDEX);
tabbables[i].setAttribute(TABINDEX, '-1');
i -= 1;
}
// hide the component
element.setAttribute(prop, 'true');
};

/**
* Show an element
* @param {HTMLElement} element
*/
const showElement = (element) => {
// get the cached property
const originalValue = element.getAttribute(prefixedProperty);
if (typeof originalValue === 'string') {
// if there is a value, set it back.
if (originalValue.length) {
element.setAttribute(prop, originalValue);
} else {
// otherwise remove the attribute entirely.
element.removeAttribute(prop);
}
retrieveOriginalAttribute(element, ARIA);
retrieveOriginalAttribute(element, TABINDEX);

// make sure element's tabbable children are restored as well
const tabbables = element.querySelectorAll(`[${OG_PREFIX + TABINDEX}]`);
let i = tabbables.length - 1;
while (i >= 0) {
retrieveOriginalAttribute(tabbables[i], TABINDEX);
i -= 1;
}
// remove the prefixed attribute
element.removeAttribute(prefixedProperty);
};

/**
Expand Down
29 changes: 4 additions & 25 deletions tests/unit/components/NavBase.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/**
* This source file is part of the Swift.org open source project
*
* Copyright (c) 2021 Apple Inc. and the Swift project authors
* Copyright (c) 2021-2022 Apple Inc. and the Swift project authors
* Licensed under Apache License v2.0 with Runtime Library Exception
*
* See https://swift.org/LICENSE.txt for license information
Expand All @@ -13,15 +13,13 @@ import { shallowMount } from '@vue/test-utils';
import NavMenuItems from 'docc-render/components/NavMenuItems.vue';
import BreakpointEmitter from 'docc-render/components/BreakpointEmitter.vue';
import scrollLock from 'docc-render/utils/scroll-lock';
import FocusTrap from 'docc-render/utils/FocusTrap';
import changeElementVOVisibility from 'docc-render/utils/changeElementVOVisibility';
import { baseNavStickyAnchorId, MenuLinkModifierClasses } from 'docc-render/constants/nav';
import { waitFrames } from 'docc-render/utils/loading';
import { createEvent } from '../../../test-utils';

jest.mock('docc-render/utils/changeElementVOVisibility');
jest.mock('docc-render/utils/scroll-lock');
jest.mock('docc-render/utils/FocusTrap');

const { BreakpointScopes, BreakpointName } = BreakpointEmitter.constants;
const { NoBGTransitionFrames, NavStateClasses } = NavBase.constants;
Expand Down Expand Up @@ -454,30 +452,11 @@ describe('NavBase', () => {
expect(scrollLock.unlockScroll).toHaveBeenCalledTimes(1);
});

it('locks the focus on expand', async () => {
it('focuses on the toggle on expand', async () => {
wrapper = await createWrapper();
expect(FocusTrap).toHaveBeenCalledTimes(1);
expect(FocusTrap).toHaveBeenCalledWith(wrapper.vm.$refs.wrapper);
wrapper.find({ ref: 'axToggle' }).trigger('click');
await wrapper.vm.$nextTick();
expect(FocusTrap.mock.results[0].value.start).toHaveBeenCalledTimes(1);
});

it('unlocks the focus on close', async () => {
wrapper = await createWrapper();
wrapper.find({ ref: 'axToggle' }).trigger('click');
await wrapper.vm.$nextTick();
expect(FocusTrap.mock.results[0].value.start).toHaveBeenCalledTimes(1);
expect(FocusTrap.mock.results[0].value.stop).toHaveBeenCalledTimes(0);
wrapper.find({ ref: 'axToggle' }).trigger('click');
expect(FocusTrap.mock.results[0].value.stop).toHaveBeenCalledTimes(1);
});

it('destroys the focus instance on component destroy', async () => {
wrapper = await createWrapper();
expect(FocusTrap.mock.results[0].value.destroy).toHaveBeenCalledTimes(0);
wrapper.destroy();
expect(FocusTrap.mock.results[0].value.destroy).toHaveBeenCalledTimes(1);
// assert the toggle is focused
expect(document.activeElement).toEqual(wrapper.find({ ref: 'axToggle' }).element);
});

it('changes the sibling visibility to `hidden` on expand', async () => {
Expand Down
54 changes: 47 additions & 7 deletions tests/unit/utils/changeElementVOVisibility.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/**
* This source file is part of the Swift.org open source project
*
* Copyright (c) 2021 Apple Inc. and the Swift project authors
* Copyright (c) 2021-2022 Apple Inc. and the Swift project authors
* Licensed under Apache License v2.0 with Runtime Library Exception
*
* See https://swift.org/LICENSE.txt for license information
Expand Down Expand Up @@ -44,18 +44,23 @@ describe("changeElementVOVisibility", () => {
expect(document.querySelector(".main .navigation").getAttribute("aria-hidden")).toBe("true");
expect(document.querySelector(".main .target").getAttribute("aria-hidden")).toBeFalsy();
expect(document.querySelector(".footer").getAttribute("aria-hidden")).toBe("true");

expect(document.querySelector(".header").getAttribute("tabindex")).toBe("-1");
expect(document.querySelector(".main .navigation").getAttribute("tabindex")).toBe("-1");
expect(document.querySelector(".footer").getAttribute("tabindex")).toBe("-1");

expect(document.body.outerHTML).toMatchInlineSnapshot(`
<body>
<div>
<div>
<div class="header" data-original-aria-hidden="" aria-hidden="true">Header</div>
<div class="header" data-original-aria-hidden="" data-original-tabindex="" aria-hidden="true" tabindex="-1">Header</div>
<main class="main">
<nav class="navigation" data-original-aria-hidden="" aria-hidden="true">Navigation</nav>
<nav class="navigation" data-original-aria-hidden="" data-original-tabindex="" aria-hidden="true" tabindex="-1">Navigation</nav>
<div class="target">
<div class="inside">Inside</div>
</div>
</main>
<div class="footer" data-original-aria-hidden="" aria-hidden="true">Footer</div>
<div class="footer" data-original-aria-hidden="" data-original-tabindex="" aria-hidden="true" tabindex="-1">Footer</div>
</div>
</div>
</body>
Expand Down Expand Up @@ -110,14 +115,49 @@ describe("changeElementVOVisibility", () => {
<body>
<div>
<div>
<div class="header" aria-hidden="true" data-original-aria-hidden="true">Header</div>
<div class="header" aria-hidden="true" data-original-aria-hidden="true" data-original-tabindex="" tabindex="-1">Header</div>
<main class="main">
<nav class="navigation" aria-hidden="true" data-original-aria-hidden="false" data-original-tabindex="" tabindex="-1">Navigation</nav>
<div class="target">
<div class="inside">Inside</div>
</div>
</main>
<div class="footer" data-original-aria-hidden="" data-original-tabindex="" aria-hidden="true" tabindex="-1">Footer</div>
</div>
</div>
</body>
`);
changeElementVOVisibility.show(target);
expect(document.body.outerHTML).toEqual(cachedHTML);
});

it("preserves previously set tabindex values", () => {
DOM.querySelector(".header").setAttribute("tabindex", "2");
DOM.querySelector(".navigation").setAttribute("tabindex", "-1");
document.body.appendChild(DOM);
const cachedHTML = document.body.outerHTML;

const target = document.querySelector(".target");
// hide all
changeElementVOVisibility.hide(target);
expect(document.querySelector(".header").getAttribute("data-original-tabindex")).toBe(
"2"
);
expect(document.querySelector(".navigation").getAttribute("data-original-tabindex")).toBe(
"-1"
);
expect(document.body.outerHTML).toMatchInlineSnapshot(`
<body>
<div>
<div>
<div class="header" tabindex="-1" data-original-aria-hidden="" data-original-tabindex="2" aria-hidden="true">Header</div>
<main class="main">
<nav class="navigation" aria-hidden="true" data-original-aria-hidden="false">Navigation</nav>
<nav class="navigation" tabindex="-1" data-original-aria-hidden="" data-original-tabindex="-1" aria-hidden="true">Navigation</nav>
<div class="target">
<div class="inside">Inside</div>
</div>
</main>
<div class="footer" data-original-aria-hidden="" aria-hidden="true">Footer</div>
<div class="footer" data-original-aria-hidden="" data-original-tabindex="" aria-hidden="true" tabindex="-1">Footer</div>
</div>
</div>
</body>
Expand Down