1366188 - Enable dom.forms.datetime by default on Nightly

Status: RESOLVED FIXED

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

Description

[Jessica Jong [:jessica] (inactive)]

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

Comment 1

[Jessica Jong [:jessica] (inactive)]

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

Comment 2

[Andreas Tolfsen ❲:ato❳]

(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.

Comment 3

[Jessica Jong [:jessica] (inactive)]

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.

Comment 4

[Jessica Jong [:jessica] (inactive)]

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?

Comment 5

[Andreas Tolfsen ❲:ato❳]

(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);
> };

Comment 6

[Andreas Tolfsen ❲:ato❳]

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

Comment 7

[Jessica Jong [:jessica] (inactive)]

(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 8

[Jessica Jong [:jessica] (inactive)]

Attachment: fix marionette DateTimeValue.

Comment 9

[Andreas Tolfsen ❲:ato❳]

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.

Comment 10

[Jessica Jong [:jessica] (inactive)]

(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

Comment 11

[Jessica Jong [:jessica] (inactive)]

Attachment: patch, v2.

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()`?

Comment 12

[Andreas Tolfsen ❲:ato❳]

(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 13

[Andreas Tolfsen ❲:ato❳]

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.

Comment 14

[Jessica Jong [:jessica] (inactive)]

(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.

Comment 15

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 1: fix Marionette's DateTimeValue. r=ato

addressed review comment 14, carrying r+.

Comment 16

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 2: enable input date/time pref on Nightly.

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

Comment 17

[Mike Conley (:mconley) (:⚙️)]

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...

Comment 18

[Jessica Jong [:jessica] (inactive)]

(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!

Comment 19

[Jessica Jong [:jessica] (inactive)]

Attachment: Part 2: enable date/time pref on Nightly, v2.

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

Comment 20

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 21

[Jessica Jong [:jessica] (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=99466e041f6e1c491e2fbe6e34881c66b1b69319

Comment 22

[Pulsebot]

Pushed by [e-mail]:
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

Comment 23

[Ryan VanderMeulen [:RyanVM]]

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

Comment 24

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Congrats to Jessica and the Date/time team!

Comment 25

[Jessica Jong [:jessica] (inactive)]

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

\o/

Comment 26

[Sebastian Zartner [:sebo]]

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

Comment 27

[shivarajnaidu]

Still Not Enabled In Firefox 55 On Ubuntu 16.04 .

Comment 28

[shivarajnaidu]

(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..

Comment 29

[sivaprathap.konduru]

(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 ..

Comment 30

[Sebastian Zartner [:sebo]]

(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

Comment 31

[Jessica Jong [:jessica] (inactive)]

(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

Comment 32

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

Enabled on beta (57) recently. see Bug 1399036