Note: There are a few cases of duplicates in user autocompletion which are being worked on.

[DateTimeInput] Integration of input type=date input box with picker

RESOLVED FIXED in Firefox 53

Status

()

Core
Layout: Form Controls
P1
normal
RESOLVED FIXED
8 months ago
6 months ago

People

(Reporter: jessica, Assigned: jessica)

Tracking

(Blocks: 2 bugs)

Trunk
mozilla53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

(Whiteboard: [milestone4])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

8 months ago
Bug 1286182 is about the layout and the functions of the input box for input type=date. In this bug, we'll integrate the input box with the date picker, which is implemented in Bug 1283385.
Whiteboard: [milestone4]
(Assignee)

Comment 1

8 months ago
Created attachment 8816061 [details] [diff] [review]
Bug 1320225 - [DateTimeInput] Integration of input type=date picker with input box (part 1)

patch based on Bug 1286182.
Assignee: nobody → jjong
Priority: -- → P1
Comment hidden (mozreview-request)

Comment 3

7 months ago
(In reply to Scott Wu [:scottwu] from comment #2)
> Created attachment 8820574 [details]
> Bug 1320225 - [DateTimeInput] Integration of input type=date picker with
> input box (part 2)
> 
> Review commit: https://reviewboard.mozilla.org/r/100054/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/100054/

This patch integrates the picker with the input box.

I noticed that there are two inconsistencies with the layout and picker portion.
- First being that the month in layout starts from 1 to 12 whereas in picker it starts from 0 to 11, following JavaScript convention.
- Second thing is that in layout the day of the month is named "day", whereas in picker it's named "date". I'm not sure which is less confusing, since both terms have more than one meaning: "date" could mean the whole thing (ymd) or just the date (d), and "day" could mean day of the week too. I chose "date" to align with JS APIs.

Right now this patch just maps one to the other without altering the conventions.
(Assignee)

Comment 4

7 months ago
(In reply to Scott Wu [:scottwu] from comment #3)
> This patch integrates the picker with the input box.
> 
> I noticed that there are two inconsistencies with the layout and picker
> portion.
> - First being that the month in layout starts from 1 to 12 whereas in picker
> it starts from 0 to 11, following JavaScript convention.
> - Second thing is that in layout the day of the month is named "day",
> whereas in picker it's named "date". I'm not sure which is less confusing,
> since both terms have more than one meaning: "date" could mean the whole
> thing (ymd) or just the date (d), and "day" could mean day of the week too.
> I chose "date" to align with JS APIs.
> 
> Right now this patch just maps one to the other without altering the
> conventions.

Thanks Scott for pointing this out. The reason I use the name "day" as day of the month and "1-12" based month is because the spec [1] uses them, so I didn't think too much while implementing. For the webidl part (and maybe DOM part), I prefer to keep consistent with the spec, the rest can be discussed. Let's see what our reviewer thinks. ;)

[1] https://html.spec.whatwg.org/multipage/infrastructure.html#valid-date-string
(Assignee)

Updated

7 months ago
Attachment #8816061 - Attachment description: patch, v1. → Bug 1320225 - [DateTimeInput] Integration of input type=date picker with input box (part 1)

Updated

7 months ago
Blocks: 1328219
(Assignee)

Comment 5

7 months ago
Comment on attachment 8816061 [details] [diff] [review]
Bug 1320225 - [DateTimeInput] Integration of input type=date picker with input box (part 1)

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

Hi Mike, there are two parts for the integration of input box and picker. This part are changes for the input box and passing the data in e10s. The second part are changes for the picker. May we have your review? Thanks.
Attachment #8816061 - Flags: review?(mconley)
(Assignee)

Updated

7 months ago
Attachment #8820574 - Flags: review?(mconley)
Comment on attachment 8816061 [details] [diff] [review]
Bug 1320225 - [DateTimeInput] Integration of input type=date picker with input box (part 1)

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

Thanks!

::: dom/webidl/HTMLInputElement.webidl
@@ +238,5 @@
>  
>  dictionary DateTimeValue {
>    long hour;
>    long minute;
> +  long year;

This looks okay to me, but you're going to need a DOM peer to sign off on this change, I think.

::: toolkit/content/browser-content.js
@@ +1735,5 @@
>          });
>          break;
>        }
>        case "MozUpdateDateTimePicker": {
>          let value = this._inputElement.getDateTimeInputBoxValue();

getDateTimeInputBoxValue will _always_ return an Object, right?
Attachment #8816061 - Flags: review?(mconley) → review+
Comment on attachment 8820574 [details]
Bug 1320225 - [DateTimeInput] Integration of input type=date picker with input box (part 2).

https://reviewboard.mozilla.org/r/100054/#review103028

Seems okay to me. Thanks!

::: toolkit/content/widgets/datetimepopup.xml:21
(Diff revision 1)
>        <field name="TIME_PICKER_WIDTH" readonly="true">"12em"</field>
>        <field name="TIME_PICKER_HEIGHT" readonly="true">"21em"</field>
> +      <field name="DATE_PICKER_WIDTH" readonly="true">"23.1em"</field>
> +      <field name="DATE_PICKER_HEIGHT" readonly="true">"20.7em"</field>

At some point, we should really find a way of pulling these out into our themeing CSS somehow.

::: toolkit/content/widgets/datetimepopup.xml:84
(Diff revision 1)
> +                  // datetimeinputbox uses 'day' instead of 'date' to represent the days in a month
> +                  date: day

:/
Attachment #8820574 - Flags: review?(mconley) → review+
(Assignee)

Comment 8

7 months ago
Thanks Mike for the review.

(In reply to Mike Conley (:mconley) from comment #6)
> Comment on attachment 8816061 [details] [diff] [review]
> Bug 1320225 - [DateTimeInput] Integration of input type=date picker with
> input box (part 1)
> 
> Review of attachment 8816061 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks!
> 
> ::: dom/webidl/HTMLInputElement.webidl
> @@ +238,5 @@
> >  
> >  dictionary DateTimeValue {
> >    long hour;
> >    long minute;
> > +  long year;
> 
> This looks okay to me, but you're going to need a DOM peer to sign off on
> this change, I think.

Sure, will ask :smaug for that.

> 
> ::: toolkit/content/browser-content.js
> @@ +1735,5 @@
> >          });
> >          break;
> >        }
> >        case "MozUpdateDateTimePicker": {
> >          let value = this._inputElement.getDateTimeInputBoxValue();
> 
> getDateTimeInputBoxValue will _always_ return an Object, right?

Yes, it's a bug if not..
(Assignee)

Comment 9

7 months ago
Comment on attachment 8816061 [details] [diff] [review]
Bug 1320225 - [DateTimeInput] Integration of input type=date picker with input box (part 1)

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

Hi Olli, may I have your review on the .webidl part? Thanks.
Attachment #8816061 - Flags: review?(bugs)

Comment 10

7 months ago
(In reply to Mike Conley (:mconley) from comment #7)
> ::: toolkit/content/widgets/datetimepopup.xml:84
> (Diff revision 1)
> > +                  // datetimeinputbox uses 'day' instead of 'date' to represent the days in a month
> > +                  date: day
> 
> :/

Yeah it's sub-optimal, but we're not sure which naming convention to adopt, for reasons detailed in comment 4. Any suggestions?
Flags: needinfo?(mconley)

Updated

7 months ago
Attachment #8816061 - Flags: review?(bugs) → review+

Comment 11

7 months ago
That was for the .webidl
Comment hidden (mozreview-request)

Comment 13

7 months ago
mozreview-review-reply
Comment on attachment 8820574 [details]
Bug 1320225 - [DateTimeInput] Integration of input type=date picker with input box (part 2).

https://reviewboard.mozilla.org/r/100054/#review103028

> At some point, we should really find a way of pulling these out into our themeing CSS somehow.

Initially I tried to move the style to datetimepopup.css which styles the xbl, but I wasn't able to select the iframe, possibly because it's being wrapped inside the arrow panel elements. I could add it to browser.css, with the downside of making browser.css few lines fatter, but maybe it's better than having it in xbl?
(In reply to Scott Wu [:scottwu] from comment #10)
> (In reply to Mike Conley (:mconley) from comment #7)
> > ::: toolkit/content/widgets/datetimepopup.xml:84
> > (Diff revision 1)
> > > +                  // datetimeinputbox uses 'day' instead of 'date' to represent the days in a month
> > > +                  date: day
> > 
> > :/
> 
> Yeah it's sub-optimal, but we're not sure which naming convention to adopt,
> for reasons detailed in comment 4. Any suggestions?

Not sure if "date" or "day" is better, but the idea of using the spec's terminology makes sense to me. What matters more probably is that we're just consistent where it's used. If the spec uses "day", then let's use "day" everywhere, and if we ever decide that the language could be better, we should update ourselves all at once (and try to update the spec too! :) ).
Flags: needinfo?(mconley)
Comment on attachment 8820574 [details]
Bug 1320225 - [DateTimeInput] Integration of input type=date picker with input box (part 2).

https://reviewboard.mozilla.org/r/100054/#review103028

> Initially I tried to move the style to datetimepopup.css which styles the xbl, but I wasn't able to select the iframe, possibly because it's being wrapped inside the arrow panel elements. I could add it to browser.css, with the downside of making browser.css few lines fatter, but maybe it's better than having it in xbl?

Hm... reading https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XBL/XBL_1.0_Reference/Anonymous_Content#Binding_Stylesheets and https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XBL/XBL_1.0_Reference/Elements#stylesheet, nothing in here suggests why you wouldn't be able to select the iframe. I'm pretty certain this should be possible... perhaps dao might know why?

Comment 16

6 months ago
mozreview-review-reply
Comment on attachment 8820574 [details]
Bug 1320225 - [DateTimeInput] Integration of input type=date picker with input box (part 2).

https://reviewboard.mozilla.org/r/100054/#review103028

> Hm... reading https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XBL/XBL_1.0_Reference/Anonymous_Content#Binding_Stylesheets and https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XBL/XBL_1.0_Reference/Elements#stylesheet, nothing in here suggests why you wouldn't be able to select the iframe. I'm pretty certain this should be possible... perhaps dao might know why?

Ok I think I know what's going on now. Since my binding doesn't have its own <content>, and is getting its element with <children> instead, the element (iframe) doesn't actually belong to the binding, so the binding cannot style it. I'll open a separate bug for this issue.

> :/

I'll mark this as fixed then, since from DOM's perspective it makes more sense to use "day" to align with the spec, and from picker's perspective it makes sense to use "date" to align with JavaScript methods.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 19

6 months ago
mozreview-review-reply
Comment on attachment 8820574 [details]
Bug 1320225 - [DateTimeInput] Integration of input type=date picker with input box (part 2).

https://reviewboard.mozilla.org/r/100054/#review103028

> I'll mark this as fixed then, since from DOM's perspective it makes more sense to use "day" to align with the spec, and from picker's perspective it makes sense to use "date" to align with JavaScript methods.

After discussion with Jessica I think it makes sense for me to follow the DOM and the spec convension. I have changed "date" to "day", and changed "day" to "dayOfWeek".

Comment 20

6 months ago
For some reason the r+ flag on bugzilla got lost but the one on mozreview is still there. I wonder if it matters? And Mike, would you like to take a look again before I check it in?
Flags: needinfo?(mconley)
Comment on attachment 8820574 [details]
Bug 1320225 - [DateTimeInput] Integration of input type=date picker with input box (part 2).

https://reviewboard.mozilla.org/r/100054/#review104946

Looks good to me!

A suggestion for the future - when doing follow-up work (like renaming "date" to "day"), probably best to put that as a separate commit to make it easier to atomically review. Thanks!
Attachment #8820574 - Flags: review+
(Assignee)

Comment 22

6 months ago
Created attachment 8826527 [details] [diff] [review]
Bug 1320225 - [DateTimeInput] Integration of input type=date picker with input box (part 1), v2.

Rebased and added reviewer's name.
Attachment #8816061 - Attachment is obsolete: true
Attachment #8826527 - Flags: review+
Comment hidden (mozreview-request)

Comment 24

6 months ago
mozreview-review-reply
Comment on attachment 8820574 [details]
Bug 1320225 - [DateTimeInput] Integration of input type=date picker with input box (part 2).

https://reviewboard.mozilla.org/r/100054/#review104946

Thanks Mike. I'll make sure I do that next time!

Comment 25

6 months ago
Rebased and added reviewer's name.
Flags: needinfo?(mconley)
Keywords: checkin-needed

Comment 26

6 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/91a65ad6d3ca
[DateTimeInput] Integration of input type=date picker with input box (part 2). r=mconley
Keywords: checkin-needed

Comment 27

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/91a65ad6d3ca
Status: NEW → RESOLVED
Last Resolved: 6 months ago
status-firefox53: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53

Comment 28

6 months ago
There's also a part 1, but it's not on mozreview so maybe that's why it's not picked up by autolander.
Status: RESOLVED → REOPENED
Flags: needinfo?(wkocher)
Resolution: FIXED → ---

Comment 29

6 months ago
Pushed by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/87a0e0ccdc34
[DateTimeInput] Integration of input type=date input box with picker (part 1). r=mconley,smaug

Comment 30

6 months ago
Pushed part 1 to m-i in comment 29, and wait for merge to m-c
Flags: needinfo?(wkocher)
(Assignee)

Comment 31

6 months ago
(In reply to Iris Hsiao [:ihsiao] from comment #30)
> Pushed part 1 to m-i in comment 29, and wait for merge to m-c

Thank you for helping out.

Comment 32

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/87a0e0ccdc34
Status: REOPENED → RESOLVED
Last Resolved: 6 months ago6 months ago
Resolution: --- → FIXED

Comment 33

6 months ago
Seems there is a little bug with the implementation which I have described in bug 1334038
You need to log in before you can comment on or make changes to this bug.