This pref enables <input type=date> and <input type=time>.

Hi David, We would like to turn on dom.forms.datetime on Nightly, but found some issues [1] with a marionette feature `DateTimeValue` introduce in bug 908441. Looking at its implementation, it is sending keys to the element according to date/time format defined in the spec. However, we've implemented so that the format changes according to the locale, and no need to enter the "-" (dash). You can try it on Nightly by enabling the pref. Do you have any idea how we to fix this (and only for Nightly for now)? Searching at the codebase, it looks like currently there are no tests using it. [1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=85c0938c5783ff9d94b551215d6d5e00cee3fc57&group_state=expanded

(In reply to Jessica Jong [:jessica] from comment #1) > We would like to turn on dom.forms.datetime on Nightly, but found some > issues [1] with a marionette feature `DateTimeValue` introduce in bug > 908441. Looking at its implementation, it is sending keys to the element > according to date/time format defined in the spec. However, we've > implemented so that the format changes according to the locale, and no need > to enter the "-" (dash). You can try it on Nightly by enabling the pref. It is the purpose of the WebDriver service in Marionette to emulate user interaction. Unless these new form controls have accessibility features we can access from JS to set the date and time in a manner that is in line with how a user would interact with them, I suggest handling them in the same way as we do <option> elements, which is by faking the relevant DOM events and setting the control’s value property. See an example from selecting <option>: https://searchfox.org/mozilla-central/rev/24c443a440104dabd9447608bd41b8766e8fc2f5/testing/marionette/interaction.js#235-275 You can find out what DOM events are meant to fire when interacting with <input type=date> and <input type=time> from using the monitorEvents() function in Chrome devtools: https://developers.google.com/web/tools/chrome-devtools/console/events Unfortunately, the WebDriver specification (https://w3c.github.io/webdriver/webdriver-spec.html) does not provide specialised APIs for any form controls. This means the input format coming from the client will be the same as the value property on <input type=date>, e.g. "1988-10-05". We are supposed to handle <input type=color> in a similar fashion. When the input is "#FF0000", the control’s regular DOM events will fire as if it was clicked by the user, ignoring the native OS colour picking widget, and then the control’s value property is set. I hope this helps. Feel free to reach out to me for more details.

Thanks :ato for the information provided. <input type=date> or <input type=time> is a little bit different from <option> or <input type=color>, since user usually types in individual keys and not just click on it to select a value. I used Chrome's monitorEvents() and for each key typed, it fires keydown/keypress/keyup for the corresponding key. So, it seems we still need to know the keys to be sent and its order. In mochitest, I also used sendString/synthesizeKey, but I assumed the locale is `en-US` (which is not nice). See http://searchfox.org/mozilla-central/source/dom/html/test/forms/test_input_date_key_events.html.

Trying to fix this, I suggest we remove this feature instead, given the following reasons: - The methods have been there for almost 4 years, but there are no tests using it - For now, we need to have two different implementations for nightly and beta/release - For <input type=time>, the second and millisecond part is optional, so the second and millisecond field appear only if "value" attribute has a second/millisecond part, so minute field may be followed by a second field or AM/PM field or nothing - For <input type=time>, it may be 12/24hr format depending on locale - For <input type=date>, fields may have different ordering depending on locale This is too much overhead, IMHO, I think we should let the test developer to just use "send_keys()" directly, to avoid unexpected results, since the test developer should know best how will the control look like. :ato, what do you think?

(In reply to Jessica Jong [:jessica] from comment #4) > Trying to fix this, I suggest we remove this feature instead, given > the following reasons: What feature are you referring to when you say “this”? > - For <input type=time>, the second and millisecond part is optional, > so the second and millisecond field appear only if "value" attribute > has a second/millisecond part, so minute field may be followed by a > second field or AM/PM field or nothing - For <input type=time>, it may > be 12/24hr format depending on locale - For <input type=date>, fields > may have different ordering depending on locale I think these points highlight how synthesising faked DOM events could be difficult, but it also demonstrates that delegating the responsibility for getting the input format right to the user isn’t really improving the situation. > This is too much overhead, IMHO, I think we should let the test > developer to just use "send_keys()" directly, to avoid unexpected > results, since the test developer should know best how will the > control look like. We can reach a middle ground here if we accept the input to <input type=date> as it is given from the element’s value property, but instead of synthesising each key event we only fire a DOM change event. If I set <input type=date> to 10 May 1988, it’s value property will return "1988-10-05" which we must respect as the input. I suggest we special case <input type=date> in https://searchfox.org/mozilla-central/rev/6c2dbacbba1d58b8679cee700fd0a54189e0cf1b/testing/marionette/listener.js#1450-1458, like we do for <input type=file>, and then introduce a new function in testing/marionette/interaction.js to set a form control’s value property. An untested prototype: > interaction.setFormControlValue = function (el, val) { > el.value = val; > event.change(el); > };

Whether to call interaction.setFormControlValue could be gated on whether Preferences.get("dom.forms.datetime") returns true.

(In reply to Andreas Tolfsen ‹:ato› from comment #5) > (In reply to Jessica Jong [:jessica] from comment #4) > > > Trying to fix this, I suggest we remove this feature instead, given > > the following reasons: > > What feature are you referring to when you say “this”? I meant to make DateTimeValue work :) > > > - For <input type=time>, the second and millisecond part is optional, > > so the second and millisecond field appear only if "value" attribute > > has a second/millisecond part, so minute field may be followed by a > > second field or AM/PM field or nothing - For <input type=time>, it may > > be 12/24hr format depending on locale - For <input type=date>, fields > > may have different ordering depending on locale > > I think these points highlight how synthesising faked DOM events > could be difficult, but it also demonstrates that delegating the > responsibility for getting the input format right to the user isn’t > really improving the situation. > > > This is too much overhead, IMHO, I think we should let the test > > developer to just use "send_keys()" directly, to avoid unexpected > > results, since the test developer should know best how will the > > control look like. > > We can reach a middle ground here if we accept the input to > <input type=date> as it is given from the element’s value property, > but instead of synthesising each key event we only fire a DOM change > event. > > If I set <input type=date> to 10 May 1988, it’s value > property will return "1988-10-05" which we must respect as > the input. I suggest we special case <input type=date> in > https://searchfox.org/mozilla-central/rev/ > 6c2dbacbba1d58b8679cee700fd0a54189e0cf1b/testing/marionette/listener.js#1450- > 1458, > like we do for <input type=file>, and then introduce a new function > in testing/marionette/interaction.js to set a form control’s value > property. > > An untested prototype: > > > interaction.setFormControlValue = function (el, val) { > > el.value = val; > > event.change(el); > > }; This sounds like a feasible solution, although it doesn't emulate exactly the user interaction that may be involved. I'll upload a patch based on your suggestion. Thanks for the help here!

Comment on attachment 8870729 [details] [diff] [review] fix marionette DateTimeValue. Review of attachment 8870729 [details] [diff] [review]: ----------------------------------------------------------------- s/marionette/Marionette/ in the commit message. I think the approach you use here is fine, but I think interaction.setInputElementValue can be simplified. We don’t want to assume the element is being interacted with using a mouse. When the mouse-related DOM events are removed, you also don’t have to special-case INPUT_TYPES_WITH_INPUT_BOX because the "change" and "input" events should be fired for all elements. I’d also quite like to see tests. ::: testing/marionette/interaction.js @@ +373,5 @@ > + * > + * @throws TypeError > + * If |el| is not an input element. > + */ > +interaction.setInputElementValue = function* (el, value) { The reason I suggested setFormControlValue was so that it might be reused for <textarea> and <div contentEditable=true>. They behave the same way as HTMLInputElement. @@ +383,5 @@ > + if (!INPUT_TYPES_WITH_INPUT_BOX.has(tagName)) { > + // Consider having an explicit function to activate or set the value of > + // these input types. > + el.value = value; > + return; You still need to fire the DOM "change" and "input" events. @@ +391,5 @@ > + event.mousemove(el); > + event.mousedown(el); > + event.focus(el); > + event.mouseup(el); > + event.click(el); I don’t think we should assume that the form control is being clicked. It can be set using only the keyboard as well. I suggest dropping mouseover, mousemove, mousedown, focus, mouseup, and click. This means you can also get rid of INPUT_TYPES_WITH_INPUT_BOX and the if-condition. ::: testing/marionette/listener.js @@ +1454,5 @@ > let el = seenEls.get(id, curContainer); > if (el.type == "file") { > yield interaction.uploadFile(el, val); > + } else if ((el.type == "date" || el.type == "time") && > + Preferences.get(INPUT_DATETIME_PREF)) { Use four spaces for indentation of continued statements.

(In reply to Andreas Tolfsen ‹:ato› from comment #9) > Comment on attachment 8870729 [details] [diff] [review] > fix marionette DateTimeValue. > > Review of attachment 8870729 [details] [diff] [review]: > ----------------------------------------------------------------- > > s/marionette/Marionette/ in the commit message. Will do. > > I think the approach you use here is fine, but I think > interaction.setInputElementValue can be simplified. We don’t want to assume > the element is being interacted with using a mouse. When the mouse-related > DOM events are removed, you also don’t have to special-case > INPUT_TYPES_WITH_INPUT_BOX because the "change" and "input" events should be > fired for all elements. > > I’d also quite like to see tests. The original test already covers date/time inputs, will add tests for other form controls. > > ::: testing/marionette/interaction.js > @@ +373,5 @@ > > + * > > + * @throws TypeError > > + * If |el| is not an input element. > > + */ > > +interaction.setInputElementValue = function* (el, value) { > > The reason I suggested setFormControlValue was so that it might be reused > for <textarea> and <div contentEditable=true>. They behave the same way as > HTMLInputElement. Ok, I just thought not all form elements has the value property, e.g. fieldset, but most of the commonly used have, so I think it's ok to change it to setFormControlValue. > > @@ +383,5 @@ > > + if (!INPUT_TYPES_WITH_INPUT_BOX.has(tagName)) { > > + // Consider having an explicit function to activate or set the value of > > + // these input types. > > + el.value = value; > > + return; > > You still need to fire the DOM "change" and "input" events. Not all input elements fire the DOM "change" and "input" events: "When the input and change events apply (which is the case for all input controls other than buttons and those with the type attribute in the Hidden state), the events are fired to indicate that the user has interacted with the control." [1] And not all input elements fire the "change" and "input" events when the value property is changed. For example, checkbox and radio fire these events when they are "activated/deactivated". So, we should still filter the above ones. > > @@ +391,5 @@ > > + event.mousemove(el); > > + event.mousedown(el); > > + event.focus(el); > > + event.mouseup(el); > > + event.click(el); > > I don’t think we should assume that the form control is being clicked. It > can be set using only the keyboard as well. I suggest dropping mouseover, > mousemove, mousedown, focus, mouseup, and click. > > This means you can also get rid of INPUT_TYPES_WITH_INPUT_BOX and the > if-condition. > > ::: testing/marionette/listener.js > @@ +1454,5 @@ > > let el = seenEls.get(id, curContainer); > > if (el.type == "file") { > > yield interaction.uploadFile(el, val); > > + } else if ((el.type == "date" || el.type == "time") && > > + Preferences.get(INPUT_DATETIME_PREF)) { > > Use four spaces for indentation of continued statements. Will do. [1] https://html.spec.whatwg.org/multipage/forms.html#common-input-element-events

Here is the updated patch. I'm not sure how to test `interaction.setFormControlValue()` directly, since calling `textarea.send_keys()` will fall into calling `interaction.sendKeysToElement()`. Do I need to expose a function in Marionette just for calling `interaction.setFormControlValue()`?

(In reply to Jessica Jong [:jessica] (AFK May 27-30) from comment #10) > Not all input elements fire the DOM "change" and "input" events: > > "When the input and change events apply (which is the case for all input > controls other than buttons and those with the type attribute in the Hidden > state), the events are fired to indicate that the user has interacted with > the control." [1] > > And not all input elements fire the "change" and "input" events when the > value property is changed. For example, checkbox and radio fire these events > when they are "activated/deactivated". Thanks for correcting me. You are absolutely right.

Comment on attachment 8871189 [details] [diff] [review] patch, v2. Review of attachment 8871189 [details] [diff] [review]: ----------------------------------------------------------------- You’re on the right path! I’m giving this an r+wc, provided you fix up the last remaining nits. ::: testing/marionette/interaction.js @@ +78,5 @@ > > +/** > + * Common form controls that user can change the value property interactively. > + */ > +const COMMON_FORM_CONTROLS = new Set([ Make both COMMON_FORM_CONTROLS’s and INPUT_TYPES_NO_EVENT’s entries lowercase and use el.localName below. It’s just a historical thing that we use upper case for the values above because of <xul:menu> &c. @@ +389,5 @@ > + > + let type = el.type ? el.type.toUpperCase() : null; > + if (type && INPUT_TYPES_NO_EVENT.has(type)) { > + return; > + } I think you can just to INPUT_TYPES_NO_EVENT.has(el.type) here, as long as the set uses lower-cased entries. el.type on an element which doesn’t have this property would evaluate to INPUT_TYPES_NO_EVENT.has(undefined), which would return false.

(In reply to Andreas Tolfsen ‹:ato› from comment #13) > Comment on attachment 8871189 [details] [diff] [review] > patch, v2. > > Review of attachment 8871189 [details] [diff] [review]: > ----------------------------------------------------------------- > > You’re on the right path! I’m giving this an r+wc, provided you fix up the > last remaining nits. Thanks :ato for the review. > > ::: testing/marionette/interaction.js > @@ +78,5 @@ > > > > +/** > > + * Common form controls that user can change the value property interactively. > > + */ > > +const COMMON_FORM_CONTROLS = new Set([ > > Make both COMMON_FORM_CONTROLS’s and INPUT_TYPES_NO_EVENT’s entries > lowercase and use el.localName below. It’s just a historical thing that we > use upper case for the values above because of <xul:menu> &c. I see, will use lowercase instead. > > @@ +389,5 @@ > > + > > + let type = el.type ? el.type.toUpperCase() : null; > > + if (type && INPUT_TYPES_NO_EVENT.has(type)) { > > + return; > > + } > > I think you can just to INPUT_TYPES_NO_EVENT.has(el.type) here, as long as > the set uses lower-cased entries. el.type on an element which doesn’t have > this property would evaluate to INPUT_TYPES_NO_EVENT.has(undefined), which > would return false. Right, will do.

addressed review comment 14, carrying r+.

Hi Mike, we'd like to enable input date/time on Nightly so that more people can try it.

Comment on attachment 8872874 [details] [diff] [review] Part 2: enable input date/time pref on Nightly. Review of attachment 8872874 [details] [diff] [review]: ----------------------------------------------------------------- Thanks! Happy that this is finally getting turned on! I think there are some small issues with this patch though - see below. ::: modules/libpref/init/all.js @@ +1252,5 @@ > // platforms which don't have a color picker implemented yet. > pref("dom.forms.color", true); > > // Support for input type=date and type=time. By default, disabled. > pref("dom.forms.datetime", false); This line seems redundant now. @@ -1256,5 @@ > pref("dom.forms.datetime", false); > > -// Support for input type=month, type=week and type=datetime-local. By default, > -// disabled. > -pref("dom.forms.datetime.others", false); Should this pref be going away? It still looks like it's being used...

(In reply to Mike Conley (:mconley) from comment #17) > Comment on attachment 8872874 [details] [diff] [review] > Part 2: enable input date/time pref on Nightly. > > Review of attachment 8872874 [details] [diff] [review]: > ----------------------------------------------------------------- > > Thanks! Happy that this is finally getting turned on! I think there are some > small issues with this patch though - see below. > > ::: modules/libpref/init/all.js > @@ +1252,5 @@ > > // platforms which don't have a color picker implemented yet. > > pref("dom.forms.color", true); > > > > // Support for input type=date and type=time. By default, disabled. > > pref("dom.forms.datetime", false); > > This line seems redundant now. > > @@ -1256,5 @@ > > pref("dom.forms.datetime", false); > > > > -// Support for input type=month, type=week and type=datetime-local. By default, > > -// disabled. > > -pref("dom.forms.datetime.others", false); > > Should this pref be going away? It still looks like it's being used... Sorry, I messed up, I should be replacing the pref "dom.forms.datetime" and not "dom.forms.datetime.others". "dom.forms.datetime.others" is used for input type=month, week and datetime-local, so it should stay pref-off. Thanks for catching it!

This one is correct, could you take a look? Thanks.

Comment on attachment 8873080 [details] [diff] [review] Part 2: enable date/time pref on Nightly, v2. Review of attachment 8873080 [details] [diff] [review]: ----------------------------------------------------------------- Looks good! Thanks.

Pushed by cbook@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/f4448d4bcf98 Part 1: Fix Marionette's DateTimeValue when dom.forms.datetime is enabled. r=ato https://hg.mozilla.org/integration/mozilla-inbound/rev/7abafd0fead9 Part 2: Enable dom.forms.datetime by default on Nightly. r=mconley

https://hg.mozilla.org/mozilla-central/rev/f4448d4bcf98 https://hg.mozilla.org/mozilla-central/rev/7abafd0fead9

Congrats to Jessica and the Date/time team!

(In reply to Hsin-Yi Tsai [:hsinyi] from comment #24) > Congrats to Jessica and the Date/time team! \o/

Updated compatibility notes: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input#Browser_compatibility https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/date#Browser_compatibility https://developer.mozilla.org/en-US/Firefox/Experimental_features#HTML Sebastian

Still Not Enabled In Firefox 55 On Ubuntu 16.04 .

(In reply to shivarajnaidu from comment #27) > Still Not Enabled In Firefox 55 On Ubuntu 16.04 . I am using Firefox 55.0.2 in Ubuntu 16.04 and I have checked that date and time picker is working or not.. But It's Still showing normal text input.. and I can enable it from about:config but not by default..

(In reply to shivarajnaidu from comment #27) > Still Not Enabled In Firefox 55 On Ubuntu 16.04 . Not Working In Firefox 56 (Developer Edition) On Windows 10 ..

(In reply to sivaprathap.konduru from comment #29) > (In reply to shivarajnaidu from comment #27) > > Still Not Enabled In Firefox 55 On Ubuntu 16.04 . > > Not Working In Firefox 56 (Developer Edition) On Windows 10 .. Please note that this feature is only enabled on *Nightly* by default yet, as the summary of this bug states. You may want to follow their implementation in bug 888320. Jessica, what's the bug for shipping the date and time inputs? Is that also 888320? (Though I think you shouldn't wait for the other input types to ship the currently implemented ones, only fix the remaining issues.) Sebastian

(In reply to Sebastian Zartner [:sebo] from comment #30) > (In reply to sivaprathap.konduru from comment #29) > > (In reply to shivarajnaidu from comment #27) > > > Still Not Enabled In Firefox 55 On Ubuntu 16.04 . > > > > Not Working In Firefox 56 (Developer Edition) On Windows 10 .. > > Please note that this feature is only enabled on *Nightly* by default yet, > as the summary of this bug states. > You may want to follow their implementation in bug 888320. > > Jessica, what's the bug for shipping the date and time inputs? Is that also > 888320? (Though I think you shouldn't wait for the other input types to ship > the currently implemented ones, only fix the remaining issues.) > > Sebastian Yes, this feature is only enabled on *Nightly* right now and is expected to ship in Firefox v57. We don't have the bug to enable this feature on release versions yet, but you can track bugs and roadmap here: https://wiki.mozilla.org/TPE_DOM/Date_time_input_types#Roadmap

Enabled on beta (57) recently. see Bug 1399036