Jessica Jong [:jessica] (inactive) Assignee
	Description • 8 years ago

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!

	Jessica Jong [:jessica] (inactive) Assignee
	Comment 1 • 8 years ago

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.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 2 • 8 years ago

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.

	Jessica Jong [:jessica] (inactive) Assignee
	Comment 3 • 8 years ago

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.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 4 • 8 years ago

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

	Jessica Jong [:jessica] (inactive) Assignee
	Comment 5 • 8 years ago

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

	David Baron :dbaron:
	Comment 6 • 8 years ago

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

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 7 • 8 years ago

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

	David Baron :dbaron:
	Comment 8 • 8 years ago

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

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 9 • 8 years ago

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

	Jessica Jong [:jessica] (inactive) Assignee
	Comment 12 • 8 years ago

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

	Mike Conley (:mconley) (:⚙️)
	Comment 13 • 8 years ago

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?

	Jessica Jong [:jessica] (inactive) Assignee
	Comment 14 • 8 years ago

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

	Jessica Jong [:jessica] (inactive) Assignee
	Comment 15 • 8 years ago

Addressed review comment 13.

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

	Jessica Jong [:jessica] (inactive) Assignee
	Comment 16 • 8 years ago

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

	Pulsebot
	Comment 17 • 8 years ago

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

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 18 • 8 years ago
bugherder

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

	Sylvestre Ledru [:Sylvestre]
	Comment 19 • 8 years ago

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)]: ?

	Sebastian Zartner [:sebo]
	Comment 20 • 8 years ago

(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

	Sylvestre Ledru [:Sylvestre]
	Comment 21 • 8 years ago

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

	Sebastian Zartner [:sebo]
	Comment 22 • 8 years ago

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

	Jessica Jong [:jessica] (inactive) Assignee
	Comment 23 • 8 years ago

(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

	Sebastian Zartner [:sebo]
	Comment 24 • 8 years ago

(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

	Jessica Jong [:jessica] (inactive) Assignee
	Comment 25 • 8 years ago

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

	Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)
	Comment 26 • 7 years ago

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.