ryersondmp / sa11y

Sa11y is an accessibility quality assurance tool that visually highlights common accessibility and usability issues. Geared towards content authors, Sa11y straightforwardly identifies errors or warnings at the source with a simple tooltip on how to fix them.
https://sa11y.netlify.app
Other
280 stars 43 forks source link

RTL support feedback #43

Closed thibaudcolas closed 1 year ago

thibaudcolas commented 1 year ago

👋 follow-up on #41 – @albinazs and I have been testing the RTL support in 2.3.6-development. Here is our feedback based on looking at the code and manual testing. Note neither of us are fluent with a RTL language, so this is just based on our understanding of RTL, mainly coming from the RTL Styling 101 guide. Hope this helps!

Approach

If browser support allows, we’d recommend using CSS logical properties and values wherever possible, rather than [dir="rtl"] overrides. It’s the approach used in jooa11y. It means that for the vast majority of styles there’s only one version of the code written, that mirrors itself – it’s much less code to maintain and less error-prone.

Since Sa11y uses Stylelint, @albinazs ran stylelint checks to look for properties that would require mirroring. She’ll be able to provide the results from Stylelint if it helps. Here are our Stylelint rules to do this (written in JS so there can be code comments):

        "declaration-property-value-allowed-list": {
            // Only allow logical values, and resets.
            "clear": ["both", "none"],
            // Only allow logical values, and resets.
            "float": ["inline-start", "inline-end", "none", "unset"],
            // Only allow logical values.
            "text-align": ["start", "end", "center"]
        },
        "property-disallowed-list": [
            // Disallow positioning with physical properties. Use logical ones instead.
            "/left/",
            "/right/",
        ],

Specific issues

Refactoring

While testing this we spotted dead code here: https://github.com/ryersondmp/sa11y/blob/8f92283b60b6b900be055e9877683dd16d19a7d2/src/scss/sa11y.scss#L257-L262

This is part of the #sa11y-container #sa11y-notification-badge styles, which don’t have a div element within:

<div id="sa11y-notification-badge">
            <span id="sa11y-notification-count"></span>
            <span id="sa11y-notification-text" class="sa11y-visually-hidden"></span>
</div>
brianteeman commented 1 year ago

yes css-logical properties are the way to go although there may be a few places where that isnt possible. there are still afew gaps in browser support for some of them.

the other option that can be used either together with or as an alternative to that is to use the rtlcss preprocessor https://rtlcss.com/

bootstrap uses a combination of these techniques although I personally recommend using css logical properties were possible.

Some icons need to be mirrored: "good" checkmark "warning" question mark

Be very careful here. Some RTL languages do use a mirrored question mark but some do not. Just as Spanish also has an inverted question mark at the beginning. (just seen this is specifically mentioned in the link)

And a tick checkmark is not mirrored (https://developers.google.com/fonts/docs/material_icons#rtl_icons_on_the_web)

The settings’ switches needs mirroring, with "on" state to the left

Again this is not necessary or required - these "switches" are designed to mimic real world physical switches and those switches are the same whichever language you speak https://rtlstyling.com/

thibaudcolas commented 1 year ago

👍 Apologies about the checkmark, I thought I had researched this one before but clearly got it wrong. For the switches, I think they need mirroring because that’s what iOS does. I don’t have any further awareness about them than that, so if that’s wrong that’s fine by me.

brianteeman commented 1 year ago

material design (android) has extensive guidelines about adjusting for all sorts of things for RTL. Annoyingly they make no mention at all about switches. I take their comments about all other UI elements that do change in RTL is that they dont for switches

adamchaboryk commented 1 year ago

Sorry for the slow reply - i'm just getting back from the holiday break. Thanks so much for looking into this and your comments everyone!

brianteeman commented 1 year ago

welcome back says Brian currently in Israel doing RTL research ;)

brianteeman commented 1 year ago

Some screenshots from two israeli apps and from android itself. As you can see the two apps that are hebrew first do not change the switch but android which is english first does change the switch.

In general asking people here in Israel about this - they said both were correct - in other words they were suprised I was asking.

image

image

image

adamchaboryk commented 1 year ago

Thank you both once again for looking into this. I'm taking a hiatus now until the end of February, but I'll revisit this when I'm back!

brianteeman commented 1 year ago

Let me know if you need anything

brianteeman commented 1 year ago

@adamchaboryk just took my first very quick look at the 3.0 branch. I can see a few potential rtl issues with the css. I am sorry I dont have time for the next few weeks to really dig into it. Happy to do the work so I guess I am just leaving this as a memo for myself

adamchaboryk commented 1 year ago

No worries, @brianteeman. There's no rush given we don't have any translations yet. I'm also working on high contrast mode at the moment, so we can revisit this later!

Appreciate you having a look.

Let me know if you have any thoughts with the new toggleable colour filters feature too!