-
Notifications
You must be signed in to change notification settings - Fork 166
[terra-search-field] Remove redundant search group #4019
Conversation
@KV106606Viswanath please add the before and after evidence. |
@@ -268,7 +268,6 @@ class SearchField extends React.Component { | |||
); | |||
|
|||
const inputAriaLabelText = inputAttributes && Object.prototype.hasOwnProperty.call(inputAttributes, 'aria-label') ? inputAttributes['aria-label'] : intl.formatMessage({ id: 'Terra.searchField.search' }); | |||
const groupNameValue = groupName === 'Search' ? intl.formatMessage({ id: 'Terra.searchField.search' }) : groupName; |
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.
This line allows users to set custom aria-label via groupName prop. Removing this breaks that functionality. Pls test this example on SR https://engineering.cerner.com/terra-ui/components/cerner-terra-core-docs/search-field/examples/search-field-with-no-label. Before and after your change. It should announce the custom aria label
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.
Passing "groupName" prop to input field that should announce the custom visual label.
<div className={cx('input-group')}> | ||
<input | ||
{...additionalInputAttributes} | ||
className={inputClass} | ||
type="search" |
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.
I feel even this should not be removed . See Here https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/search. It should announce like this. Changing default aria label (groupName default value) should be the way to go.
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.
Added.
@@ -104,7 +104,7 @@ const propTypes = { | |||
const defaultProps = { | |||
defaultValue: undefined, | |||
disableAutoSearch: false, | |||
groupName: 'Search', | |||
groupName: 'Search a clinic', |
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.
Did someone from UX ask to change this value to search a clinic
?
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.
Yes.
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.
I think we need to discuss with UX regarding this default value and add translations also for whatever text is finalised.
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.
Reverted default groupName to "Search".
@@ -21,7 +21,7 @@ Terra.describeViewports('Search Field', ['medium'], () => { | |||
}); | |||
|
|||
it('should display Search Field with scrolled text', () => { | |||
expect($('input[type="search"]').isFocused()).toBeTruthy(); | |||
expect($('input').isFocused()).toBeTruthy(); |
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.
Since type=search
has been added back these wdio spec changes can be reverted
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.
Added.
@@ -2,6 +2,9 @@ | |||
|
|||
## Unreleased | |||
|
|||
* Fixed | |||
* Removed unnecessary aria labels and roles to fix redundant announcement of serach group issue. |
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.
* Removed unnecessary aria labels and roles to fix redundant announcement of serach group issue. | |
* Removed unnecessary aria labels and roles to fix redundant announcement of search group issue. |
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.
Updated.
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.
Please add back type=search for all the tests. You just reverted for one of them
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.
Updated.
Can you update the tests also with a visible label? |
Updated. |
<div role="search" className={cx('search-role-container')} aria-label={intl.formatMessage({ id: 'Terra.searchField.search' })}> | ||
<div role="group" aria-label={groupNameValue} {...customProps} className={searchFieldClassNames}> | ||
<div className={cx('search-role-container')}> | ||
<div role="group" aria-label={intl.formatMessage({ id: 'Terra.searchField.search' })} {...customProps} className={searchFieldClassNames}> |
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.
what will be the screen reader response when groupName is Search
. ( since this div and input will have same value for aria-label attribute will it result in redundant response
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.
Yes, it's causing redundant response of "Search".
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.
then we should remove the default value of search
from input aria-label to avoid the redundant response
@KV106606Viswanath |
No, we don't have any separate jira for this, we already have internationalised string for "Search" as "Terra.searchField.search": "Search". |
+1, as recommended a contextual and practical label is provided to the Search Edit field and the Search field along with the button is grouped. |
Can update the prop description of groupName to indicate default value |
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.
Jest snapshots are not updated for recent code changes, needs update !!
Summary
When the search field input gets focussed, screen reader reads the unnecessary search group such as "Search, Search region, Search group, Search edit field". With this fix redundant announcement of search group has been removed.
What was changed:
Redundant announcement of search group has been removed.
Why it was changed:
Testing
This change was tested using:
Reviews
In addition to engineering reviews, this PR needs:
Additional Details
This PR resolves:
UXPLATFORM-10147
Thank you for contributing to Terra.
@cerner/terra