Prevent Nav from Trapping while tabbing #427

hqhhuang · 2022-09-06T05:47:36Z

Bug/issue #, if applicable: rdar://94044029 ([AX] Localnav should not FocusTrap for documentation pages)

Summary

For accessibility considerations, when we tab to the end of the nav menu, we should tab to the browser menu bar next (instead of tabbing to the beginning of the nav menu again).

Dependencies

NA

Testing

Steps:

Medium or small viewport, tab open the nav menu bar
tab through the items in the menu, at the end, focus should be brought to the browser address bar. It should not take you to the first link within our nav menu bar.

Checklist

Make sure you check off the following items. If they cannot be completed, provide a reason.

Added tests
Ran npm test, and it succeeded
Updated documentation if necessary - NA

Initial work on setting tabindex to -1 deeply

Refactor setting tabindex

Added test for tabindex

hqhhuang · 2022-09-06T05:47:54Z

@swift-ci test

dobromir-hristov

I did a quick test and it looks to work good.

The code is OK, just left a few small comments.

dobromir-hristov · 2022-09-06T06:04:24Z

src/components/NavBase.vue

      this.$emit('open');
      // lock focus
+      await this.$nextTick();


I know why we added this, but do we need it for this fix?

dobromir-hristov · 2022-09-06T06:05:19Z

tests/unit/components/Asset.spec.js

@@ -185,7 +185,7 @@ describe('Asset', () => {
      expect(asset.exists()).toBe(true);
      expect(asset.props()).toEqual({
        alt: image.alt,
-        loading: "lazy",


Lets revert these as they are not related to the PR.

Yea I figured. These 2 are generated when I ran test.

Should we open a separate PR to fix these?

dobromir-hristov · 2022-09-06T06:06:55Z

src/utils/changeElementVOVisibility.js

+function setOriginalValue(element, prop) {
+  // make sure that prop isn't cached already
+  let originalValue = element.getAttribute(PREFIX + prop);
+  if (originalValue === undefined || originalValue === null || originalValue === '') {


You can probably just check for hasAttribute instead?

dobromir-hristov · 2022-09-06T06:09:20Z

src/utils/changeElementVOVisibility.js

 * 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 PREFIX = 'data-original-';


maybe something like this, so its more clear what the prefix is?

Suggested change

const PREFIX = 'data-original-';

const OG_PREFIX = 'data-original-';

dobromir-hristov · 2022-09-06T06:12:02Z

src/utils/changeElementVOVisibility.js

+const ARIA = 'aria-hidden';
+const TABINDEX = 'tabindex';
+
+function setOriginalValue(element, prop) {


Maybe call it something like preserveAttribute or cacheAttribute?

setOriginalValue is very vague.

dobromir-hristov · 2022-09-06T06:13:34Z

src/utils/changeElementVOVisibility.js

+  // remove the prefixed attribute
+  element.removeAttribute(PREFIX + prop);
+
+  if (typeof originalValue === 'string') {


I wonder why the check if its a string 🤔

I tested this. If we call element.getAttribute on an attribute that doesn't exist, the return value is null and the type is therefore an object.
In our case, the cacheOriginalAttribute ensures that even where there's no original value, we still set that attribute, just with an empty string. So in our case, the type of originalValue here should always be a string. However, if we had a bug where we somehow didn't set the attribute correctly, we would want to check the type here.

Thanks for the explanation, @hqhhuang , maybe then it's easier to do something like this?

function retrieveOriginalAttribute(element, prop) { // exit function if attribute doesn't exist if (!element.hasAttribute(OG_PREFIX + prop)) return; // get the cached property const originalValue = element.getAttribute(OG_PREFIX + prop); // remove the prefixed attribute element.removeAttribute(OG_PREFIX + prop); if (originalValue.length) { element.setAttribute(prop, originalValue); } else { // otherwise remove the attribute entirely. element.removeAttribute(prop); } }

Cant you just check for if(originalValue) ? Its always a string or null, so a simple truthy check should work.

We could also probably store OG_PREFIX + prop as a var in the function, so we dont concat it 3 times.

This is a good idea!
if (!element.hasAttribute(OG_PREFIX + prop)) return;

dobromir-hristov · 2022-09-06T06:17:49Z

src/utils/changeElementVOVisibility.js

+  retrieveOriginalValue(element, TABINDEX);
+
+  // make sure element's tabbable children are restored as well
+  const tabbables = element.querySelectorAll(`[${PREFIX + TABINDEX}]`);


TabManager.getTabbableElements would not work here now, would it? 🤔 Because they are no longer tabbable.

Yup. If we use getTabbableElements here, the only ones that would get restored would be the ones that are focusable by nature.

Change function/variable name and revert unrelated changes

Remove `FocusTrap` from `NavBase`

Remove unnecessary async

Revert change to `Asset.spec.js`

hqhhuang · 2022-09-06T23:22:07Z

@swift-ci test

dobromir-hristov

LGTM, left a small final question.

dobromir-hristov · 2022-09-07T10:51:09Z

src/utils/changeElementVOVisibility.js

+function cacheOriginalAttribute(element, prop) {
+  // make sure that prop isn't cached already
+  let originalValue = element.getAttribute(OG_PREFIX + prop);
+  if (!element.hasAttribute(OG_PREFIX + prop) || originalValue === '') {


Shouldnt we just check if there is a cached attribute and only cache if there is not?

Suggested change

if (!element.hasAttribute(OG_PREFIX + prop) || originalValue === '') {

if (!element.hasAttribute(OG_PREFIX + prop)) {

Is there any reason why this function doesn't look like this?

function cacheOriginalAttribute(element, prop) { // make sure that prop isn't cached already if (element.hasAttribute(OG_PREFIX + prop)) return; const originalValue = element.getAttribute(prop) || ''; element.setAttribute(OG_PREFIX + prop, originalValue); }

I don't understand why adding the originalValue as a let var before.

so we wrote the same comment @dobromir-hristov 😆 #427 (comment)
actually you wrote it first!

Because if the element has a cached attribute, but its value is an empty string; we don't want to return here on this line:
if (element.hasAttribute(OG_PREFIX + prop)) return;

We would still want to set the attribute. Did I miss something?

then what about if (!!element.getAttribute(OG_PREFIX + prop)) return; ?

marinaaisa · 2022-09-07T13:49:48Z

src/utils/changeElementVOVisibility.js

@@ -1,12 +1,44 @@
 /**
 * This source file is part of the Swift.org open source project
 *
- * Copyright (c) 2021 Apple Inc. and the Swift project authors
+ * Copyright (c) 2022 Apple Inc. and the Swift project authors


I believe we should do 2021-2022 instead of 2022 if we edit a file. :)

dobromir-hristov · 2022-09-07T14:04:34Z

src/utils/changeElementVOVisibility.js

+function cacheOriginalAttribute(element, prop) {
+  // make sure that prop isn't cached already
+  let originalValue = element.getAttribute(OG_PREFIX + prop);
+  if (!element.hasAttribute(OG_PREFIX + prop) || originalValue === '') {
+    originalValue = element.getAttribute(prop) || '';
+    element.setAttribute(OG_PREFIX + prop, originalValue);
+  }
+}


or maybe even:

Suggested change

function cacheOriginalAttribute(element, prop) {

// make sure that prop isn't cached already

let originalValue = element.getAttribute(OG_PREFIX + prop);

if (!element.hasAttribute(OG_PREFIX + prop) || originalValue === '') {

originalValue = element.getAttribute(prop) || '';

element.setAttribute(OG_PREFIX + prop, originalValue);

}

}

function cacheOriginalAttribute(element, prop) {

// make sure that prop isn't cached already

if(element.hasAttribute(OG_PREFIX + prop)) return

// cache the attribute

const value = element.getAttribute(prop) || '';

element.setAttribute(OG_PREFIX + prop, value);

}

Replied above under Marina's comment: https://github.com/apple/swift-docc-render/pull/427/files#r965056810

dobromir-hristov · 2022-09-07T14:08:00Z

src/utils/changeElementVOVisibility.js

+  // remove the prefixed attribute
+  element.removeAttribute(PREFIX + prop);
+
+  if (typeof originalValue === 'string') {


Cant you just check for if(originalValue) ? Its always a string or null, so a simple truthy check should work.

We could also probably store OG_PREFIX + prop as a var in the function, so we dont concat it 3 times.

marinaaisa · 2022-09-07T14:35:57Z

src/components/NavBase.vue

      // 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();


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

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

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

I think we introduced a regression here....

Fix header and simplify methods

Fix header and revert whiteline change

simplify cacheAttribute function

hqhhuang · 2022-09-07T18:33:03Z

@swift-ci test

hqhhuang added 3 commits August 29, 2022 22:22

Initial work on setting tabindex to -1 deeply

6f55b84

Initial work on setting tabindex to -1 deeply

Refactor setting tabindex

407a90f

Refactor setting tabindex

Added test for tabindex

5ea0443

Added test for tabindex

hqhhuang requested review from mportiz08, marinaaisa and dobromir-hristov September 6, 2022 05:47

Merge branch 'main' into nav-focus-trap

cc5c50d

dobromir-hristov reviewed Sep 6, 2022

View reviewed changes

hqhhuang added 4 commits September 6, 2022 15:38

Change function/variable name and revert unrelated changes

bc86684

Change function/variable name and revert unrelated changes

Remove FocusTrap from NavBase

257f7e7

Remove `FocusTrap` from `NavBase`

Remove unnecessary async

80b8b59

Remove unnecessary async

Revert change to Asset.spec.js

0fa70b9

Revert change to `Asset.spec.js`

dobromir-hristov reviewed Sep 7, 2022

View reviewed changes

marinaaisa reviewed Sep 7, 2022

View reviewed changes

dobromir-hristov reviewed Sep 7, 2022

View reviewed changes

marinaaisa reviewed Sep 7, 2022

View reviewed changes

Fix header and simplify methods

466d1d4

Fix header and simplify methods

hqhhuang requested review from marinaaisa and dobromir-hristov September 7, 2022 16:41

hqhhuang added 2 commits September 7, 2022 09:43

Fix header and revert whiteline change

8218e96

Fix header and revert whiteline change

simplify cacheAttribute function

e8ce464

simplify cacheAttribute function

dobromir-hristov approved these changes Sep 8, 2022

View reviewed changes

hqhhuang merged commit 787c2fb into swiftlang:main Sep 9, 2022

hqhhuang deleted the nav-focus-trap branch September 9, 2022 07:27

marinaaisa mentioned this pull request May 19, 2023

[AX] Localnav should not FocusTrap for documentation pages #341

Closed

3 tasks

	const PREFIX = 'data-original-';
	const OG_PREFIX = 'data-original-';

	if (!element.hasAttribute(OG_PREFIX + prop) \|\| originalValue === '') {
	if (!element.hasAttribute(OG_PREFIX + prop)) {

Prevent Nav from Trapping while tabbing #427

Prevent Nav from Trapping while tabbing #427

Uh oh!

Conversation

hqhhuang commented Sep 6, 2022

Summary

Dependencies

Testing

Checklist

Uh oh!

hqhhuang commented Sep 6, 2022

Uh oh!

dobromir-hristov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marinaaisa Sep 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hqhhuang Sep 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hqhhuang commented Sep 6, 2022

Uh oh!

dobromir-hristov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hqhhuang Sep 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marinaaisa Sep 7, 2022 •

edited

Loading

hqhhuang Sep 6, 2022 •

edited

Loading

hqhhuang Sep 7, 2022 •

edited

Loading