This repository has been archived by the owner on May 24, 2024. It is now read-only.
[terra-slide-panel] Disclosing node capturing method update #2095
There are no files selected for viewing
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.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,102 @@ | ||
| import React, { useState } from 'react'; | ||
| import SlidePanel, { SlidePanelPositions } from 'terra-slide-panel'; | ||
| import classNames from 'classnames/bind'; | ||
| import styles from './SlidePanelNoMainContent.test.module.scss'; | ||
|
|
||
| const cx = classNames.bind(styles); | ||
|
|
||
| const mainContentForSlidePanel = (togglePanelHandler) => ( | ||
| <div> | ||
| <header className={cx('header-content')}> | ||
| <h3>Main Content</h3> | ||
| <button id="mainToggleBtn" type="button" onClick={togglePanelHandler} className={cx('custom-button')}>Toggle Panel 1</button> | ||
| {/* eslint-disable-next-line jsx-a11y/no-interactive-element-to-noninteractive-role */} | ||
| <button id="mainToggleBtnWithWrongRole" type="button" role="presentation" onClick={togglePanelHandler} className={cx('extension-button')}>Toggle Panel 2</button> | ||
| { /* eslint-disable-next-line jsx-a11y/click-events-have-key-events, jsx-a11y/no-noninteractive-element-interactions */} | ||
| <p id="mainToggleParagraph" onClick={togglePanelHandler} className={cx('extension-button')}>Toggle Panel 3</p> | ||
| </header> | ||
| <div className={cx('content-wrapper')}> | ||
| <p> | ||
| This is the main content area of the slide panel. | ||
| The overall height of the SlidePanel is determined by | ||
| the intrinsic height of the content in this container. | ||
| </p> | ||
| <p> | ||
| {'Focus is moved to the toggle button in the panel container when the panel is opened via the componentDidUpdate lifecycle hook in '} | ||
| <a href="https://github.com/cerner/terra-framework/blob/main/packages/terra-slide-panel/src/terra-dev-site/doc/example/DefaultSlidePanel.jsx">the example code</a> | ||
| . | ||
| </p> | ||
| <ul> | ||
| <li>Item 1</li> | ||
| <li>Item 2</li> | ||
| <li>Item 3</li> | ||
| <li>Item 4</li> | ||
| <li>Item 5</li> | ||
| <li>Item 6</li> | ||
| <li>Item 7</li> | ||
| <li>Item 8</li> | ||
| </ul> | ||
| </div> | ||
| </div> | ||
| ); | ||
|
|
||
| const panelContentForSlidePanel = (togglePanelHandler) => ( | ||
| <div> | ||
| <header className={cx('header-content')}> | ||
| <h3 className={cx('heading')}>Panel Content</h3> | ||
| <button id="panelToggleBtn" type="button" onClick={togglePanelHandler} className={cx('extension-button')}>Toggle Panel</button> | ||
| </header> | ||
| <div className={cx('content-wrapper')}> | ||
| <p>This is the panel content area of the slide panel.</p> | ||
| <p> | ||
| {'Focus is moved to the toggle button in the main container when the panel is closed via the componentDidUpdate lifecycle hook in '} | ||
| <a href="https://github.com/cerner/terra-framework/blob/main/packages/terra-slide-panel/src/terra-dev-site/doc/example/DefaultSlidePanel.jsx">the example code</a> | ||
| . | ||
| </p> | ||
| <ul> | ||
| <li>Item 1</li> | ||
| <li>Item 2</li> | ||
| <li>Item 3</li> | ||
| <li>Item 4</li> | ||
| <li>Item 5</li> | ||
| <li>Item 6</li> | ||
| <li>Item 7</li> | ||
| <li>Item 8</li> | ||
| <li>Item 9</li> | ||
| <li>Item 10</li> | ||
| <li>Item 11</li> | ||
| <li>Item 12</li> | ||
| <li>Item 13</li> | ||
| <li>Item 14</li> | ||
| <li>Item 15</li> | ||
| </ul> | ||
| </div> | ||
| </div> | ||
| ); | ||
|
|
||
| const SlidePanelNoMainContent = () => { | ||
| const [isOpen, setIsOpen] = useState(false); | ||
|
|
||
| const handlePanelToggle = () => { | ||
| setIsOpen(!isOpen); | ||
| }; | ||
|
|
||
| return ( | ||
| <div className={cx('example-container')}> | ||
| {mainContentForSlidePanel(handlePanelToggle)} | ||
| <div className={isOpen ? cx('panel-container') : null}> | ||
| <SlidePanel | ||
| id="test-slide" | ||
| panelContent={panelContentForSlidePanel(handlePanelToggle)} | ||
| panelAriaLabel="Panel content area" | ||
| panelSize="small" | ||
| panelBehavior="overlay" | ||
| panelPosition={SlidePanelPositions.END} | ||
| isOpen={isOpen} | ||
| /> | ||
| </div> | ||
| </div> | ||
| ); | ||
| }; | ||
|
|
||
| export default SlidePanelNoMainContent; |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,41 @@ | ||
| :local { | ||
| .example-container { | ||
| position: relative; | ||
| } | ||
|
|
||
| .panel-container > div:first-child { | ||
| position: absolute; | ||
| top: 0; | ||
| bottom: 0; | ||
| right: 0; | ||
| left: 0; | ||
| } | ||
|
|
||
| .header-content { | ||
| background-color: #d3d3d3; | ||
|
|
||
| p { | ||
| width: auto; | ||
| display: inline-block; | ||
| margin: 0 !important; | ||
| } | ||
| } | ||
|
|
||
| .header-content > :not(:last-child){ | ||
| margin-right: 5px; | ||
| } | ||
|
|
||
| .header-content h3 { | ||
| display: inline-block; | ||
| margin: 0; | ||
| padding: 5px; | ||
| } | ||
|
|
||
| .content-wrapper { | ||
| margin: 5px; | ||
| } | ||
|
|
||
| .custom-button { | ||
| display: inline-block; | ||
| } | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -85,33 +85,65 @@ const defaultProps = { | |
| panelSize: 'small', | ||
| }; | ||
|
|
||
| /** | ||
| * This method ensures that aria-expanded attribute is allowed with the node role to avoid accessibility violations. | ||
| * More info on associated roles can be found here: https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Attributes/aria-expanded#associated_roles | ||
| * @param {HTMLElement} node - a node to check | ||
| * @returns true if node's role is compatible with aria-expanded attribute | ||
| */ | ||
| const roleIsCompartible = (node) => { | ||
|
There was a problem hiding this comment. What does compartible mean? There was a problem hiding this comment. It checks if node's role is one compatible (sorry for typo, will fix if we won't remove it at the end) with aria-expanded attribute. Here is the documentation for roles associated with aria-expanded attribute. There was a problem hiding this comment. Oh, it is just a typo :). I still don't think we need a method like this though. |
||
| const role = node.getAttribute('role'); | ||
| return ( | ||
| role === 'application' | ||
| || role === 'button' | ||
| || role === 'checkbox' | ||
| || role === 'combobox' | ||
| || role === 'gridcell' | ||
| || role === 'link' | ||
| || role === 'listbox' | ||
| || role === 'menuitem' | ||
| || role === 'row' | ||
| || role === 'rowheader' | ||
| || role === 'tab' | ||
| || role === 'treeitem' | ||
| || role === null | ||
| || role === undefined | ||
| ); | ||
|
There was a problem hiding this comment. This looks like a lot of conditions that will always be sequentially checked. Can we optimize it using a Set and the Set.has method? There was a problem hiding this comment. And I'm confused why are we testing for all the possible values if we don't care that role is null/undefined? There was a problem hiding this comment. @sdadn aria-expanded attribute should be used only with specific roles, otherwise it causes accessibility violation. This attribute was not introduced in this change, but it was causing accessibility violations with specific roles only, so I added a check. I'll double check with @mjpalazzo what would be the best way to address that |
||
| }; | ||
|
There was a problem hiding this comment. We should not need a method like this. It makes me feel like we might be doing something incorrectly. There was a problem hiding this comment. Removed, the role in question changed in the example |
||
|
|
||
| class SlidePanel extends React.Component { | ||
| constructor(props) { | ||
| super(props); | ||
| this.setPanelNode = this.setPanelNode.bind(this); | ||
| this.mainNode = React.createRef(); | ||
| this.setDisclosingNode = this.setDisclosingNode.bind(this); | ||
| this.mainAriaDescribedByID = `detail-panel-warning-${uuidv4()}`; | ||
| } | ||
|
|
||
| componentDidUpdate(prevProps) { | ||
| if (this.props.isOpen && this.props.isOpen !== prevProps.isOpen) { | ||
| // Save the disclosing node for returning focus when panel is closed | ||
| this.setDisclosingNode(document.activeElement); | ||
| this.panelNode.focus(); | ||
| return; | ||
| } | ||
|
|
||
| if (!this.props.isOpen && this.props.isOpen !== prevProps.isOpen) { | ||
| if (this.disclosingNode?.focus) { | ||
| // Return focus to the disclosing element | ||
| if (this.disclosingNode.getAttribute('aria-expanded')) { | ||
| // Set aria-expanded attribute to false for nodes that had that attribute only | ||
| this.disclosingNode.setAttribute('aria-expanded', 'false'); | ||
|
There was a problem hiding this comment. I am not clear on why we need to set the aria-expanded attribute on the disclosing node. That element is not being expanded. There was a problem hiding this comment. aria-expanded attribute was not added in this change, it was added before. I'll check with @mjpalazzo if it can be removed. |
||
| } | ||
| this.disclosingNode.focus(); | ||
| return; | ||
| } | ||
| // The disclosing element doesn't exist or isn't focusable, return focus to the main div or body | ||
| if (this.mainNode?.current) { | ||
| this.mainNode.current.focus(); | ||
| } else { | ||
| document.body.focus(); | ||
| } | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -122,13 +154,12 @@ class SlidePanel extends React.Component { | |
| this.panelNode = node; | ||
| } | ||
|
|
||
| setDisclosingNode(node) { | ||
| if (node) { | ||
| if (roleIsCompartible(node)) { | ||
| // Set aria-expanded attribute to true for nodes with associated roles or no role only | ||
| node.setAttribute('aria-expanded', 'true'); | ||
| } | ||
|
There was a problem hiding this comment. This node is not being expanded. I don't think we should be adding this role. There was a problem hiding this comment. aria-expanded attribute wasn't added by me, it was there before the change, I just added a role check, as it can be added only with specific roles, otherwise it causes accessibility violation. I'll double check with @mjpalazzo if aria-expanded attribute needs to be removed. |
||
| this.disclosingNode = node; | ||
| } | ||
| } | ||
|
|
@@ -189,8 +220,6 @@ class SlidePanel extends React.Component { | |
| aria-hidden={isOpen && isFullscreen ? 'true' : 'false'} | ||
| ref={this.mainNode} | ||
| role="main" | ||
| > | ||
| <VisuallyHiddenText | ||
| tabIndex="-1" | ||
|
|
@@ -204,11 +233,11 @@ class SlidePanel extends React.Component { | |
| const content = (panelPosition === SlidePanelPositions.START) ? ( | ||
| <React.Fragment> | ||
| {panelDiv} | ||
| {mainContent && mainDiv} | ||
| </React.Fragment> | ||
| ) : ( | ||
| <React.Fragment> | ||
| {mainContent && mainDiv} | ||
| {panelDiv} | ||
| </React.Fragment> | ||
| ); | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this right? This seems like it would result in an accessibility violation because there's not enough contrast.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like the highlighted style was updated? May need UX to confirm if this is the correct styling.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WDIO screenshots for terra-agregator test example which uses slide panel were updated, as with new functionality added, the disclosing node in terra-agregator received the focus. However, the focus style itself hasn't changed, it just wasn't receiving focus before as Slide Panel had no means to return the focus to that node. The affected example is a test example for some other functionality, it is not presented on the doc page and is not relevant to anything important. I don't think we should fix it or log a bug.