Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added internationalized screen reader helpers to EuiMark. #5739

Merged
merged 14 commits into from
Apr 7, 2022

Conversation

1Copenut
Copy link
Contributor

@1Copenut 1Copenut commented Mar 21, 2022

Summary

This issue adds screen reader helper text to denote the beginning and end of a highlighted passage of text. The screen reader text uses the i18n helper so they are translated properly.

This PR closes #3072

Checklist

  • Checked in both light and dark modes
  • Checked in mobile
  • Checked in Chrome, Safari, Edge, and Firefox
  • Props have proper autodocs and playground toggles
  • Added documentation
  • Checked Code Sandbox works for any docs examples
  • Added or updated jest and cypress tests
  • Checked for breaking changes and labeled appropriately
  • Checked for accessibility including keyboard-only and screenreader modes
  • Updated the Figma library counterpart
  • A changelog entry exists and is marked appropriately

* Added SR-only text using EuiScreenReaderOnly.
* Adding i18n to the highlight start and end.
* Updating tests and declaring React Fragment.
@1Copenut 1Copenut marked this pull request as ready for review March 21, 2022 21:27
upcoming_changelogs/5739.md Outdated Show resolved Hide resolved
Co-authored-by: Greg Thompson <thompson.glowe@gmail.com>
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5739/

@thompsongl
Copy link
Contributor

Raising this for discussion because I think I looked into this a couple years ago:

Now that we're committed to CSS-in-JS, we could use this approach: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/mark#accessibility_concerns

I think i18n may have been a blocker previously. Almost certain that there are tradeoffs here that I'm not aware of that perhaps makes your approach better.

@cchaos
Copy link
Contributor

cchaos commented Mar 22, 2022

Thinking through how EuiHighlight is generally used, which is to show where strings are matching... Are we concerned about this repetition when we get into places like EuiComboBox that might have hundreds of matching items and as the user arrows through, they constantly have to hear this "highlight start/end" text on each option?
Screen Shot 2022-03-21 at 23 13 35 PM

@thompsongl
Copy link
Contributor

To clarify my previous comment:

Add this to the existing css block in mark.styles.ts:

&:before,
&:after {
  clip-path: inset(100%);
  clip: rect(1px, 1px, 1px, 1px);
  height: 1px;
  overflow: hidden;
  position: absolute;
  white-space: nowrap;
  width: 1px;
}

In mark.tsx:

...

const highlightStart = useEuiI18n('euiMark.highlightStart', 'highlight start');
const highlightEnd = useEuiI18n('euiMark.highlightEnd', 'highlight end');

const pseudoStyles = css`
&:before {
  content: " [${highlightStart}] ";
}

&:after {
  content: " [${highlightEnd}] ";
}
`;

...


<mark css={[styles, pseudoStyles]} className={classes} {...rest}>
  {children}
</mark>

@1Copenut
Copy link
Contributor Author

I've refactored the CSS to use Emotion. Thanks @thompsongl for the code snippet.

@cchaos I tested with VoiceOver, and it does repeat the :before and :after blocks in every combo box option listing. It was verbose, but not overwhelming. I'll ask Zuhair his opinion tomorrow.

src/components/mark/mark.tsx Outdated Show resolved Hide resolved
src/components/mark/mark.styles.ts Outdated Show resolved Hide resolved
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5739/

@1Copenut 1Copenut requested a review from cchaos March 23, 2022 21:01
@1Copenut
Copy link
Contributor Author

Ready for re-test. Thanks team!

Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

The concept works! Just had some suggestions and found one issue with the TS

src/components/mark/mark.styles.ts Outdated Show resolved Hide resolved
src/components/mark/mark.tsx Outdated Show resolved Hide resolved
src/services/theme/hooks.tsx Outdated Show resolved Hide resolved
src/components/mark/mark.styles.ts Outdated Show resolved Hide resolved
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5739/

@1Copenut 1Copenut requested a review from cchaos March 24, 2022 15:36
@1Copenut
Copy link
Contributor Author

Ready for next iteration review. I added the hasScreenReaderHelpText prop to the EuiHighlight component and updated unit test to account for the default and override behavior.

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5739/

…it test.

* Picking EuiMark prop for cleaner EuiHighlight typing.
* Updated EuiMark test to include Emotion output in snapshot.
@1Copenut 1Copenut requested a review from cchaos April 5, 2022 15:27
@1Copenut
Copy link
Contributor Author

1Copenut commented Apr 5, 2022

jenkins test this

1 similar comment
@chandlerprall
Copy link
Contributor

jenkins test this

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5739/

Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

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

LGTM, but I'll leave it to @thompsongl to do the eng review.

@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5739/

Copy link
Contributor

@thompsongl thompsongl left a comment

Choose a reason for hiding this comment

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

LGTM!

@1Copenut
Copy link
Contributor Author

1Copenut commented Apr 7, 2022

I talked with Zuhair today and affirmed this is working well in NVDA and JAWS. Shipping it. Thanks everyone.

@1Copenut 1Copenut merged commit 6912640 into elastic:main Apr 7, 2022
@1Copenut 1Copenut deleted the feature/tpierce-eui-3072 branch May 10, 2023 19:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[EuiMark] Accessibility
5 participants