From cdc67833b5d225fa06693f7bca4988a163574eb6 Mon Sep 17 00:00:00 2001 From: nicolethoen Date: Fri, 5 May 2023 09:52:23 -0400 Subject: [PATCH 1/4] fix(keyboard): Notification drawer and tree view keyboard handling bug fix --- .../NotificationDrawerListItem.tsx | 9 ++++-- .../src/components/Tabs/examples/Tabs.md | 2 +- .../src/components/TreeView/TreeViewRoot.tsx | 3 +- .../notificationdrawerbasic.spec.ts | 29 ++++++++++------- .../notificationdrawergroups.spec.ts | 31 ++++++++++--------- .../NotificationDrawerBasicDemo.tsx | 11 +++---- .../NotificationDrawerGroupsDemo.tsx | 6 ++-- 7 files changed, 48 insertions(+), 43 deletions(-) diff --git a/packages/react-core/src/components/NotificationDrawer/NotificationDrawerListItem.tsx b/packages/react-core/src/components/NotificationDrawer/NotificationDrawerListItem.tsx index 5972a7b6b42..8ba793ea7cb 100644 --- a/packages/react-core/src/components/NotificationDrawer/NotificationDrawerListItem.tsx +++ b/packages/react-core/src/components/NotificationDrawer/NotificationDrawerListItem.tsx @@ -31,9 +31,12 @@ export const NotificationDrawerListItem: React.FunctionComponent { 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(); + console.log(event.target); + if (!(event.target as HTMLElement).parentElement.classList.contains('pf-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 ( diff --git a/packages/react-core/src/components/Tabs/examples/Tabs.md b/packages/react-core/src/components/Tabs/examples/Tabs.md index 69f88826111..8bf499ec12e 100644 --- a/packages/react-core/src/components/Tabs/examples/Tabs.md +++ b/packages/react-core/src/components/Tabs/examples/Tabs.md @@ -228,4 +228,4 @@ To add multiple actions to a tab, create a `` component for each acti The following example passes in both help popover and close actions. ```ts file="./TabsHelpAndClose.tsx" isBeta -``` \ No newline at end of file +``` diff --git a/packages/react-core/src/components/TreeView/TreeViewRoot.tsx b/packages/react-core/src/components/TreeView/TreeViewRoot.tsx index 055e629c4ec..d0a4415fdbe 100644 --- a/packages/react-core/src/components/TreeView/TreeViewRoot.tsx +++ b/packages/react-core/src/components/TreeView/TreeViewRoot.tsx @@ -67,7 +67,8 @@ export class TreeViewRoot extends React.Component { } 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-c-tree-view__node')) { return; } const activeElement = document.activeElement; diff --git a/packages/react-integration/cypress/integration/notificationdrawerbasic.spec.ts b/packages/react-integration/cypress/integration/notificationdrawerbasic.spec.ts index fe38de960ae..27e344c634d 100644 --- a/packages/react-integration/cypress/integration/notificationdrawerbasic.spec.ts +++ b/packages/react-integration/cypress/integration/notificationdrawerbasic.spec.ts @@ -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) => { - 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) => { - 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'); }); }); diff --git a/packages/react-integration/cypress/integration/notificationdrawergroups.spec.ts b/packages/react-integration/cypress/integration/notificationdrawergroups.spec.ts index bbf2d9002dd..b411537e5da 100644 --- a/packages/react-integration/cypress/integration/notificationdrawergroups.spec.ts +++ b/packages/react-integration/cypress/integration/notificationdrawergroups.spec.ts @@ -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) => { - 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 @@ -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) => { - 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'); }); }); diff --git a/packages/react-integration/demo-app-ts/src/components/demos/NotificationDrawerDemo/NotificationDrawerBasicDemo.tsx b/packages/react-integration/demo-app-ts/src/components/demos/NotificationDrawerDemo/NotificationDrawerBasicDemo.tsx index 44fc3b01653..223749676a0 100644 --- a/packages/react-integration/demo-app-ts/src/components/demos/NotificationDrawerDemo/NotificationDrawerBasicDemo.tsx +++ b/packages/react-integration/demo-app-ts/src/components/demos/NotificationDrawerDemo/NotificationDrawerBasicDemo.tsx @@ -60,12 +60,11 @@ export class BasicNotificationDrawerDemo extends React.Component< ]; return ( - + this.setState({ isOpen: new Array(6).fill(false) })} - id="notification-0" popperProps={{ position: 'right' }} toggle={(toggleRef: React.Ref) => ( - + this.setState({ isOpen: new Array(6).fill(false) })} - id="notification-1" popperProps={{ position: 'right' }} toggle={(toggleRef: React.Ref) => ( this.onToggle(1)} - id="toggle-id-0" + id="toggle-id-1" variant="plain" > - + this.setState({ isOpen: new Array(6).fill(false) })} - id="notification-2" popperProps={{ position: 'right' }} toggle={(toggleRef: React.Ref) => ( - + this.setState({ isOpenMap: {} })} - id="notification-0" popperProps={{ position: 'right' }} toggle={(toggleRef) => ( - + this.setState({ isOpenMap: {} })} - id="notification-9" popperProps={{ position: 'right' }} toggle={(toggleRef) => ( Date: Fri, 5 May 2023 10:04:07 -0400 Subject: [PATCH 2/4] fix linting error --- .../components/NotificationDrawer/NotificationDrawerListItem.tsx | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/react-core/src/components/NotificationDrawer/NotificationDrawerListItem.tsx b/packages/react-core/src/components/NotificationDrawer/NotificationDrawerListItem.tsx index 8ba793ea7cb..7fa32c7e552 100644 --- a/packages/react-core/src/components/NotificationDrawer/NotificationDrawerListItem.tsx +++ b/packages/react-core/src/components/NotificationDrawer/NotificationDrawerListItem.tsx @@ -31,7 +31,6 @@ export const NotificationDrawerListItem: React.FunctionComponent { const onKeyDown = (event: React.KeyboardEvent) => { - console.log(event.target); if (!(event.target as HTMLElement).parentElement.classList.contains('pf-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 === ' ') { From f8ed82173f9dc1b1c7cf3ef6ffdaaf1c7b7aa18e Mon Sep 17 00:00:00 2001 From: nicolethoen Date: Fri, 5 May 2023 16:35:28 -0400 Subject: [PATCH 3/4] add v5 prefix to classname in my code --- .../NotificationDrawer/NotificationDrawerListItem.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/react-core/src/components/NotificationDrawer/NotificationDrawerListItem.tsx b/packages/react-core/src/components/NotificationDrawer/NotificationDrawerListItem.tsx index 7fa32c7e552..29968e72493 100644 --- a/packages/react-core/src/components/NotificationDrawer/NotificationDrawerListItem.tsx +++ b/packages/react-core/src/components/NotificationDrawer/NotificationDrawerListItem.tsx @@ -31,7 +31,7 @@ export const NotificationDrawerListItem: React.FunctionComponent { const onKeyDown = (event: React.KeyboardEvent) => { - if (!(event.target as HTMLElement).parentElement.classList.contains('pf-c-notification-drawer__list-item-action')) { + 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(); From 931142d32caaa47c21394aa5559418cb69c0b7be Mon Sep 17 00:00:00 2001 From: nicolethoen Date: Fri, 5 May 2023 16:36:37 -0400 Subject: [PATCH 4/4] add more v5 prefixes to css classnames --- .../react-core/src/components/TreeView/TreeViewRoot.tsx | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/react-core/src/components/TreeView/TreeViewRoot.tsx b/packages/react-core/src/components/TreeView/TreeViewRoot.tsx index d0a4415fdbe..1e387654636 100644 --- a/packages/react-core/src/components/TreeView/TreeViewRoot.tsx +++ b/packages/react-core/src/components/TreeView/TreeViewRoot.tsx @@ -46,7 +46,7 @@ export class TreeViewRoot extends React.Component { } 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; @@ -68,7 +68,7 @@ export class TreeViewRoot extends React.Component { handleKeys = (event: KeyboardEvent) => { if (!this.treeRef.current.contains(event.target as HTMLElement) || - !(event.target as HTMLElement).classList.contains('pf-c-tree-view__node')) { + !(event.target as HTMLElement).classList.contains('pf-v5-c-tree-view__node')) { return; } const activeElement = document.activeElement; @@ -95,7 +95,7 @@ export class TreeViewRoot extends React.Component { 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) {