WordPress.org

Make WordPress Core

Opened 3 weeks ago

Closed 2 weeks ago

#42394 closed defect (bug) (fixed)

Customize: Order of fields in date/time control does not honor locale

Reported by: munyagu Owned by: westonruter
Milestone: 4.9 Priority: normal
Severity: normal Version: 4.9
Component: Customize Keywords: has-patch commit dev-reviewed
Focuses: Cc:

Description

The field order of the schedule setting field in customizer is not changed for each language.
The order is changed in 'Published on' in Edit Post etc.

Attachments (17)

post-schedule-date.png (63.0 KB) - added by atachibana 3 weeks ago.
Success case - Post Scheduled date
customizer-schedule-date.png (82.5 KB) - added by atachibana 3 weeks ago.
Error case - Customizer Scheduled date
42394.0.diff (3.7 KB) - added by westonruter 3 weeks ago.
https://github.com/xwp/wordpress-develop/pull/299
en-US.png (46.3 KB) - added by westonruter 3 weeks ago.
es-MX.png (48.4 KB) - added by westonruter 3 weeks ago.
fr-CA.png (44.3 KB) - added by westonruter 3 weeks ago.
42394.1.diff (4.8 KB) - added by westonruter 3 weeks ago.
Δ https://github.com/xwp/wordpress-develop/pull/299/commits/2c0e14f
Screen Shot 2017-11-02 at 19.45.21.png (21.3 KB) - added by Mirucon 3 weeks ago.
42394.1.diff - with format Y年n月j日
Screen Shot 2017-11-02 at 19.35.43.png (22.3 KB) - added by Mirucon 3 weeks ago.
42394.1.diff - with format Y年n月j日
jp-date-format.png (55.1 KB) - added by westonruter 3 weeks ago.
49rc1-customizer-date-42394-1-diff.png (48.1 KB) - added by atachibana 3 weeks ago.
After 42394.1.diff, Customizer scheduled date in Japanese locale
42394.2.diff (4.6 KB) - added by westonruter 3 weeks ago.
Remove unused var
Screen Shot 2017-11-03 at 08.24.33.png (57.5 KB) - added by Mirucon 3 weeks ago.
Scheduling a post
Screen Shot 2017-11-02 at 10.03.35 PM.png (1.8 MB) - added by dlh 2 weeks ago.
42394.3.diff (763 bytes) - added by dlh 2 weeks ago.
42394.4.diff (1.4 KB) - added by westonruter 2 weeks ago.
42394.diff (1.5 KB) - added by Mirucon 2 weeks ago.

Change History (56)

#1 @atachibana
3 weeks ago

Reproduced.
It seems to I18N issue. Customizer schedule's date seems to be fixed as "Month Day, Year".

Step:
Preparation - Set locale to Japanese (日本語)

  1. Administration Screen -> Settings -> General
  2. Select '日本語' (Japanese) => Data Format should be "2017年11月1日" (Y年n月j日)

Open New Post and Click Schedule.
=> You will see correct ordered Year, Month, Day Field boxes.

Open Customizer -> Settings -> Schedule.
=> The Order is Month Day, Year

#2 @Mirucon
3 weeks ago

  • Component changed from Customize to I18N
  • Keywords needs-patch added
  • Summary changed from The field order of the schedule setting field in customizer is not changed. to i18n: The field order of the schedule setting field in customizer is not changed.

@atachibana
3 weeks ago

Success case - Post Scheduled date

@atachibana
3 weeks ago

Error case - Customizer Scheduled date

#3 @westonruter
3 weeks ago

  • Component changed from I18N to Customize
  • Milestone changed from Awaiting Review to 4.9
  • Summary changed from i18n: The field order of the schedule setting field in customizer is not changed. to The field order of the schedule setting field in customizer is not changed.

#4 @westonruter
3 weeks ago

  • Summary changed from The field order of the schedule setting field in customizer is not changed. to Customize: Order of fields in date/time control does not honor locale

#5 @SergeyBiryukov
3 weeks ago

Edit Post screen uses touch_time(), which allows translators to customize the date/time format as needed:

/* 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 );

Ideally, the same string should be used in WP_Customize_Date_Time_Control for consistency, but it uses a completely different markup and a different approach to 12/24 hour format, so the fix appears to be less straightforward than I thought.

#6 @westonruter
3 weeks ago

At the very least we could just detect the ordering of placeholders 1, 2, and 3 and make sure that the corresponding fields for month, year, and day are printed in the same order. This would also need to account for punctuation usage before/after the placeholders.

@westonruter
3 weeks ago

@westonruter
3 weeks ago

@westonruter
3 weeks ago

#7 @westonruter
3 weeks ago

  • Keywords has-patch added; needs-patch removed

See 42394.0.diff for one possible solution. It's not perfect in particular because it isn't able to know whether the content between the date and the time is associated with the date or the time.

I think what we need is new translation strings for the date and time separately.

So in addition to:

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

We also need:

<?php
/* translators: 1: month, 2: day, 3: year, 4: hour, 5: minute */
$date_format = __( '%1$s %2$s, %3$s' );

/* translators: 1: hour, 2: minute, 3: meridian (optional) */
$time_format = __( '%1$s:%2$s %3$s' );

The meridian part gets tricky, however. Right now the meridian part is conditionally included based on whether or not it appears in the time_format option, and not whether or not it is in the translation string.

Maybe for 4.9 we should fix the issue specifically with the year/month/day fields, but then continue with the current hard-coded approach for the hour/minute/meridian fields, since the time format is much less likely to vary between locales (AFAIK).

#8 @westonruter
3 weeks ago

Or, instead of using the '%1$s %2$s, %3$s @ %4$s:%5$s' translation string we could instead use the date_format and time_format strings to decide the ordering of the fields.

#9 @Mirucon
3 weeks ago

Use of the date_format and time_format instead of translation string sounds good to me. At least in Japanese, the order of date is always the same (Y/m/d), but there are some of the formats. E.g. some prefer 2016年11月1日, but some prefer 2016/11/1 or 2016-11-1.

Should be their own choice rather than the locale.

Last edited 3 weeks ago by Mirucon (previous) (diff)

#10 @westonruter
3 weeks ago

@Mirucon @SergeyBiryukov Thoughts on 42394.1.diff?

#11 @westonruter
3 weeks ago

  • Owner set to westonruter
  • Status changed from new to accepted

#12 @Mirucon
3 weeks ago

  • Keywords needs-patch added; has-patch removed

@westonruter Order of the date is correct, but since that is just considered to have Y, y, o, F, m, M, n, j, d, if the format contains anything else of these string, the extra items will show up after the forms. The screenshot shows what happens with the default Japanese date format - Y年n月j日

@Mirucon
3 weeks ago

42394.1.diff - with format Y年n月j日

@Mirucon
3 weeks ago

42394.1.diff - with format Y年n月j日

#13 @Mirucon
3 weeks ago

Format for the first screenshot is actually d/m/Y :)

#14 @westonruter
3 weeks ago

  • Keywords has-patch needs-testing added; needs-patch removed

@Mirucon please clear your cache and try again. jp-date-format.png is what I see if I supply a date format of Y年n月j日 and it looks good to me.

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


3 weeks ago

#16 @jbpaul17
3 weeks ago

  • Keywords reporter-feedback added

#17 @atachibana
3 weeks ago

@westonruter @Mirucon

I confirmed the patch works successfully.

For the first time, I got the same error with @Mirucon, but minify CSS (customize-controls.min.css) had not been updated yet. After minify of it and clearing cache, it shows the date with correct format.

@atachibana
3 weeks ago

After 42394.1.diff, Customizer scheduled date in Japanese locale

#18 @atachibana
3 weeks ago

fyi.

Already Japanese Translation Team is discussing about the fix of duplicate '月'(=month) by modifying Japanese resources. After this fix was merged into the Core, they will work.

#19 @westonruter
3 weeks ago

  • Keywords dev-feedback added; needs-testing reporter-feedback removed

@Mirucon in the future you can avoid having to re-minify sources by adding define( 'SCRIPT_DEBUG', true ); to your wp-config.php

This ticket was mentioned in Slack in #core-committers by westonruter. View the logs.


3 weeks ago

#21 @Mirucon
3 weeks ago

Sorry, since that was a new environment, I forgot to add it! Thanks @atachibana, @westonruter

No, it was set - so the cache wasn't cleared properly.

Last edited 3 weeks ago by Mirucon (previous) (diff)

#22 @obenland
3 weeks ago

In 42394.1.diff, it doesn't look like $format is used anywhere.

@westonruter
3 weeks ago

Remove unused var

#23 @westonruter
3 weeks ago

@obenland good catch. Removed in 42394.2.diff.

@Mirucon
3 weeks ago

Scheduling a post

#24 follow-up: @Mirucon
3 weeks ago

This should be applied to posts as well. Shall I create a new ticket or is it handled here?

#25 in reply to: ↑ 24 @westonruter
3 weeks ago

Replying to Mirucon:

This should be applied to posts as well. Shall I create a new ticket or is it handled here?

I don't think that is necessary. Please review how the date is displayed in Gutenberg. It's an actual calendar picker component. That is what core will be moving toward, and the Customizer will likely re-use it instead of what it is including in 4.9, eventually.

#26 @obenland
3 weeks ago

  • Keywords commit dev-reviewed added; dev-feedback removed

42394.2.diff LGTM. Separators can look a little off at times, but I'm not sure this is something we have much control over

#27 @westonruter
3 weeks ago

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

In 42111:

Customize: Present the date/time control input fields in the same format (and order) as date_format.

Props westonruter, obenland, atachibana for testing, Mirucon for testing.
See #42373, #28721.
Fixes #42394.

@dlh
2 weeks ago

#28 @dlh
2 weeks ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

If date_format is set to a custom string without at least one of the month, day, or year tokens, then the Publish Settings are knocked out of whack, as illustrated in the above screenshot with a date_format of Y-m.

42394.3.diff includes one possible workaround of falling back to the previously default date format if any of the three placeholders are missing.

@westonruter
2 weeks ago

#29 @westonruter
2 weeks ago

  • Keywords dev-feedback added; dev-reviewed removed

@dlh Good catch, though I wonder how many sites would not include all three parts to the date.

I think, however, that the fallback date format should actually be ISO date format: Y-m-d.

See 42394.4.diff which also ensures that each format character occurs once and only once.

I also realized the foreach loops could be eliminated.

@Mirucon
2 weeks ago

#30 @Mirucon
2 weeks ago

What if it is fallback to locate default?

#31 @obenland
2 weeks ago

  • Keywords has-patch commit dev-feedback removed

#32 @westonruter
2 weeks ago

  • Keywords has-patch added

@Mirucon I think the point of using ISO as fallback is that it would be an international format and so it wouldn't need to be translated. Otherwise, if it is translatable then it should maybe it should be what I have in comment #7:

<?php
/* translators: 1: month, 2: day, 3: year, 4: hour, 5: minute */
$date_format = __( '%1$s %2$s, %3$s' );

#33 @dlh
2 weeks ago

42394.4.diff works well for me.

I agree that a date_format without one part of the date would be unusual. The example I was thinking of at the time was a monthly magazine where the day on which a post was published would be unimportant to the site.

That said, if a translated string like __( '%1$s %2$s, %3$s' ) is an option, then is matching the fields to date_format necessary?

#34 @westonruter
2 weeks ago

@dlh you're right. If __( '%1$s %2$s, %3$s' ) is available then parsing date_format would not be necessary. I was trying to get by without introducing a new translation string, but if translators are happy with having a locale date format string to translate, we can just use it instead. @Mirucon @atachibana thoughts?

This ticket was mentioned in Slack in #core-i18n by westonruter. View the logs.


2 weeks ago

#36 @atachibana
2 weeks ago

@westonruter

I believe translators will be happy to do it.
In fact, as @SergeyBiryukov said in comment:5, Edit Post screen has such template string for some time.

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


2 weeks ago

#38 @obenland
2 weeks ago

  • Keywords commit dev-reviewed added

We're in RC and with that in string freeze—let's try to avoid introducing new translation strings.
Given how unusual we all think this use case is, I think we should go with 42394.4.diff for now.

#39 @westonruter
2 weeks ago

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

In 42117:

Customize: Fall back to ISO date format for date/time control inputs if year, month, or day are missing from the date_format option.

Amends [42111].
Props dlh, westonruter.
Fixes #42394.

Note: See TracTickets for help on using tickets.