Skip to content
This repository has been archived by the owner on May 24, 2024. It is now read-only.

[Terra-SearchSelect]SR doesn't announce the placeholder when the input field gains focus. #3984

Merged
merged 22 commits into from
Dec 5, 2023

Conversation

AH106586Harika
Copy link
Contributor

@AH106586Harika AH106586Harika commented Nov 21, 2023

Summary

Fixed- SR doesn't announce the placeholder when the input field gains focus.

What was changed:
Added a condition to announce input field placeholder upon focus, resolving screen reader issues where placeholder announcement were missing.

Why it was changed:
The placeholder isn't announced across all browsers.

Testing

Post fix:

Screen.Recording.2023-11-27.at.10.55.36.AM.mov

This change was tested using:

  • WDIO
  • Jest
  • Visual testing (please attach a screenshot or recording)
  • Other (please describe below)
  • No tests are needed

Reviews

In addition to engineering reviews, this PR needs:

  • UX review
  • Accessibility review
  • Functional review

Additional Details

This PR resolves:

UXPLATFORM-9836

Thank you for contributing to Terra.
@cerner/terra

@AH106586Harika AH106586Harika requested a review from a team as a code owner November 21, 2023 23:19

const defaultAriaLabel = intl.formatMessage({ id: 'Terra.form.select.ariaLabel' });
const dimmed = intl.formatMessage({ id: 'Terra.form.select.dimmed' });
const isChrome = navigator.userAgent.indexOf('Chrome') !== -1;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why are we making chrome specific fix. I can see place holders are not being read on Safari browser as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was using Safari 16.5 version hence not able to repro the issue, in latest 17.1 version the issue is reproducible, I have ensured uniform functionality across all browsers now.

@@ -8,7 +8,7 @@ const cx = classNames.bind(styles);
const SearchSelectExample = () => (
<>
<label htmlFor="color-field-1"><strong>Colors</strong></label>
<SearchSelect placeholder="Select a color" className={cx('form-select')} inputId="color-field-1">
<SearchSelect placeholder="Select a color" ariaLabel="Colors" className={cx('form-select')} inputId="color-field-1">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this code updates Colors in two places, one at input control as aria-label prop and another at added for label values Since input has got role as combobox I think aria-label should be sufficient for all both VO and JAWS .
I think we can remove the label span and keep the prop aria-label on form-search-select.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed as we are passing aria-label.

@@ -2,6 +2,9 @@

## Unreleased

* Added
* Added Chrome-specific condition to announce the placeholder.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is it still chrome specific condition

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Comment on lines 523 to 525
return ariaLabel === undefined
? `${defaultAriaLabel} ${placeholder} ${dimmed}`
: `${ariaLabel} ${placeholder} ${dimmed}`;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return ariaLabel === undefined
? `${defaultAriaLabel} ${placeholder} ${dimmed}`
: `${ariaLabel} ${placeholder} ${dimmed}`;
return ariaLabel === undefined
? `${defaultAriaLabel}, ${placeholder} ${dimmed}`
: `${ariaLabel}, ${placeholder} ${dimmed}`;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Comment on lines 527 to 529
return ariaLabel === undefined
? `${defaultAriaLabel} ${placeholder}`
: `${ariaLabel} ${placeholder}`;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return ariaLabel === undefined
? `${defaultAriaLabel} ${placeholder}`
: `${ariaLabel} ${placeholder}`;
return ariaLabel === undefined
? `${defaultAriaLabel}, ${placeholder}`
: `${ariaLabel}, ${placeholder}`;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

aria-disabled="false"
aria-expanded="false"
aria-invalid="false"
aria-label="Terra.form.select.ariaLabel"
aria-label="Terra.form.select.ariaLabel undefined"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

undefined should not be part of Aria-label

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Modified.

@AH106586Harika AH106586Harika changed the title [Terra-SearchSelect]Chrome doesn't announce the placeholder when the input field gains focus. [Terra-SearchSelect]SR doesn't announce the placeholder when the input field gains focus. Nov 23, 2023

return ariaLabel === undefined ? defaultAriaLabel : ariaLabel;
return placeholder ? `${modifiedAriaLabel}, ${placeholder}` : modifiedAriaLabel;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After selection of an item, we still hear the placeholder text being announced along with the item selected when navigated back to searchselect component. Do we still want to hear this?
single-select does not announce this way.

Copy link
Contributor

@PK106552 PK106552 Dec 5, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have removed placeholder text from aria label.
Placeholder prop has been deprecated and will be Removed on next major version release, Visual label should be used instead for better Accessibility experience.
Reference link => https://www.w3.org/WAI/GL/low-vision-a11y-tf/wiki/Placeholder_Research

@@ -89,7 +89,7 @@ const propTypes = {
*/
optionFilter: PropTypes.func,
/**
* Placeholder text. defaults to 'Select or Enter'
* Placeholder prop has been deperecated and will be updated on next major version relase, placeholder can be only visual text.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* Placeholder prop has been deperecated and will be updated on next major version relase, placeholder can be only visual text.
* ![IMPORTANT](https://badgen.net/badge/prop/deprecated/red)
* Placeholder prop has been deprecated and will be Removed on next major version release, Visual label should be used instead for better Accessibility experience.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated - 5497751

@PK106552 PK106552 force-pushed the AH106586-Terra-SearchSelect branch from 0fe10ea to 5497751 Compare December 5, 2023 11:25
@PK106552 PK106552 force-pushed the AH106586-Terra-SearchSelect branch from 4c6c11f to 5891c74 Compare December 5, 2023 11:40
@github-actions github-actions bot temporarily deployed to preview-pr-3984 December 5, 2023 11:42 Destroyed
@PK106552 PK106552 requested a review from sugan2416 December 5, 2023 12:02
@supreethmr supreethmr merged commit c39f4e2 into main Dec 5, 2023
21 checks passed
@supreethmr supreethmr deleted the AH106586-Terra-SearchSelect branch December 5, 2023 18:45
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants