WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 4 weeks ago

#30506 closed defect (bug) (fixed)

RTL: Hours and minutes fields order reversed in post editing

Reported by: leac Owned by: adamsilverstein
Milestone: 5.3 Priority: normal
Severity: normal Version: 4.0
Component: Editor Keywords: has-patch needs-testing
Focuses: ui, rtl Cc:
PR Number:

Description (last modified by yoavf)

Steps to reproduce:

  1. Use RTL language (i.e. Hebrew, Arabic)
  2. Edit post
  3. Edit scheduled publish time
  4. See that the order of the hours field and the minutes field are reversed:

https://core.trac.wordpress.org/raw-attachment/ticket/30506/wp_bug_1.jpg

It happens in Quick edit too:

  1. Use RTL language (i.e. Hebrew, Arabic)
  2. Quick edit a post
  3. See that the order of the hours field and the minutes field are reversed:

https://core.trac.wordpress.org/raw-attachment/ticket/30506/wp_bug_2.jpg

Attachments (5)

wp_bug_1.jpg (112.1 KB) - added by leac 5 years ago.
RTl reversed time field order in edit post
wp_bug_2.jpg (64.0 KB) - added by leac 5 years ago.
RTl reversed time field order in quick edit post
30506.patch (5.5 KB) - added by SergeyBiryukov 3 years ago.
30506-post-edit.png (10.1 KB) - added by ramiy 3 months ago.
30506-quick-edit.png (15.6 KB) - added by ramiy 3 months ago.

Download all attachments as: .zip

Change History (55)

@leac
5 years ago

RTl reversed time field order in edit post

@leac
5 years ago

RTl reversed time field order in quick edit post

#1 @yoavf
5 years ago

  • Description modified (diff)
  • Keywords needs-patch added

#2 @leac
5 years ago

  • Summary changed from RTL: Hours and minutes fields order reversed in post quick editing to RTL: Hours and minutes fields order reversed in post editing

#3 @ocean90
5 years ago

  • Component changed from Administration to Posts, Post Types
  • Focuses ui added

#4 follow-up: @leac
5 years ago

I dug into the code, and found that the code that creates all text fields in schedule editing is in
wp-admin\includes\template.php (row 753 in 4.0.1):

/* translators: 1: month, 2: day, 3: year, 4: hour, 5: minute */
printf( __( '%1$s %2$s, %3$s @ %4$s : %5$s' ), $month, $day, $year, $hour, $minute );

Then I checked if that string appears anywhere else. I found that it is used one other place, in
wp-includes\script-loader.php (row 444 in 4.0.1), where it's assigned to the dateFormat field in the postL10n object:

'dateFormat' => __('%1$s %2$s, %3$s @ %4$s : %5$s'),

That object and field is used in wp-admin/js/post.js (row 728 in 4.0.1), after submitting the new scheduled date.

So I was thinking that the easiest solution for the reversed hour and minute order problem would be to change the order of parameters %4$s and %5$s in the translation file:

%2$s %1$s, %3$s בשעה %5$s : %4$s

But that isn't a good solution semantically, because the hour and minute aren't really supposed to be reversed: the reversal would be done for display reasons only, and that's not semantically correct.

So I need a way to show the %4$s and %5$s from left to right, without changing their order in the string.

#5 in reply to: ↑ 4 @ramiy
5 years ago

Replying to leac:

I need a way to show the %4$s and %5$s from left to right, without changing their order in the string.

You can use the RTL Tester Plugin.

#6 @leac
5 years ago

I have it. How does it help me in this case?
I know how it looks in RTL, and I know how it looks in LTR.
I know where the problem is, just not how to solve it.

Last edited 5 years ago by leac (previous) (diff)

#7 @iseulde
5 years ago

  • Component changed from Posts, Post Types to Editor

#8 @SergeyBiryukov
3 years ago

  • Keywords has-patch commit added; needs-patch removed
  • Milestone changed from Awaiting Review to 4.8

#9 @adamsilverstein
2 years ago

tested 30506.patch and verified it fixes the issue reported in the ticket:

https://d3vv6lp55qjaqc.cloudfront.net/items/272O3u433L2X461D0D38/Image%202017-05-18%20at%2010.08.11%20AM.png

This ticket was mentioned in Slack in #core by flixos90. View the logs.


2 years ago

#11 @adamsilverstein
2 years ago

@SergeyBiryukov shall we go ahead and commit this or wait for 4.9?

This ticket was mentioned in Slack in #core by adamsilverstein. View the logs.


2 years ago

This ticket was mentioned in Slack in #core by obenland. View the logs.


2 years ago

#14 @obenland
2 years ago

  • Milestone changed from 4.8 to Future Release

#15 @leac
2 years ago

Thanks a lot for the patch! I too tested it and it works.
Would love to see it commited.

#16 @SergeyBiryukov
2 years ago

WP_Customize_Date_Time_Control::content_template() would likely need this change as well, see #42394.

#18 in reply to: ↑ 17 @leac
9 months ago

Replying to iseulde:

This is no longer relevant after the merge of the block editor. Let's preserve the way it's currently built for the old editor.

I'm sorry, but could you explain why this is no longer relevant? The bug appears in the block editor as well.

In addition, as stated before, this bug exists also out of the editor - It happens in Quick edit too.

#19 follow-up: @adamsilverstein
9 months ago

Verifying I still see this issue in the block editor. I created a post at 12:20pm and switched my language to Hebrew, the result was this for the date picker:

https://d2ddoduugvun08.cloudfront.net/items/0w1J3i1f131z422s3W1Z/Image%202019-01-06%20at%2012.24.06%20PM.png

If I understand correctly, the hour/minute fields should be reversed here with the left field showing the hours and the right field the minutes, is that right @leac ?

#20 @adamsilverstein
9 months ago

  • Status changed from new to reopened

Reopening because the reversed time field order in "quick edit" is still a valid issue.

@iseulde would it be better to create an issue in GitHub for the block editor issue?

#21 @adamsilverstein
9 months ago

  • Owner set to adamsilverstein
  • Status changed from reopened to assigned

#22 in reply to: ↑ 19 @leac
9 months ago

Replying to adamsilverstein:

Verifying I still see this issue in the block editor. I created a post at 12:20pm and switched my language to Hebrew, the result was this for the date picker:

https://d2ddoduugvun08.cloudfront.net/items/0w1J3i1f131z422s3W1Z/Image%202019-01-06%20at%2012.24.06%20PM.png

If I understand correctly, the hour/minute fields should be reversed here with the left field showing the hours and the right field the minutes, is that right @leac ?

Hi Adam,

Thank you so much for taking the time to look into this and for reopening the ticket.

Yes, what your screenshot shows is exactly the bug.

Thanks again.

#23 @iseulde
9 months ago

Thanks @adamsilverstein!

#24 @SergeyBiryukov
7 months ago

  • Milestone set to Future Release

#26 follow-up: @adamsilverstein
4 months ago

  • Milestone changed from Future Release to 5.3

#27 in reply to: ↑ 26 @leac
3 months ago

Replying to adamsilverstein:
Thank you for taking care of this!
Will the fix be only in the Gutenberg editor, or will it be also in the classic editor as well as the quick edit feature of the post list screen?

#28 follow-up: @ramiy
3 months ago

The bug was FIXED.

The reason is wrong translation of the string %1$s %2$s, %3$s @ %4$s:%5$s. Fixed by replacing : with : , It should be separated by spaces. Tested on WordPress 5.2. Then fixed in older versions, all the way back to WordPress 4.3. Before that, there was a different string.

Note: The patch is not required, in 12 hours all the Hebrew installations will be updated with new translation strings - WordPress 5.2 already updated.

#29 in reply to: ↑ 28 @leac
3 months ago

Hi Rami,

Thank you for the fix! It works great in the quick edit feature of the post list screen.

However, in the Classic Editor, the space makes the hours break to the next line. I don't know if you mind that, since people are supposed to be using Gutenberg, but I thought I'd at least bring it to your attention.
https://cldup.com/iiqfl4okPV.jpg

Replying to ramiy:

The bug was FIXED.

The reason is wrong translation of the string %1$s %2$s, %3$s @ %4$s:%5$s. Fixed by replacing : with : , It should be separated by spaces. Tested on WordPress 5.2. Then fixed in older versions, all the way back to WordPress 4.3. Before that, there was a different string.

Note: The patch is not required, in 12 hours all the Hebrew installations will be updated with new translation strings - WordPress 5.2 already updated.

Last edited 3 months ago by leac (previous) (diff)

#30 @adamsilverstein
3 months ago

@ramiy to clarify - this issue was fixed in the translation files? is the patch no longer required? what about the Gutenberg change?

The reason is wrong translation of the string %1$s %2$s, %3$s @ %4$s:%5$s. Fixed by replacing : with : , It should be separated by spaces.

Can you clarify where you made that change? does this fix apply for all RTL languages?

Last edited 3 months ago by adamsilverstein (previous) (diff)

#31 @ramiy
3 months ago

@leac Thanks for reporting, I'm looking for a solution.

@adamsilverstein The patch no longer required. Also, this fix is not affecting Gutenberg.

#32 @adamsilverstein
3 months ago

Thanks for clarifying. I'm still seeing the incorrect order when switching my language to hebrew.

#33 follow-up: @ramiy
3 months ago

@leac try now.

@adamsilverstein you need to visit Dashboard > Updates, press the "Check Again" button and update the language packs.

#34 in reply to: ↑ 33 ; follow-up: @leac
3 months ago

Works, thank you!

Might I ask, out of curiosity, what you did to fix it?

Replying to ramiy:

@leac try now.

#35 in reply to: ↑ 34 ; follow-up: @ramiy
3 months ago

Replying to leac:

Might I ask, out of curiosity, what you did to fix it?


I changed the %1$s %2$s, %3$s @ %4$s:%5$s translation string. I tested all kind of combinations - with spaces, without spaces, original order, reverse order. In RTL languages we can also use the HTML ‏ entity. There are many solutions. Each has it's own benefits. But also side effects like the one you reported above.

In any case, those are the strings I've updated to fix the issue:

#36 in reply to: ↑ 35 @leac
3 months ago

Thank you very much for the detailed explanation. And of course for the fix :)

Replying to ramiy:

Replying to leac:
I changed the %1$s %2$s, %3$s @ %4$s:%5$s translation string. I tested all kind of combinations - with spaces, without spaces, original order, reverse order. In RTL languages we can also use the HTML ‏ entity. There are many solutions. Each has it's own benefits. But also side effects like the one you reported above.

In any case, those are the strings I've updated to fix the issue:

#37 @adamsilverstein
3 months ago

@ramiy @leac can we close this as resolved? The Gutenberg fix was also merged in https://github.com/WordPress/gutenberg/pull/16375

#38 @ramiy
3 months ago

@adamsilverstein Yes. This ticket can be closed. The bug was fixed.

#39 @yoavf
3 months ago

Thanks @ramiy - this is indeed fixed now, but I just want to mention that switching the hours/minutes order in the RTL translation is semantically incorrect. But since this only affects the classic editor, I'm fine with it - though this is something we'd want to share with all the other RTL language teams (Arabic uses the extra space method you've mentioned before, and Farsi doesn't have any "fix" in the translation).

CC @nabilmoqbel @alex-ye @dedidata @man4toman

#40 @leac
3 months ago

Yes, @adamsilverstein. Joining @ramiy and @yoavf, it's fine by me to close this as resolved.
Thank you all very much.

#41 @man4toman
3 months ago

Thanks @yoavf
I'll fix this asap for Farsi(Persian) language.

Update:
Thanks @ramiy but it doesn't fix only by replacing : with : , we must remove/replace @ too.

Last edited 3 months ago by man4toman (previous) (diff)

#42 @adamsilverstein
3 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

#43 @alex-ye
3 months ago

It doesn't feel right to reverse the translations particularly because the string has no specific context. Is it possible to use _x() with this string in the future releases?

Example:

_x( '%1$s %2$s, %3$s @ %4$s:%5$s', 'date-edit' )

#44 @JeffPaul
2 months ago

  • Keywords needs-testing fixed-major added
  • Milestone changed from 5.3 to 5.2.3
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening this so it can be back-ported to the 5.2 branch, also needs testing to validate if this ticket is good to land in 5.2.3.

This ticket was mentioned in Slack in #core by marybaum. View the logs.


2 months ago

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


8 weeks ago

#47 @adamsilverstein
8 weeks ago

It doesn't feel right to reverse the translations particularly because the string has no specific context. Is it possible to use _x() with this string in the future releases?

Good suggestion, can we open up a follow up ticket to address this?

This ticket was mentioned in Slack in #core by whyisjake. View the logs.


8 weeks ago

#49 @JeffPaul
8 weeks ago

  • Milestone changed from 5.2.3 to 5.3

This specific issue has been resolved in Gutenberg and will land in core once a major release includes that specific Gutenberg release. I'm re-milestoning this to 5.3 as that's likely when this issue will land in core, though subsequent or related issues can be considered before 5.3 as necessary.

#50 @adamsilverstein
4 weeks ago

  • Keywords close added; commit removed

#51 @johnbillion
4 weeks ago

  • Keywords fixed-major close removed
  • Resolution set to fixed
  • Status changed from reopened to closed

If switching to _x() makes sense, please open a followup ticket.

Note: See TracTickets for help on using tickets.