Closed Bug 1320225 Opened 8 years ago Closed 7 years ago

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

Categories

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

Product:
Core
Component:
Layout: Form Controls
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox53 --- fixed

People

(Reporter: jessica, Assigned: jessica)

References

(Blocks 1 open bug)

Details

(Whiteboard: [milestone4])

Attachments

(2 files, 1 obsolete file)

Bug 1320225 - [DateTimeInput] Integration of input type=date picker with input box (part 2).
59 bytes, text/x-review-board-request
mconley
: review+
Details
Bug 1320225 - [DateTimeInput] Integration of input type=date picker with input box (part 1), v2.
3.79 KB, patch
jessica
: review+
Details | Diff | Splinter Review 
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]
Priority: -- → P1
(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.
(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
Attachment #8816061 - Attachment description: patch, v1. → Bug 1320225 - [DateTimeInput] Integration of input type=date picker with input box (part 1)
Blocks: 1328219
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)
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+
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..
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)
(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)
Attachment #8816061 - Flags: review?(bugs) → review+
That was for the .webidl
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 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 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".
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+
Rebased and added reviewer's name.
Attachment #8816061 - Attachment is obsolete: true
Attachment #8826527 - Flags: 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/#review104946

Thanks Mike. I'll make sure I do that next time!
Rebased and added reviewer's name.
Flags: needinfo?(mconley)
Keywords: checkin-needed
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
https://hg.mozilla.org/mozilla-central/rev/91a65ad6d3ca
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox53: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
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 → ---
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
Pushed part 1 to m-i in comment 29, and wait for merge to m-c
Flags: needinfo?(wkocher)
(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.
https://hg.mozilla.org/mozilla-central/rev/87a0e0ccdc34
Status: REOPENED → RESOLVED
Closed: 7 years ago → 7 years ago
Resolution: --- → FIXED
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.

Attachment

General

Creator:
Created:
Updated:
Size: