Closed
Bug 1320225
Opened 8 years ago
Closed 8 years ago
[DateTimeInput] Integration of input type=date input box with picker
Categories
(Core :: Layout: Form Controls, defect, P1)
Core
Layout: Form Controls
Tracking
()
RESOLVED
FIXED
mozilla53
Tracking | Status | |
---|---|---|
firefox53 | --- | fixed |
People
(Reporter: jessica, Assigned: jessica)
References
(Blocks 1 open bug)
Details
(Whiteboard: [milestone4])
Attachments
(2 files, 1 obsolete file)
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.
Updated•8 years ago
|
Whiteboard: [milestone4]
Updated•8 years ago
|
Priority: -- → P1
Comment hidden (mozreview-request) |
Comment 3•8 years 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•8 years 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•8 years ago
|
Attachment #8816061 -
Attachment description: patch, v1. → Bug 1320225 - [DateTimeInput] Integration of input type=date picker with input box (part 1)
Assignee | ||
Comment 5•8 years 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•8 years ago
|
Attachment #8820574 -
Flags: review?(mconley)
Comment 6•8 years 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]:
-----------------------------------------------------------------
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 7•8 years ago
|
||
mozreview-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•8 years 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•8 years 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•8 years 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•8 years ago
|
Attachment #8816061 -
Flags: review?(bugs) → review+
Comment 11•8 years ago
|
||
That was for the .webidl
Comment hidden (mozreview-request) |
Comment 13•8 years 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?
Comment 14•8 years ago
|
||
(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 15•8 years 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
> 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•8 years 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•8 years 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•8 years 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 21•8 years ago
|
||
mozreview-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
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•8 years ago
|
||
Rebased and added reviewer's name.
Attachment #8816061 -
Attachment is obsolete: true
Attachment #8826527 -
Flags: review+
Comment hidden (mozreview-request) |
Comment 24•8 years 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•8 years ago
|
||
Rebased and added reviewer's name.
Flags: needinfo?(mconley)
Keywords: checkin-needed
Comment 26•8 years 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•8 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Comment 28•8 years 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•8 years 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•8 years ago
|
||
Pushed part 1 to m-i in comment 29, and wait for merge to m-c
Flags: needinfo?(wkocher)
Assignee | ||
Comment 31•8 years 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•8 years ago
|
||
bugherder |
Status: REOPENED → RESOLVED
Closed: 8 years ago → 8 years ago
Resolution: --- → FIXED
Comment 33•8 years 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.
Description
•