Last Comment Bug 1286182 - Implement the layout for <input type=date>
: Implement the layout for <input type=date>
Status: RESOLVED FIXED
[milestone4]
:
Product: Core
Classification: Components
Component: Layout: Form Controls (show other bugs)
: Trunk
: Unspecified Unspecified
P1 normal with 4 votes (vote)
: mozilla53
Assigned To: Jessica Jong [:jessica] (OOO Feb. 25-28)
:
: Jet Villegas (:jet)
Mentors:
Depends on:
Blocks: datetime 1320227 1320225
  Show dependency treegraph
 
Reported: 2016-07-12 01:31 PDT by Jessica Jong [:jessica] (OOO Feb. 25-28)
Modified: 2017-01-24 01:29 PST (History)
15 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
?


Attachments
WIP (22.35 KB, patch)
2016-11-21 01:49 PST, Jessica Jong [:jessica] (OOO Feb. 25-28)
no flags Details | Diff | Splinter Review
patch, v1. (55.30 KB, patch)
2016-11-25 20:52 PST, Jessica Jong [:jessica] (OOO Feb. 25-28)
mconley: review+
Details | Diff | Splinter Review
patch, v2. (55.17 KB, patch)
2016-12-19 00:50 PST, Jessica Jong [:jessica] (OOO Feb. 25-28)
bugs: review+
Details | Diff | Splinter Review

Description User image Jessica Jong [:jessica] (OOO Feb. 25-28) 2016-07-12 01:31:06 PDT
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 User image Jessica Jong [:jessica] (OOO Feb. 25-28) 2016-07-12 08:18:50 PDT
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 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2016-07-12 08:37:18 PDT
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 User image Jessica Jong [:jessica] (OOO Feb. 25-28) 2016-07-18 02:55:13 PDT
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 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2016-07-18 04:14:12 PDT
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 User image Jessica Jong [:jessica] (OOO Feb. 25-28) 2016-07-18 19:39:27 PDT
(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 User image David Baron :dbaron: ⌚️UTC-8 2016-07-18 23:33:32 PDT
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 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2016-07-19 03:27:23 PDT
(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 User image David Baron :dbaron: ⌚️UTC-8 2016-07-19 06:03:03 PDT
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 User image Olli Pettay [:smaug] (pto-ish for couple of days) 2016-07-19 06:37:13 PDT
Sure, if HTML+CSS can be used, great.
Comment 10 User image Jessica Jong [:jessica] (OOO Feb. 25-28) 2016-11-21 01:49:48 PST
Created attachment 8812717 [details] [diff] [review]
WIP
Comment 11 User image Jessica Jong [:jessica] (OOO Feb. 25-28) 2016-11-25 20:52:44 PST
Created attachment 8814531 [details] [diff] [review]
patch, v1.
Comment 12 User image Jessica Jong [:jessica] (OOO Feb. 25-28) 2016-11-28 01:46:51 PST
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 User image Mike Conley (:mconley) 2016-12-12 16:27:26 PST
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 User image Jessica Jong [:jessica] (OOO Feb. 25-28) 2016-12-18 19:55:03 PST
(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 User image Jessica Jong [:jessica] (OOO Feb. 25-28) 2016-12-19 00:50:20 PST
Created attachment 8819799 [details] [diff] [review]
patch, v2.

Addressed review comment 13.

Olli, could you take a look at the native code changes? Thanks.
Comment 17 User image Pulsebot 2016-12-20 01:07:19 PST
Pushed by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5122a14c6fe6
Implement the layout for <input type=date>. r=mconley,smaug
Comment 18 User image Wes Kocher (:KWierso) 2016-12-20 11:44:24 PST
https://hg.mozilla.org/mozilla-central/rev/5122a14c6fe6
Comment 19 User image Sylvestre Ledru [:sylvestre] 2017-01-22 08:43:57 PST
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 User image Sebastian Zartner [:sebo] 2017-01-22 13:47:26 PST
(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 User image Sylvestre Ledru [:sylvestre] 2017-01-22 13:49:24 PST
I haven't added it to the release notes, it is just to keep it on our radar.
Comment 22 User image Sebastian Zartner [:sebo] 2017-01-22 13:59:59 PST
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 User image Jessica Jong [:jessica] (OOO Feb. 25-28) 2017-01-23 01:39:44 PST
(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 User image Sebastian Zartner [:sebo] 2017-01-23 04:38:30 PST
(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 User image Jessica Jong [:jessica] (OOO Feb. 25-28) 2017-01-24 01:29:57 PST
(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!

Note You need to log in before you can comment on or make changes to this bug.