[terra-slide-panel] Disclosing node capturing method update #2095
Conversation
adoroshk
changed the title
There was a problem hiding this comment.
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.
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.
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.
| * @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.
| this.disclosingNode.setAttribute('aria-expanded', 'false'); | ||
| 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.
| const roleIsCompartible = (node) => { | ||
| 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.
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
| 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
| 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.
|
@mjpalazzo, it looks like we need your help.
|
|
This PR changes slide panel disclosing node capturing method that allows to use slide panel without mainContent prop.
Summary
In order to return the focus to the disclosing node upon closing the Slide panel, the disclosing node needs to be captured upon panel opening. The existing implementation requires a mainContent prop to be passed, which would allow the Slide panel to create a main content node with onClick and onKeyUp event listeners being in charge of capturing disclosing node. With that approach in cases when mainContent prop is omitted, the disclosing node is not captured upon panel opening and there is no node saved to return focus upon panel closing.
What was changed:
Testing
This change was tested using:
Reviews
In addition to engineering reviews, this PR needs:
Additional Details
This PR resolves:
UXPLATFORM-10319