WordPress.org

Make WordPress Core

Opened 3 weeks ago

Closed 3 weeks ago

Last modified 3 weeks ago

#42373 closed defect (bug) (fixed)

Customizer: Can't Schedule 0:00 - 0:59 when 24-hour clock setting

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

Description

In the Customizer of Version 4.9 Beta 4, I can't specify between 0:00 - 0:59 for Schedule when settings is 24-hour clock.

Reproduce Step

Preparation

  1. Administration Screen -> Settings -> General
  2. Select 12:xx / 'H:i' (3rd option) at Time Format
  3. Click Save Changes

Customizer

  1. Start Customizer
  2. Click Settings
  3. Select Schedule
  4. Input '0' at hour field in Time.

Then 'Invalid hour' is displayed and can't specify it.

Attachments (8)

49beta4_customizer_schedule.jpg (16.8 KB) - added by atachibana 3 weeks ago.
'Invalid hour' for 0 in 24-hour time (= 12 am in 12-hour time)
42373.diff (1.2 KB) - added by Presskopp 3 weeks ago.
patch-42373_customizer_schedule.jpg (17.1 KB) - added by atachibana 3 weeks ago.
After patch applied, even 12-hour clock, 12:30 is warned.
42373.2.diff (1.4 KB) - added by Presskopp 3 weeks ago.
42373.3.diff (1.5 KB) - added by peterwilsoncc 3 weeks ago.
42373.4.diff (7.8 KB) - added by westonruter 3 weeks ago.
42373.5.diff (8.7 KB) - added by westonruter 3 weeks ago.
42373.6.diff (8.7 KB) - added by westonruter 3 weeks ago.
Add radix to parseInt

Download all attachments as: .zip

Change History (27)

@atachibana
3 weeks ago

'Invalid hour' for 0 in 24-hour time (= 12 am in 12-hour time)

#1 @Presskopp
3 weeks ago

confirmed, with any time setting

#2 @Presskopp
3 weeks ago

@westonruter

Looks like this can easily get patched changing L.145 in

class-wp-customize-date-time-control.php to

<input id="{{ idPrefix }}date-time-hour" type="number" size="2" autocomplete="off" class="date-input hour" data-component="hour" min="0" max="{{ maxHour-1 }}">

#3 @westonruter
3 weeks ago

  • Milestone changed from Awaiting Review to 4.9
  • Owner set to westonruter
  • Status changed from new to accepted

@Presskopp
3 weeks ago

#4 @Presskopp
3 weeks ago

  • Keywords has-patch added

I'm now quite sure that's it - here the patch

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

#5 @atachibana
3 weeks ago

@Presskopp @westonruter

I tested the patch.

For 24-hour clock, it is OK.

But even 12-hour clock, '0' have to be specified instead of '12'. For example, '0:30 AM' for 30 min past midnight.
I'm not familiar with 12-hour clock in US/EU, but it won't be acceptable...

@atachibana
3 weeks ago

After patch applied, even 12-hour clock, 12:30 is warned.

@Presskopp
3 weeks ago

#6 @Presskopp
3 weeks ago

Uh, yes, maybe not in the most elegant way, but I changed the patch and it should be fixing it now

#7 @atachibana
3 weeks ago

@Presskopp Thank you for the fix!
I tested below cases and could conduct all successfully.

T/C 01 : 24-hour clock

1) 0:0
2) 0:59
3) 23:59
4) 24:00 - should be error 'Invalid hour'

T/C 02 - 12-hour clock

1) 0:00 -> If you type '0', then automatically converted to '12'. It is good.
2) 12:00
3) 12:59
4) 13:00 - should be error 'Invalid hour'

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

@westonruter
3 weeks ago

#8 @westonruter
3 weeks ago

I found some additional issues with date time control.

  • Min/max could be simplified, as @peterwilsoncc patched in 42373.3.diff.
  • Setting validity message was not properly translatable.
  • Emptying an input or giving it an invalid value would not trigger an error to keep the customizer from saving (silently accepted).
  • Zero-padding was getting in the way of user input.

I don't really see the need for updateMinutesForHour. The max for minutes can be just always 59. I don't think we need to worry about supporting 24:00.

@westonruter
3 weeks ago

#9 @westonruter
3 weeks ago

42373.5.diff also removed updateMinutesForHour.

@atachibana @Presskopp Would you please review and test this?

#10 @peterwilsoncc
3 weeks ago

@westonruter ran some manual tests:

  • Date to November 31
    • Browser shows warning on day box
    • Invalid date warning notification shows
    • Schedule button blocks saving
  • Change month to October (ie schedule for October 31)
    • Browser warning disappears
    • Invalid date notification disappears
    • Schedule button works.
  • Change month to November
    • Day changes to 30


  • Refreshing customiser after scheduling shows correct date/time.
  • Scheduling for minute [5], as opposed to [05], works and refreshing will display the leading zero

#11 follow-up: @atachibana
3 weeks ago

@westonruter
Comment #7 test cases were confirmed, except no more automatic conversion for typing '0' to '12'. (and, This is OK).
Also, Leading zero function is beautiful :-)

One confirmation about UI.
Pop up error message such as 'Invalid hour' is shown only when the first error occurred.
Is it as your expectation? If so, then OK.

Step
1) Type '24' in hour field. 'Invalid hour' will be popup and border color be Red.
2) Type '2' in hour field. Border color will be Blue.
3) Type '24' in hour field, again. Border color is Red, but no popup message.

#12 in reply to: ↑ 11 @westonruter
3 weeks ago

Replying to atachibana:

Pop up error message such as 'Invalid hour' is shown only when the first error occurred.
Is it as your expectation? If so, then OK.

Yes, only the first is supposed to be flagged as an error.

Step
1) Type '24' in hour field. 'Invalid hour' will be popup and border color be Red.
2) Type '2' in hour field. Border color will be Blue.
3) Type '24' in hour field, again. Border color is Red, but no popup message.

An error notification should appear if you tab out of the hour field.

#13 @atachibana
3 weeks ago

@westonruter

Thank you for the explanation. I confirmed.
Now, I can schdedule my theme change at midnight :-)

#14 @westonruter
3 weeks ago

  • Keywords commit added

I'll commit this in ~10 hours unless someone finds something amiss before then.

@westonruter
3 weeks ago

Add radix to parseInt

#15 @obenland
3 weeks ago

LGTM!

An enhancement for a future release could be to in/decrement hours and days while looping through minutes/hours.

#16 @westonruter
3 weeks ago

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

In 42042:

Customize: Allow 0:00-0:59 in date/time control when 24-hour time used.

  • Let min hour be 0 and max be 23 in 24-hour time; let min hour be 1 and max be 12 in 12-hour time.
  • Show error notification when an invalid date value is provided, not just when not a future date.
  • Fix translation of custom validity message.
  • Start checking for validity after all inputs have been initially populated.
  • Remove support for being able to enter 24:00.
  • Cease forcing date input elements from being casted to integers, to allow for invalid inputs to be detected.

Props westonruter, Presskopp, peterwilsoncc, atachibana for testing.
See #39896, #28721.
Fixes #42373.

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


3 weeks ago

#18 @westonruter
3 weeks ago

In 42104:

Customize: Strip zero padding when setting month to prevent de-selection; add zero-padding for number input fields when blurring.

Amends [42042].
See #42373.
Fixes #42392.

#19 @westonruter
3 weeks ago

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.

Note: See TracTickets for help on using tickets.