backdrop / backdrop-issues

Issue tracker for Backdrop core.
144 stars 40 forks source link

Add new translatable HTML5 date form elements: html_datetime, html_date, html_time #3972

Closed indigoxela closed 4 years ago

indigoxela commented 5 years ago

Description of the bug If the strings "am" and "pm" (context ampm) have translations in the currently used language, these translations prevent date validation of node "Authored on" and "Publish on" date.

Steps To Reproduce To reproduce the behavior:

  1. Install an additional language in Backdrop
  2. Go to admin/config/regional/translate and translate "am" and "pm" (if there's no translation yet)
  3. Make sure, the core date module is enabled (it is by default)
  4. Add or edit a node on the additional language (not "en") and try to save it

Actual behavior Error for german (as an example): Die Werteingabe für das Feld ist ungültig: The date "2019-08-08 01:11 vormittags" does not match the expected format.

The problem is that "am" got translated to "vormittags" and of course this won't validate. A date string like "2019-08-11 08:24 vormittags" will always return FALSE in strtotime().

Expected behavior I'd expect that either the "Authored on" date isn't translated or a proper date format for current language is used, that validates.

Additional information

Validation fails in class BackdropDateTime

The date pattern in use is defined in core/modules/node/node.pages.inc, hardcoded to 'Y-m-d g:i a'.

My current workaround is to delete the translations for am/pm via translation interface. Translating it to am/pm is another option.

Same bug in Drupal Here's the related Drupal date module Issue. Please note that the provided patch there doesn't fix the problem.


PR by @indigoxela adding html 5 date input https://github.com/backdrop/backdrop/pull/2852

~PR by @klonos (crossport of the patch in https://www.drupal.org/project/date/issues/2359653#comment-9393607): https://github.com/backdrop/backdrop/pull/2813~ [Crossing out since it seems that testing was primarily focused on @indigoxela's PR.

olafgrabienski commented 5 years ago

@indigoxela Thanks for your report! Do you know if it happens in Drupal 7 as well?

klonos commented 5 years ago

Yup, known issue with the 7.x Date module: https://www.drupal.org/project/date/issues/2359653 (still open - since 2014 😞)

klonos commented 5 years ago

I have crossported the current patch from that d.org issue in the Date 7.x issue queue (comment #8), with a small change as it made sense. Tests are green, but that does not necessarily mean that there won't be any other problems 😅

@indigoxela @olafgrabienski since you guys seem to have some multilingual sites, can you pease field-test this and see if it solves the issue without causing any other problems? It would be great if you applied the patch/PR to one or more sites, and tested for a period; then report back any issues.

Thanks in advance.

indigoxela commented 5 years ago

@klonos I've grabbed your branch "patch-185". Things became worse unfortunately. Now I see two error messages per node, one for the current date and one for the date from "Authored on". (I did flush cache and double checked, your changes are in.)

Honestly - I don't understand in how far the crossported patch should help preventing the unwanted translation of "am" and "pm" anyway. (If that's what you were after)

indigoxela commented 5 years ago

Something else that might need discussion:

Drupal 7 uses an ISO Format for node dates, but it adds some explanation, how to use it.

In Backdrop it's a popup calendar (very comfortable!), but with a localized date format for U.S. ("Y-m-d g:i a"), which might need some explanation outside U.S.

Would it be possible to use a configurable (localized) date format for node "Authored on" dates? Does someone else think, this would be beneficial?

oadaeh commented 5 years ago

I think it would be beneficial to be able to specify the date format for the Authored on field (much like being able to specify the date format for the log dates). I hacked core on one site in order to set a date format the way I needed it. I was going to get around to creating an issue and patch eventually, but at the time, I was trying to solve a different problem and the date format was stopping me. However, I think a customizable date format is a different issue than this one.

klonos commented 5 years ago

@oadaeh we have a contrib module for that: https://github.com/backdrop-contrib/submitted_by ...and an issue to get that feature in core (with a working PR by @BWPanda): #2379 😉

oadaeh commented 5 years ago

I did not know about those, but they look like they only affect the display and not the form, but that was just a quick look. I'll take a longer look a little later on. Thanks @klonos.

indigoxela commented 5 years ago

@klonos I agree with @oadaeh - the submitted_by contrib module and BWPanda's PR are about display (with tokens), not validation. Valuable, but off topic here.

The point here is that am/pm get translated in node forms before validation and that can't validate because a date string like for instance "2019-08-11 08:24 vormittags" will always return FALSE in strtotime().

Validation happens in core/includes/date.class.inc, function __construct().

One thing I really don't understand: why is am/pm translated anyway. In which context does it ever make sense to translate a part of a localized date format? Am I missing something?

indigoxela commented 5 years ago

I've created an extra issue for localized input formats (unrelated to this validation bug). https://github.com/backdrop/backdrop-issues/issues/3976

Regarding validation errors:

To my opinion am/pm shouldn't get translated at all, as they are part of a date format, but I might be wrong.

I tried to figure out, when and why this was implemented, but stranded somewhere in the drupal date module history. It has been there "forever", at least since drupal 6, possibly drupal 5. No hint on why someone thought, am/pm should get translated.

klonos commented 5 years ago

Right. I see what the problem is now. Trickier 🤔

indigoxela commented 5 years ago

Assuming, that am/pm never should get translated, I've created a pull request that fixes the validation errors.

Please note: this simple change really only fixes the validation problem. The code inherited from date module is sometimes obscure and there are still some kinky and likely obsolete code parts regarding am/pm handling.

klonos commented 5 years ago

Hmm 🤔 ...what is the logic of this PR though? If we are not showing the translated strings in the UI, then that may be considered a (multilingual) UX bug sometime down the road.

Although tricky, I believe that we should be doing something like this instead:

I have not tested the above, just brainstorming over what could work and be more future-proof.

indigoxela commented 5 years ago

what is the logic of this PR though

It simply removes t() from am and pm, as I don't see, where a translation of these date parts could ever be useful. If anyone knows such a use-case, I'm all ears. ;)

indigoxela commented 5 years ago

Ah, wait a minute... There seem to be some countries/languages using 12-hour clock system, where am/pm are translated. Some examples I've found: Colombia, Malaysia, Malta (not sure though), Arabic

Source of my info: https://en.wikipedia.org/wiki/12-hour_clock and https://github.com/unicode-org/cldr/tree/master/common/main

So we can't simply skip translation. On the other hand PHP can't handle these translated formats directly.

@klonos I'll be there for testing, as soon as you elaborate your hidden value approach.

klonos commented 5 years ago

There seem to be some countries/languages using 12-hour clock system, where am/pm are translated.

Yup; add Greek to the list of those languages.

I'll be there for testing, as soon as you elaborate your hidden value approach.

We may need someone more date-module-savvy to chime in re this, but to elaborate on my idea...

https://api.backdropcms.org/form_api#value

Used to set values that cannot be edited by the user.

I am speculating that we could be using such a hidden field/value, to store the actual value that gets saved in the db. We would then need to be translating only what is shown in the date widget + what is shown during render/display.

So the idea is that the user uses the widget (which has translated date format, including the am/pm) to pick/edit a date, and we then programatically update the #value field with the respective language-agnostic date value (timestamp?).

On display, we read the value of the field from the db, and update the widget accordingly, to show the translated format of the date (based on what language preference is set for the user).

I hope that all that makes sense.

indigoxela commented 5 years ago

We may need someone more date-module-savvy to chime in

That would be cool, but I'm not too optimistic about that. ;)

I am speculating that we could be using such a hidden field/value, to store the actual value that gets saved in the db

How can we transform the visible value to a parseable date string?

Class BackdropDateTime tries several ways to parse a date, but definitely gives up on our faulty format ("2019-08-11 08:24 vormittags" or the like).

IntlDateFormatter could be helpful, but that requires exact strings including the exact official translation for am/pm. I can provide some examples, if needed.

Then I thought about using a value callback in node forms, but I'm not sure yet, if we could fix the problem with it.

Anyway, it seems to me, we might need some time to get this complex issue solved. Shouldn't we warn translators in the meantime. Currently translations can break node date validation. We should do something.

klonos commented 5 years ago

Hmm, I still think that our best bet is to have the field widget somehow store a locale/language-agnostic date format, and only translate for render/presentation. I kinda have a feeling that this will need to happen in the Field/Form API level.

indigoxela commented 5 years ago

I still think that our best bet is to have the field widget somehow store a locale/language-agnostic date format, and only translate for render/presentation

Sure, let's give it a try. I have no clue, how to combine this with the datepicker, so it's on you to elaborate it.

Btw: If we're not able to solve this, I'd suggest to get back to the ISO format used by node dates, if date module is disabled.

indigoxela commented 5 years ago

Some more experiments with the node date fields...

A hidden field and showing something different still doesn't look like a feasible solution to me, at least not without bloating the code and/or relying too much on javascript.

I've experimented with the field type _datepopup (which we currently use), and '#datepicker_options' (namely altField and altFormat settings), but that's only for the date part. The timepicker (from date module) has no extra options and it's the time part, which causes the problem (11:15 am translates to 11:15 vormittags and php can't parse that).

Then I took a look, how D8 does it. It uses html5 input types (date and time). But this needs two different input tags to have a datepicker from browser. We'd need to change the node form fields including validation to use these.

Example: <input type="date" value="2019-09-02" /> <input type="time" value="09:22" /> becomes screenshot-html5-date-time-input in a "german speaking" Firefox. Note the localized date format.

To my opinion the easiest solution by far would be to change the pattern to 24-hour-system, as that isn't affected by translations.

olafgrabienski commented 5 years ago

To my opinion the easiest solution by far would be to change the pattern to 24-hour-system, as that isn't affected by translations.

To be clear, the proposal is for the time part of "Authored on" and "Publish on" dates in the node form, right? In my opinion, it's anyway a good idea to change the time to a 24-hour format which seems to be more common internationally. If it also fixes the validation issue, perfect!

indigoxela commented 5 years ago

I've created a pull request, that adds html5 input fields (type="date", type="time") to core. Feel free to play with it.

With this branch, node "Authored on" looks like this: screenshot-browser-popup-cal

The advantage of html5 input fields is, that the browser takes care of localization. And we don't need any javascript for that popup calendar.

I still think, switching to 24-hour-system is a valid option. And a quicker solution for sure, as my branch isn't ready yet.

Feedback is welcome!

herbdool commented 5 years ago

I think the html5 inputs make sense here. I did a quick test and it seems promising though still a work in progress like you mention.

indigoxela commented 5 years ago

Some progress in my PR. Some tests are still failing and I struggle hard with page preview and lost values. So it's still incomplete.

olafgrabienski commented 5 years ago

@indigoxela Great to hear! Let me know when the PR is ready for testing it via the user interface.

indigoxela commented 5 years ago

Tests are passing now. The change is bigger, than I thought, so there's a lot to review.

Feedback is welcome.

indigoxela commented 5 years ago

Anyone willing to test or review? ;)

indigoxela commented 5 years ago

Some info for possible testers: While the date and time input fields are widely supported nowadays, there are some exceptions, most notably Safari on Mac.

See: https://caniuse.com/#feat=input-datetime

We will need some sort of fallback anyway - for older browsers. We could use the html5 placeholder attribute for that. In browsers without support for date/time input, the form field degrades gracefully to type "text" and that supports placeholders. This could be a follow-up issue.

olafgrabienski commented 5 years ago

@indigoxela I've had a quick look at the PR's sandbox site, in Chrome and in Safari on Mac OS X. The handling in Chrome is good. In contrast, the text input in Safari could indeed need placeholders, or a short help text below, with information about (or an example of) the input patterns.

I've noticed one strange issue: When I create a post and set the the time in Chrome to something like 20:30:00, I can't save the post but get the following mesage:

  • 20:30 is not a valid time.
  • You need to set both, date and time.

When I try the same in the Safari text input, everything is okay, and I'm able to save the post. Then, when I edit the post in Chrome again and don't change anything, I'm able to save the post. But if I edit the post in Chrome again and change the time, I get the message again and can't save.

It looks like the issue happens if the seconds are set to 00.

Apart of that, I'm not sure what else should be tested.

indigoxela commented 5 years ago

@olafgrabienski many thanks for taking a look.

Regarding the Chrome quirk. It seems to ignore the "step" attribute, if seconds are zero. I need to dig a bit deeper, thanks for reporting. What's your Chrome version?

indigoxela commented 5 years ago

I've updated my pull request to prevent problems, when browsers omit seconds.

Added placeholder attribute for node->date and node->scheduled

olafgrabienski commented 5 years ago

@indigoxela Looks good to me in the sandbox! The issue with seconds in Chrome is fixed for me, and in Safari there are placeholders, good!

klonos commented 5 years ago

...it's anyway a good idea to change the time to a 24-hour format which seems to be more common internationally.

I still think, switching to 24-hour-system is a valid option. And a quicker solution for sure ...

I think the html5 inputs make sense here. ...

I think it does make sense too. It is a quicker solution for sure, but in Australia for example, they call this "military format" and people don't use it - most get confused by it.

See: https://caniuse.com/#feat=input-datetime

The general percentage that is green is close to 80%, which matches our 2nd principle: "Simplicity: Write code for the majority."; so 👍 from me.

So in general, I am not opposed to this change; although it would be nice to see if we can use some shim/polyfill. I would be more confident with this change if we did that. See:

https://github.com/zoltan-dulac/html5Forms.js https://code.tutsplus.com/tutorials/quick-tip-create-cross-browser-datepickers-in-minutes--net-20236

...I still need to test/review before I can provide any actual feedback.

indigoxela commented 5 years ago

Regarding a possible fallback for browsers neither supporting the date/time input nor the html5 placeholder:

I'm not really happy with it, if this means we need another polyfill/shim js library in core. And finding one that fits our need (localized input, but outputs php-parsable formats) will be hard.

The placeholder attribute has good support in all recent browsers: https://caniuse.com/#feat=input-placeholder

Web accessibility might be an issue. I'm no expert regarding this, so I'm open to suggestions. In general my impression is that the date/time input fields were appreciated. This doesn't mean, their support in clients (e.g. braille) is good.

indigoxela commented 5 years ago

It seems, this issue/solution needs an advocate... ;-)

Why date/time input fields in core are a good idea:

Sooo.... is there any volunteer to review the PR?

herbdool commented 4 years ago

@indigoxela I'm going to try review your PR.

I've crossed-out @klonos PR to reduce confusion. It seems like it is not the preferred solution going by the comments.

herbdool commented 4 years ago

@indigoxela looks pretty good. I've left a review https://github.com/backdrop/backdrop/pull/2852#pullrequestreview-310169146

I'm not that concerned about browser support since there is a fallback to a regular textfield, which while not ideal, at least means it's still usable. I would really not want to add a shim.

indigoxela commented 4 years ago

@herbdool you're my hero! Thanks for having a look. I've updated my PR and wait, what your further review yields.

herbdool commented 4 years ago

@indigoxela thanks for your work on this! I think it's ready for final look by @quicksketch.

Even though this is a bug, I'm thinking it might be too much of a dramatic change for a bug fix. I think it can go in at the next minor release 1.15.x. We'll see what others think.

quicksketch commented 4 years ago

It seems, this issue/solution needs an advocate... ;-)

Could that be you @indigoxela? I agree with @herbdool this change is far-reaching and could only go in 1.15.0.

This looks looks really good in general. I'm actually a bit surprised we don't have these HTML5 date elements supported officially in core already.

As this makes the date elements in node.module use the new HTML5 elements, it makes me want to have them supported by date.module in fields as a supported widget type. I'd like to make it so that at least it's possible to have all date fields act the same way on the node form if you add fields to the form. Right now that's possible if you use the "popup" widget, then all date fields work the same way.

quicksketch commented 4 years ago

Looks like we need some styling in the Seven theme to make these input elements match the display of other elements. Note the difference in padding inside the date fields and the thin border compared to the "Authored by" text field above:

image

We also need test coverage for the validation of all these new input types.

indigoxela commented 4 years ago

this change is far-reaching and could only go in 1.15.0.

I agree, 1.15.0 is a good choice. It's a bug, but an easy workaround exists.

Looks like we need some styling in the Seven theme to make these input elements match the display of other elements

Any volunteers for that? ;-)

We also need test coverage for the validation of all these new input types.

Currently there are the tests for node date fields (authored / publish). Are there any suggestions or hints?

And we also need documentation in our form API reference, like there is for the old date field (combined select lists). Who has access? How can I help?

indigoxela commented 4 years ago

I've updated my PR - added some tests and while I was writing them I realized that I need to improve validation :wink:

As I've never worked on seven theme, I'd be glad if someone else could take over the theme job.

I'm not sure, if the PR is now ready for a next review or it still "needs work". @quicksketch what do you think?

olafgrabienski commented 4 years ago

@indigoxela I can try to adapt the look in the Seven theme if nobody beats me to it. I won't have time to work on it immediately, but for 1.15 we have some time.

herbdool commented 4 years ago

Olaf you'll have to take a look at the date field in different browsers for theming since they all implement the widget a bit differently. Though looks like you can just focus on the padding and border.

I believe the API documentation will be updated from the code base so we just need to make sure we have updated the docblocks.

If you think it's ready for another test/review you can change the labels.

herbdool commented 4 years ago

I'm wondering if having this work for all date fields is something we want to tackle here or have a follow-up issue. It depends partly on whether we think we can get it done before the next release.

serundeputy commented 4 years ago

i'd vote for a follow up issue to bring this out to the date fields provided by date module.

indigoxela commented 4 years ago

I strongly recommend to do further core module implementations in follow up issues. This one is already a big PR. If we add even more, it will get huge. Furthermore it would be possible then that different people work on different tasks (comment, date, theme(s)).

@herbdool I'm afraid, the API documentation won't get updated automatically. Automating this could be a follow-up issue as well. In an unrelated issue it was already discovered that the form API docs lag behind a bit.

@olafgrabienski glad you're taking over. You're for sure more familiar with seven/basis than I am.

indigoxela commented 4 years ago

Some tiny improvements in my PR. It's now possible to use the time field without seconds (only hour:minute). But now I really stop working on it and wait for the next review. ;-)

olafgrabienski commented 4 years ago

I can try to adapt the look in the Seven theme

As a start, here is a comparison of how the "Authored on" input padding and borders looks like (in Chrome on Mac OS):

Current core:

screen-backdrop-authored-on-current

New, with the PR:

screen-backdrop-authored-on-new