1286182 - Implement the layout for <input type=date>

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect, P1)

Description

[Jessica Jong [:jessica] (inactive)]

This bug is about the layout part for date input box. The date picker is handled in Bug 1283385. 

During MozLondon, I asked :jwatt how we should implement the layout pieces for date/time input. The options we had were:
1. Shadow DOM
2. XBL, like videocontrols
3. Pure native anonymous content

For option 1, we asked :wchen about the feasibilty of using shadow dom, but he said shadow dom is not ready to use internally, due to some security concerns. 

After some discussion on implementation, we tend to use XBL, since it'd be easier to handle the complexity of date/time inputs.
Some of the complexity we can think of now are:
- Multiple text boxes (year, month, day, hour, minute, etc) for one input type.
- Focus transferring to the corresponding text box.
- Localization, different ordering for year, month and day. Also, different separators.
- Hiding fields dynamically (milliseconds in time is optional).
- Interaction between picker and the input box.

I am not sure if it's the best way, but I'll try to implement using XBL first, and see if it can satisfy our requirements.

Suggestions are welcome!

Comment 1

[Jessica Jong [:jessica] (inactive)]

One more thing to add is, we believe that this, and other input types, will switch to use shadow dom when it's ready to use internally, since it's pure html technology and more flexible, but until then, we'll try using XBL.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

FWIW, I don't know what makes shadow DOM more flexible than XBL. XBL certainly has more features than shadow dom v1. But sure, once shadow DOM v1 is done, we'll need to figure out how to integrate that to native anonymous content and start using it there.

Comment 3

[Jessica Jong [:jessica] (inactive)]

Oh, I thought using shadow DOM, it would be easier for web authors to customize the input control by replacing the shadow content tree. But as you said, we can figure out how to do it once shadow DOM is ready.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Web visible Shadom DOM v1 is explicitly disabled for elements like <input>.
See http://w3c.github.io/webcomponents/spec/shadow/#widl-Element-attachShadow-ShadowRoot-ShadowRootInit-shadowRootInitDict

Comment 5

[Jessica Jong [:jessica] (inactive)]

(In reply to Olli Pettay [:smaug] from comment #4)
> Web visible Shadom DOM v1 is explicitly disabled for elements like <input>.
> See
> http://w3c.github.io/webcomponents/spec/shadow/#widl-Element-attachShadow-
> ShadowRoot-ShadowRootInit-shadowRootInitDict

Oh, right! I remember you mentioned this during MozLondon. Anyway, I'll keep working using XBL for now.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I would think that for either the Shadow DOM or the XBL option, we would be attaching the anonymous content tree (Shadow DOM or XBL) to a native-anonymous element *inside* the input, not to the input itself.  At least, I'd certainly think that's how we would do the XBL option, since I don't think XBL has the security properties we need to attach it directly to an element that the Web page can access.  It seems to me that we *could* do the same with the shadow DOM.  Is there a reason to still prefer XBL over shadow DOM if we were to attach the shadow DOM to native-anonymous content inside the input?

(I'm rather hesitant to add yet more code in a language that we know we'd like to get rid of in the long term.)

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

(In reply to David Baron :dbaron: ⌚️UTC+8 (review requests must explain patch) from comment #6)
> I would think that for either the Shadow DOM or the XBL option, we would be
> attaching the anonymous content tree (Shadow DOM or XBL) to a
> native-anonymous element *inside* the input, not to the input itself.
Yes, at least for now.

>  At
> least, I'd certainly think that's how we would do the XBL option, since I
> don't think XBL has the security properties we need to attach it directly to
> an element that the Web page can access.  It seems to me that we *could* do
> the same with the shadow DOM.  Is there a reason to still prefer XBL over
> shadow DOM if we were to attach the shadow DOM to native-anonymous content
> inside the input?
We don't have Shadow DOM v1 implemented. v0 implementation will go away.
Use of v0 is equally deprecated to XBL usage, but XBL won't be going away any time soon. v0 will go away once v1 is there.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

OK, sounds like XBL is the way to go, then.

(However, I am still encouraging Jessica to use HTML + CSS Flexbox inside the XBL, rather than XUL flexbox.  That is stable and more supported than XUL flexbox.)

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Sure, if HTML+CSS can be used, great.

Comment 10

[Jessica Jong [:jessica] (inactive)]

Attachment: WIP

Comment 11

[Jessica Jong [:jessica] (inactive)]

Attachment: patch, v1.

Comment 12

[Jessica Jong [:jessica] (inactive)]

Comment on attachment 8814531 [details] [diff] [review]
patch, v1.

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

This patch is to implement the input box for input type=date, so most of the work is in the XBL part. Integration with the date picker will be done in bug 1320225.

Mike, could you take a look at the XBL bits?
I'll ask for Olli's review for DOM part once he is back from vacation.

Thanks. :)

Comment 13

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

Comment on attachment 8814531 [details] [diff] [review]
patch, v1.

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

Hey Jessica,

This looks great! I do think you should have some dom / layout peers look at the work inside dom/html, and layout/[base|forms]. I did review the tests though - it's the native code changes that I'm less sure of.

The tests and the XBL binding changes look okay to me, with some minor notes below (the one that jumps out to me is the leapyear calculation one).

::: accessible/tests/mochitest/jsat/test_output.html
@@ -618,5 @@
>        <label for="input5">Boring label</label><input id="input5" type="checkbox" checked></input>
>        <label for="password">Secret Password</label><input id="password" type="password"></input>
>        <label for="radio_unselected">any old radio button</label><input id="radio_unselected" type="radio"></input>
>        <label for="radio_selected">a unique radio button</label><input id="radio_selected" type="radio" checked></input>
> -      <input id="date" type="date" value="2011-09-29" />

Why is this test being removed? What _is_ going to be uttered when an <input type="date"> is selected?

::: toolkit/content/widgets/datetimebox.xml
@@ +123,5 @@
> +            return;
> +          }
> +
> +          this.log("setFieldsFromInputValue: " + value);
> +          let [year, month, day] = value.split('-');

Might as well be consistent and use double-quotes here.

@@ +139,5 @@
> +        <parameter name="aMonth"/>
> +        <parameter name="aYear"/>
> +        <body>
> +        <![CDATA[
> +          if (aMonth == 2) {

This scares me a little. Maybe leapyears always kinda do.

Can't we use JavaScript's Date() to do this work for us? Example: http://stackoverflow.com/questions/1184334/get-number-days-in-a-specified-month-using-javascript

@@ +193,5 @@
> +            // set to empty.
> +            return;
> +          }
> +
> +          let date = year + "-" + month + "-" + day;

Alternatively:

let date = [year, month, day].join("-");

Although I can't see too much advantage doing it one way over the other.

@@ +230,5 @@
> +            targetField.select();
> +
> +            let n = Number(buffer);
> +            let max = targetField.getAttribute("max");
> +            if (buffer.length >= targetField.maxLength || n * 10 > max) {

Can you explain this a bit more? What's this buffer being used for exactly[1], and where's the magic multiplier "10" coming from?

[1]: Is the buffer to make it so that the field can accept numbers from the keyboard without being interrupted by non-numeric characters?

@@ +327,5 @@
> +
> +      <method name="getCurrentValue">
> +        <body>
> +        <![CDATA[
> +          let year;

Just to be explicit: we're okay with these being undefined if empty?

Comment 14

[Jessica Jong [:jessica] (inactive)]

(In reply to Mike Conley (:mconley) from comment #13)
> Comment on attachment 8814531 [details] [diff] [review]
> patch, v1.
> 
> Review of attachment 8814531 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hey Jessica,
> 
> This looks great! I do think you should have some dom / layout peers look at
> the work inside dom/html, and layout/[base|forms]. I did review the tests
> though - it's the native code changes that I'm less sure of.

Thanks Mike, I'll ask :samug for the native code changes.

> 
> The tests and the XBL binding changes look okay to me, with some minor notes
> below (the one that jumps out to me is the leapyear calculation one).
> 
> ::: accessible/tests/mochitest/jsat/test_output.html
> @@ -618,5 @@
> >        <label for="input5">Boring label</label><input id="input5" type="checkbox" checked></input>
> >        <label for="password">Secret Password</label><input id="password" type="password"></input>
> >        <label for="radio_unselected">any old radio button</label><input id="radio_unselected" type="radio"></input>
> >        <label for="radio_selected">a unique radio button</label><input id="radio_selected" type="radio" checked></input>
> > -      <input id="date" type="date" value="2011-09-29" />
> 
> Why is this test being removed? What _is_ going to be uttered when an <input
> type="date"> is selected?

type="date" is not longer a text input, so the test fails. Our MVP for type="date" does not include localization nor accessibility, so I'm removing the test for now.

> 
> ::: toolkit/content/widgets/datetimebox.xml
> @@ +123,5 @@
> > +            return;
> > +          }
> > +
> > +          this.log("setFieldsFromInputValue: " + value);
> > +          let [year, month, day] = value.split('-');
> 
> Might as well be consistent and use double-quotes here.

Sure, will do.

> 
> @@ +139,5 @@
> > +        <parameter name="aMonth"/>
> > +        <parameter name="aYear"/>
> > +        <body>
> > +        <![CDATA[
> > +          if (aMonth == 2) {
> 
> This scares me a little. Maybe leapyears always kinda do.
> 
> Can't we use JavaScript's Date() to do this work for us? Example:
> http://stackoverflow.com/questions/1184334/get-number-days-in-a-specified-
> month-using-javascript

Using JavaScript's Date() sounds good to me. 

> 
> @@ +193,5 @@
> > +            // set to empty.
> > +            return;
> > +          }
> > +
> > +          let date = year + "-" + month + "-" + day;
> 
> Alternatively:
> 
> let date = [year, month, day].join("-");
> 
> Although I can't see too much advantage doing it one way over the other.

Using join() looks nicer, thanks. :)

> 
> @@ +230,5 @@
> > +            targetField.select();
> > +
> > +            let n = Number(buffer);
> > +            let max = targetField.getAttribute("max");
> > +            if (buffer.length >= targetField.maxLength || n * 10 > max) {
> 
> Can you explain this a bit more? What's this buffer being used for
> exactly[1], and where's the magic multiplier "10" coming from?
> 
> [1]: Is the buffer to make it so that the field can accept numbers from the
> keyboard without being interrupted by non-numeric characters?

The buffer is to save what is already typed in by the user, it is different from what is shown in the input field. For example, in day field, if user types in "2", the buffer would be "2", but it is shown as "02", then user types in "3", the buffer would be "23", and shown as "23".
When the buffer's length reaches the maxlength, we'll advance to the next input field, also if the user enters a digit that does not allow more digits (this is the "n * 10 > max" part), e.g. "2" in month field or "4" in day field, we'll advance the next field.

> 
> @@ +327,5 @@
> > +
> > +      <method name="getCurrentValue">
> > +        <body>
> > +        <![CDATA[
> > +          let year;
> 
> Just to be explicit: we're okay with these being undefined if empty?

Yes, if the field is undefined, the picker does not update that field.

Comment 15

[Jessica Jong [:jessica] (inactive)]

Attachment: patch, v2.

Addressed review comment 13.

Olli, could you take a look at the native code changes? Thanks.

Comment 16

[Jessica Jong [:jessica] (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9367784fcca3fe10013fa070f9eddb6aacfae437

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5122a14c6fe6
Implement the layout for <input type=date>. r=mconley,smaug

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5122a14c6fe6

Comment 19

[Sylvestre Ledru [:Sylvestre]]

Release Note Request (optional, but appreciated)
[Why is this notable]: New cool feature
[Affects Firefox for Android]: Don't know?!
[Suggested wording]: New form element: <input type=date>
[Links (documentation, blog post, etc)]: ?

Comment 20

[Sebastian Zartner [:sebo]]

(In reply to Sylvestre Ledru [:sylvestre] from comment #19)
> Release Note Request (optional, but appreciated)
> [Why is this notable]: New cool feature
> [Affects Firefox for Android]: Don't know?!
> [Suggested wording]: New form element: <input type=date>
> [Links (documentation, blog post, etc)]: ?

This feature is still behind the preference dom.forms.datetime, so it is probably too early to request a release note.
Or did I miss something and it is planned to let it ride the trains already?

Sebastian

Comment 21

[Sylvestre Ledru [:Sylvestre]]

I haven't added it to the release notes, it is just to keep it on our radar.

Comment 22

[Sebastian Zartner [:sebo]]

I see. Thank you for the fast reply!
Anyway, it would be good to know the current plan about releasing this feature. I assume it will be enabled by default in bug 1283381. Is that correct, Jessica? Or will all input types be implemented first before releasing it, i.e. will users have to wait for bug 888320 to be fixed?

Sebastian

Comment 23

[Jessica Jong [:jessica] (inactive)]

(In reply to Sebastian Zartner [:sebo] from comment #22)
> I see. Thank you for the fast reply!
> Anyway, it would be good to know the current plan about releasing this
> feature. I assume it will be enabled by default in bug 1283381. Is that
> correct, Jessica? Or will all input types be implemented first before
> releasing it, i.e. will users have to wait for bug 888320 to be fixed?
> 
> Sebastian

Our current plan is to finish implementing input type=date and type=time, including localization and blocker bugs, and ship these two types first, since they are the most used among the five datetime input types. We don't have a specific date for shipping, but if you're interested, you can follow our wiki page: https://wiki.mozilla.org/TPE_DOM/Date_time_input_types#Roadmap

Comment 24

[Sebastian Zartner [:sebo]]

(In reply to Jessica Jong [:jessica] from comment #23)
> Our current plan is to finish implementing input type=date and type=time,
> including localization and blocker bugs, and ship these two types first,
> since they are the most used among the five datetime input types. We don't
> have a specific date for shipping, but if you're interested, you can follow
> our wiki page: https://wiki.mozilla.org/TPE_DOM/Date_time_input_types#Roadmap

Thank you for the info!
FWIW, I've added an entry to the experimental features at https://developer.mozilla.org/en-US/Firefox/Experimental_features#HTML.

Sebastian

Comment 25

[Jessica Jong [:jessica] (inactive)]

(In reply to Sebastian Zartner [:sebo] from comment #24)
> Thank you for the info!
> FWIW, I've added an entry to the experimental features at
> https://developer.mozilla.org/en-US/Firefox/Experimental_features#HTML.
> 
> Sebastian

Cool, thanks!

Comment 26

[Liz Henry (:lizzard) (relman/hg->git project)]

Since this is documented on MDN in developer features, I don't think we need a release note. At least, not until we release it as on by default.