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

RangeControl: RTL rendering issue #29519

Closed
ItsJonQ opened this issue Mar 3, 2021 · 7 comments
Closed

RangeControl: RTL rendering issue #29519

ItsJonQ opened this issue Mar 3, 2021 · 7 comments
Assignees
Labels
[Package] Components /packages/components [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended

Comments

@ItsJonQ
Copy link

ItsJonQ commented Mar 3, 2021

Screenshot 2021-03-03 at 11 19 09

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 Shot 2021-03-03 at 9 36 23 AM

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).

@ItsJonQ ItsJonQ added [Type] Bug An existing feature does not function as intended [Package] Components /packages/components labels Mar 3, 2021
@hellofromtonya
Copy link
Contributor

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
Copy link
Author

ItsJonQ commented Mar 3, 2021

@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.

@github-actions github-actions bot added the [Status] In Progress Tracking issues with work in progress label Mar 3, 2021
@hellofromtonya
Copy link
Contributor

hellofromtonya commented Mar 3, 2021

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
Copy link
Author

ItsJonQ commented Mar 3, 2021

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)

Screen Shot 2021-03-03 at 12 53 20 PM

In this screenshot, notice how the Toolbar is docked to the left. This is via RTL Tester:
Screen Shot 2021-03-03 at 12 56 06 PM

However, if we were to swap the language (e.g. Arabic), you can see that it's correctly positioned on the right hand side:
Screen Shot 2021-03-03 at 12 55 43 PM

@youknowriad
Copy link
Contributor

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
Copy link
Author

ItsJonQ commented Mar 4, 2021

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
Copy link
Member

ndiego commented Jul 19, 2022

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.

@ndiego ndiego closed this as completed Jul 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Components /packages/components [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants