Skip to content
This repository has been archived by the owner on May 24, 2024. It is now read-only.

[terra-folder-tree] Added tabIndex for only last focused item #1967

Merged
merged 6 commits into from
Jan 8, 2024
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
3 changes: 3 additions & 0 deletions packages/terra-folder-tree/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,9 @@

## Unreleased

* Changed
* Only the last focused item has a tab index of 0.

## 1.0.0-alpha.4 - (December 18, 2023)

* Added
Expand Down
16 changes: 11 additions & 5 deletions packages/terra-folder-tree/src/FolderTree.jsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import React, { useRef } from 'react';
import React, { useRef, useEffect } from 'react';
import PropTypes from 'prop-types';
import classNames from 'classnames/bind';
import { injectIntl } from 'react-intl';
Expand All @@ -10,6 +10,7 @@ import Button from 'terra-button';
import Toolbar from 'terra-toolbar';
import { IconCollapseRow, IconExpandRow } from 'terra-icon';

import FolderTreeUtils from './FolderTreeUtils';
import styles from './FolderTree.module.scss';

const cx = classNames.bind(styles);
Expand Down Expand Up @@ -73,8 +74,8 @@ const FolderTree = ({
const currentIndex = Array.from(visibleListItems).indexOf(event.target);
const lastIndex = visibleListItems.length - 1;

const handleHomeKey = () => visibleListItems[0].focus();
const handleEndKey = () => visibleListItems[lastIndex].focus();
const handleHomeKey = () => FolderTreeUtils.handleMoveFocus(visibleListItems[currentIndex], visibleListItems[0]);
const handleEndKey = () => FolderTreeUtils.handleMoveFocus(visibleListItems[currentIndex], visibleListItems[lastIndex]);

switch (event.keyCode) {
case KeyCode.KEY_END:
Expand All @@ -88,13 +89,13 @@ const FolderTree = ({
case KeyCode.KEY_UP: {
event.preventDefault();
const previousIndex = currentIndex - 1;
visibleListItems[previousIndex]?.focus();
FolderTreeUtils.handleMoveFocus(visibleListItems[currentIndex], visibleListItems[previousIndex]);
break;
}
case KeyCode.KEY_DOWN: {
event.preventDefault();
const nextIndex = currentIndex + 1;
visibleListItems[nextIndex]?.focus();
FolderTreeUtils.handleMoveFocus(visibleListItems[currentIndex], visibleListItems[nextIndex]);
break;
}
case KeyCode.KEY_LEFT: {
Expand Down Expand Up @@ -122,6 +123,11 @@ const FolderTree = ({
}
};

useEffect(() => {
const listItems = listNode.current.querySelectorAll('[data-item-show-focus=true]');
listItems[0].tabIndex = 0;
}, []);

return (
<div className="folder-tree-container">
<ActionHeader
Expand Down
14 changes: 14 additions & 0 deletions packages/terra-folder-tree/src/FolderTreeUtils.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
/* eslint-disable no-param-reassign */
const handleMoveFocus = (elementA, elementB) => {
sycombs marked this conversation as resolved.
Show resolved Hide resolved
if (!elementA || !elementB) { return; }
elementA.tabIndex = -1;

elementB.tabIndex = 0;
elementB.focus();
};

const FolderTreeUtils = {
handleMoveFocus,
};

export default FolderTreeUtils;
13 changes: 8 additions & 5 deletions packages/terra-folder-tree/src/subcomponents/FolderTreeItem.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import Spacer from 'terra-spacer';
import Arrange from 'terra-arrange';
import { IconFolder, IconCaretRight, IconCaretDown } from 'terra-icon';
import ThemeContext from 'terra-theme-context';

import FolderTreeUtils from '../FolderTreeUtils';
import styles from './FolderTreeItem.module.scss';

const cx = classNames.bind(styles);
Expand Down Expand Up @@ -51,7 +51,10 @@ const propTypes = {
* @private
* Ref to the parent folder of the current item.
*/
parentRef: PropTypes.number,
parentRef: PropTypes.oneOfType([
PropTypes.func,
PropTypes.shape({ current: PropTypes.instanceOf(Element) }),
]),
/**
* @private
* intl object programmatically imported through injectIntl from react-intl.
Expand Down Expand Up @@ -137,7 +140,7 @@ const FolderTreeItem = ({
if (isFolder && isExpanded) {
handleToggle(event);
} else {
parentRef?.current.focus();
FolderTreeUtils.handleMoveFocus(itemNode.current, parentRef?.current);
}

break;
Expand All @@ -150,7 +153,7 @@ const FolderTreeItem = ({
handleToggle(event);
} else if (isFolder) {
const firstFolderChild = subFolderNode.current.querySelector('[data-item-show-focus=true]');
firstFolderChild?.focus();
FolderTreeUtils.handleMoveFocus(itemNode.current, firstFolderChild);
}

break;
Expand All @@ -170,7 +173,7 @@ const FolderTreeItem = ({
onClick={isFolder ? handleToggle : onClick}
onKeyDown={handleKeyDown}
data-item-show-focus
tabIndex={0}
tabIndex={-1}
ref={itemNode}
>
<input
Expand Down
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
17 changes: 17 additions & 0 deletions packages/terra-folder-tree/tests/wdio/folder-tree-spec.js
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks good, +1!

Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,23 @@ Terra.describeViewports('FolderTree', ['medium'], () => {
Terra.validates.screenshot('last item focused keyboard', { selector: '#expand-collapse-folder-tree' });
});

it('remembers the last focused item when tabbing in and out', () => {
browser.url('/raw/tests/cerner-terra-framework-docs/folder-tree/expand-collapse-folder-tree');

$('aria/Expand All').click();
browser.keys('Tab');
browser.keys('Tab');
browser.keys('ArrowDown');
browser.keys('ArrowDown');
Terra.validates.screenshot('level two folder focused keyboard', { selector: '#expand-collapse-folder-tree' });

browser.keys('Tab');
Terra.validates.screenshot('tabbed out of focus', { selector: '#root' });

browser.keys(['Shift', 'Tab']);
Terra.validates.screenshot('level two folder focused keyboard', { selector: '#expand-collapse-folder-tree' });
Copy link
Contributor

Choose a reason for hiding this comment

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

I like that we're avoiding adding extra screenshots here, just wondering if there are any potential issues with validating the same screenshot twice in this test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've done this in the past before, and both screenshots should result in the same identical image, so I believe it should be alright

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a need to validate teh same screenshot twice? If we're only checking for focus, we can use the isFocused() validator since that's more efficient than doing a screenshot comparison.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh that's right! I keep forgetting we have that, I'll update it to use that

});

it('wraps items with long labels', () => {
browser.url('/raw/tests/cerner-terra-framework-docs/folder-tree/wrapped-label-folder-tree');

Expand Down
Loading