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

feat(web-components): ensure screen-reader-only text is hidden for RTL languages #2953

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from

Conversation

irl
Copy link
Contributor

@irl irl commented Dec 20, 2024

Summary of the changes

When using RTL languages, elements using left: -9999px and roughly equivalent styles will cause a large scrollable but blank region to appear on the left of the screen. By using the :dir pseudo-class it is possible to instead shift these elements to be right: -9999px which the browser will hide when <html dir="rtl"> in the same way as it does for elements to the left when <html dir="ltr">.

This change also standardises on using -9999px from the start of the page as the offscreen position to make it easier to locate occurrences of this pattern with search. I also found -10000px and -625rem but these are roughly or exactly equivalent.

There is a question as to whether this is the correct change, or if the correct change is to have just one class that can be applied to these elements. Seeing multiple classes defined as:

  • .sr-only
  • .screen-reader-only-text
  • .offscreen
  • .visually-hidden

feels redundant to me. The technique is also applied directly to component-specific classes instead of defining the styles just once for the technique and re-using that class where it is needed.

Related issue

Closes: #2906

Checklist

General

  • Changes to docs package checked and committed.
  • All acceptance criteria reviewed and met.

Testing

  • Relevant unit tests and visual regression tests added.
  • Visual testing against Figma component specification completed.
  • Playground stories in React Storybook up to date, with any prop changes and additions addressed.
  • Compare performance of modified components against develop using Performance addon in React Storybook.

Accessibility

  • Accessibility Insights FastPass performed.
  • A11y unit test added and yields no issues.
  • A11y plug-in on Storybook yields no issues.
  • Manual screen reader testing performed using NVDA and VoiceOver.
  • Manual keyboard testing for keyboard controls and logical focus order.
  • Correct roles used and ARIA attributes used correctly where required.
  • Logical heading structure is maintained, and the HTML elements used for headings can be changed to fit within the wider page structure.

Resize/zoom behaviour

  • Page can be zoomed to 400% with no loss of content.
  • Screen magnifier used with no issues.
  • Text resized to 200% with no loss of content.
  • Text spacing increased as per the WCAG 1.4.12 success criterion with no loss of content.

System modes

  • Browser setting 'prefers reduced motion' tested. No animations or motion visible whilst this setting is on.
  • Windows High Contrast mode tested with no loss of content.
  • System light and dark mode tested with no loss of content.
  • Browser support tested (Chrome, Safari, Firefox and Edge).

Testing content extremes

  • Min/max content examples tested with no loss of content or overflow.
  • All prop combinations work without issue.
  • Tested for FOUC (Flash of Unstyled Content) in both SSR (Server-Side Rendering) and SSG (Static Site Generation) settings.
  • Controlled and uncontrolled input components tested.
  • Props/slots can be updated after initial render.

lz405 and others added 6 commits December 19, 2024 16:58
add a new dismissLabel prop which allows the user to change the text of the label on the dismiss
button

feat mi6#2839
Add prop to customise tooltip label for chip. Add Cypress test and stories

. mi6#2839
…agination-bar-items-per-page-prop

Prevent duplication of pagination bar items per page dropdown items
…p-text

feat(web-components): [ic-chip] add prop to customise tooltip text
…L languages

This change also standardises on using -9999px from the start of the page as the
offscreen position to make it easier to locate occurrences of this pattern with
search.

Closes: mi6#2906
@GCHQ-Developer-847
Copy link
Contributor

GCHQ-Developer-847 commented Jan 2, 2025

Hi @irl, thank you for your contribution!

I agree that it is unnecessary / redundant to use different classes across components. If we were to just use one shared class, I think the helpers.css file would be the place to add it.

However, I feel as though implementing the use of a single class may be a bit more involved as the screen-reader-only classes currently applied to individual components have slightly different styles applied; it would require checking that the styles on a single shared class work for all components, alongside some refactoring.

I’d say it’s up to you if you want to fix the redundancy in this PR - but it is a bit out of scope for the ticket so we’d be happy if you want to create a separate ticket for it (or let us know if you want us to create one!).



(Also, for the checklist on the PR, you can mark any task as done if it isn’t applicable or related to your changes. They will need to all be ticked before the PR can be merged.)

@@ -209,6 +209,10 @@ slot[name="app-title"]::slotted(a) {
transition: opacity var(--ic-easing-transition-slow);
}

:host(.sm-collapsed) slot[name="app-title"]:dir(rtl)::slotted(a) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the same for line 204 needs adding here too:

Suggested change
:host(.sm-collapsed) slot[name="app-title"]:dir(rtl)::slotted(a) {
:host(.sm-collapsed) slot[name="app-title"]:dir(rtl)::slotted(ic-typography),
:host(.sm-collapsed) slot[name="app-title"]:dir(rtl)::slotted(a) {

@GCHQ-Developer-847
Copy link
Contributor

Hi @irl, just wondered if you are planning to update this PR soon? There are at least a few conflicts and one comment which needs resolving

Let us know if you have any more questions or issues

@irl
Copy link
Contributor Author

irl commented Feb 25, 2025

Hi @irl, just wondered if you are planning to update this PR soon? There are at least a few conflicts and one comment which needs resolving

Yes, thanks for the prompt. I do want to get this done soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants