RangeControl: RTL rendering issue

WordPress / gutenberg

The Block Editor project for WordPress and beyond. Plugin is available from the official repository.

https://wordpress.org/gutenberg/

Other

10.52k stars 4.21k forks source link

RangeControl: RTL rendering issue #29519

Closed ItsJonQ closed 2 years ago

ItsJonQ commented 3 years ago

Recently, there were reports that the RangeControl component wasn't working currently for RTL.

Is anyone else seeing issues with RTL behavior of Gutenberg components in 5.7 RC2? RangeControl is broken for me, but works fine in 5.6

Also happens with GB 10.1, FWIW

(via Slack, https://wordpress.slack.com/archives/C02QB2JS7/p1614770359426200)

When I test it locally at the latest trunk, it seemed to work okay:

Screen Capture on 2021-03-03 at 09-36-39

(Screenshot of Gutenberg running locally. Arabic set as the language. Cover block's Opacity slider is rendering correctly)

I've tested this with release/10.1 and wp/truck as well, but the range slider is working correctly in RTL for me (for multiple blocks).

hellofromtonya commented 3 years ago

Able to reproduce where slide motion is opposite when RTL is selected when using the RTL Tester plugin with WordPress 5.7 RC2:

GH29519-5 7RC2

But not able to reproduce when RTL Tester plugin is disabled and site language is set to a RTL language with WordPress 5.7 RC2:

GB29519-rlt-lang

ItsJonQ commented 3 years ago

@hellofromtonya Haii! That's really helpful! Thank you 🙏

I looked into it, and I can confirm the bug.

As you noted in your feedback, I've confirmed that the RTL check was based on language (coming from @wordpress/i18n), rather than document setting (which is how the RTL Tester is forcing the change).

I can create a PR that resolves this issue for RangeControl.

However, this (language based) RTL check is used in other parts of the JS code. I suspect the same issue would arise for those instances as well.

hellofromtonya commented 3 years ago

I can create a PR that resolves this issue for RangeControl.

@ItsJonQ Thank you. Would really appreciate you doing this. Update: PR #29522.

However, this (language based) RTL check is used in other parts of the JS code. I suspect the same issue would arise for those instances as well.

What and where should we looking and testing more broadly to surface where else these other potential instances are?

ItsJonQ commented 3 years ago

What and where should we looking and testing more broadly to surface where else these other potential instances are?

It's tricky to say. If we do a search in the code for this isRTL() function, you can see that it's scattered throughout the codebase (JS):

https://github.com/WordPress/gutenberg/search?l=JavaScript&q=isrtl%28%29

From what I can see, it typically affects icons, position, and alignment for things.

In the screenshot below, we can see the < and > icons aren't reversing correctly. This is via RTL Tester (not language)

In this screenshot, notice how the Toolbar is docked to the left. This is via RTL Tester:

However, if we were to swap the language (e.g. Arabic), you can see that it's correctly positioned on the right hand side:

youknowriad commented 3 years ago

I'm removing this from the "must have" board, it's seems it's more related to the testing plugin, the actual production behavior is good.

ItsJonQ commented 3 years ago

the testing plugin, the actual production behavior is good.

Thanks for your input @youknowriad ! Realistically, I think detecting RTL from language makes sense. I'm not sure of a realistic scenario where you'd force dir="rtl" on the document, outside of testing. (I may be wrong though).

ndiego commented 2 years ago

This issue was reviewed today during the Editor Bug Scrub. Given that it appears to be primarily related to the RTL testing plugin, we have decided to close this issue. If anyone feels differently, feel free to reopen and provide additional context.