[terra-form-select] Fixed focus issue in MultiSelect #4089
Conversation
|
+1 |
| event.stopPropagation(); | ||
| onDeselect(value); | ||
| const previousLi = tagRef.current.previousElementSibling; | ||
| const selectInput = tagRef.current.closest('ul').parentElement.parentElement.children[1].children[0]; |
There was a problem hiding this comment.
This way of accessing input control might result in unexpected failure when dom changes are made to component. I would suggest you to create a callback ref for input to set focus.
There was a problem hiding this comment.
I have now used inputRef from a callback to set focus on the input element.
| const previousLi = tagRef.current.previousElementSibling; | ||
| const selectInput = tagRef.current.closest('ul').parentElement.parentElement.children[1].children[0]; | ||
| if (previousLi) { | ||
| const deselectElement = previousLi.children[1]; |
There was a problem hiding this comment.
why is the index being hard-coded to 1 here ?
There was a problem hiding this comment.
I have removed the hard coded index. focused input element using inputRef.
| @@ -2376,6 +2376,17 @@ Terra.describeViewports('Select', ['tiny'], () => { | |||
| it('should display selected option', () => { | |||
| Terra.validates.element('tag controlled selected option'); | |||
| }); | |||
|
|
|||
| it('should focus on pressing tab key', () => { | |||
There was a problem hiding this comment.
Does this test validates disabled tag navigations as well?
There was a problem hiding this comment.
@supreethmr I have added tests to validate disabled tag navigations
| @@ -65,7 +65,7 @@ Terra.describeViewports('Alert', ['tiny', 'large'], () => { | |||
| it('alert content is focused when rendered with an action element', () => { | |||
| browser.url('/raw/tests/cerner-terra-core-docs/alert/custom-prop-alert'); | |||
|
|
|||
| browser.keys(['Tab', 'Tab', 'Tab', 'Tab', 'Enter']); | |||
| browser.keys(['Tab', 'Tab', 'Tab', 'Tab', 'Tab', 'Tab', 'Enter']); | |||
There was a problem hiding this comment.
why are the additional tab key is required to bring focus to alert content..?
There was a problem hiding this comment.
@supreethmr Earlier pressing tab key was not focusing inside the input field, that is the deselect option. Now since there are two options in the list, to get out side of the input box we need to press tab two more times
@SwethaM03 , Screen Reader issue is now fixed with this PR |
|
There is a Axe Violation on disabled multi-select I think this will be same for other variants of select also. Hence it's resulting in below axe voilation. It would be better to remove the aria-expanded attribute for the disabled frame since it has no role. OR we can add role back but it looks like `role='application' is not consistent with screen readers for disabled elements.
|
@supreethmr Since this is not related to this PR, logged a new JIRA here. |
|
+1, both keyboard and screen reader focus is managed as expected. And the focus is also managed when options are deselected. |
Co-authored-by: Supreeth <[email protected]>
Co-authored-by: Supreeth <[email protected]>
Co-authored-by: Supreeth <[email protected]>
sugan2416
force-pushed
the
UXPLATFORM-10191_multiselect
branch
from
f53740a to
a551809
Compare
sugan2416
added
⭐ Accessibility Reviewed
Summary
What was changed:
aria-labelattribute inTagcomponentroleattribute logic depending on theisInputFocusedprop.inputRefas callback inMultiSelectcomponent and passed down toTagcomponent.disabledandisInputFocusedprops inTagcomponentMultiSelectcomponentWhy it was changed:
Keyboard TAB focus also not moving to the added list of options. The User is not able to access the previous selected options that appears above the multi-select field.
Testing
This change was tested using:
Reviews
In addition to engineering reviews, this PR needs:
Additional Details
This PR resolves:
UXPLATFORM-10191
Thank you for contributing to Terra.
@cerner/terra