ToggleGroupControl: Fix arrow key navigation in RTL

[mirka]

Reported in #64963 (comment)

What?

Fixes arrow key navigation when ToggleGroupControl is in RTL mode.

Testing Instructions for Keyboard

See default Storybook story for ToggleGroupControl and toggle it to RTL mode in the toolbar. Test that arrow key navigation works as expected.

CleanShot.2024-09-30.at.19.03.16.mp4

[github-actions]

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: mirka <0mirka00@git.wordpress.org>
Co-authored-by: tyxla <tyxla@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

[tyxla]

LGTM 🚀

Tests well with both left-right and up-down arrows 👍

[github-actions]

I just cherry-picked this PR to the wp/6.7 branch to get it included in the next release: aa59d85

[t-hamano]

Thanks for the fix! I would like to update #64963 and clarify the remaining tasks.

[DaniGuardiola]

Looking at the implementation of isRTL, this seems highly WP-specific. That might be what we want, but I wonder if doing a more straightforward DOM check would work while being more context-agnostic (and lightweight, isRTL seems a bit chunky)?

For example, the dom package also exports an isRTL function implemented like this:

[ciampo]

It would be good to unify the way we check for RTL. I don't know if there are any hidden implications (ie., could a user ever end up where the WP language is a RTL language BUT the document's direction is not rtl ), but:

• if there are such differences, we should find out why and if they are intended. And we could probably rename the functions to better showcase what they effectively do — ie. isRtlDirection vs isRtlLanguage ?
• if there's no difference, we could decide what's the best version and deprecate the other?

Doesn't seem high priority, though.

[mirka]

I don't currently see an issue with isRTL(). We're tied to @wordpress/i18n for string translations anyway, and a DOM check would be less ergonomic given that devs would need to pick out and supply a DOM element for every invocation.

[jsnajdr]

For some reason the MDN page for the direction CSS attribute is discouraging people from using it at all:

Warning: Where possible, authors are encouraged to avoid using the direction CSS property and use the HTML dir global attribute instead.

According to that the right way to do WP-agnostic DOM-only RTL check is:

element.ownerDocument.documentElement.dir === 'rtl'

[DaniGuardiola]

According to that the right way to do WP-agnostic DOM-only RTL check is:

element.ownerDocument.documentElement.dir === 'rtl'

Whether you use the CSS property or the HTML attribute, the computed result is the same. I made a demo to illustrate this: https://stackblitz.com/edit/stackblitz-starters-mc6hcb?file=script.js

I don't currently see an issue with isRTL(). We're tied to @wordpress/i18n for string translations anyway

That's true, though I still think it's worth removing extra dependencies when possible. Keeping a minimal dep footprint gives us more flexibility and, in this case, might allow us to tree-shake that part of the library away.

and a DOM check would be less ergonomic given that devs would need to pick out and supply a DOM element for every invocation.

That's not necessarily a problem if we're okay with checking document.body by default. E.g. this would work fine:

export default function isRTL( element = document.body ) {
	return getComputedStyle( element ).direction === 'rtl';
}
isRTL();

While still allowing an element in, which is useful for us to do something like:

isRTL(containerRef);

Which is more specific, and in the rare case that the direction is different in the subtree that component is in, it will correctly pick its value up. So, in a way, this would potentially improve detection and prevent some edge cases.

That is, in the case that the current isRTL function isn't doing something we don't know (like the example @ciampo gave: "could a user ever end up where the WP language is a RTL language BUT the document's direction is not rtl").

Another point in favor of doing this is that we might want to use :dir(rtl) in CSS (here's a current example), which is very useful, and this approach in JS land would bring it closer to that, preventing potential future inconsistencies.