-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Fix text direction for URL and email fields in block editor for RTL languages #68188
base: trunk
Are you sure you want to change the base?
Fix text direction for URL and email fields in block editor for RTL languages #68188
Conversation
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 If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR!
I think the purpose of this PR is reasonable, but it is not desirable to write styles for input elements in the stylesheet of a specific block (Form Input block).
In my opinion, it might be better to solve this problem at the component level.
That is, add direction:ltr
here in the InputControl
component if the type
is email
or url
.
Also, if we want to solve a similar problem with the Textcontrol component, I think we would need to add CSS here.
Hi @t-hamano, For the TextControl component, we’ll need to add the type prop wherever the input type is set to url or email. Currently, the type prop isn't passed, which is why the CSS is not being applied. I’ve compiled a list of places where this prop can be added. Could you please review it and let me know if I've missed any? formgutenberg/packages/block-library/src/form/edit.js Lines 109 to 126 in 48b8624
gutenberg/packages/block-library/src/form/edit.js Lines 148 to 162 in 48b8624
navigation-linkgutenberg/packages/block-library/src/navigation-link/edit.js Lines 192 to 205 in 48b8624
navigation-submenugutenberg/packages/block-library/src/navigation-submenu/edit.js Lines 422 to 431 in 48b8624
There are several places where the rel attribute is used for links. I'm not entirely sure if the type prop is necessary in these cases. Could you please clarify if adding type="url" is required here? Sample Link relgutenberg/packages/block-library/src/post-title/edit.js Lines 164 to 172 in 48b8624
Screenshots for navigation-link
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update!
Sorry, I'm starting to think that since the TextControl
component is so widely used, it might be better to address it in a separate PR. Can this PR address just the InputControl
component?
&[type='email'], | ||
&[type='url'] { | ||
/* rtl:ignore */ | ||
direction: ltr; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you reference the props directly, you should be able to output CSS conditionally. A possible approach would be something like this:
Details
diff --git a/packages/components/src/input-control/styles/input-control-styles.tsx b/packages/components/src/input-control/styles/input-control-styles.tsx
index 39eea8fdb0..ece01451db 100644
--- a/packages/components/src/input-control/styles/input-control-styles.tsx
+++ b/packages/components/src/input-control/styles/input-control-styles.tsx
@@ -4,7 +4,7 @@
import type { SerializedStyles } from '@emotion/react';
import { css } from '@emotion/react';
import styled from '@emotion/styled';
-import type { CSSProperties, ReactNode } from 'react';
+import type { CSSProperties, ReactNode, HTMLInputTypeAttribute } from 'react';
/**
* Internal dependencies
@@ -141,6 +141,7 @@ type InputProps = {
dragCursor?: CSSProperties[ 'cursor' ];
paddingInlineStart?: CSSProperties[ 'paddingInlineStart' ];
paddingInlineEnd?: CSSProperties[ 'paddingInlineEnd' ];
+ type?: HTMLInputTypeAttribute;
};
const disabledStyles = ( { disabled }: InputProps ) => {
@@ -262,6 +263,15 @@ const dragStyles = ( { isDragging, dragCursor }: InputProps ) => {
`;
};
+const directionStyles = ( { type }: InputProps ) => {
+ if ( type !== 'url' && type !== 'email' ) {
+ return '';
+ }
+ return css`
+ direction: ltr;
+ `;
+};
+
// TODO: Resolve need to use &&& to increase specificity
// https://github.com/WordPress/gutenberg/issues/18483
@@ -283,6 +293,7 @@ export const Input = styled.input< InputProps >`
${ fontSizeStyles }
${ sizeStyles }
${ customPaddings }
+ ${ directionStyles }
&::-webkit-input-placeholder {
line-height: normal;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason why we would prefer this over plain CSS selectors? I think in general we should use plain CSS instead of JS when possible, since it will be simpler both in terms of lines of code and runtime work. Another reason is that we are actually planning to move off of CSS-in-JS (#66806).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason why we would prefer this over plain CSS selectors?
I did it for consistency with existing approaches (sizeStyles
, customPaddings
, etc.). But if we are planning to move off of CSS-in-JS, I think the plain CSS is better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @t-hamano , I have reverted back to using CSS. Please review my work once again.
Hey @t-hamano , Thank you for your insightful suggestions on conditionally implementing the CSS. I truly appreciate the feedback, and I’m learning a lot from it. I have made the changes you requested and have created this PR specifically to incorporate the updates to the Looking forward to your thoughts! InputControl componentgutenberg/packages/fields/src/fields/slug/slug-edit.tsx Lines 88 to 119 in d517683
gutenberg/packages/editor/src/components/post-url/index.js Lines 111 to 162 in d517683
|
@t-hamano, I’ll be opening a new PR with the changes to the component shortly and will link it here once it's ready. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@im3dabasia Thanks for the update. Finally, it would be nice to add a CHANGELOG entry. In my opinion, this is a bug fix.
@WordPress/gutenberg-components
I think this PR is OK to ship, but please note that it will make a visual change to the placeholder. The text will be left-aligned regardless of whether it is an LTR or RTL language:
Ideally maybe only the placeholder should be right-aligned, but for some reason the code below didn't work for me:
diff --git a/packages/components/src/input-control/styles/input-control-styles.tsx b/packages/components/src/input-control/styles/input-control-styles.tsx
index db24a5e60f..3fc293e9c3 100644
--- a/packages/components/src/input-control/styles/input-control-styles.tsx
+++ b/packages/components/src/input-control/styles/input-control-styles.tsx
@@ -292,6 +292,11 @@ export const Input = styled.input< InputProps >`
&[type='url'] {
/* rtl:ignore */
direction: ltr;
+
+ &::-webkit-input-placeholder {
+ // For RTL languages, the value changes to "rtl".
+ direction: ltr !important;
+ }
}
}
`;
Furthermore, we cannot know in advance whether the text that goes into the placeholder is for an RTL language or whether it is something that is meant to be displayed in LTR, such as a URL.
In any case, since the text doesn't become unreadable, I think it's fine to always left-aligned the placeholder.
@t-hamano , I'm not quite sure how to create a CHANGELOG entry or which CHANGELOG I should add an entry to. I'd really appreciate it if you could guide me here. |
@im3dabasia No need to create a new file, just access this file and add the new line here. Essentially, if any changes are made to the |
According to the spec, when the placeholder needs to be with a different directionality than the content, we wrap contents with |
4ec51ae
to
8606e29
Compare
@im3dabasia thanks for working on it! However, @t-hamano mentioned that the Furthermore, as I mentioned above, the spec recommends a different approach, have you tried that? |
I tried a combination of approaches.. sharing with you the results trunkThe entire placeholder text is right-aligned, and the ellipsis is placed first, followed by the text itself. Current branchThe placeholder is left-aligned, with the text displayed first, followed by the ellipsis. These are the 3 approaches I tried: 1. The entire placeholder is left-aligned, and the ellipsis appears after the text.
The entire placeholder is left-aligned, and the ellipsis appears after the text.
The placeholder is left-aligned, but the ellipsis appears before the text. Based on my understanding, the goal is for the placeholder text to be in RTL (right-to-left), just like plain text would be, and for the placeholder to be right-aligned within the input box. This means the placeholder text should be displayed in RTL, but the input box itself should remain rendered in LTR, with only the placeholder text aligned and displayed in RTL. During my experimentation, I was able to achieve the RTL text alignment as shown in Case 3 with what @tyxla suggested. However, I found that when I removed the __() translation function and used plain text, the text direction switched to RTL. The resulted text was RTL but was left aligned, which is not the desired behavior. The alignment still remains incorrect. Also removing the translation function isn't a suitable solution. EDIT:It's also worth mentioning that after adding the HTML entities in the placeholder (as shown in Case 3), the output for an LTR language is as follows: Hardcoding the entities will cause an override for all languages, both LTR and RTL. While adding them will fix the alignment in RTL languages, it will disrupt the alignment in LTR languages.
Please let me know your thoughts on this. |
Thanks for trying it out 👍 If that approach doesn't work, we might want to resort to the placeholder one. I know we've historically used all ( |
@im3dabasia Thank you for testing various things.
Perhaps we should first discuss whether this is an ideal behavior. If the placeholder text is normal RTL text, it is expected to be RTL and right-aligned: But the placeholder can be changed. Some developers may want to display a sample URL as the placeholder text. In this case, the right alignment may feel a bit unnatural: Probably the ideal solution would be to parse the placeholder property and make it left-aligned and LTR if it's a url/email, and right-aligned and RTL otherwise, but that would be a bit more complicated to implement. Personally, I'd prefer to always left-align regardless of what the placeholder text is. What do you think? |
I agree that keeping it left-aligned would be a good option, as having different alignments for the placeholder and the input box can make things unnecessarily complicated. Out of curiosity, I checked how other sites handle RTL languages for email input fields specifically. Here's what I found—I’ll attach some screenshots for reference. Interestingly, I noticed that they align the Arabic text to the left. As @t-hamano rightly pointed out, the end developer might use a URL or email as a placeholder. In such cases, aligning the text to the right would indeed be incorrect. |
Agreed. Let's go with this for now, especially because it is simpler 😄 |
Happy to go with the simplest way 👍 |
12552e1
to
7056c85
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
packages/components/CHANGELOG.md
Outdated
@@ -25,6 +25,7 @@ | |||
- `BoxControl`: Better respect for the `min` prop in the Range Slider ([#67819](https://github.com/WordPress/gutenberg/pull/67819)). | |||
- `FontSizePicker`: Add `display:contents` rule to fix overflowing text in the custom size select. ([#68280](https://github.com/WordPress/gutenberg/pull/68280)). | |||
- `BoxControl`: Fix aria-valuetext value ([#68362](https://github.com/WordPress/gutenberg/pull/68362)). | |||
- `InputControl`: Ensures email and url inputs have consistent LTR alignment in RTL languages ([#65893](https://github.com/WordPress/gutenberg/issues/65893)). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- `InputControl`: Ensures email and url inputs have consistent LTR alignment in RTL languages ([#65893](https://github.com/WordPress/gutenberg/issues/65893)). | |
- `InputControl`: Ensures email and url inputs have consistent LTR alignment in RTL languages ([#68188](https://github.com/WordPress/gutenberg/pull/68188)). |
This must be the pull request number, not the issue number.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In addition to that, it needs to be put under the "Unreleased" section, instead of under an already released one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be a good idea to add a Bug Fixes section under the Unreleased section and add an entry there. Like this:
<!-- Learn how to maintain this file at https://github.com/WordPress/gutenberg/tree/HEAD/packages#maintaining-changelogs. -->
## Unreleased
### Bug Fixes
- `InputControl`: Ensures email and url inputs have consistent LTR alignment in RTL languages ([#68188](https://github.com/WordPress/gutenberg/pull/68188)).
## 29.1.0 (2025-01-02)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
Should be good to go once the CHANGELOG issues are addressed.
packages/components/CHANGELOG.md
Outdated
@@ -25,6 +29,7 @@ | |||
- `BoxControl`: Better respect for the `min` prop in the Range Slider ([#67819](https://github.com/WordPress/gutenberg/pull/67819)). | |||
- `FontSizePicker`: Add `display:contents` rule to fix overflowing text in the custom size select. ([#68280](https://github.com/WordPress/gutenberg/pull/68280)). | |||
- `BoxControl`: Fix aria-valuetext value ([#68362](https://github.com/WordPress/gutenberg/pull/68362)). | |||
- `InputControl`: Ensures email and url inputs have consistent LTR alignment in RTL languages ([#68188](https://github.com/WordPress/gutenberg/pull/68188)). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the trouble, but we can now remove this one now that it's included in the "Unreleased" section.
- `InputControl`: Ensures email and url inputs have consistent LTR alignment in RTL languages ([#68188](https://github.com/WordPress/gutenberg/pull/68188)). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tyxla , Fixed it . Thank you for the quick review!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
Thanks again @im3dabasia 🙌
Closes : #65893
What?
Adds
direction: ltr
for URL and email input fields in the block editor to ensure correct text alignment, especially for RTL languages.Why?
Screenshots or screencast
Here are a few samples which I tested out.
Embed Block
RSS Block
Audio Block which uses url-popover component