EasyAdminBundle icon indicating copy to clipboard operation
EasyAdminBundle copied to clipboard

Add step attribute to time widgets to fix Safari bug

Open andyexeter opened this issue 2 years ago • 3 comments

My attempt at fixing #5309

This PR adds a step attribute with a value of 1 (seconds) to HTML5 datetime/datetime-local/time widgets whose formatted value contains seconds.

This resolves a bug in Safari where a form cannot be submitted as described in the linked issue.

andyexeter avatar Jun 23 '22 12:06 andyexeter

Thanks for creating this PR to fix the issue.

I was playing with the PR and found something strange. I just added ->setFormat('short') to a DateTimeField and I saw this:

datetimefield-with-seconds

The field doesn't have anything else configured explicitly, so this is the default behavior. But, in your code we have the DateTimeField::FORMAT_SHORT !== $timeFormat condition, so I don't know why this activates the change.

Just asking ... my guess is that it's uncommon to need to select seconds in a datetime widget. What if we add instead a explicit method to render seconds too (e.g. ->withSeconds(), ->showSecondsSelector(), ->withSecondPrecision(), etc.)

javiereguiluz avatar Jun 23 '22 18:06 javiereguiluz

I think it's activating the change because when you call ->setFormat('short'), the timeFormat parameter is omitted and so defaults to none:

https://github.com/EasyCorp/EasyAdminBundle/blob/7acf93529b57188ed4d4258d8e701dea6120838e/src/Field/DateTimeField.php#L86

I've updated the if statement so that we make sure $timeFormat is not none or short, rather than just short.

The problem with adding an explicit method to render seconds is that this will require developers to make changes to their application code to fix the issue. I agree that it is probably uncommon to need to select seconds, but currently if you have a DateTimeField mapped to a property whose default value is the current time, then the form cannot be saved in Safari unless the time happens to have no seconds elapsed (e.g 10:15:00)

It would be nice if we could provide a fix which wouldn't require any developer effort after a composer update :) WDYT?

andyexeter avatar Jun 26 '22 20:06 andyexeter

I realised that Symfony's DateTimeType field already has a with_seconds options which when true, adds the step attribute with a value of 1. I've updated the PR to use this instead of adding the attribute ourselves.

Having thought about this issue a bit more, I have realised that the format is actually irrelevant when using an HTML5 date/time widget - it doesn't affect what is displayed in the widget.

Given that the seconds input currently appears when the field has a value which contains seconds, if we were to add an explicit ->withSeconds() I believe we would need to change the default behaviour to strip seconds from the field's value. Would this be a BC break?

andyexeter avatar Jun 27 '22 08:06 andyexeter

Closing as this issue has been fixed in Symfony 6.3 (https://github.com/symfony/symfony/pull/50059)

andyexeter avatar May 31 '23 09:05 andyexeter