Closed Bug 825294 Opened 12 years ago Closed 8 years ago

Implement framework for using date/time input types on Desktop

Categories

(Core :: DOM: Core & HTML, defect)

Product:
Core
Component:
DOM: Core & HTML
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla50
Tracking Flags:
Tracking Status
firefox50 --- fixed

People

(Reporter: mounir, Assigned: jordandev678)

References

(Blocks 1 open bug)

Details

(Keywords: html5, Whiteboard: [parity-webkit][parity-blink][parity-opera][parity-edge])

Attachments

(1 file, 8 obsolete files)

Generic base for <input=type=date> widget implementations
38.68 KB, patch
jordandev678
: review+
RyanVM
: checkin+
Details | Diff | Splinter Review 
      No description provided.
Depends on: 769352
Depends on: 773205
Depends on: 777279
Blocks: datetime
Forgive the update post, but after checking through linking bugs, I still can't determine the progress of or even interest in implementing these. To the best I can determine, "An intern was working on these, but he left".

If this isn't the appropriate place, can an appropriate contact point be provided? You may email me to keep this thread clean.
Flags: needinfo?
Flags: needinfo?
Depends on: 856265
Blocks: 872630
Is this sort of Bug viable for fx-backlog? Trying the flag …
Flags: firefox-backlog?
Flags: firefox-backlog? → firefox-backlog-
seeing as this has come up in 2012, is still unresolved and has no one assigned to it, is it safe to assume that it will not be implemented for a while or not at all?
(In reply to pdiprogrammers from comment #5)
> seeing as this has come up in 2012, is still unresolved and has no one
> assigned to it, is it safe to assume that it will not be implemented for a
> while or not at all?

As this was marked with 'firefox-backlog-' I guess the priorities currently lie somewhere else. Though that this issue lies around for some years already, doesn't mean it will never be implemented. Some things are implemented immediately, some take a long time to be done.
This being said, I'm not working for Mozilla, so I don't know what the priorities actually are at the moment and why this issue is not actively worked on yet.

At least someone should add the keywords [parity-chrome] and [parity-opera] as WebKit already has that feature.

Sebastian
I think that is a very important function. Forms enriched with UI to select the dates are convenient for users. I know I can write web pages for Chrome, but many people use Firefox, so you force web developers to use JS prostheses. In my opinion you need to do it yesterday. It's not impossible. Selectors are available for each system and do not need to create them from scratch.
Keywords: html5
Whiteboard: [parity-webkit][parity-blink][parity-opera]
This is holding back the web! Until all major browsers support this, we web developers have to use hacky javascript workarounds for date pickers, which are never properly i18n etc. Surely you can call a default system date picker if one is present, and just fall back to nothing if one is not?
I wouldn't be too harsh on Mozilla because of this, because they are few and priorites are everywhere they look around.
My experience is also that many times, designers simply require the same look in different browsers (even on the same platform it's far from the same) and custom solution is still the only answer, many times.
But I agree that for many other/smaller web projects (let's say 50% of them? Maybe 60%?) this will be deliverenace to have it shipped finally, at least with some very basic UI. Whole implementation is done and the only UI Mozilla crucially needed was one for FirefoxOS, so they did it. 
Now, because of press of other things-to-do, they left it unfinished in hope/waiting, that some community member will do the last part missing, UI and help them a bit.
But it is not happening, unfortunatelly...
But instead of bitching, I would rather ask them (and send some donation, at least, like I periodically do) to re-consider theit strategy/priority in this case, because:
- desktop is still Firefox's major pillar, do not overestimate you hopes for FirefoxOS yet, please
- it looks like no one will do the hard work for you, but since most of the wok is done already, wouldn't it be better to focus on finishing it rather than wasting previous work on hundreds of thousands FirefoxOS users rather than deliver it to hundreds of millions of your desktop users?
- it is one of the most visible features to the naked eye of common web coder/developer, struggling with web forms every day and you can ease their lives a lot... and hey, that's your mission, right?

Wish you progress and success in this area (althought I'm not able to help in better way than donate, sorry), IMHO date and time would be enough for the beginning to ship,
Marek
Please take the philosophical debate elsewhere. This is a place to discuss implementation of the requested enhancement.

The fastest way to see this feature implemented is to submit a patch implementing it. Otherwise, complaining about it not being implemented is not going to get it implemented any faster.
Hello, as user and web developer I have been annoyed by the lack of support of time inputs, too. I have started a small javascript project on https://github.com/joerg-pfruender/js-clock/ to demonstrate how a cool new time picker could look like. This is of course not ready to be submitted as a patch, but maybe someone can take it as a basis for an own implementation or help me to make it more usable. Thank you.
Any updates on this bug
Date related input types are listed "in development" for IE/Spartan:

https://status.modern.ie/?term=date%20related

That would leave Firefox as the only major browser not having them implemented.
Firefox is getting the rear light (latest adopter) for this feature
http://caniuse.com/#feat=input-datetime

Many other browsers already support it. See a demo and try with e.g. Chrome:
http://demo.agektmr.com/datalist/
Whiteboard: [parity-webkit][parity-blink][parity-opera] → [parity-webkit][parity-blink][parity-opera][parity-edge]
This bug was just called out by Smashing Magazine's article about input frustrations, noting how uncharacteristicly behind it is for Firefox, and its age:

Form Inputs: The Browser Support Issue You Didn’t Know You Had
http://www.smashingmagazine.com/2015/05/05/form-inputs-browser-support-issue/
Perhaps I may be wrong as I am not an expert, but would not it be possible to integrate the input with the XUL datapicker control?

https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/datepicker
I've taken a run at this. I'm still looking into adding unit tests and tidying up the UI; it is not currently obvious that your looking at a date box (I'm thinking just a little down arrow on the right to open it).

Any feedback about what I have so far though would be appreciated.
Attachment #8651230 - Flags: feedback?
Very quick look at the patch ... looks rather good.
The main question I have is: do all the desktop platforms provide native date pickers?
If not, what is the plan for those platforms?
And how to support later datetime, month, week, time?
Or should we use XUL datepicker?
GTK and Windows both provide datepicker controls (although I'm not setup for windows dev, someone else might need to provide a windows implementation of nsDatePicker).
The XUL datepicker is an option I can look into. If the XUL one can be used what would be preferred: XUL used everywhere (consistent style for FF) or native where possible (consistent with OS)?

As for the other types, a quick search suggests windows also has a native time picker, unlike GTK.
Due to the small set of month values a month picker could be just a pre-set dropdown list of months.
That leaves a GTK version of time and what to do with the month type.
What about OSX?
OSX also seems to have one (developer.apple reference: http://tinyurl.com/qan9hgc)
The question is, do we actually want to support OS pickers or should a custom date picker widget be created for all platforms like the other browsers do it. And that question should better be answered in bug 773205.

Sebastian
The reality is that many web devs don't care how the picker looks, just that it's functional and useable. It doesn't make sense to me to block deploying the functionality, and dropping the need for a polyfill, just for discussions about the pros and cons of implementing a Firefox specific cross platform UI.
In any case, there is no uniformity between different OSes' file pickers (Save Page, Upload file, etc), so why should it be between color pickers?
Consistency between OSes isn't, IMO, too important, but consistency between different input types is.
So, type="datetime", type="time", type="month" etc shouldn't all look totally different. Either all them should be able to use some OS level widgets, or all should use XUL or HTML based widgets.
The latter is probably more flexible. jordandev678, want to explore that approach?
Personally, functionally is ahead of the aesthetics because it helps bring support more in line with other browsers. Bug 773205 and the consistency decisions could be tackled later.

With regards to a XUL/HTML based widget; others like the colour picker and file picker use native OS consistent widgets. It would seem strange having a single widget such as the datepicker consistent when others use the OS widget. 

I'm planning to spend my next run at it tidying up my previous patch and checking it against the code style guidelines. I'll finish that off then post it back here (with the GtkWidget) so the option is available.

After that Olli, I'll take a look at a XUL option (Fernando mentioned a XUL datepicker already exists) and see what I can do with it.
(In reply to jordandev678 from comment #39)
> After that Olli, I'll take a look at a XUL option (Fernando mentioned a XUL
> datepicker already exists) and see what I can do with it.

I just want to remind that XUL will die sooner or later. So using the XUL date picker[1] would just be a temporary solution. Furthermore it seems (from the documentation) this picker only supports picking a day, not a month or week.

Sebastian

[1] https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/datepicker
I was under the impression the XUL deprecation applied only to addons/extensions use. Perhaps it's not worth looking into if someone will only get the job of replacing it in the near-ish future.
If you're choosing between XUL or HTML widget, pick HTML.
Here is the GTK datepicker patch.

If GTK is used then clicking on a date input opens a GtkCalendar to select a value. When GTK is not available the datepicker will not be registered in nsContentProcesWidgetFactory and the input behaves like a normal text input.

Tested on Ubuntu 14.04 and it works correctly, including acting like a regular textbox when the datepicker is not registered.

It would be good if someone set up to build on other systems could check that it still works correctly as a text-box there also.
Attachment #8651230 - Attachment is obsolete: true
Attachment #8651230 - Flags: feedback?
Attachment #8656820 - Flags: feedback?
Hi, any update on this? 
IE has it's implementation in preview...

https://dev.modern.ie/platform/status/timerelatedinputtypes/
I've been looking at styling up the input field but haven’t gotten far (lack of time). It would be useful if someone could pick up Bug 773205 for me (style the input field). Even if they just discuss and get an answer to the question from Comment #35 it'd be useful.

As for implementation, I'm in favour of using OS widgets (as I did with GTK in my patch) but I can't do one for OSX (no Macs) and I don't know if/when I'll get time to do Windows.

If Windows and OSX don't have implementations once Bug 773205 is done I would quite like to get it in just for GTK (and have the others fall back to [type=text]). It gets good progress going and means whoever picks up the other versions doesn't have anything to do other than manage the OS widget appropriately. What's the stance on that sort of thing?
(In reply to Sebastian Zartner [:sebo] from comment #35)
> The question is, do we actually want to support OS pickers or should a
> custom date picker widget be created for all platforms like the other
> browsers do it. And that question should better be answered in bug 773205.
> 
> Sebastian

Here is my humble opinion. Lack of this feature is such a pain for web developers (Implement all this calendars/date-pickers only because Firefox does not support it), that I think the quickest approach should be acceptable. If OS pickers can be implemented easily why not?
(In reply to Alex Art from comment #46)
> (In reply to Sebastian Zartner [:sebo] from comment #35)
> > The question is, do we actually want to support OS pickers or should a
> > custom date picker widget be created for all platforms like the other
> > browsers do it. And that question should better be answered in bug 773205.
> > 
> > Sebastian
> 
> Here is my humble opinion. Lack of this feature is such a pain for web
> developers (Implement all this calendars/date-pickers only because Firefox
> does not support it), that I think the quickest approach should be
> acceptable. If OS pickers can be implemented easily why not?

There is still the missing IE implementation. Edge seems to have it now: http://caniuse.com/#feat=input-datetime

After 3 years of this issue, it would be really awesome if someone can implement it
Comment on attachment 8656820 [details] [diff] [review]
Add datepicker widget for type="date" v2

Review of attachment 8656820 [details] [diff] [review]:
-----------------------------------------------------------------

Olli, can you advise Jordan with this patch or redirect to someone who should look at this?

Thanks!
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #48)
> Olli, can you advise Jordan with this patch or redirect to someone who
> should look at this?
So we obviously need patches supporting the feature on all the platforms (or in other words the feature could be enabled on desktop if supported on all the desktop platforms, it could be enabled on Android if supported there and similarly on FFOS).
We could do something similar to Blink where date picker is something which is used also for datetime etc.,  time part is implemented purely inside the layout. But we need date picker working on all the platforms. 
mstange is more familiar with OSX, does it have easily useable date picker for this case?
If so, we could use native picker in all the cases.

(though personally I'd still prefer some XUL or HTML based picker which worked the same way cross-desktop-platforms, but that might take some time to implement.)
Comment on attachment 8656820 [details] [diff] [review]
Add datepicker widget for type="date" v2



>+  nsDatePickerShownCallback(HTMLInputElement* aInput,
>+                             nsIDatePicker* aDatePicker)
Nit, align params


>+private:
>+  nsRefPtr<HTMLInputElement> mInput;
>+  nsCOMPtr<nsIDatePicker>   mDatePicker;
Nit, either align variables, or remove 2 spaces before mDatePicker



>+nsDatePickerShownCallback::Done(const nsAString& aDate)
>+{
>+  nsAutoString oldValue;
>+
>+  mInput->PickerClosed();
>+  mInput->GetValue(oldValue);
>+
>+  if(!oldValue.Equals(aDate)){
>+    mInput->SetValue(aDate);
>+    return nsContentUtils::DispatchTrustedEvent(mInput->OwnerDoc(),
>+                            static_cast<nsIDOMHTMLInputElement*>(mInput.get()),
>+                            NS_LITERAL_STRING("change"), true,
>+                            false);
>+  }
>+
I don't see where we dispatch input event.

>+HTMLInputElement::InitDatePicker()
>+{
>+  if (mPickerRunning) {
>+    NS_WARNING("Just one nsIDatePicker is allowed");
>+    return NS_ERROR_FAILURE;
>+  }
>+
>+  nsCOMPtr<nsIDocument> doc = OwnerDoc();
>+
>+  nsCOMPtr<nsPIDOMWindow> win = doc->GetWindow();
>+  if (!win) {
>+    return NS_ERROR_FAILURE;
>+  }
>+
>+  if (IsPopupBlocked()) {
>+    win->FirePopupBlockedEvent(doc, nullptr, EmptyString(), EmptyString());
>+    return NS_OK;
>+  }
>+
>+  // Get Loc title
>+  nsXPIDLString title;
>+  nsContentUtils::GetLocalizedString(nsContentUtils::eFORMS_PROPERTIES,
>+                                     "DatePicker", title);
>+
It would be great if we could refactor the common code dealing with various pickers to some helper method


>+  //rv = colorPicker->Open(callback);
letfover?

>   static bool IsExperimentalMobileType(uint8_t aType)
>   {
>-    return aType == NS_FORM_INPUT_DATE || aType == NS_FORM_INPUT_TIME;
>+    return aType == NS_FORM_INPUT_TIME;
I'd prefer if this was controlled by some pref so that we could perhaps land code first to some platform and then
enable the pref once all the platforms support the feature

>+DatePickerParent::DatePickerShownCallback::Done(const nsAString& aDate)
>+{
>+  if (mDatePickerParent) {
>+    unused << mDatePickerParent->Send__delete__(mDatePickerParent,
>+                                                 nsString(aDate));
nit, align params

>+  DatePickerParent(const nsString& aTitle,
>+                    const nsString& aInitialDate)
ditto

>+TabParent::AllocPDatePickerParent(const nsString& aTitle,
>+                                   const nsString& aInitialDate)
ditto

>+NS_IMETHODIMP nsDatePicker::Init(nsIDOMWindow *parent, const nsAString& title,
>+                                 const nsAString& initialDate)
Nit,
NS_IMETHODIMP
nsDatePicker::Init(nsIDOMWindow* aParent, const nsAString& aTitle,
                   const nsAString& aInitialDate)





>+  //Check for parent
>+  void *parent_win = mParentWidget->GetNativeData(NS_NATIVE_SHELLWIDGET);
>+  if (!parent_win) {
>+    NS_WARNING("nsDatePicker: Error getting parent window");
>+    return NS_ERROR_FAILURE;
>+  }
>+  GtkWindow *parent_window = GTK_WINDOW(parent_win);
>+
>+  //Create dialog and put a calendar in it
>+  GtkDialogFlags flags = (GtkDialogFlags)(
>+                         GTK_DIALOG_MODAL | GTK_DIALOG_DESTROY_WITH_PARENT);
>+  mDialog = gtk_dialog_new_with_buttons(title, parent_window, flags,
>+                            ToNewUTF8String(cancelString), GTK_RESPONSE_REJECT,
>+                            ToNewUTF8String(selectString), GTK_RESPONSE_ACCEPT,
>+                            NULL);
>+  mCalendar = gtk_calendar_new();
>+  GtkWidget *content_area = gtk_dialog_get_content_area(GTK_DIALOG(mDialog));
>+  gtk_container_add(GTK_CONTAINER(content_area), mCalendar);
>+
>+  if (hasInitialDate) {
>+    gtk_calendar_select_month(GTK_CALENDAR(mCalendar), month, year);
>+    gtk_calendar_select_day(GTK_CALENDAR(mCalendar), day);
>+  }
>+
>+  //Connect widgets to this.
>+  g_signal_connect(mCalendar, "day-selected-double-click",
>+                   G_CALLBACK(OnDayDoubleClick), this);
>+  g_signal_connect(mDialog, "response", G_CALLBACK(OnResponse), this);
>+  g_signal_connect(mDialog, "destroy", G_CALLBACK(OnDestroy), this);
>+
>+  gtk_widget_show_all(mDialog);
karlt should comment on this stuff


>+
>+  NS_ADDREF_THIS();
>+
Please add a comment why 'this' is addrefed, and where it is released.


>+nsDatePicker::GetValueAsDate(const nsAString& aValue,
>+                                 uint32_t* aYear,
>+                                 uint32_t* aMonth,
>+                                 uint32_t* aDay) const
can we move this method to some helper class so that also non-gtk code could use it?
>+uint32_t
>+nsDatePicker::NumberOfDaysInMonth(uint32_t aMonth, uint32_t aYear) const
...same with this and DigitSubStringToNumber
Perhaps add a new class DateHelper.[h|cpp] with static methods to widget/ ?


>+ * February has 29 days during leap years which are years that are divisible by 400.
>+ * or divisible by 100 and 4. 
This comment doesn't look right. You're explicitly checking that year%100!=0, yet year%4==0

>+nsDatePicker::DigitSubStringToNumber(const nsAString& aStr,
>+                                         uint32_t aStart, uint32_t aLen,
>+                                         uint32_t* aRetVal)
align params



So, looking good. Do you think you could separate the non-platform specific code to one patch, and then file follow bugs for different platforms and
upload the gtk parts to one of them.  I could review cross-platform parts, and then others could look at the platform specific code
(including implementing them). But we need that pref to be able to have this all disabled by default until supported everywhere.
This all assuming we can have platform specific datepickers implemented easily - I guess we can even on OSX.
Attachment #8656820 - Flags: feedback? → feedback+
(In reply to Olli Pettay [:smaug] from comment #52)
Thanks for looking it over, next chance I get I'll make those tidy up changes and then look into splitting it up.
Hopefully should be done within the next few weeks.
(In reply to Olli Pettay [:smaug] from comment #52)

I've fixed all the nits - still working on some things before I post it.
Other comments below:

> Comment on attachment 8656820 [details] [diff] [review]
> Add datepicker widget for type="date" v2
> >+nsDatePickerShownCallback::Done(const nsAString& aDate)
> >+{
> >+  nsAutoString oldValue;
> >+
> >+  mInput->PickerClosed();
> >+  mInput->GetValue(oldValue);
> >+
> >+  if(!oldValue.Equals(aDate)){
> >+    mInput->SetValue(aDate);
> >+    return nsContentUtils::DispatchTrustedEvent(mInput->OwnerDoc(),
> >+                            static_cast<nsIDOMHTMLInputElement*>(mInput.get()),
> >+                            NS_LITERAL_STRING("change"), true,
> >+                            false);
> >+  }
> >+
> I don't see where we dispatch input event.
Would the correct behaviour be to fire both at the same time?
I can certainly do that, I was under the impression if you fired "change" you didn't also have to fire "input".

> >+HTMLInputElement::InitDatePicker()
> >+{
> >+  if (mPickerRunning) {
> >+    NS_WARNING("Just one nsIDatePicker is allowed");
> >+    return NS_ERROR_FAILURE;
> >+  }
> >+
> >+  nsCOMPtr<nsIDocument> doc = OwnerDoc();
> >+
> >+  nsCOMPtr<nsPIDOMWindow> win = doc->GetWindow();
> >+  if (!win) {
> >+    return NS_ERROR_FAILURE;
> >+  }
> >+
> >+  if (IsPopupBlocked()) {
> >+    win->FirePopupBlockedEvent(doc, nullptr, EmptyString(), EmptyString());
> >+    return NS_OK;
> >+  }
> >+
> >+  // Get Loc title
> >+  nsXPIDLString title;
> >+  nsContentUtils::GetLocalizedString(nsContentUtils::eFORMS_PROPERTIES,
> >+                                     "DatePicker", title);
> >+
> It would be great if we could refactor the common code dealing with various
> pickers to some helper method
It would, but I'd be tempted to leave that as a tidy-up bug afterwards (keep this bug & patch from touching other pickers unnecessarily).

> >+nsDatePicker::GetValueAsDate(const nsAString& aValue,
> >+                                 uint32_t* aYear,
> >+                                 uint32_t* aMonth,
> >+                                 uint32_t* aDay) const
> can we move this method to some helper class so that also non-gtk code could
> use it?
> >+uint32_t
> >+nsDatePicker::NumberOfDaysInMonth(uint32_t aMonth, uint32_t aYear) const
> ...same with this and DigitSubStringToNumber
> Perhaps add a new class DateHelper.[h|cpp] with static methods to widget/ ?
I thought the same, the class you want to share it with is dom/html/HTMLInputElement.cpp (which is where I took some of them from). What would be the best place to put the helper so dom/html and widget/ can use it?

> >+ * February has 29 days during leap years which are years that are divisible by 400.
> >+ * or divisible by 100 and 4. 
> This comment doesn't look right. You're explicitly checking that
> year%100!=0, yet year%4==0
> 
Hah, I lifted that from HTMLInputElement.cpp, which also has the same mistake. I've fixed it in nsDatePicker. Should I fix it in HTMLInputElement.cpp to? (I suppose technically that is a different bug?). Not sure how strict you guys are with that kinda incidental clean-up in other patches.

>>   static bool IsExperimentalMobileType(uint8_t aType)
>>   {
>>-    return aType == NS_FORM_INPUT_DATE || aType == NS_FORM_INPUT_TIME;
>>+    return aType == NS_FORM_INPUT_TIME;
>I'd prefer if this was controlled by some pref so that we could perhaps land code first to some
>platform and then
>enable the pref once all the platforms support the feature
Done - added dom.forms.date in line with dom.forms.color for the color picker.
What this means is whoever added the experimental preference (dom.experimental_forms) for the mobile types will also have to use dom.forms.date to enable the date picker. Its no longer switched by IsExperimentalMobileType(). I assume this is ok (as it's in line with the color picker)?

> So, looking good. Do you think you could separate the non-platform specific
> code to one patch, and then file follow bugs for different platforms and
> upload the gtk parts to one of them.  I could review cross-platform parts,
> and then others could look at the platform specific code
> (including implementing them). But we need that pref to be able to have this
> all disabled by default until supported everywhere.
> This all assuming we can have platform specific datepickers implemented
> easily - I guess we can even on OSX.
I took a run at it and couldn't quite get it to split up nicely. It seems (still looking/debugging) if you didn't have an implementation (the second gtk patch) but had (manually) enabled the dom.forms.date preference and clicked the element the tab crashed. It seemed do_CreateInstance returned a (valid) IPC proxy but once you actually tried to use it no valid instance on the other end resulted in a crash.

I hadn't expected do_CreateInstance to return NS_OK in this instance. Splitting it may take some time as I don't know a whole lot about this area of FF's workings.

That said, I'm ambivalent as to weather or not this is a problem. Obviously a known way to trigger  tab crash is bad - however the code is avoided if you don't enable the preference manually and it would only be enabled on various platforms as patches adding their widgets came in (thereby making it not a problem for them). I'd be tempted to go with it (with the preference disabled) and if you enable it without a widget implementation then as about:config says, you've voided your warranty.

Thoughts?
(In reply to jordandev678 from comment #57)
> Would the correct behaviour be to fire both at the same time?
> I can certainly do that, I was under the impression if you fired "change"
> you didn't also have to fire "input".

Please test also other browsers.
The spec says
"The input event fires whenever the user has modified the data of the control. The change event fires when the value is committed, if that makes sense for the control, or else when the control loses focus. In all cases, the input event comes before the corresponding change event (if any)."
And per spec both input and change events apply to date and time and such.

https://html.spec.whatwg.org/multipage/forms.html#common-input-element-events

> > It would be great if we could refactor the common code dealing with various
> > pickers to some helper method
> It would, but I'd be tempted to leave that as a tidy-up bug afterwards (keep
> this bug & patch from touching other pickers unnecessarily).
that way, or do the refactoring first and then this patch. either way is fine.


> > >+nsDatePicker::GetValueAsDate(const nsAString& aValue,
> > >+                                 uint32_t* aYear,
> > >+                                 uint32_t* aMonth,
> > >+                                 uint32_t* aDay) const
> > can we move this method to some helper class so that also non-gtk code could
> > use it?
> > >+uint32_t
> > >+nsDatePicker::NumberOfDaysInMonth(uint32_t aMonth, uint32_t aYear) const
> > ...same with this and DigitSubStringToNumber
> > Perhaps add a new class DateHelper.[h|cpp] with static methods to widget/ ?
> I thought the same, the class you want to share it with is
> dom/html/HTMLInputElement.cpp (which is where I took some of them from).
> What would be the best place to put the helper so dom/html and widget/ can
> use it?
Depending on how many generic methods are needed... if not too many, you could just put them to
nsContentUtils.h/cpp
Or if a separate file makes the code easier to read, dom/html should be fine.
widget/* can use that once moz.build's LOCAL_INCLUDES contains the relevant directory.


> > >+ * February has 29 days during leap years which are years that are divisible by 400.
> > >+ * or divisible by 100 and 4. 
> > This comment doesn't look right. You're explicitly checking that
> > year%100!=0, yet year%4==0
> > 
> Hah, I lifted that from HTMLInputElement.cpp, which also has the same
> mistake. I've fixed it in nsDatePicker. Should I fix it in
> HTMLInputElement.cpp to? (I suppose technically that is a different bug?).
Please file a separate bug and attach patch there

> Not sure how strict you guys are with that kinda incidental clean-up in
> other patches.
depends on the case. If the feature/patch is already huge like this, better to not do random other fixes. Makes
reviewing hard.


> What this means is whoever added the experimental preference
> (dom.experimental_forms) for the mobile types will also have to use
> dom.forms.date to enable the date picker. Its no longer switched by
> IsExperimentalMobileType(). I assume this is ok (as it's in line with the
> color picker)?
http://mxr.mozilla.org/mozilla-central/search?string=dom.experimental_forms
shows (dom.experimental_forms is set in couple of places. So those should be update to set also the
new pref.


> 
> That said, I'm ambivalent as to weather or not this is a problem. Obviously
> a known way to trigger  tab crash is bad - however the code is avoided if
> you don't enable the preference manually and it would only be enabled on
> various platforms as patches adding their widgets came in (thereby making it
> not a problem for them). I'd be tempted to go with it (with the preference
> disabled) and if you enable it without a widget implementation then as
> about:config says, you've voided your warranty.
Sounds fine.
The pref could be on on Nightlies only and only on those platforms which have the backend implemented.
You'll need some ifdefs around the pref.
http://mxr.mozilla.org/mozilla-central/source/modules/libpref/init/all.js has plenty of examples.
Blocks: 1240718
No longer blocks: 1240718
Depends on: 1240718
Creates the generic base for future widget implementations of <input type=date>.
Attachment #8656820 - Attachment is obsolete: true
Attachment #8709969 - Flags: review?(bugs)
bear with me. I'm so sorry about the delay here. (tons of other stuff to do, bad excuse).
Trying to prioritize this.
Comment on attachment 8709969 [details] [diff] [review]
Generic base for <input=type=date> widget implementations


> // Enable new experimental html forms
> pref("dom.experimental_forms", true);
> pref("dom.forms.number", true);
>+pref("dom.forms.date", true);
Hmm, so we need to somehow explicitly disable the DatePicker in b2g, since it doesn't have an implementation for it.
b2g supports it in some other way. I guess we can keep the pref this way, but then in the C++ code 
#ifndef MOZ_B2G around the code which would trigger DatePicker in HTMLInputElement


>+class nsDatePickerShownCallback final : public nsIDatePickerShownCallback
Maybe drop ns prefix here. New code doesn't really need it.
Though, there is that other class DatePickerShownCallback, but it is inner class, so should be fine.
Or call that class DatePickerShownCallbackInParent or something like that.

>+private:
>+  RefPtr<HTMLInputElement> mInput;
>+  nsCOMPtr<nsIDatePicker>    mDatePicker;
nit, some odd indentation here

>+
>+  // Get Loc title
>+  nsXPIDLString title;
>+  nsContentUtils::GetLocalizedString(nsContentUtils::eFORMS_PROPERTIES,
>+                                     "DatePicker", title);
>+
>+  nsresult rv;
>+  nsCOMPtr<nsIDatePicker> datePicker = do_CreateInstance("@mozilla.org/datepicker;1", &rv);
>+  if (!datePicker) {
>+    return rv;
>+    //return NS_ERROR_FAILURE;
>+  }
drop // return NS_ERROR_VALUE




>+  nsCOMPtr<nsIDOMWindow> window = do_QueryInterface(ownerElement->OwnerDoc()->GetWindow());
There has been some renaming and such, so you need to use mozIDOMWindowProxy interface, not nsIDOMWindow.

>+TabChild::DeallocPDatePickerChild(PDatePickerChild* aDatePicker)
>+{
>+  nsDatePickerProxy* picker = static_cast<nsDatePickerProxy*>(aDatePicker);
>+  NS_RELEASE(picker);
>+  return true;
>+}
Could you possible add a comment to the place where you do ADDREF_THIS that release is called in DeallocPDatePickerChild


>+#if defined(MOZ_WIDGET_GTK)
>+    { "@mozilla.org/datepicker;1", &kNS_DATEPICKER_CID, Module::CONTENT_PROCESS_ONLY },
>+#endif
ok, this kind of define is a bit odd, but I guess it can be removed once supported in all the platforms


>+class nsDatePickerProxy final : public nsIDatePicker,
>+                                 public mozilla::dom::PDatePickerChild
nit, align the the latter public underneath the first one.
Attachment #8709969 - Flags: review?(bugs) → review+
(so those things fixed, r+)
That should be the patch updated to the most recent nightly.
B2g code needs reviewed.
Attachment #8709969 - Attachment is obsolete: true
Attachment #8744741 - Flags: review?(bugs)
Comment on attachment 8744741 [details] [diff] [review]
Generic base for <input=type=date> widget implementations

Just minor styling nits.

>+  DatePickerShownCallback(HTMLInputElement* aInput,
>+                            nsIDatePicker* aDatePicker)
indentation of nsIDatePicker should be below HTMLInputElement

>+  RefPtr<HTMLInputElement> mInput;
>+  nsCOMPtr<nsIDatePicker>  mDatePicker;
I'd just use 1 space after > in both cases

> {
>   if (aNamespaceID == kNameSpaceID_None) {
>     if (aAttribute == nsGkAtoms::type) {
>       // XXX ARG!! This is major evilness. ParseAttribute
>       // shouldn't set members. Override SetAttr instead
>       int32_t newType;
>       bool success = aResult.ParseEnumValue(aValue, kInputTypeTable, false);
>       if (success) {
>+	bool b2g = true;
>+	#ifndef MOZ_B2G
>+		b2g = false;
you have some \t characters here.
Use spaces for indentation

>-  [Throws, Pref="dom.experimental_forms"]
>+  [Throws, Pref="dom.forms.date"]
>            attribute Date? valueAsDate;
Hmm, so this hides valueAsDate from b2g. Not sure that matters much in practice now, but, hmm, could we change
dom.forms.date so dom.forms.datepicker and it would mean that whenever it is enabled, valueAsDate property is enabled too, but
not having it enabled, but having just dom.experimental_forms enabled wouldn't change any behavior.
It means that Pref="..." here needs to become Func=... See MDN's page about webidl to see how Func works.

>+++ b/layout/forms/nsMeterFrame.h
>@@ -6,16 +6,18 @@
> #ifndef nsMeterFrame_h___
> #define nsMeterFrame_h___
> 
> #include "mozilla/Attributes.h"
> #include "nsContainerFrame.h"
> #include "nsIAnonymousContentCreator.h"
> #include "nsCOMPtr.h"
> 
>+using namespace mozilla;
looks like unrelated change.
>+++ b/mobile/android/app/mobile.js
>@@ -180,16 +180,17 @@ pref("findhelper.autozoom", true);
> /* autocomplete */
> pref("browser.formfill.enable", true);
> 
> /* spellcheck */
> pref("layout.spellcheckDefault", 0);
> 
> /* new html5 forms */
> pref("dom.experimental_forms", true);
>+pref("dom.forms.date", true);
and if we had the dom.forms.datepicker setup, this change wouldn't be needed.


Sorry that I didn't think about the pref setup before. Just don't want to break any existing behavior.
Attachment #8744741 - Flags: review?(bugs) → review-
(In reply to Olli Pettay [:smaug] from comment #66)
> Comment on attachment 8744741 [details] [diff] [review]
> Generic base for <input=type=date> widget implementations

Thanks for reviewing it so quickly, I'll take a look ASAP.

> >+++ b/layout/forms/nsMeterFrame.h
> >@@ -6,16 +6,18 @@
> > #ifndef nsMeterFrame_h___
> > #define nsMeterFrame_h___
> > 
> > #include "mozilla/Attributes.h"
> > #include "nsContainerFrame.h"
> > #include "nsIAnonymousContentCreator.h"
> > #include "nsCOMPtr.h"
> > 
> >+using namespace mozilla;
> looks like unrelated change.

I'ts semi-related - nsMeterFrame is missing the using and I think it's hidden by unified builds (I assume it's leeching of a 'using' from another file).
When adding the new files for this patch to the unified build list nsMeterFrame doesn't compile without this change (I assume the 'using' it was leeching of is no longer available with the new file in the middle).

Should I split this off into a different bug?
I see. ok fine to keep it here.
That should hopefully do it.
dom.experimental_forms behaviour preserved when dom.forms.datepicker is disabled (the default).
dom.forms.datepicker=true will enable the date input also, regardless of dom.experimental_forms.
Attachment #8744741 - Attachment is obsolete: true
Attachment #8747537 - Flags: review?(bugs)
Comment on attachment 8747537 [details] [diff] [review]
Generic base for <input=type=date> widget implementations

>+HTMLInputElement::ValueAsDateEnabled(JSContext* cx, JSObject* obj)
>+{
>+  return Preferences::GetBool("dom.experimental_forms", false) ||
>+  Preferences::GetBool("dom.forms.datepicker", false);
Nit, missing indentation before the latter Preferences::...

>+bool
>+HTMLInputElement::IsExperimentalMobileType(uint8_t aType)
>+{
>+  return aType == NS_FORM_INPUT_TIME || (
>+    aType == NS_FORM_INPUT_DATE && !Preferences::GetBool("dom.forms.datepicker", false)
>+  );
odd use of ();
  return aType == NS_FORM_INPUT_TIME ||
    (aType == NS_FORM_INPUT_DATE &&
     !Preferences::GetBool("dom.forms.datepicker", false));
>         newType = aResult.GetEnumValue();
>         if ((IsExperimentalMobileType(newType) &&
>              !Preferences::GetBool("dom.experimental_forms", false)) ||
>             (newType == NS_FORM_INPUT_NUMBER &&
>              !Preferences::GetBool("dom.forms.number", false)) ||
>             (newType == NS_FORM_INPUT_COLOR &&
>-             !Preferences::GetBool("dom.forms.color", false))) {
>+             !Preferences::GetBool("dom.forms.color", false))
>+           ) {
>+
unrelated and unneeded change.

>-  SpecialPowers.pushPrefEnv({'set': [["dom.experimental_forms", false]]}, function() {
>+  SpecialPowers.pushPrefEnv({'set': [["dom.experimental_forms", false], ["dom.forms.datepicker",false]]}, function() {
>     input.type = "date";
>     is(input.type, "text", "input type shouldn't be date when the experimental forms are disabled");
>     is(input.getAttribute('type'), "date", "input 'type' attribute should not change");
>-
>-    SpecialPowers.pushPrefEnv({'set': [["dom.experimental_forms",true]]}, function() {
>-      // Change the type of input to text and then back to date, 
>-      // so that HTMLInputElement::ParseAttribute gets called with the pref enabled.
>-      input.type = "text";
>-      input.type = "date";
>-      is(input.type, "date", "input type should be date when the experimental forms are enabled");
>-      is(input.getAttribute('type'), "date", "input 'type' attribute should not change");
>-    
>-      SimpleTest.finish();
>-    });
>   });
> 
>+  SpecialPowers.pushPrefEnv({'set': [["dom.experimental_forms",true], ["dom.forms.datepicker",false]]}, function() {
>+    // Change the type of input to text and then back to date,
>+    // so that HTMLInputElement::ParseAttribute gets called with the pref enabled.
>+    input.type = "text";
>+    input.type = "date";
>+    is(input.type, "date", "input type should be date when the experimental forms are enabled");
>+    is(input.getAttribute('type'), "date", "input 'type' attribute should not change");
>+  });
I'd prefer if you kept this SpecialPowers.pushPrefEnv inside the first one, like in the original code.


>+
>+  SpecialPowers.pushPrefEnv({'set': [["dom.experimental_forms",false], ["dom.forms.datepicker",true]]}, function() {
>+    // Change the type of input to text and then back to date,
>+    // so that HTMLInputElement::ParseAttribute gets called with the pref enabled.
>+    input.type = "text";
>+    input.type = "date";
>+    is(input.type, "date", "input type should be date when the datepicker is enabled");
>+    is(input.getAttribute('type'), "date", "input 'type' attribute should not change");
>+
>+    SimpleTest.finish();
>+  });
and this one inside that, since it isn't clear in which way SpecialPowers.pushPrefEnv does its stuff asynchronously and so.

>+  tabChild->SendPDatePickerConstructor(this,
>+                                        nsString(aTitle),
>+                                        nsString(aInitialDate));
extra space before those nsStrings

>+[scriptable, uuid(7becfc64-966b-4d53-87d2-9161f36bd3b3)]
>+interface nsIDatePicker : nsISupports
>+{
>+ /**
>+  * Initialize the date picker widget. The date picker will not be shown until
>+  * open() is called.
>+  * If the initialDate parameter does not follow the format "YYYY-MM-DD" then
>+  * the behavior will be unspecified.
>+  *
>+  * @param      parent       nsIDOMWindow parent. This dialog will be dependent
>+  *                          on this parent. parent may be null.
>+  * @param      title        The title for the date picker widget.
>+  * @param      initialDate  The date to show when the widget is opened. The
>+  *                          parameter has to follow the format "YYYY-MM-DD"
>+  */
>+  void init(in mozIDOMWindowProxy parent, in AString title, in AString initialDate);
I realized we want to add some coordinate information here, since the picker should be shown next to the <input type=date>.
But coordinates can be added in a followup bug.

>+ /**
>+  * Opens the date dialog asynchrounously.
>+  * The results are provided via the callback object.
>+  */
>+  //void open(in nsIDatePickerShownCallback aDatePickerShownCallback);
>+  void open(in nsIDatePickerShownCallback callback);
remove the commented out method.
Attachment #8747537 - Flags: review?(bugs) → review+
Nits tidied up. With regards to coordinate information though the colour picker is also lacking - would need to add coordinates to both in order to keep them consistent.
Attachment #8747537 - Attachment is obsolete: true
Attachment #8749146 - Flags: review?(bugs)
Are there builds (on the FTP server maybe?) which include the date/time implementation pieces which are already there? So it's possible to take a look.

When can we expect this to land on Nightly?

(sorry for not really contributing anything technical, at least not yet)
(In reply to Markus Popp from comment #72)
> Are there builds (on the FTP server maybe?) which include the date/time
> implementation pieces which are already there? So it's possible to take a
> look.
> 
> When can we expect this to land on Nightly?
> 
> (sorry for not really contributing anything technical, at least not yet)

I don't have any full builds anywhere I'm afraid, but this patch and the one in Bug 1240718 is everything I have on the topic of the date/time pieces so far.

Perhaps if someone keeping track of this bug with access could push these two patches together up to the try server and post a link back here (or to the other bug) - I believe the try server lets you download the builds it creates.
There are several steps you will have to complete in order to get a patch into Mozilla:

1. Request Level 1 Commit Access and CC Olli Pettay or someone else who can vouch for you: https://www.mozilla.org/en-US/about/governance/policies/commit/
2. Push a branch with your revision to the Try Server: https://wiki.mozilla.org/ReleaseEngineering/TryServer
3. Post a link to the bug report showing that the Try Server found no new failures.
4. Request Bugzilla editbugs permissions: https://bugzilla.mozilla.org/page.cgi?id=get_permissions.html
5. Add the keyword "checkin-needed" to the bug report, and at the same time leave a comment like "Requesting checkin of attachment 8749146 [details] [diff] [review]".

It took me a while to figure all of this out, but once you get the hang of it, you will be able to contribute to Mozilla much more easily.

Thanks for working on this bug, it is dearly needed...
Oops, sorry, there was just one nit there.
The bug description didn't reflect the fact well so I modified it a bit. :)
Summary: Implement date/time input types on Desktop → Implement date/time input types on Linux Desktop
No longer depends on: 773205
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #77)
> The bug description didn't reflect the fact well so I modified it a bit. :)

Regarding the new summary, I'm wondering whether there will/should be a separate bug for the Windows and OS X Desktop implementation.

Sebastian
Flags: needinfo?(htsai)
(In reply to Sebastian Zartner [:sebo] from comment #78)
> (In reply to Hsin-Yi Tsai [:hsinyi] from comment #77)
> > The bug description didn't reflect the fact well so I modified it a bit. :)
> 
> Regarding the new summary, I'm wondering whether there will/should be a
> separate bug for the Windows and OS X Desktop implementation.
> 
> Sebastian

Yes, I think so. Once the UX spec is ready so that we have a clear picture of what need to be done by how, we will re-organize the existing bugs (which block bug 888320) and file separate bugs properly if needed.
Flags: needinfo?(htsai)
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #81)
> (In reply to Sebastian Zartner [:sebo] from comment #78)
> > (In reply to Hsin-Yi Tsai [:hsinyi] from comment #77)
> > > The bug description didn't reflect the fact well so I modified it a bit. :)
> > 
> > Regarding the new summary, I'm wondering whether there will/should be a
> > separate bug for the Windows and OS X Desktop implementation.
> > 
> > Sebastian
> 
> Yes, I think so. Once the UX spec is ready so that we have a clear picture
> of what need to be done by how, we will re-organize the existing bugs (which
> block bug 888320) and file separate bugs properly if needed.

Ok, so then I'll wait for that and I won't create those reports right now.

Sebastian
(In reply to Hsin-Yi Tsai [:hsinyi] from comment #77)
> The bug description didn't reflect the fact well so I modified it a bit. :)

This bug was meant to be the set-up for UI implementations later (so it shouldn't be specific to Linux) - I created Bug 1240718 to track the GTK/Linux implementation that builds on this base one.
über-nit eradicated.
Attachment #8749146 - Attachment is obsolete: true
Attachment #8755962 - Flags: review?(bugs)
Comment on attachment 8755962 [details] [diff] [review]
Generic base for <input=type=date> widget implementations

In general no need to ask review if just fixing some minor nits mentioned in the previous r+ review.
Attachment #8755962 - Flags: review?(bugs) → review+
Updated to apply cleanly to recent nightly (changeset:301993:5f95858f8ddf) and removed preference=false from files (false is the default anyway).

Test results: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9aac3c0dbe34
Attachment #8755962 - Attachment is obsolete: true
Attachment #8763584 - Flags: review?(bugs)
Comment on attachment 8763584 [details] [diff] [review]
Generic base for <input=type=date> widget implementations

>+HTMLInputElement::InitDatePicker()
>+{
>+  if (!Preferences::GetBool("dom.forms.datepicker", false)) return NS_OK;
Always {} with if
So, 
if (!Preferences::GetBool("dom.forms.datepicker", false)) {
  return NS_OK;
}
Attachment #8763584 - Flags: review?(bugs) → review+
Updated, ready to go in.
Attachment #8763584 - Attachment is obsolete: true
Attachment #8763713 - Flags: review+
Attachment #8763713 - Flags: checkin?
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/14ac8b409bcd
Add framework for datepicker widgets to input[type=date]. r=smaug
Assignee: nobody → jordandev678
Attachment #8763713 - Flags: checkin? → checkin+
https://hg.mozilla.org/mozilla-central/rev/14ac8b409bcd
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Fixing the summary so people aren't confused tomorrow when there's no linux implementation in their builds... (bug 1240718).
Summary: Implement date/time input types on Linux Desktop → Implement framework for using date/time input types on Desktop
It's marked as fixed/closed, but it is still not available in Firefox 50, nor in the nightly build of FF 53. :/
In the hopes someone reads to the bottom of the bug before replying...

This bug used to be about "implement a UI for <input type="datetime">, oh by the way I'm on Linux", then turned into "Attach the XUL datepicker to <input type="datetime-local">", then became "Build a way for platform-specific datepickers to be attached" (which was eventually RESO FIXED). There is no UI for this in Firefox yet, but the foundations one could be built on have been poured.

- Linux-specific support was split off into Bug 1240718 (RESO WONTFIX due to later decisions to unify the UI).
- Windows-specifc support was never addressed.
- Mac-specific support was briefly mentioned in a few comments here and elsewhere, but never became a bug of its own.

- Support for date/time as a whole is now Bug 888320.
- The unified UI I mentioned above is Bug 1069609, currently stuck in quibble hell.
- Actually implementing that UI is bug 1283381.
- Making JS aware that Firefox "knows about" these types is Bug 1005268, though it's not clear whether that's mobile-only or all-platform.

- Android and iOS have been decreed out of scope for this family of bugs.

Hope this helps explain why a bug that was called "Implement date/time input types on Linux Desktop" got RESO FIXED when date/time input types on Linux Desktop are not yet fully implemented.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: