Skip to content

fix(keyboard): Notification drawer and tree view keyboard handling bug fixes #9071

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 4 commits into from
May 10, 2023
Merged

fix(keyboard): Notification drawer and tree view keyboard handling bug fixes #9071

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
cdc6783
fix(keyboard): Notification drawer and tree view keyboard handling bu…
nicolethoen May 5, 2023
6af5af5
fix linting error
nicolethoen May 5, 2023
f8ed821
add v5 prefix to classname in my code
nicolethoen May 5, 2023
931142d
add more v5 prefixes to css classnames
nicolethoen May 5, 2023
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
8 changes: 5 additions & 3 deletions packages/react-core/src/components/NotificationDrawer/NotificationDrawerListItem.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,11 @@ export const NotificationDrawerListItem: React.FunctionComponent<NotificationDra
...props
}: NotificationDrawerListItemProps) => {
const onKeyDown = (event: React.KeyboardEvent) => {
// Accessibility function. Click on the list item when pressing Enter or Space on it.
if (event.key === 'Enter' || event.key === ' ') {
(event.target as HTMLElement).click();
if (!(event.target as HTMLElement).parentElement.classList.contains('pf-v5-c-notification-drawer__list-item-action')) {
// Accessibility function. Click on the list item when pressing Enter or Space on it.
if (event.key === 'Enter' || event.key === ' ') {
(event.target as HTMLElement).click();
}
}
};
return (
Expand Down
2 changes: 1 addition & 1 deletion packages/react-core/src/components/Tabs/examples/Tabs.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,4 +228,4 @@ To add multiple actions to a tab, create a `<TabAction>` component for each acti
The following example passes in both help popover and close actions.

```ts file="./TabsHelpAndClose.tsx" isBeta
```
```
7 changes: 4 additions & 3 deletions packages/react-core/src/components/TreeView/TreeViewRoot.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ export class TreeViewRoot extends React.Component<TreeViewRootProps> {
}
if (this.props.hasSelectableNodes) {
const firstTextButton = this.treeRef.current.getElementsByClassName(
'pf-c-tree-view__node-text'
'pf-v5-c-tree-view__node-text'
)[0] as HTMLElement;
if (firstTextButton) {
firstTextButton.tabIndex = 0;
Expand All @@ -67,7 +67,8 @@ export class TreeViewRoot extends React.Component<TreeViewRootProps> {
}

handleKeys = (event: KeyboardEvent) => {
if (!this.treeRef.current.contains(event.target as HTMLElement)) {
if (!this.treeRef.current.contains(event.target as HTMLElement) ||
!(event.target as HTMLElement).classList.contains('pf-v5-c-tree-view__node')) {
return;
}
const activeElement = document.activeElement;
Expand All @@ -94,7 +95,7 @@ export class TreeViewRoot extends React.Component<TreeViewRootProps> {

if (['ArrowLeft', 'ArrowRight'].includes(key)) {
const isExpandable =
activeElement?.firstElementChild?.firstElementChild?.classList.contains('pf-c-tree-view__node-toggle');
activeElement?.firstElementChild?.firstElementChild?.classList.contains('pf-v5-c-tree-view__node-toggle');
const isExpanded = activeElement?.closest('li')?.classList.contains('pf-m-expanded');
if (key === 'ArrowLeft') {
if (isExpandable && isExpanded) {
Expand Down
29 changes: 17 additions & 12 deletions packages/react-integration/cypress/integration/notificationdrawerbasic.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,26 +44,31 @@ describe('Notification Drawer Basic Demo Test', () => {
});

// Accessibility test
// TODO: Remove skip once issues with new Dropdown keyboard handling are resolved
it.skip('Verify keyboard events happen correctly', () => {
it('Verify keyboard events happen correctly', () => {
// Verify the list header toggle button keyboard interactivity opens/closes dropdown menu
// press Enter on toggle button, check whether the dropdown menu exsit and whether it focuses on the first item
// then press Tab on toggle button, check whether the dropdown menu is closed
cy.get('#toggle-id-0').then((toggleButton: JQuery<HTMLButtonElement>) => {
cy.wrap(toggleButton).trigger('keydown', { key: 'Enter' });
cy.get('#notification-0').find('.pf-v5-c-dropdown__menu.pf-m-align-right').should('exist');
cy.get('#notification-0').find('.pf-v5-c-dropdown__menu-item').first().should('be.focused');
cy.wrap(toggleButton).trigger('keydown', { key: 'Tab' });
cy.get('#notification-0').find('.pf-v5-c-dropdown__menu.pf-m-align-right').should('not.exist');
cy.wrap(toggleButton).type(' ', {waitForAnimations:true});
cy.get('#notification-0')
.find('.pf-v5-c-menu')
.should('exist');
cy.wrap(toggleButton).type('{esc}', {waitForAnimations:true});
cy.get('#notification-0')
.find('.pf-v5-c-menu')
.should('not.exist');
});
// Verify the list item header toggle button keyboard interactivity opens/closes dropdown menu
// the method is the same as above
cy.get('#toggle-id-1').then((toggleButton: JQuery<HTMLButtonElement>) => {
cy.wrap(toggleButton).trigger('keydown', { key: 'Enter' });
cy.get('#notification-1').find('.pf-v5-c-dropdown__menu.pf-m-align-right').should('exist');
cy.get('#notification-1').find('.pf-v5-c-dropdown__menu-item').first().should('be.focused');
cy.wrap(toggleButton).trigger('keydown', { key: 'Tab' });
cy.get('#notification-1').find('.pf-v5-c-dropdown__menu.pf-m-align-right').should('not.exist');
cy.wrap(toggleButton).type(' ', {waitForAnimations:true});
cy.get('#notification-1')
.find('.pf-v5-c-menu')
.should('exist');
cy.wrap(toggleButton).type('{esc}', {waitForAnimations:true});
cy.get('#notification-1')
.find('.pf-v5-c-menu')
.should('not.exist');
});
});

Expand Down
31 changes: 16 additions & 15 deletions packages/react-integration/cypress/integration/notificationdrawergroups.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,17 +52,19 @@ describe('Notification Drawer Groups Demo Test', () => {
});

// Accessibility test
// TODO: Remove skip once issues with new Dropdown keyboard handling are resolved
it.skip('Verify keyboard events happen correctly', () => {
it('Verify keyboard events happen correctly', () => {
// Verify the list header toggle button keyboard interactivity opens/closes dropdown menu
// press Enter on toggle button, check whether the dropdown menu exsit and whether it focuses on the first item
// then press Tab on toggle button, check whether the dropdown menu is closed
cy.get('#toggle-id-0').then((toggleButton: JQuery<HTMLButtonElement>) => {
cy.wrap(toggleButton).trigger('keydown', { key: 'Enter' });
cy.get('#notification-0').find('.pf-v5-c-dropdown__menu.pf-m-align-right').should('exist');
cy.get('#notification-0').find('.pf-v5-c-dropdown__menu-item').first().should('be.focused');
cy.wrap(toggleButton).trigger('keydown', { key: 'Tab' });
cy.get('#notification-0').find('.pf-v5-c-dropdown__menu.pf-m-align-right').should('not.exist');
cy.wrap(toggleButton).type('{enter}', {waitForAnimations:true});
cy.get('#notification-0')
.find('.pf-v5-c-menu')
.should('exist');
cy.wrap(toggleButton).type('{esc}', {waitForAnimations:true});
cy.get('#notification-0')
.find('.pf-v5-c-menu')
.should('not.exist');
});
// Verify the group header keyboard interactivity opens/closes the whole group
// check whether the whole group is expanded, then press Enter on the group header and check whether the whole group is closed
Expand All @@ -71,15 +73,14 @@ describe('Notification Drawer Groups Demo Test', () => {
cy.get('.pf-v5-c-notification-drawer__group').first().should('not.have.class', 'pf-m-expanded');
// Verify the list item header toggle button keyboard interactivity opens/closes dropdown menu
cy.get('#toggle-id-9').then((toggleButton: JQuery<HTMLButtonElement>) => {
cy.wrap(toggleButton).trigger('keydown', { key: 'Enter' });
cy.get('#notification-9').find('.pf-v5-c-dropdown__menu.pf-m-align-right').should('exist');
cy.wrap(toggleButton).type('{enter}', {waitForAnimations:true});
cy.get('#notification-9')
.find('.pf-v5-c-dropdown__menu.pf-m-align-right')
.first()
.find('.pf-v5-c-dropdown__menu-item')
.should('be.focused');
cy.wrap(toggleButton).trigger('keydown', { key: 'Tab' });
cy.get('#notification-9').find('.pf-v5-c-dropdown__menu.pf-m-align-right').should('not.exist');
.find('.pf-v5-c-menu')
.should('exist');
cy.wrap(toggleButton).type('{esc}', {waitForAnimations:true});
cy.get('#notification-9')
.find('.pf-v5-c-menu')
.should('not.exist');
});
});

Expand Down
11 changes: 4 additions & 7 deletions ...n/demo-app-ts/src/components/demos/NotificationDrawerDemo/NotificationDrawerBasicDemo.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,12 +60,11 @@ export class BasicNotificationDrawerDemo extends React.Component<
];
return (
<NotificationDrawer>
<NotificationDrawerHeader count={2} onClose={this.onDrawerClose}>
<NotificationDrawerHeader count={2} id="notification-0" onClose={this.onDrawerClose}>
<Dropdown
onSelect={this.onSelect}
isOpen={isOpen[0]}
onOpenChange={(_isOpen) => this.setState({ isOpen: new Array(6).fill(false) })}
id="notification-0"
popperProps={{ position: 'right' }}
toggle={(toggleRef: React.Ref<MenuToggleElement>) => (
<MenuToggle
Expand All @@ -84,7 +83,7 @@ export class BasicNotificationDrawerDemo extends React.Component<
</NotificationDrawerHeader>
<NotificationDrawerBody>
<NotificationDrawerList>
<NotificationDrawerListItem variant="info">
<NotificationDrawerListItem id="notification-1" variant="info">
<NotificationDrawerListItemHeader
id="info-alert-item"
variant="info"
Expand All @@ -96,14 +95,13 @@ export class BasicNotificationDrawerDemo extends React.Component<
onSelect={this.onSelect}
isOpen={isOpen[1]}
onOpenChange={(_isOpen) => this.setState({ isOpen: new Array(6).fill(false) })}
id="notification-1"
popperProps={{ position: 'right' }}
toggle={(toggleRef: React.Ref<MenuToggleElement>) => (
<MenuToggle
ref={toggleRef}
isExpanded={isOpen[1]}
onClick={() => this.onToggle(1)}
id="toggle-id-0"
id="toggle-id-1"
variant="plain"
>
<EllipsisVIcon aria-hidden="true" />
Expand All @@ -117,7 +115,7 @@ export class BasicNotificationDrawerDemo extends React.Component<
This is an info notification description.
</NotificationDrawerListItemBody>
</NotificationDrawerListItem>
<NotificationDrawerListItem variant="danger">
<NotificationDrawerListItem variant="danger" id="notification-2">
<NotificationDrawerListItemHeader
variant="danger"
title="Unread danger notification title. This is a long title to show how the title will wrap if it is long and wraps to multiple lines."
Expand All @@ -127,7 +125,6 @@ export class BasicNotificationDrawerDemo extends React.Component<
onSelect={this.onSelect}
isOpen={isOpen[2]}
onOpenChange={(_isOpen) => this.setState({ isOpen: new Array(6).fill(false) })}
id="notification-2"
popperProps={{ position: 'right' }}
toggle={(toggleRef: React.Ref<MenuToggleElement>) => (
<MenuToggle
Expand Down
6 changes: 2 additions & 4 deletions .../demo-app-ts/src/components/demos/NotificationDrawerDemo/NotificationDrawerGroupsDemo.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,12 +97,11 @@ export class GroupsNotificationDrawerDemo extends React.Component<
];
return (
<NotificationDrawer>
<NotificationDrawerHeader count={4} onClose={this.onDrawerClose}>
<NotificationDrawerHeader count={4} id="notification-0" onClose={this.onDrawerClose}>
<Dropdown
onSelect={this.onSelect}
isOpen={isOpenMap['toggle-id-0'] || false}
onOpenChange={(_isOpen) => this.setState({ isOpenMap: {} })}
id="notification-0"
popperProps={{ position: 'right' }}
toggle={(toggleRef) => (
<MenuToggle
Expand Down Expand Up @@ -267,7 +266,7 @@ export class GroupsNotificationDrawerDemo extends React.Component<
tooltipPosition="bottom"
>
<NotificationDrawerList isHidden={!secondGroupExpanded}>
<NotificationDrawerListItem variant="info">
<NotificationDrawerListItem variant="info" id="notification-9">
<NotificationDrawerListItemHeader
variant="info"
title="Unread info notification title"
Expand All @@ -277,7 +276,6 @@ export class GroupsNotificationDrawerDemo extends React.Component<
onSelect={this.onSelect}
isOpen={isOpenMap['toggle-id-9'] || false}
onOpenChange={(_isOpen) => this.setState({ isOpenMap: {} })}
id="notification-9"
popperProps={{ position: 'right' }}
toggle={(toggleRef) => (
<MenuToggle
Expand Down