Problem/Motivation
Implement the strategy we have in #1252178: Add Modernizr to core for date inputs.
Some browsers doesn't support the HTML 5 date input, so we need to add a JS fallback to support those.
This should improve the usability for people using there browsers (like firefox, IE) dramatically, as all people expect to get support entering a date with the correct format in a date field.
Note this issue it not about the design of the date picker in various core themes.
Proposed resolution
Add a JS falback using modenizr
Remaining tasks
User interface changes
API changes
Data model changes
Testing
Before patch
Use a browser that doesn't support html5 date input fields like firefox, result no datepicker available.
After patch
Use a browser that doesn't support html5 date input fields like firefox, result datepicker is now available.Use a browser that support html5 date input fields like Chrome, result native datepicker is still being used.
Beta phase evaluation
Issue category | Task |
---|---|
Issue priority | Major because this is really important for end users |
Prioritized changes | The main goal of this issue is usability |
Comment | File | Size | Author |
---|---|---|---|
#53 | interdiff.txt | 2.58 KB | nod_ |
#53 | core-js-date-polyfill-1835016-53.patch | 5.43 KB | nod_ |
#51 | interdiff.txt | 2.12 KB | googletorp |
#51 | polyfill_date_input_type-1835016-51.patch | 5.34 KB | googletorp |
#42 | failing.png | 29.08 KB | droplet |
Comments
Comment #1
nod_Parts of the patch from #501428-50: Date and time field type in core. Parts of it are missing apparently, wasn't too careful during the extraction of this changes.
Comment #2
KarenS CreditAttribution: KarenS commentedThat patch is missing things, it's more like this.
Comment #3
KarenS CreditAttribution: KarenS commentedActually that's still missing things, it's this.
Comment #4
ruplMarking this active since #1252178: Add Modernizr to core is pending!
Comment #5
KarenS CreditAttribution: KarenS commentedAnd #1252178: Add Modernizr to core is committed! I'm not quite sure how to implement this, I'm hoping someone can provide a patch or example.
Comment #6
ruplI can certainly help with the Modernizr part! Since this looks to be mainly JavaScript changes, you can access the relevant test by writing a conditional like so:
Or if you only want to cover the NOT:
You can see the full list of support within your browser by going to your browser's JS console and evaluating
Modernizr.inputtypes
. It will return an Object full of booleans corresponding to each test result. Here's the output I got from Chrome 23.0.1271.64 on OSX:Comment #7
klonos...coming from #504524: Extend Authored on field with jQuery UI Date Picker
Comment #8
Mark TrappAlso coming from there. I've been maintaining Date Popup Authored, which was borne out of that issue, for d6 and d7, but all it really does is replace the Authored On textfield element with (contrib) Date's date_popup element (plus a few extra things to ensure Drupal gets the correct input).
Looks like this pretty much obsoletes the module for D8, am I right?
Comment #9
KarenS CreditAttribution: KarenS commentedSplitting the authored on field into HTML5 date and time elements is covered in #501428: Date and time field type in core. If the polyfill is added we'll also have a fallback to the jQuery datepicker. So yes, I think this will obsolete the Date Popup Authored module.
Comment #10
sunComment #11
webchickHm. I think this (or another issue that covers this) might be major, or possibly critical...
Here's what a datetime field looks like on Chrome:
So far, so good...
But here's what it looks like in Firefox:
There's absolutely no guidance about what to do until you submit the form and get an error message. :(
Also, since there's a patch here, marking needs review.
Comment #12
nod_This should user Modernizr for feature detection.
And see #2088383: Datetime FAPI DX for the whole date input thing.
(and yay for collapsed text format!)
Comment #13
klonosComment #14
Sharique CreditAttribution: Sharique commentedTrying to add jquery popup widget for date.
Comment #15
Sharique CreditAttribution: Sharique commentedThis is not working perfectly, I wasn't expecting tests to be passed, hoping failed test might be help find right path.
Comment #16
Sharique CreditAttribution: Sharique commentedHere is updated patch, atleast jquery popup is working now.
Comment #17
googletorp CreditAttribution: googletorp at Reveal IT commentedI've reposted the patch from #2467351: Add jQuery UI datepicker for the Date element of the datetime field here.
I've taken a very different approach than the current patch. The idea is to add a JS fallback to all date fields instead of creating a separate widget for it.
Comment #18
GaëlG#17 works well on Firefox, and Chrome behavior is unchanged (Chrome natively handles the input type date). We needed #2467449: jQuery UI datepicker styles broken in Seven for datepicker styling.
There is no timepicker for hours and minutes, but I guess this is another issue?
Comment #20
googletorp CreditAttribution: googletorp at Reveal IT commentedThere is a separate issue for the time #1838234: Add jQuery Timepicker for the Time element of the datetime field.
Comment #21
googletorp CreditAttribution: googletorp at Reveal IT commentedComment #22
nod_Sorry not there yet. New direction looks good though.
We don't use $.each, use a filtered for loop, better yet, see below for using data attribute.
The behavior needs a detach function
I would do the format translation in the JS side, in case someone wants to override the jquery ui lib and use their own stuff that has a different time format format.
We're trying to get rid of ids in the JS, can you put the date format in a
data-drupal-date-format
attribute (or something with a better name), like we're doing with #states? (see states.js and thedata-drupal-states
attribute).Thanks!
Comment #23
googletorp CreditAttribution: googletorp at Reveal IT commented@nod_ Thanks for the review, I'll look into this
Comment #24
googletorp CreditAttribution: googletorp at Reveal IT commentedI've followed the guidelines in #22, putting the settings on the element and adding a detach method to the behavior.
Comment #29
googletorp CreditAttribution: googletorp at Reveal IT commentedSo test bot is finally working again - ready for review :)
Comment #30
nod_Still an eslint error:
I've looked up the HTML5 spec, the format is hardcoded for the date field, no need to make it customizable or anything. We can just hardcode it
Let's inverse this and reduce nesting:
Since we don't need to have a configurable date format (the HTML5 spec say that the format is always yyyy-mm-dd) we can select
input[type="date"]
. It's also more in the spirit of a polyfill than relying on a data attribute.you're reusing
$(this)
5 times in the worst case. Let's put that in a variable like$input = $(this)
and use that afterwards.Where does this class comes from? I would expect jquery ui to remove it on destroy.
We have an api for that stuff:
Makes me remember you need to use .once in the attach function.
Comment #31
googletorp CreditAttribution: googletorp at Reveal IT commentedRegarding 1 + 3
Date fields, in core or contrib might allow that you enter the date in more than one way. Eventhough the format is hardcoded in html5 date fields, the format the the end users will see is localized. This means that the JavaScript fallback will stick out to the end users.
If we want this to be usable, we need to make it flexible. As I already have made one Drupal 8 site, where I implemeted a fallback JS datepicker, I know that this is a real requirement, which is important for some site owners.
I know we can save a few lines of code by not supporting this, but seeing how few countries actually support the Y-m-d date format (http://en.wikipedia.org/wiki/Date_format_by_country) I vote for making it configurable.
I've addressed the other issues in the review.
Comment #32
googletorp CreditAttribution: googletorp at Reveal IT commentedComment #34
googletorp CreditAttribution: googletorp at Reveal IT commentedSeems patch from #31 was an interdiff as well - uploading the correct patch instead.
Comment #35
googletorp CreditAttribution: googletorp at Reveal IT commentedComment #36
googletorp CreditAttribution: googletorp at Reveal IT commentedI fixed a few lint issues, now I'm finally using the proper Drupal ESLint conf.
Comment #38
googletorp CreditAttribution: googletorp at Reveal IT commentedSomething went haywire in the last patch, so fixed that. Also fixed array using the short syntax instead.
Comment #39
mgifford@googletorp - can you describe how to test this?
Unfortunately, it needs a reroll.
Comment #40
Nitesh Sethia CreditAttribution: Nitesh Sethia as a volunteer commentedRerolled the patch successfully as per the latest D8 release.
Comment #41
googletorp CreditAttribution: googletorp at Reveal IT commented@mgifford To test this, you need to use a browser which doesn't support HTML 5 date fields and edit a date field (fx node creation time).
Before the patch, you need to enter the date by hand, after the patch the jQuery datepicker should be used. You should also test that the jQuery datepicker is not used for browsers that do support native HTML5 date field.
Firefox doesn't support, but chrome does (last time I tested)
Comment #42
droplet CreditAttribution: droplet commentedI'm an unluckily developer, my clients always blame me and asked "WHY THIS IS DIFFERENT STYLE!!!"
We should add an option here to save all unluckily developers
Comment #43
mpdonadioThis will need an IS and Beta update to it before we can proceed to RTBC. How to test will also need to be added to the IS.
Comment #44
googletorp CreditAttribution: googletorp at Reveal IT commentedComment #45
googletorp CreditAttribution: googletorp at Reveal IT commentedComment #46
googletorp CreditAttribution: googletorp at Reveal IT commentedAdded beta evaluation, and better issue summary including how to test.
Comment #47
mpdonadioThanks @googletorp. I'll do manual testing tonight, but I think this is good to go and will improve UX.
@droplet, what did you have in mind? I think normalizing the appearance is definitely out of scope for this issue, though.
Comment #48
andypostit makes sense to file follow-up for "time" that firefox still have no support
Comment #49
mpdonadioOK, got a close look at this. The patch works well, and I think this will be a good addition. I verified it with fields from the DateTime module, as well as timestamp fields (eg, created on entities), and it works as described. Since this works w/ DateTime uninstalled on timestamps only, having the JS live in core/misc/ is a good idea.
I am a little confused why this was there in the first place, but why does the patch remove this line?
I think this comment is misplaced. Nothing in getInfo() changed. Perhaps this should be added to processDate() instead?
The comment here is a little cryptic. JS support for what? Maybe when combined with the comment above, a description can be added to processDate, that reads something like "If the element is an HTML5 date a date format is provided, then the necessary attributes are added to enable the jQuery UI Datepicker on the element."
Should be easy to fix, and then I think this is RTBC.
Comment #50
mpdonadioStill trying to triage all of these issues, some of which are pretty old...
But re #48 we already have #1838234: Add jQuery Timepicker for the Time element of the datetime field.
Comment #51
googletorp CreditAttribution: googletorp at Reveal IT commented@mpdonadio Thanks for the feedback.
#1 I think this is an error, nice catch, reintroduced the deleted line.
#2 I reworded the comment a bit, to make it more clear.
#3 Same as 2, tried to make the comment a bit more clear.
Comment #52
mpdonadioI think this is good to go. Manual testing w/ Firefox 38.0.5 on OSX 10.10 (latest version as of 2015/06/24) was fine. Removed a bit from the IS that doesn't seem to apply to the patch anymore (datepicker in Seven works fine for me).
Comment #53
nod_updated the docs according to documenting behaviors (it's brand new, you didn't miss anything :)
Used .once instead of classes, since that's what should be used to do this. and simplified a bit the use of dateFormat in the js.
Comment #54
googletorp CreditAttribution: googletorp at Reveal IT commentedI've tested this manually and reviewed the patch changes, looks good to me, RTBC imo.
Comment #55
alexpottCommitted b04d55e and pushed to 8.0.x. Thanks!
Thanks for adding the beta evaluation to the issue summary.
Comment #57
TR CreditAttribution: TR commentedThis change is causing the testbot to generate some exceptions for my tests:
"Undefined index: type Notice Date.php 58 Drupal\Core\Render\Element\Date::processDate()"
I don't see any mention above about API changes that would require me to do something different in my own code that uses a date form element, and I don't see anything above that mentions impact on contrib. (And in fact there's really no documentation on the date form element to begin with ...) processDate() was added by this patch, thus it's not something I invoke directly.
So maybe a change record to tell contrib developers what they need to change because of this patch?
Comment #58
mpdonadioCreated followup #2528482: Fix notice in Date::processDate.
Comment #59
googletorp CreditAttribution: googletorp commented@TR can you post in the some info about what you are doing that is generating notices, the root cause could be both core and in your contrib project.
Comment #60
TR CreditAttribution: TR commentedCrossposting from #2528482: Fix notice in Date::processDate ..
To reproduce, create a form with this element:
Just visit the form, no need to submit. The notice will be shown in /admin/reports/dblog.
The nasty-looking HTML generated by this is:
Comment #61
TR CreditAttribution: TR commentedOh, and the datepicker doesn't work with this example either ...
Comment #62
mpdonadioNo need to crosspost; that is what the followup is for. We will work through it there. Thanks!