1288591 - Implement the layout for <input type=time>

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect)

Description

[Jessica Jong [:jessica] (inactive)]

Discussed with front-end, we'll start with type=time, since it's simpler. Implementation would be similar to type=date (bug 1286182), that is, using XBL with HTML + CSS Flexbox inside.

Comment 1

[Jessica Jong [:jessica] (inactive)]

Attachment: WIP

Comment 2

[Jessica Jong [:jessica] (inactive)]

Attachment: WIP

This WIP does the following:
- create an input box for input type=time which consist of multiple text boxes internally
- adds a reset button next to the text boxes to clear all fields
- pops up a panel for picker (empty for now) when input box is clicked
- fill the input box if there is a preset value for input element
- update the input box when input element .value is set by script
- sets the input element value on user input
- advance to next text box automatically when current text box maxlength is reached
- transfer focus to internal text box when input element is focused
- restric numeric text boxes to only numbers
- step up/down input value when up/down key is pressed
- etc...

The patch may still be buggy, just want to know if we'are on the right direction.

Comment 3

[Jessica Jong [:jessica] (inactive)]

Comment on attachment 8776891 [details] [diff] [review]
WIP

Daniel, this is the date/time input project running in Taipei. Since :jwatt is going to be away for some time, would you help us review the layout part of date/time input?
For implementation background, discussion were made in bug 888320 and bug 1283023. This patch is the very first draft and we'd like to know if this architecture is okay.

Thanks.

Comment 4

[Daniel Holbert [:dholbert]]

Comment on attachment 8776891 [details] [diff] [review]
WIP

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

It's been a little while since I looked at form control code, but I think this looks sane! I am a little curious about how well XUL-element-nested-inside-of-modern-flexbox works in practice.  If it seems to produce reasonable layouts, then it's probably fine, I suppose.

I looked through the patch here, mostly focusing on the frame class (since that's what I'm most familiar with). I've included some minor review comments below.

::: dom/html/HTMLInputElement.cpp
@@ +2641,5 @@
> +HTMLInputElement::GetOwnerDateTimeControl()
> +{
> +  if (IsInNativeAnonymousSubtree() &&
> +      mType == NS_FORM_INPUT_TEXT &&
> +      GetParent() && GetParent()->GetParent() &&

IMO this "if" cascade would be easier to read if you added a newline after the first "GetParent() &&".  That makes the series of gradually-growing GetParent() null-checks easier to follow.

::: layout/forms/nsDateTimeControlFrame.cpp
@@ +248,5 @@
> +        GetPhysicalSize(myWM);
> +    aDesiredSize.SetBlockStartAscent(
> +       wrappersDesiredSize.BlockStartAscent() +
> +       outerWrapperFrame->BStart(aReflowInput.GetWritingMode(),
> +                             contentBoxSize));

Indentation is off here.

@@ +265,5 @@
> +
> +  aStatus = NS_FRAME_COMPLETE;
> +
> +  NS_FRAME_TRACE(NS_FRAME_TRACE_CALLS,
> +                  ("exit nsDateTimeControlFrame::Reflow: size=%d,%d",

I'm not familiar with this NS_FRAME_TRACE logging macro, but from a bit of searching, it looks like the other frame classes that use it (e.g. nsVideoFrame.cpp) tend to call it at the *entrance* to Reflow, as well as the exit.

So: you probably want to add a similar invocation at the top of this function.

@@ +275,5 @@
> +nsDateTimeControlFrame::CreateAnonymousContent(nsTArray<ContentInfo>& aElements)
> +{
> +  // Set up "datetimebox" XUL element which will be XBL-bound to the
> +  // actual controls.
> +  nsNodeInfoManager *nodeInfoManager = GetContent()->GetComposedDoc()->NodeInfoManager();

Nit: move the "*" to the left (to be type-hugging rather than variable-name-hugging).

Also, you could use "mContent" here instead of GetContent() (you've got mContent elsewhere in the patch).  GetContent() is just a getter for mContent.

@@ +278,5 @@
> +  // actual controls.
> +  nsNodeInfoManager *nodeInfoManager = GetContent()->GetComposedDoc()->NodeInfoManager();
> +  RefPtr<NodeInfo> nodeInfo;
> +  nodeInfo = nodeInfoManager->GetNodeInfo(nsGkAtoms::datetimebox, nullptr,
> +                                             kNameSpaceID_XUL, nsIDOMNode::ELEMENT_NODE);

Nit: indentation is off here. Also, might as well just combine the decl/assignment of nodeInfo:
  RefPtr<NodeInfo> nodeInfo =
    nodeInfoManager->GetNodeInfo(...

@@ +283,5 @@
> +  NS_ENSURE_TRUE(nodeInfo, NS_ERROR_OUT_OF_MEMORY);
> +
> +  NS_TrustedNewXULElement(getter_AddRefs(mDateTimeInputBox), nodeInfo.forget());
> +  if (!aElements.AppendElement(mDateTimeInputBox))
> +    return NS_ERROR_OUT_OF_MEMORY;

nsTArray::AppendElement is infallible by default (unless you explicitly pass in mozilla::fallbile as an extra argument).  So, you don't need to null-check its return value to detect allocation-failure -- that'll just cause an abort here.

You could pass in mozilla::fallible here, but probably better to just drop the check and assume that AppendElement succeeds.  That's generally what we do when the allocation-size isn't author/attacker-controlled.  (We're only adding 1 thing to an array here, rather than N things where N is some number that an attacker could control -- so if we happen to fail at allocation here, we're probably already about to crash from an allocation-failure somewhere else, so there's no big benefit to gracefully handling this one particular allocation failure with mozilla::fallible and an early-return.)

@@ +289,5 @@
> +  mDateTimeListener = new nsDateTimeListener(this);
> +  mDateTimeInputBox->AddEventListener(NS_LITERAL_STRING("ShowDateTimePicker"), mDateTimeListener,
> +                                      false, true);
> +
> +return NS_OK;

Add 2 spaces of indentation here.

@@ +316,5 @@
> +  // If script changed the <input>'s type before setting these attributes
> +  // then we don't need to do anything since we are going to be reframed.
> +  if (typeIsDateTime) {
> +    RefPtr<Runnable> event = new DispatchEventToDateTimeBox(mDateTimeInputBox,
> +      NS_LITERAL_STRING("datetime-value-changed"));

Shouldn't we check |aAttribute| before queuing up this runnable?  (i.e. do we know that the "value" attribute in particular has changed (or whatever the attribute is called), as opposed to some other arbitrary HTML attribute on this element?)

Or: if we're sure aAttribute must be the datetime value, this method should include an assertion about that to make that clearer.  (But I suspect we don't actually have any guarantees here about what the attribute is.)

::: layout/forms/nsDateTimeControlFrame.h
@@ +10,5 @@
> +#include "nsContainerFrame.h"
> +#include "nsIAnonymousContentCreator.h"
> +#include "nsCOMPtr.h"
> +
> +class nsDateTimeControlFrame final : public nsContainerFrame,

This .cpp and .h file here each need a comment at the top, like the "rendering object for the [...] element" comments at the top of other frame classes.

(See e.g. the top of nsVideoFrame.h and nsVideoFrame.cpp)

@@ +19,5 @@
> +public:
> +  friend nsIFrame* NS_NewDateTimeControlFrame(nsIPresShell* aPresShell,
> +                                              nsStyleContext* aContext);
> +
> +  virtual void DestroyFrom(nsIFrame* aDestructRoot) override;

Please drop the "virtual" keyword here & for all "override" methods in this file, per coding style guidelines[1].  This is a relatively-recent coding-style guideline, so old code doesn't follow it very well, but we should follow it in new code. (and eventually we'll clean up the old code, I hope. :))

[1] See the section starting "Method declarations must use at most one of the following keywords: virtual, override, or final" in https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Methods for more details.)

::: modules/libpref/init/all.js
@@ +1135,5 @@
>  // Enable <input type=number>:
>  pref("dom.forms.number", true);
>  
> +// Enable date/time input types
> +pref("dom.forms.datetime", true);

Depending on how confident you are about shipping this right away, you probably want to replace this with the following...
=======
#ifdef RELEASE_BUILD
pref("dom.forms.datetime", false);
#else
pref("dom.forms.datetime", true);
#endif
=======

That'll make this this disabled-by-default in beta/release (until it's been tested by nightly/aurora users for a cycle or so and we decide we're definitely ready to ship it, at which point we can simplify this to the one-liner "true" version that you've got right now)  Also, you'd want to be sure that the pref gets set for any tests that you end up including here (in https://dxr.mozilla.org/mozilla-central/source/testing/profiles/prefs_general.js for mochitests, and in reftest.list file itself using the "pref()" annotation for reftests)

Comment 5

[Daniel Holbert [:dholbert]]

Comment on attachment 8776891 [details] [diff] [review]
WIP

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

A few more nits:

::: layout/forms/nsDateTimeControlFrame.cpp
@@ +32,5 @@
> +  }
> +
> +  explicit nsDateTimeListener(nsDateTimeControlFrame* aDateTimeControl)
> +  {
> +    mDateTimeControl = aDateTimeControl;

Please use an init list to do this assignment.

(This is inside the nsDateTimeListener constructor.)

Also, probably worth asserting that the parameter is non-null here, since HandleEvent currently depends on that.

@@ +35,5 @@
> +  {
> +    mDateTimeControl = aDateTimeControl;
> +  }
> +
> +  nsDateTimeControlFrame* mDateTimeControl;

The raw pointer here is a bit scary.  This object (nsDateTimeListener) is reference-counted, and while the frame owns one reference, it's also conceivable that somebody else (maybe some JS/DOM/etc. thing) could own a reference as well & keep this alive longer than the frame, and that HandleEvent() could be called after the frame has been destroyed (which would end up being a use-after-free bug).

To avoid that, please:
 (1) Add a method to this class like the following:
  // Invoked when frame is being destroyed:
  void ClearFrame() {
    mDateTimeControl = nullptr;
  }

 (2) Call that method from nsDateTimeControlFrame::DestroyFrom, e.g.
   if (mDateTimeListener) {
     mDateTimeListener->ClearFrame();
   }

 (3) Adjust HandleEvent to null-check mDateTimeControl before using it, so that it won't crash if it happens to be called after the frame has been destroyed (and this new ClearFrame method has been called).

@@ +79,5 @@
> +
> +class DispatchEventToDateTimeBox : public Runnable
> +{
> +public:
> +  explicit DispatchEventToDateTimeBox(nsIContent* aContent,

I think you can drop the "explicit" keyword here.

(You only need it if the constructor has exactly 1 parameter -- and it's got 2 here, which means you shouldn't need that keyword.)

@@ +82,5 @@
> +public:
> +  explicit DispatchEventToDateTimeBox(nsIContent* aContent,
> +                                      const nsAString& aEventName)
> +    : mContent(aContent),
> +      mEventName(aEventName) {}

Nit: the init-list order is backwards here -- these member-variables mEventName/mContent are declared in the opposite order, which means this triggers the following warning in clang (treated as an error if you have ac_add_options --enable-warnings-as-errors in your mozconfig):

layout/forms/nsDateTimeControlFrame.cpp:85:7: error: field 'mContent' will be initialized after field 'mEventName' [-Werror,-Wreorder]

So, either the init list or the variable declaratoins need to be reordered here.

@@ +91,5 @@
> +                                         false, false);
> +    return NS_OK;
> +  }
> +  nsString mEventName;
> +  nsCOMPtr<nsIContent> mContent;

Since these get set in the init-list and are never changed, please declare them as "const"

Comment 6

[Jessica Jong [:jessica] (inactive)]

Thank you, Daniel, for the throughout review and useful tips! I'll fix them in the next version.
This feature will be disable by default (at least for now), that was just for testing. :)

I'll need some help on css though, since I'm completely new to it. For example, for the reset button, since it's an fix size image, how should we handle when the control gets very big (due to setting large fonts)?

Comment 7

[Jessica Jong [:jessica] (inactive)]

Another known issue is, the input element gets blurred and focused again when we jump from one textbox to another. 

In [1], it says, "If anonymous content underneath a focusable bound element blurs and anonymous content also underneath the bound element takes focus, then the blur and focus events are both stopped. As far as the bound element is concerned, it retains focus throughout the two events."

However, it seems that my blur/focus events are not stopped.

I fixed something similar in number input (Bug 1268556), but number input has only one anonymous textbox, so we know the number input focus is always retargeted to that textbox, hence we can use the textbox to compare whether it's the same content getting focus. But I'm not sure how to handle this with multiple anonymous textboxes.


[1] https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XBL/XBL_1.0_Reference/Anonymous_Content#Focus_and_Blur_Events

Comment 8

[Jessica Jong [:jessica] (inactive)]

Attachment: WIP2

Addressed review comment 4, comment 5 and Bug 1283384 comment 5.

TODO and enhancements:
- Track picker open/close state in datetimebox.xml
- Handle up/down key based on step values
- Non-e10s case
- Consider merging with SelectContent(Parent)Helper.jsm into a single FormControlContentHelper.jsm
- Fake passing focus to picker when Alt+Down key is pressed

Known Issues:
- input element gets blur/focus when jumping from one inner textbox to another

Comment 9

[Jessica Jong [:jessica] (inactive)]

Comment on attachment 8783441 [details] [diff] [review]
WIP2

Hi Olli, I am still can't figure out a good way to pass information from input box (datetimebox.xml) to DateTimeContentHelper (to pass it to picker). :(

For showing the picker we use custom event 'ShowDateTimePicker' from datetimebox.xml to nsDataTimeControlFrame, then nsDataTimeControlFrame sends a chrome event 'mozshowdatetimepicker', which is received by browser-content.js, and it creates the DateTimeContentHelper.

But if I am going to pass a partial value to the picker, e.g. when user enters the hour in input box, I can not attach extra information in chrome events, nor extract the detail in nsIDOMEventListener::HandleEvent. So, for now, I am still dispatching events on the input element...

Do you have any idea for this? Thanks.

Comment 10

[Jessica Jong [:jessica] (inactive)]

Attachment: WIP3

Latest patch with e10s/non-e10s enabled.

I'm hitting the following assertion when running test_input_sanitization.html:
[Child 84170] ###!!! ASSERTION: This is unsafe! Fix the caller!: 'Error', file /Users/jessica/workspace/hg/mozilla-central/dom/events/EventDispatcher.cpp, line 564

Seems that we can't use custom event on input element to pass data from datetimebox (xul) to DateTimePickerListener. Keeping the ni?smaug for this.

Comment 11

[Jessica Jong [:jessica] (inactive)]

Attachment: WIP4

Changed to use observers for passing messages between frame and browser-content, so no custom events on input element anymore. However, I still hit the following assertion:

[Child 84170] ###!!! ASSERTION: This is unsafe! Fix the caller!: 'Error', file /Users/jessica/workspace/hg/mozilla-central/dom/events/EventDispatcher.cpp, line 564

Comment 12

[Jessica Jong [:jessica] (inactive)]

Attachment: WIP5

This fixes the assertion. I think this is basically the patch for review, I'll start working on tests and submit for review then.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Sorry, I'm behind my needinfo queue.

Couldn't the xbl binding pass the data to <input> element using just some normal method, which is exposed in HTMLInputElement.webidl using [Func="IsChromeOrXBL"]. Then you could dispatch the event in HTMLInputElement.cpp using AsyncEventDispatcher and its RunDOMEventWhenSafe() method.

Comment 14

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8786285 [details] [diff] [review]
WIP5

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

This looks really good - I just want to revisit this decision to switch from events to observerables.

It looks like you switched away from events because you were hitting:

[Child 84170] ###!!! ASSERTION: This is unsafe! Fix the caller!: 'Error', file /Users/jessica/workspace/hg/mozilla-central/dom/events/EventDispatcher.cpp, line 564

I actually think firing an event makes more sense than using nsIObserverService for this sort of thing. I suspect we can get around the assertion by firing a chrome-only event.

We do something like this for <select> dropdowns to signal to browser-child.js that they should send a message to the parent to open the <select> dropdown. See http://searchfox.org/mozilla-central/rev/6536590412ea212c291719d1ed226fae65a0f917/layout/forms/nsListControlFrame.cpp#1784

Would it be possible to use a chrome only event here instead? I think that'd be much preferable to using nsIObserverService for this.

I want to point out that I didn't test this patch and only skimmed it (and even then, I only looked at the JS, CSS, XUL and XML stuff), but it looks really good. I think we're on the right path here.

Comment 15

[Jessica Jong [:jessica] (inactive)]

Thanks Mike and Olli for the feedback.

Actually, I switched from events to observers because I needed to pass extra data along with the event, and DispatchChromeEvent() does not support that. I could try using CustomEvent, but that's a little bit annoying in C++. Is there any reason observers are not preferred?

For the assertion, I got it when dispatching events from the XBL. I found the stack trace when hitting the assertion was:

nsDateTimeControlFrame::AttributeChanged() -> nsIDateTimeBox.notifyInputElementValueChanged() -> .setFieldsFromInputValue() -> .notifyPicker() -> .dispatchEvent(new CustomEvent("MozDateTimeBoxValueChanged"));

and solved it by using AddScriptRunner() in nsDateTimeControlFrame::AttributeChanged().
However, I am now changing to use ["Func="IsChromeOrXBL"], as suggested in comment 13, so no more dispatching events from XBL.

I'll upload my latest patch after running some tests.

Comment 16

[Jessica Jong [:jessica] (inactive)]

Attachment: patch, v1.

Comment 17

[Jessica Jong [:jessica] (inactive)]

NI for comment 15.

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

One issue with observer notifications is that they are global. What if there are several windows open and the foreground tab in each of them has <input type=time>. How do you check that from which window the notification is coming from.


And now I realized... do you actually need to pass any data? Would event.originalTarget be enough for the frame script. Then use originalTarget (which points to HTMLInputElement) and just ask the data from it?

Comment 19

[Jessica Jong [:jessica] (inactive)]

(In reply to Olli Pettay [:smaug] from comment #18)
> One issue with observer notifications is that they are global. What if there
> are several windows open and the foreground tab in each of them has <input
> type=time>. How do you check that from which window the notification is
> coming from.
> 

Hmm, we do check if it's the active window and foreground tab before showing the popup. But if there are still concerns, let's try to use events.

One thing I notice is that when there are several tabs open, in e10s, only one receives the observer notification, whereas in non-e10s, all of them receive the observer notifications. Not sure why is that.

> 
> And now I realized... do you actually need to pass any data? Would
> event.originalTarget be enough for the frame script. Then use originalTarget
> (which points to HTMLInputElement) and just ask the data from it?

Right, we could expose another chrome-only attribute for exposing the data, this data is different from .value, cause it may be a partial value entered by the user. Then we can use chrome events.

Comment 20

[Mike Conley (:mconley) (:⚙️)]

(In reply to Jessica Jong [:jessica] from comment #19)
> One thing I notice is that when there are several tabs open, in e10s, only
> one receives the observer notification, whereas in non-e10s, all of them
> receive the observer notifications. Not sure why is that.
> 

The observer service is process-global, so if you have several content processes open (say, for example, because dom.ipc.processCount is set to be greater than 1), then only the tabs within that process will receive the notification, and the parent (non-remote) tabs will not.

With e10s disabled, since there's only one process, all observers will get the notifications.

Comment 21

[Jessica Jong [:jessica] (inactive)]

(In reply to Mike Conley (:mconley) - (Needinfo me!) from comment #20)
> (In reply to Jessica Jong [:jessica] from comment #19)
> > One thing I notice is that when there are several tabs open, in e10s, only
> > one receives the observer notification, whereas in non-e10s, all of them
> > receive the observer notifications. Not sure why is that.
> > 
> 
> The observer service is process-global, so if you have several content
> processes open (say, for example, because dom.ipc.processCount is set to be
> greater than 1), then only the tabs within that process will receive the
> notification, and the parent (non-remote) tabs will not.

Hmm, interestingly, my dom.ipc.processCount is 1, so all the open tabs are in the same content process, right?

> 
> With e10s disabled, since there's only one process, all observers will get
> the notifications.

Comment 22

[Jessica Jong [:jessica] (inactive)]

Attachment: patch, v2.

Switch back to events instead of observers.

Comment 23

[Jessica Jong [:jessica] (inactive)]

Comment on attachment 8787515 [details] [diff] [review]
patch, v2.

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

I think this is ready for review.

This patch is to support <input type=time> frame, we implement it using XBL with HTML + CSS Flexbox inside.
The picker part is implemented in Bug 1283384, so you'll only see the input box in this bug.

Basic functions include:
- Entering values through keyboard, automatically advance to next field when field max length is reached or when it's not possible to enter a second digit on that field (e.g. 3 in hour field).
- Using Tab/Right/Left keys to move between fields
- Transfering of focus to inner text boxes
- Handling of Up/Down/Page Up/Page Down/Home/End keys
- Handling of P/p and A/a keys on AM/PM fields
- Passing values to picker while user interacts with input box
- Showing second and millisecond fields when needed
- Reflecting changes to input element's .value
Note this bug does not include localization yet. I'll file separates bugs for known issues and enhancements.

Since this bug includes DOM, layout, XBL and cross-process interaction, I'm asking review from different peers (:dholbert for frame part, :smaug for DOM part, :mconley for cross-process and XBL part maybe?). Please feel free to redirect if you are not the right person. Thanks a lot.

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

Curious, how does this all work on Android?

Comment 25

[Jessica Jong [:jessica] (inactive)]

(In reply to Olli Pettay [:smaug] from comment #24)
> Curious, how does this all work on Android?

Hmm, good point. This is for desktop only, so Android should remain as it is now, the input box is a normal textbox and the picker is the OS picker. In DOM, we use dom.experimental_forms for mobile and dom.forms.datetime for desktop. In layout, I think we can use #ifndef ANDROID in nsCSSFrameConstructor.cpp, I'll add this in the next patch.

Comment 26

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8787515 [details] [diff] [review]
patch, v2.

># HG changeset patch
># User Jessica Jong <[e-mail]>
># Parent  a551f534773cf2d6933f78ce7d82a7a33a99643e
>Bug 1288591 - Implement the layout for <input type=time>.
>
>diff --git a/browser/base/content/browser.xul b/browser/base/content/browser.xul
>--- a/browser/base/content/browser.xul
>+++ b/browser/base/content/browser.xul
>@@ -149,16 +149,23 @@
>     <!-- for url bar autocomplete -->
>     <panel type="autocomplete-richlistbox"
>            id="PopupAutoCompleteRichResult"
>            noautofocus="true"
>            hidden="true"
>            flip="none"
>            level="parent"/>
> 
>+    <panel id="DateTimePickerPanel"
>+           hidden="true"
>+           noautofocus="true"
>+           consumeoutsideclicks="false"
>+           level="parent">
>+    </panel>
>+
>     <!-- for select dropdowns. The menupopup is what shows the list of options,
>          and the popuponly menulist makes things like the menuactive attributes
>          work correctly on the menupopup. ContentSelectDropdown expects the
>          popuponly menulist to be its immediate parent. -->
>     <menulist popuponly="true" id="ContentSelectDropdown" hidden="true">
>       <menupopup rolluponmousewheel="true"
>                  activateontab="true" position="after_start"
> #ifdef XP_WIN
>@@ -1058,17 +1065,18 @@
>       <splitter id="sidebar-splitter" class="chromeclass-extrachrome sidebar-splitter" hidden="true"/>
>       <vbox id="appcontent" flex="1">
>         <notificationbox id="high-priority-global-notificationbox" notificationside="top"/>
>         <tabbrowser id="content"
>                     flex="1" contenttooltip="aHTMLTooltip"
>                     tabcontainer="tabbrowser-tabs"
>                     contentcontextmenu="contentAreaContextMenu"
>                     autocompletepopup="PopupAutoComplete"
>-                    selectmenulist="ContentSelectDropdown"/>
>+                    selectmenulist="ContentSelectDropdown"
>+                    datetimepicker="DateTimePickerPanel"/>
>       </vbox>
>       <vbox id="browser-border-end" hidden="true" layer="true"/>
>     </hbox>
> #include ../../components/customizableui/content/customizeMode.inc.xul
>   </deck>
> 
>   <html:div id="fullscreen-warning" class="pointerlockfswarning" hidden="true">
>     <html:div class="pointerlockfswarning-domain-text">
>diff --git a/browser/base/content/tabbrowser.xml b/browser/base/content/tabbrowser.xml
>--- a/browser/base/content/tabbrowser.xml
>+++ b/browser/base/content/tabbrowser.xml
>@@ -20,17 +20,17 @@
>                   flex="1" eventnode="document" xbl:inherits="handleCtrlPageUpDown"
>                   onselect="if (event.target.localName == 'tabpanels') this.parentNode.updateCurrentBrowser();">
>         <xul:tabpanels flex="1" class="plain" selectedIndex="0" anonid="panelcontainer">
>           <xul:notificationbox flex="1" notificationside="top">
>             <xul:hbox flex="1" class="browserSidebarContainer">
>               <xul:vbox flex="1" class="browserContainer">
>                 <xul:stack flex="1" class="browserStack" anonid="browserStack">
>                   <xul:browser anonid="initialBrowser" type="content-primary" message="true" messagemanagergroup="browsers"
>-                               xbl:inherits="tooltip=contenttooltip,contextmenu=contentcontextmenu,autocompletepopup,selectmenulist"/>
>+                               xbl:inherits="tooltip=contenttooltip,contextmenu=contentcontextmenu,autocompletepopup,selectmenulist,datetimepicker"/>
>                 </xul:stack>
>               </xul:vbox>
>             </xul:hbox>
>           </xul:notificationbox>
>         </xul:tabpanels>
>       </xul:tabbox>
>       <children/>
>     </content>
>@@ -1807,16 +1807,20 @@
> 
>             if (!aParams.isPreloadBrowser && this.hasAttribute("autocompletepopup")) {
>               b.setAttribute("autocompletepopup", this.getAttribute("autocompletepopup"));
>             }
> 
>             if (this.hasAttribute("selectmenulist"))
>               b.setAttribute("selectmenulist", this.getAttribute("selectmenulist"));
> 
>+           if (this.hasAttribute("datetimepicker")) {
>+              b.setAttribute("datetimepicker", this.getAttribute("datetimepicker"));
>+            }
>+
>             b.setAttribute("autoscrollpopup", this._autoScrollPopup.id);
> 
>             if (aParams.relatedBrowser) {
>               b.relatedBrowser = aParams.relatedBrowser;
>             }
> 
>             // Create the browserStack container
>             var stack = document.createElementNS(NS_XUL, "stack");
>diff --git a/browser/components/nsBrowserGlue.js b/browser/components/nsBrowserGlue.js
>--- a/browser/components/nsBrowserGlue.js
>+++ b/browser/components/nsBrowserGlue.js
>@@ -28,16 +28,17 @@ XPCOMUtils.defineLazyServiceGetter(this,
>   ["BookmarkHTMLUtils", "resource://gre/modules/BookmarkHTMLUtils.jsm"],
>   ["BookmarkJSONUtils", "resource://gre/modules/BookmarkJSONUtils.jsm"],
>   ["BrowserUITelemetry", "resource:///modules/BrowserUITelemetry.jsm"],
>   ["BrowserUsageTelemetry", "resource:///modules/BrowserUsageTelemetry.jsm"],
>   ["CaptivePortalWatcher", "resource:///modules/CaptivePortalWatcher.jsm"],
>   ["ContentClick", "resource:///modules/ContentClick.jsm"],
>   ["ContentPrefServiceParent", "resource://gre/modules/ContentPrefServiceParent.jsm"],
>   ["ContentSearch", "resource:///modules/ContentSearch.jsm"],
>+  ["DateTimePickerHelper", "resource://gre/modules/DateTimePickerHelper.jsm"],
>   ["DirectoryLinksProvider", "resource:///modules/DirectoryLinksProvider.jsm"],
>   ["Feeds", "resource:///modules/Feeds.jsm"],
>   ["FileUtils", "resource://gre/modules/FileUtils.jsm"],
>   ["FormValidationHandler", "resource:///modules/FormValidationHandler.jsm"],
>   ["LightweightThemeManager", "resource://gre/modules/LightweightThemeManager.jsm"],
>   ["LoginHelper", "resource://gre/modules/LoginHelper.jsm"],
>   ["LoginManagerParent", "resource://gre/modules/LoginManagerParent.jsm"],
>   ["NetUtil", "resource://gre/modules/NetUtil.jsm"],
>@@ -1051,16 +1052,17 @@ BrowserGlue.prototype = {
> 
>     if (!AppConstants.RELEASE_BUILD) {
>       this.checkForPendingCrashReports();
>     }
> 
>     CaptivePortalWatcher.init();
> 
>     AutoCompletePopup.init();
>+    DateTimePickerHelper.init();
> 
>     this._firstWindowTelemetry(aWindow);
>     this._firstWindowLoaded();
>   },
> 
>   /**
>    * Application shutdown handler.
>    */
>@@ -1082,16 +1084,17 @@ BrowserGlue.prototype = {
>     BrowserUsageTelemetry.uninit();
>     SelfSupportBackend.uninit();
>     NewTabMessages.uninit();
>     CaptivePortalWatcher.uninit();
>     AboutNewTab.uninit();
>     webrtcUI.uninit();
>     FormValidationHandler.uninit();
>     AutoCompletePopup.uninit();
>+    DateTimePickerHelper.uninit();
>     if (AppConstants.NIGHTLY_BUILD) {
>       AddonWatcher.uninit();
>     }
>   },
> 
>   _initServiceDiscovery: function () {
>     if (!Services.prefs.getBoolPref("browser.casting.enabled")) {
>       return;
>diff --git a/dom/base/nsGkAtomList.h b/dom/base/nsGkAtomList.h
>--- a/dom/base/nsGkAtomList.h
>+++ b/dom/base/nsGkAtomList.h
>@@ -266,16 +266,17 @@ GK_ATOM(curpos, "curpos")
> GK_ATOM(current, "current")
> GK_ATOM(cycler, "cycler")
> GK_ATOM(data, "data")
> GK_ATOM(datalist, "datalist")
> GK_ATOM(dataType, "data-type")
> GK_ATOM(dateTime, "date-time")
> GK_ATOM(datasources, "datasources")
> GK_ATOM(datetime, "datetime")
>+GK_ATOM(datetimebox, "datetimebox")
> GK_ATOM(dblclick, "dblclick")
> GK_ATOM(dd, "dd")
> GK_ATOM(debug, "debug")
> GK_ATOM(decimalFormat, "decimal-format")
> GK_ATOM(decimalSeparator, "decimal-separator")
> GK_ATOM(deck, "deck")
> GK_ATOM(declare, "declare")
> GK_ATOM(decoderDoctor, "decoder-doctor")
>@@ -1978,16 +1979,17 @@ GK_ATOM(bcTableCellFrame, "BCTableCellFr
> GK_ATOM(blockFrame, "BlockFrame")
> GK_ATOM(boxFrame, "BoxFrame")
> GK_ATOM(brFrame, "BRFrame")
> GK_ATOM(bulletFrame, "BulletFrame")
> GK_ATOM(colorControlFrame, "colorControlFrame")
> GK_ATOM(columnSetFrame, "ColumnSetFrame")
> GK_ATOM(comboboxControlFrame, "ComboboxControlFrame")
> GK_ATOM(comboboxDisplayFrame, "ComboboxDisplayFrame")
>+GK_ATOM(dateTimeControlFrame, "DateTimeControlFrame")
> GK_ATOM(deckFrame, "DeckFrame")
> GK_ATOM(detailsFrame, "DetailsFrame")
> GK_ATOM(fieldSetFrame, "FieldSetFrame")
> GK_ATOM(flexContainerFrame, "FlexContainerFrame")
> GK_ATOM(formControlFrame, "FormControlFrame") // radio or checkbox
> GK_ATOM(frameSetFrame, "FrameSetFrame")
> GK_ATOM(gfxButtonControlFrame, "gfxButtonControlFrame")
> GK_ATOM(gridContainerFrame, "GridContainerFrame")
>diff --git a/dom/html/HTMLInputElement.cpp b/dom/html/HTMLInputElement.cpp
>--- a/dom/html/HTMLInputElement.cpp
>+++ b/dom/html/HTMLInputElement.cpp
>@@ -48,16 +48,17 @@
> #include "nsIFrame.h"
> #include "nsRangeFrame.h"
> #include "nsIServiceManager.h"
> #include "nsError.h"
> #include "nsIEditor.h"
> #include "nsIIOService.h"
> #include "nsDocument.h"
> #include "nsAttrValueOrString.h"
>+#include "nsDateTimeControlFrame.h"
> 
> #include "nsPresState.h"
> #include "nsIDOMEvent.h"
> #include "nsIDOMNodeList.h"
> #include "nsIDOMHTMLCollection.h"
> #include "nsLinebreakConverter.h" //to strip out carriage returns
> #include "nsReadableUtils.h"
> #include "nsUnicharUtils.h"
>@@ -2630,16 +2631,86 @@ HTMLInputElement::MozSetDirectory(const 
> 
>   nsTArray<OwningFileOrDirectory> array;
>   OwningFileOrDirectory* element = array.AppendElement();
>   element->SetAsDirectory() = directory;
> 
>   SetFilesOrDirectories(array, true);
> }
> 
>+void HTMLInputElement::GetInputBoxValue(nsAString& aValue)
>+{
>+  MOZ_ASSERT(IsDateTimeInputType(mType),
>+             "This should be called on datetime input types only");
>+
>+  aValue = mDateTimeInputBoxValue;
>+}
>+
>+void
>+HTMLInputElement::SetValueFromDateTimePicker(const nsAString& aValue)
>+{
>+  MOZ_ASSERT(IsDateTimeInputType(mType),
>+             "This should be called on datetime input types only");
>+
>+  nsDateTimeControlFrame* frame = do_QueryFrame(GetPrimaryFrame());
>+  if (frame) {
>+    frame->SetValueFromPicker(aValue);
>+  }
>+}
>+
>+void
>+HTMLInputElement::SetDateTimePickerState(bool aOpen)
>+{
>+  MOZ_ASSERT(IsDateTimeInputType(mType),
>+             "This should be called on datetime input types only");
>+
>+  nsDateTimeControlFrame* frame = do_QueryFrame(GetPrimaryFrame());
>+  if (frame) {
>+    frame->SetPickerState(aOpen);
>+  }
>+}
>+
>+void
>+HTMLInputElement::OpenDateTimePicker(const nsAString& aInitialValue)
>+{
>+  MOZ_ASSERT(IsDateTimeInputType(mType),
>+             "This should be called on datetime input types only");
>+
>+  mDateTimeInputBoxValue = aInitialValue;
>+  nsContentUtils::DispatchChromeEvent(OwnerDoc(),
>+                                      static_cast<nsIDOMHTMLInputElement*>(this),
>+                                      NS_LITERAL_STRING("MozOpenDateTimePicker"),
>+                                      true, true);
>+}
>+
>+void
>+HTMLInputElement::SetValueFromDateTimeBox(const nsAString& aValue)
>+{
>+  MOZ_ASSERT(IsDateTimeInputType(mType),
>+             "This should be called on datetime input types only");
>+
>+  mDateTimeInputBoxValue = aValue;
>+  nsContentUtils::DispatchChromeEvent(OwnerDoc(),
>+                                      static_cast<nsIDOMHTMLInputElement*>(this),
>+                                      NS_LITERAL_STRING("MozDateTimeBoxValueChanged"),
>+                                      true, true);
>+}
>+
>+void
>+HTMLInputElement::CloseDateTimePicker()
>+{
>+  MOZ_ASSERT(IsDateTimeInputType(mType),
>+             "This should be called on datetime input types only");
>+
>+  nsContentUtils::DispatchChromeEvent(OwnerDoc(),
>+                                      static_cast<nsIDOMHTMLInputElement*>(this),
>+                                      NS_LITERAL_STRING("MozCloseDateTimePicker"),
>+                                      true, true);
>+}
>+
> bool
> HTMLInputElement::MozIsTextField(bool aExcludePassword)
> {
>   // TODO: temporary until bug 888320 is fixed.
>   if (IsExperimentalMobileType(mType) || IsDateTimeInputType(mType)) {
>     return false;
>   }
> 
>@@ -2656,16 +2727,37 @@ HTMLInputElement::GetOwnerNumberControl(
>       HTMLInputElement::FromContentOrNull(GetParent()->GetParent());
>     if (grandparent && grandparent->mType == NS_FORM_INPUT_NUMBER) {
>       return grandparent;
>     }
>   }
>   return nullptr;
> }
> 
>+HTMLInputElement*
>+HTMLInputElement::GetOwnerDateTimeControl()
>+{
>+  if (IsInNativeAnonymousSubtree() &&
>+      mType == NS_FORM_INPUT_TEXT &&
>+      GetParent() &&
>+      GetParent()->GetParent() &&
>+      GetParent()->GetParent()->GetParent() &&
>+      GetParent()->GetParent()->GetParent()->GetParent()) {
>+    // Yes, this is very very deep.
>+    HTMLInputElement* grandparent =
>+      HTMLInputElement::FromContentOrNull(
>+        GetParent()->GetParent()->GetParent()->GetParent());
>+    if (grandparent && grandparent->mType == NS_FORM_INPUT_TIME) {
>+      return grandparent;
>+    }
>+  }
>+  return nullptr;
>+}
>+
>+
> NS_IMETHODIMP
> HTMLInputElement::MozIsTextField(bool aExcludePassword, bool* aResult)
> {
>   *aResult = MozIsTextField(aExcludePassword);
>   return NS_OK;
> }
> 
> NS_IMETHODIMP
>@@ -3087,16 +3179,21 @@ HTMLInputElement::SetValueInternal(const
>           if (numberControlFrame) {
>             numberControlFrame->SetValueOfAnonTextControl(value);
>           }
>         } else if (mType == NS_FORM_INPUT_RANGE) {
>           nsRangeFrame* frame = do_QueryFrame(GetPrimaryFrame());
>           if (frame) {
>             frame->UpdateForValueChange();
>           }
>+        } else if (mType == NS_FORM_INPUT_TIME) {
>+          nsDateTimeControlFrame* frame = do_QueryFrame(GetPrimaryFrame());
>+          if (frame) {
>+            frame->UpdateInputBoxValue();
>+          }
>         }
>         if (!mParserCreating) {
>           OnValueChanged(/* aNotify = */ true,
>                          /* aWasInteractiveUserChange = */ false);
>         }
>         // else DoneCreatingElement calls us again once mParserCreating is false
>       }
> 
>@@ -3392,16 +3489,23 @@ HTMLInputElement::Focus(ErrorResult& aEr
>       HTMLInputElement* textControl = numberControlFrame->GetAnonTextControl();
>       if (textControl) {
>         textControl->Focus(aError);
>         return;
>       }
>     }
>   }
> 
>+  if (mType == NS_FORM_INPUT_TIME) {
>+    nsDateTimeControlFrame* frame = do_QueryFrame(GetPrimaryFrame());
>+    if (frame) {
>+      frame->HandleFocusEvent();
>+    }
>+  }
>+
>   if (mType != NS_FORM_INPUT_FILE) {
>     nsGenericHTMLElement::Focus(aError);
>     return;
>   }
> 
>   // For file inputs, focus the first button instead. In the case of there
>   // being two buttons (when the picker is a directory picker) the user can
>   // tab to the next one.
>@@ -3695,17 +3799,17 @@ HTMLInputElement::PreHandleEvent(EventCh
>     GetValue(mFocusedValue);
>   }
> 
>   // Fire onchange (if necessary), before we do the blur, bug 357684.
>   if (aVisitor.mEvent->mMessage == eBlur) {
>     // Experimental mobile types rely on the system UI to prevent users to not
>     // set invalid values but we have to be extra-careful. Especially if the
>     // option has been enabled on desktop.
>-    if (IsExperimentalMobileType(mType) || IsDateTimeInputType(mType)) {
>+    if (IsExperimentalMobileType(mType)) {
>       nsAutoString aValue;
>       GetValueInternal(aValue);
>       nsresult rv =
>         SetValueInternal(aValue, nsTextEditorState::eSetValue_Internal);
>       NS_ENSURE_SUCCESS(rv, rv);
>     }
>     FireChangeEventIfNeeded();
>   }
>@@ -3716,16 +3820,27 @@ HTMLInputElement::PreHandleEvent(EventCh
>     // Just as nsGenericHTMLFormElementWithState::PreHandleEvent calls
>     // nsIFormControlFrame::SetFocus, we handle focus here.
>     nsIFrame* frame = GetPrimaryFrame();
>     if (frame) {
>       frame->InvalidateFrameSubtree();
>     }
>   }
> 
>+  if (mType == NS_FORM_INPUT_TIME &&
>+      aVisitor.mEvent->mMessage == eFocus &&
>+      aVisitor.mEvent->mOriginalTarget == this) {
>+    // If original target is this and not the anonymous text control, we should
>+    // pass the focus to the anonymous text control.
>+    nsDateTimeControlFrame* frame = do_QueryFrame(GetPrimaryFrame());
>+    if (frame) {
>+      frame->HandleFocusEvent();
>+    }
>+  }
>+
>   if (mType == NS_FORM_INPUT_NUMBER && aVisitor.mEvent->IsTrusted()) {
>     if (mNumberControlSpinnerIsSpinning) {
>       // If the timer is running the user has depressed the mouse on one of the
>       // spin buttons. If the mouse exits the button we either want to reverse
>       // the direction of spin if it has moved over the other button, or else
>       // we want to end the spin. We do this here (rather than in
>       // PostHandleEvent) because we don't want to let content preventDefault()
>       // the end of the spin.
>@@ -6484,32 +6599,44 @@ HTMLInputElement::AddStates(EventStates 
> {
>   if (mType == NS_FORM_INPUT_TEXT) {
>     EventStates focusStates(aStates & (NS_EVENT_STATE_FOCUS |
>                                        NS_EVENT_STATE_FOCUSRING));
>     if (!focusStates.IsEmpty()) {
>       HTMLInputElement* ownerNumberControl = GetOwnerNumberControl();
>       if (ownerNumberControl) {
>         ownerNumberControl->AddStates(focusStates);
>+      } else {
>+        HTMLInputElement* ownerDateTimeControl = GetOwnerDateTimeControl();
>+        if (ownerDateTimeControl) {
>+          ownerDateTimeControl->AddStates(focusStates);
>+        }
>       }
>     }
>   }
>   nsGenericHTMLFormElementWithState::AddStates(aStates);
> }
> 
> void
> HTMLInputElement::RemoveStates(EventStates aStates)
> {
>   if (mType == NS_FORM_INPUT_TEXT) {
>     EventStates focusStates(aStates & (NS_EVENT_STATE_FOCUS |
>                                        NS_EVENT_STATE_FOCUSRING));
>     if (!focusStates.IsEmpty()) {
>       HTMLInputElement* ownerNumberControl = GetOwnerNumberControl();
>       if (ownerNumberControl) {
>         ownerNumberControl->RemoveStates(focusStates);
>+      } else {
>+        HTMLInputElement* ownerDateTimeControl = GetOwnerDateTimeControl();
>+        if (ownerDateTimeControl) {
>+          // TODO: Since there are multiple boxes, should we remove the state
>+          // when another text box from input=time is focused?
>+          ownerDateTimeControl->RemoveStates(focusStates);
>+        }
>       }
>     }
>   }
>   nsGenericHTMLFormElementWithState::RemoveStates(aStates);
> }
> 
> bool
> HTMLInputElement::RestoreState(nsPresState* aState)
>@@ -6674,22 +6801,24 @@ HTMLInputElement::IsHTMLFocusable(bool a
> 
> #ifdef XP_MACOSX
>   const bool defaultFocusable = !aWithMouse || nsFocusManager::sMouseFocusesFormControl;
> #else
>   const bool defaultFocusable = true;
> #endif
> 
>   if (mType == NS_FORM_INPUT_FILE ||
>-      mType == NS_FORM_INPUT_NUMBER) {
>+      mType == NS_FORM_INPUT_NUMBER ||
>+      mType == NS_FORM_INPUT_TIME) {
>     if (aTabIndex) {
>       // We only want our native anonymous child to be tabable to, not ourself.
>       *aTabIndex = -1;
>     }
>-    if (mType == NS_FORM_INPUT_NUMBER) {
>+    if (mType == NS_FORM_INPUT_NUMBER ||
>+        mType == NS_FORM_INPUT_TIME) {
>       *aIsFocusable = true;
>     } else {
>       *aIsFocusable = defaultFocusable;
>     }
>     return true;
>   }
> 
>   if (mType == NS_FORM_INPUT_HIDDEN) {
>diff --git a/dom/html/HTMLInputElement.h b/dom/html/HTMLInputElement.h
>--- a/dom/html/HTMLInputElement.h
>+++ b/dom/html/HTMLInputElement.h
>@@ -774,17 +774,26 @@ public:
>   int32_t GetTextLength(ErrorResult& aRv);
> 
>   void MozGetFileNameArray(nsTArray<nsString>& aFileNames, ErrorResult& aRv);
> 
>   void MozSetFileNameArray(const Sequence< nsString >& aFileNames, ErrorResult& aRv);
>   void MozSetFileArray(const Sequence<OwningNonNull<File>>& aFiles);
>   void MozSetDirectory(const nsAString& aDirectoryPath, ErrorResult& aRv);
> 
>+  void GetInputBoxValue(nsAString& aValue);
>+  void SetValueFromDateTimePicker(const nsAString& aValue);
>+  void SetDateTimePickerState(bool aOpen);
>+
>+  void OpenDateTimePicker(const nsAString& aInitialValue);
>+  void SetValueFromDateTimeBox(const nsAString& aValue);
>+  void CloseDateTimePicker();
>+
>   HTMLInputElement* GetOwnerNumberControl();
>+  HTMLInputElement* GetOwnerDateTimeControl();
> 
>   void StartNumberControlSpinnerSpin();
>   enum SpinnerStopState {
>     eAllowDispatchingEvents,
>     eDisallowDispatchingEvents
>   };
>   void StopNumberControlSpinnerSpin(SpinnerStopState aState =
>                                       eAllowDispatchingEvents);
>@@ -1436,16 +1445,23 @@ protected:
>   /**
>    * If mIsDraggingRange is true, this is the value that the input had before
>    * the drag started. Used to reset the input to its old value if the drag is
>    * canceled.
>    */
>   Decimal mRangeThumbDragStartValue;
> 
>   /**
>+   * Current value in the input box value, in json format. Could be not
>+   * complete, e.g., if user only entered hour in type=time, then it is
>+   * { hour: 9 }.
>+   */
>+  nsString mDateTimeInputBoxValue;
>+
>+  /**
>    * The selection properties cache for number controls.  This is needed because
>    * the number controls don't recycle their text field, so the normal cache in
>    * nsTextEditorState cannot do its job.
>    */
>   nsTextEditorState::SelectionProperties mSelectionProperties;
> 
>   // Step scale factor values, for input types that have one.
>   static const Decimal kStepScaleFactorDate;
>@@ -1514,17 +1530,18 @@ private:
>     return mType == NS_FORM_INPUT_TEXT || mType == NS_FORM_INPUT_SEARCH ||
>            mType == NS_FORM_INPUT_URL || mType == NS_FORM_INPUT_TEL ||
>            mType == NS_FORM_INPUT_PASSWORD;
>   }
> 
>   static bool MayFireChangeOnBlur(uint8_t aType) {
>     return IsSingleLineTextControl(false, aType) ||
>            aType == NS_FORM_INPUT_RANGE ||
>-           aType == NS_FORM_INPUT_NUMBER;
>+           aType == NS_FORM_INPUT_NUMBER ||
>+           aType == NS_FORM_INPUT_TIME;
>   }
> 
>   /**
>    * Returns true if the key code may induce the input's value changed to fire
>    * a "change" event accordingly.
>    */
>   bool MayFireChangeOnKeyUp(uint32_t aKeyCode) const;
> 
>diff --git a/dom/html/moz.build b/dom/html/moz.build
>--- a/dom/html/moz.build
>+++ b/dom/html/moz.build
>@@ -13,16 +13,17 @@ MOCHITEST_MANIFESTS += [
> MOCHITEST_CHROME_MANIFESTS += [
>     'test/chrome.ini',
>     'test/forms/chrome.ini',
> ]
> 
> BROWSER_CHROME_MANIFESTS += ['test/browser.ini']
> 
> XPIDL_SOURCES += [
>+    'nsIDateTimeInputBox.idl',
>     'nsIFormSubmitObserver.idl',
>     'nsIHTMLMenu.idl',
>     'nsIImageDocument.idl',
>     'nsIMenuBuilder.idl',
>     'nsIPhonetic.idl',
> ]
> 
> XPIDL_MODULE = 'content_html'
>diff --git a/dom/html/nsIDateTimeInputBox.idl b/dom/html/nsIDateTimeInputBox.idl
>new file mode 100644
>--- /dev/null
>+++ b/dom/html/nsIDateTimeInputBox.idl
>@@ -0,0 +1,31 @@
>+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*-
>+ *
>+ * This Source Code Form is subject to the terms of the Mozilla Public
>+ * License, v. 2.0. If a copy of the MPL was not distributed with this
>+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>+
>+#include "nsISupports.idl"
>+
>+[scriptable, uuid(c877f879-d39d-4fb7-ae17-3b23ef46d331)]
>+interface nsIDateTimeInputBox : nsISupports
>+{
>+  /**
>+   * Called from DOM/Layout when input element value has changed.
>+   */
>+  void notifyInputElementValueChanged();
>+
>+  /**
>+   * Called when date/time picker value has changed.
>+   */
>+  void setValueFromPicker(in DOMString value);
>+
>+ /**
>+   * Called from DOM/Layout to set focus on inner text box.
>+   */
>+  void setFocusToTextBox();
>+
>+ /**
>+   * Current status of the picker.
>+   */
>+  attribute boolean isPickerOpen;
>+};
>\ No newline at end of file
>diff --git a/dom/html/nsIFormControl.h b/dom/html/nsIFormControl.h
>--- a/dom/html/nsIFormControl.h
>+++ b/dom/html/nsIFormControl.h
>@@ -262,17 +262,16 @@ nsIFormControl::IsSingleLineTextControl(
> {
>   return aType == NS_FORM_INPUT_TEXT ||
>          aType == NS_FORM_INPUT_EMAIL ||
>          aType == NS_FORM_INPUT_SEARCH ||
>          aType == NS_FORM_INPUT_TEL ||
>          aType == NS_FORM_INPUT_URL ||
>          // TODO: those are temporary until bug 773205 is fixed.
>          aType == NS_FORM_INPUT_DATE ||
>-         aType == NS_FORM_INPUT_TIME ||
>          aType == NS_FORM_INPUT_MONTH ||
>          aType == NS_FORM_INPUT_WEEK ||
>          (!aExcludePassword && aType == NS_FORM_INPUT_PASSWORD);
> }
> 
> bool
> nsIFormControl::IsSubmittableControl() const
> {
>diff --git a/dom/html/test/forms/mochitest.ini b/dom/html/test/forms/mochitest.ini
>--- a/dom/html/test/forms/mochitest.ini
>+++ b/dom/html/test/forms/mochitest.ini
>@@ -58,16 +58,18 @@ skip-if = os == "android" || appname == 
> [test_input_range_key_events.html]
> skip-if = (buildapp == 'b2g' && toolkit != 'gonk') #Bug 931116, b2g desktop specific, initial triage
> [test_input_range_mouse_and_touch_events.html]
> skip-if = (toolkit == 'gonk' && debug) #debug-only failure; bug 926546
> [test_input_range_rounding.html]
> skip-if = (buildapp == 'b2g' && toolkit != 'gonk') #Bug 931116, b2g desktop specific, initial triage
> [test_input_sanitization.html]
> [test_input_textarea_set_value_no_scroll.html]
>+skip-if = os == "android" || appname == "b2g"
>+[test_input_time_key_events.html]
> [test_input_types_pref.html]
> [test_input_typing_sanitization.html]
> skip-if = buildapp == 'mulet'
> [test_input_untrusted_key_events.html]
> [test_input_url.html]
> [test_interactive_content_in_label.html]
> [test_label_control_attribute.html]
> [test_label_input_controls.html]
>diff --git a/dom/html/test/forms/test_input_time_key_events.html b/dom/html/test/forms/test_input_time_key_events.html
>new file mode 100644
>--- /dev/null
>+++ b/dom/html/test/forms/test_input_time_key_events.html
>@@ -0,0 +1,155 @@
>+<!DOCTYPE HTML>
>+<html>
>+<!--
>+https://bugzilla.mozilla.org/show_bug.cgi?id=1288591
>+-->
>+<head>
>+  <title>Test key events for time control</title>
>+  <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
>+  <script type="text/javascript" src="/tests/SimpleTest/EventUtils.js"></script>
>+  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
>+  <meta charset="UTF-8">
>+</head>
>+<body>
>+<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=1288591">Mozilla Bug 1288591</a>
>+<p id="display"></p>
>+<div id="content">
>+  <input id="input" type="time">
>+</div>
>+<pre id="test">
>+<script type="application/javascript">
>+
>+SimpleTest.waitForExplicitFinish();
>+// Turn off Spatial Navigation because it hijacks arrow keydown events:
>+SimpleTest.waitForFocus(function() {
>+  SpecialPowers.pushPrefEnv({"set":[["snav.enabled", false]]}, function() {
>+    test();
>+    SimpleTest.finish();
>+  });
>+});
>+
>+var testData = [
>+  { 
>+    keys: ["1030", "VK_DOWN"],
>+    initialVal: "",
>+    expectedVal: "10:30"
>+  },
>+  // Type 3 in the hour field will automatically advance to the minute field.
>+  { 
>+    keys: ["330", "VK_DOWN"],
>+    initialVal: "",
>+    expectedVal: "03:30"
>+  },
>+  // Type 5 in the hour field will automatically advance to the minute field.
>+  // Type 7 in the minute field will automatically advance to the AM/PM field.
>+  { 
>+    keys: ["57", "VK_DOWN"],
>+    initialVal: "",
>+    expectedVal: "05:07"
>+  },
>+  { keys: ["VK_TAB", "VK_TAB", "VK_DOWN"],
>+    initialVal: "10:30",
>+    expectedVal: "22:30"
>+  },
>+  // Right key should do the same thing as TAB key.
>+  { keys: ["VK_RIGHT", "VK_RIGHT", "VK_DOWN"],
>+    initialVal: "10:30",
>+    expectedVal: "22:30"
>+  },
>+  { keys: ["VK_RIGHT", "VK_LEFT", "VK_DOWN"],
>+    initialVal: "10:30",
>+    expectedVal: "09:30"
>+  },
>+  { keys: ["VK_UP"],
>+    initialVal: "16:00",
>+    expectedVal: "17:00"
>+  },
>+  { keys: ["VK_TAB", "VK_DOWN"],
>+    initialVal: "16:00",
>+    expectedVal: "16:59"
>+  },
>+  { keys: ["VK_TAB", "VK_UP"],
>+    initialVal: "16:59", 
>+    expectedVal: "16:00"
>+  },
>+  { keys: ["VK_PAGE_UP"],
>+    initialVal: "05:00", 
>+    expectedVal: "08:00"
>+  },
>+  { keys: ["VK_PAGE_DOWN"],
>+    initialVal: "05:00", 
>+    expectedVal: "02:00"
>+  },
>+  { keys: ["VK_TAB", "VK_PAGE_UP"],
>+    initialVal: "14:00", 
>+    expectedVal: "14:10"
>+  },
>+  { keys: ["VK_TAB", "VK_PAGE_DOWN"],
>+    initialVal: "14:00", 
>+    expectedVal: "14:50"
>+  },
>+  { keys: ["VK_HOME"],
>+    initialVal: "03:10", 
>+    expectedVal: "01:10"
>+  },
>+  { keys: ["VK_END"],
>+    initialVal: "03:10", 
>+    expectedVal: "00:10"
>+  },
>+  { keys: ["VK_TAB", "VK_HOME"],
>+    initialVal: "19:30", 
>+    expectedVal: "19:00"
>+  },
>+  { keys: ["VK_TAB", "VK_END"],
>+    initialVal: "19:30", 
>+    expectedVal: "19:59"
>+  },
>+  // Second field will show up when needed.
>+  { keys: ["VK_TAB", "VK_TAB", "VK_PAGE_UP"],
>+    initialVal: "08:10:10",
>+    expectedVal: "08:10:20"
>+  },
>+  { keys: ["VK_TAB", "VK_TAB", "VK_PAGE_DOWN"],
>+    initialVal: "08:10:10",
>+    expectedVal: "08:10:00"
>+  },
>+  { keys: ["VK_TAB", "VK_TAB", "VK_HOME"],
>+    initialVal: "16:00:30",
>+    expectedVal: "16:00:00"
>+  },
>+  { keys: ["VK_TAB", "VK_TAB", "VK_END"],
>+    initialVal: "16:00:30",
>+    expectedVal: "16:00:59"
>+  },
>+];
>+
>+function sendKeys(aKeys) {
>+  for (let i = 0; i < aKeys.length; i++) {
>+    let key = aKeys[i];
>+    if (key.startsWith("VK")) {
>+      synthesizeKey(key, {});
>+    } else {
>+      sendString(key);
>+    }
>+  }
>+}
>+
>+function test() {
>+  var elem = document.getElementById("input");
>+  
>+  for (let i = 0; i < testData.length; i++) {
>+    let data = testData[i];
>+    elem.focus();
>+    elem.value = data.initialVal;
>+    sendKeys(data.keys);
>+    elem.blur();
>+    is(elem.value, data.expectedVal,
>+       "Test with " + data.keys + ", result should be " + data.expectedVal);
>+    elem.value = "";
>+  }
>+}
>+
>+</script>
>+</pre>
>+</body>
>+</html>
>diff --git a/dom/html/test/forms/test_input_typing_sanitization.html b/dom/html/test/forms/test_input_typing_sanitization.html
>--- a/dom/html/test/forms/test_input_typing_sanitization.html
>+++ b/dom/html/test/forms/test_input_typing_sanitization.html
>@@ -156,37 +156,16 @@ function runTest()
>         '-',
>         '2012-01',
>         '2013-01-1',
>         '1011-23-21',
>         '1000-12-99',
>       ]
>     },
>     {
>-      type: 'time',
>-      validData: [
>-        '00:00',
>-        '09:09:00',
>-        '08:23:23.1',
>-        '21:43:56.12',
>-        '23:12:45.100',
>-      ],
>-      invalidData: [
>-        '00:',
>-        '00:00:',
>-        '25:00',
>-        '-00:00',
>-        '00:00:00.',
>-        '00:60',
>-        '10:58:99',
>-        ':19:10',
>-        '23:08:09.1012',
>-      ]
>-    },
>-    {
>       type: 'month',
>       validData: [
>         '0001-01',
>         '2012-12',
>         '100000-01',
>       ],
>       invalidData: [
>         '1-01',
>diff --git a/dom/webidl/HTMLInputElement.webidl b/dom/webidl/HTMLInputElement.webidl
>--- a/dom/webidl/HTMLInputElement.webidl
>+++ b/dom/webidl/HTMLInputElement.webidl
>@@ -191,17 +191,17 @@ partial interface HTMLInputElement {
>   // to update this list if nsIDOMNSEditableElement changes.
> 
>   [Pure, ChromeOnly]
>   readonly attribute nsIEditor? editor;
> 
>   // This is similar to set .value on nsIDOMInput/TextAreaElements, but handling
>   // of the value change is closer to the normal user input, so 'change' event
>   // for example will be dispatched when focusing out the element.
>-  [ChromeOnly]
>+  [Func="IsChromeOrXBL"]
>   void setUserInput(DOMString input);
> };
> 
> partial interface HTMLInputElement {
>   [Pref="dom.input.dirpicker", SetterThrows]
>   attribute boolean allowdirs;
> 
>   [Pref="dom.input.dirpicker"]
>@@ -229,8 +229,28 @@ HTMLInputElement implements MozPhonetic;
> // Webkit/Blink
> partial interface HTMLInputElement {
>   [Pref="dom.webkitBlink.filesystem.enabled", Frozen, Cached, Pure]
>   readonly attribute sequence<FileSystemEntry> webkitEntries;
> 
>   [Pref="dom.webkitBlink.dirPicker.enabled", BinaryName="WebkitDirectoryAttr", SetterThrows]
>           attribute boolean webkitdirectory;
> };
>+
>+partial interface HTMLInputElement {
>+  [Pref="dom.forms.datetime", ChromeOnly]
>+  void setValueFromDateTimePicker(DOMString value);
How is this different from setUserInput?

>+
>+  [Pref="dom.forms.datetime", ChromeOnly]
>+  void setDateTimePickerState(boolean open);
>+
>+  [Pref="dom.forms.datetime", Func="IsChromeOrXBL"]
>+  void openDateTimePicker(DOMString initialValue);
Hmm, how is setDateTimePickerState(true) different from openDateTimePicker("") ?


>+
>+  [Pref="dom.forms.datetime", Func="IsChromeOrXBL"]
>+  void setValueFromDateTimeBox(DOMString value);
Could we call this something like updateDateTimePickerValue(DOMString value) ?


>+
>+  [Pref="dom.forms.datetime", Func="IsChromeOrXBL"]
>+  void closeDateTimePicker();
>+
>+  [Pref="dom.forms.datetime", ChromeOnly]
>+  readonly attribute DOMString inputBoxValue;
This could perhaps have some other name. Something which hints this is for date/time

>+static inline bool
>+IsDateTimeInputBox(nsXULElement* aElement)
>+{
>+  nsIContent *content = aElement;
>+  if (content &&
>+      content->NodeInfo()->Equals(nsGkAtoms::datetimebox, kNameSpaceID_XUL)) {
>+    return true;
>+  }
>+  return false;
>+}
Do you need this? Wouldn't node->IsXULElement(nsGkAtoms::datetimebox); work

(I don't recall why we need any xul elements, but maybe we have limited xbl in non-chrome or something)

>+class InputBoxNotifier : public Runnable
>+{
>+public:
>+  explicit InputBoxNotifier(nsIDateTimeInputBox* aInputBox)
>+    : mInputBox(aInputBox)
>+  {
>+    MOZ_ASSERT(mInputBox);
>+  }
>+
>+  NS_IMETHOD Run()
>+  {
>+    mInputBox->NotifyInputElementValueChanged();
>+    return NS_OK;
>+  }
>+
>+private:
>+  ~InputBoxNotifier() {}
>+
>+  nsCOMPtr<nsIDateTimeInputBox> mInputBox;
>+};

You don't need this. nsRunnableMethod should work here.
Something like NewRunnableMethod(aInputBox, &nsIDateTimeInputBox::NotifyInputElementValueChanged) and then pass that to AddScriptRunner.


Overall looks great, but this is a large patch so needs couple of iterations. I didn't yet look at the xbl part. mconley, feel free to look at it too.
And dholbert should definitely look at the reflowing bits.

Comment 27

[Olli Pettay [:smaug][[e-mail]]]

and sorry, apparently I didn't cut enough out from the patch when uploading comments.

Comment 28

[Jessica Jong [:jessica] (inactive)]

(In reply to Olli Pettay [:smaug] from comment #26)
> Comment on attachment 8787515 [details] [diff] [review]
> patch, v2.
> 
> >diff --git a/dom/webidl/HTMLInputElement.webidl b/dom/webidl/HTMLInputElement.webidl
> >--- a/dom/webidl/HTMLInputElement.webidl
> >+++ b/dom/webidl/HTMLInputElement.webidl
> >@@ -229,8 +229,28 @@ HTMLInputElement implements MozPhonetic;
> > // Webkit/Blink
> > partial interface HTMLInputElement {
> >   [Pref="dom.webkitBlink.filesystem.enabled", Frozen, Cached, Pure]
> >   readonly attribute sequence<FileSystemEntry> webkitEntries;
> > 
> >   [Pref="dom.webkitBlink.dirPicker.enabled", BinaryName="WebkitDirectoryAttr", SetterThrows]
> >           attribute boolean webkitdirectory;
> > };
> >+
> >+partial interface HTMLInputElement {
> >+  [Pref="dom.forms.datetime", ChromeOnly]
> >+  void setValueFromDateTimePicker(DOMString value);
> How is this different from setUserInput?

With setUserInput, value gets sanitized if it's not a valid time string, but we may need to pass partial value, e.g. only hour and not minute, from picker to input box (and viceversa), so that the input box/picker can change accordingly.

> 
> >+
> >+  [Pref="dom.forms.datetime", ChromeOnly]
> >+  void setDateTimePickerState(boolean open);
> >+
> >+  [Pref="dom.forms.datetime", Func="IsChromeOrXBL"]
> >+  void openDateTimePicker(DOMString initialValue);
> Hmm, how is setDateTimePickerState(true) different from
> openDateTimePicker("") ?

This is for cases where popup is not closed by XBL, e.g., when clicking outside the panel.

> 
> 
> >+
> >+  [Pref="dom.forms.datetime", Func="IsChromeOrXBL"]
> >+  void setValueFromDateTimeBox(DOMString value);
> Could we call this something like updateDateTimePickerValue(DOMString value)
> ?

Sure, then setValueFromDateTimePicker() would become updateDateTimeInputBoxValue(DOMString value), right?

> 
> 
> >+
> >+  [Pref="dom.forms.datetime", Func="IsChromeOrXBL"]
> >+  void closeDateTimePicker();
> >+
> >+  [Pref="dom.forms.datetime", ChromeOnly]
> >+  readonly attribute DOMString inputBoxValue;
> This could perhaps have some other name. Something which hints this is for
> date/time

Hmm, that would be .dateTimeInputBoxValue...

> 
> >+static inline bool
> >+IsDateTimeInputBox(nsXULElement* aElement)
> >+{
> >+  nsIContent *content = aElement;
> >+  if (content &&
> >+      content->NodeInfo()->Equals(nsGkAtoms::datetimebox, kNameSpaceID_XUL)) {
> >+    return true;
> >+  }
> >+  return false;
> >+}
> Do you need this? Wouldn't node->IsXULElement(nsGkAtoms::datetimebox); work

Yes, that would work. Thanks.

> 
> (I don't recall why we need any xul elements, but maybe we have limited xbl
> in non-chrome or something)
> 
> >+class InputBoxNotifier : public Runnable
> >+{
> >+public:
> >+  explicit InputBoxNotifier(nsIDateTimeInputBox* aInputBox)
> >+    : mInputBox(aInputBox)
> >+  {
> >+    MOZ_ASSERT(mInputBox);
> >+  }
> >+
> >+  NS_IMETHOD Run()
> >+  {
> >+    mInputBox->NotifyInputElementValueChanged();
> >+    return NS_OK;
> >+  }
> >+
> >+private:
> >+  ~InputBoxNotifier() {}
> >+
> >+  nsCOMPtr<nsIDateTimeInputBox> mInputBox;
> >+};
> 
> You don't need this. nsRunnableMethod should work here.
> Something like NewRunnableMethod(aInputBox,
> &nsIDateTimeInputBox::NotifyInputElementValueChanged) and then pass that to
> AddScriptRunner.

Cool, thanks for this.

> 
> 
> Overall looks great, but this is a large patch so needs couple of
> iterations. I didn't yet look at the xbl part. mconley, feel free to look at
> it too.
> And dholbert should definitely look at the reflowing bits.

Thank you all for the review effort! :)

Comment 29

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8787515 [details] [diff] [review]
patch, v2.

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

I think this is really slick, Jessica. Having reviewed the JS, CSS, XBL and XUL, I think there's some really great stuff in here.

I have some questions and suggestions. See below.

::: browser/base/content/browser.xul
@@ +157,5 @@
> +    <panel id="DateTimePickerPanel"
> +           hidden="true"
> +           noautofocus="true"
> +           consumeoutsideclicks="false"
> +           level="parent">

Might as well stick to the above convention, and close the panel with />

::: browser/base/content/tabbrowser.xml
@@ +1811,5 @@
>  
>              if (this.hasAttribute("selectmenulist"))
>                b.setAttribute("selectmenulist", this.getAttribute("selectmenulist"));
>  
> +           if (this.hasAttribute("datetimepicker")) {

Busted indentation here.

::: dom/html/nsIDateTimeInputBox.idl
@@ +18,5 @@
> +   * Called when date/time picker value has changed.
> +   */
> +  void setValueFromPicker(in DOMString value);
> +
> + /**

Nit - busted indentation here and on line 27.

::: dom/html/test/forms/mochitest.ini
@@ +63,5 @@
>  skip-if = (buildapp == 'b2g' && toolkit != 'gonk') #Bug 931116, b2g desktop specific, initial triage
>  [test_input_sanitization.html]
>  [test_input_textarea_set_value_no_scroll.html]
> +skip-if = os == "android" || appname == "b2g"
> +[test_input_time_key_events.html]

Was this skip-if rule supposed to apply to test_input_time_key_events.html? If so, it needs to go beneath the entry in the manifest and not above.

::: dom/html/test/forms/test_input_time_key_events.html
@@ +27,5 @@
> +    SimpleTest.finish();
> +  });
> +});
> +
> +var testData = [

Can you add documentation about the testing structure here, what's expected, what's optional, etc?

@@ +28,5 @@
> +  });
> +});
> +
> +var testData = [
> +  { 

Nit: Trailing whitespcae here, and on a few other lines in this file.

@@ +33,5 @@
> +    keys: ["1030", "VK_DOWN"],
> +    initialVal: "",
> +    expectedVal: "10:30"
> +  },
> +  // Type 3 in the hour field will automatically advance to the minute field.

I like that you're describing what this test does. Can you do that for every test in here?

@@ +136,5 @@
> +
> +function test() {
> +  var elem = document.getElementById("input");
> +  
> +  for (let i = 0; i < testData.length; i++) {

Alternatively,

for (let data of testData) {
}

or,

for (let { keys, initialVal, expectedVal } of testData) {
}

::: layout/reftests/forms/input/datetime/from-time-to-other-type-unthemed.html
@@ +10,5 @@
> +    }
> +    document.addEventListener("MozReftestInvalidate", setToCheckbox);
> +  </script>
> +  <body>
> +    <input type='time' id='i' style="-moz-appearance:none;">

Nit - you've got a mix of single and double quotes in here. Should probably pick one and stick to it.

::: layout/reftests/forms/input/datetime/to-time-from-other-type-unthemed.html
@@ +9,5 @@
> +    }
> +    document.addEventListener("MozReftestInvalidate", setToTime);
> +  </script>
> +  <body>
> +    <input type='checkbox' id='i' style="-moz-appearance:none;">

Same nit as above, re: consistency in quoting.

::: toolkit/components/satchel/test/test_form_autocomplete.html
@@ +918,5 @@
>  
>      case 406:
> +        checkMenuEntries([]); // type=time with it's own control frame does not
> +                              // have a drop down menu for now
> +        checkForm("");

You might want to check with MattN that this test still makes sense.

::: toolkit/content/browser-content.js
@@ +1510,5 @@
>  
>  AutoCompletePopup.init();
> +
> +var DateTimePickerListener = {
> +  init: function() {

Each of these functions need documentation. A comment at the top of what DateTimePickerListener is responsible for would also be welcome.

@@ +1576,5 @@
> +  },
> +
> +  isDataAvailable: function(aData) {
> +    let obj = JSON.parse(aData);
> +    return Object.keys(obj).some(function(prop) {

This seems a little odd. Could we not have the API provide us null if there is no data available on the item, and an object otherwise?

@@ +1583,5 @@
> +  },
> +
> +  receiveMessage: function(aMessage) {
> +    switch (aMessage.name) {
> +      case "FormDateTime:PickerClosed":

Let's put scopes around each of these cases.

@@ +1618,5 @@
> +                                               : this._inputElement.value,
> +            step: this._inputElement.step,
> +            min: this._inputElement.min,
> +            max: this._inputElement.max,
> +            locale: this.getLocale(),

The locale is something that the parent should be able to determine on its own, without having the child send it up.

@@ +1626,5 @@
> +      }
> +      case "MozDateTimeBoxValueChanged": {
> +        let value = this._inputElement.inputBoxValue;
> +        sendAsyncMessage("FormDateTime:InputBoxValueChanged",
> +                         { value: value });

Can just do { value }.

@@ +1629,5 @@
> +        sendAsyncMessage("FormDateTime:InputBoxValueChanged",
> +                         { value: value });
> +        break;
> +      }
> +      case "MozCloseDateTimePicker":

Let's put scopes around this case and the next one while we're at it.

@@ +1638,5 @@
> +        if (this._inputElement &&
> +            this._inputElement.ownerDocument == aEvent.target) {
> +          sendAsyncMessage("FormDateTime:HidePicker");
> +          this.close();
> +        }

This is going to fallthrough to the default handler, which is probably fine behaviour wise, but is a footgun for future hacking. We should break here.

::: toolkit/content/widgets/datetimebox.xml
@@ +3,5 @@
> +<!-- This Source Code Form is subject to the terms of the Mozilla Public
> +   - License, v. 2.0. If a copy of the MPL was not distributed with this
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +
> +<bindings id="datetimeboxBindings"

Throughout this file, please use "let" instead of "var".

@@ +20,5 @@
> +
> +    <implementation>
> +      <constructor>
> +      <![CDATA[
> +        // TODO: localization

Leftover TODO here. What's the plan for it?

@@ +43,5 @@
> +        this.mMinPageUpDownInterval = 10;
> +
> +        this.mHourField =
> +          document.getAnonymousElementByAttribute(this, "anonid", "input-one");
> +        this.mHourField.classList.add("numeric");

Why not add these classes and attributes in the mark-up, out of curiosity?

@@ +96,5 @@
> +          this.mMinuteSeparator.setAttribute("class", "datetime-separator");
> +          container.insertBefore(this.mMinuteSeparator, this.mSpaceSeparator);
> +
> +          this.mSecondField = document.createElementNS(HTML_NS, "input");
> +          this.mSecondField.classList.add("textbox-input");

I believe classList.add can also take an Array of strings.

@@ +127,5 @@
> +          this.mSecondSeparator.textContent = this.milliSeparator;
> +          this.mSecondSeparator.setAttribute("class", "datetime-separator");
> +          container.insertBefore(this.mSecondSeparator, this.mSpaceSeparator);
> +
> +          this.mMillisecondField = document.createElementNS(HTML_NS, "input");

Why is this being added dynamically, out of curiosity? If it's necessary, we should probably find a way of de-duplicating this with insertSecondField, since there seems to be quite a bit of overlap.

@@ +223,5 @@
> +
> +          var hour = Number(this.mHourField.value);
> +          if (!this.is24Hour) {
> +            var ampm = this.mAMPMField.value;
> +            if (ampm == this.pmIndicator && hour < 12) {

Lots of magic numbers to set to constants in here.

@@ +447,5 @@
> +              this.log("NaN on setFieldValue!");
> +              return;
> +            }
> +
> +            if (aField.maxLength == 2) {

Lots of magic numbers in here. We should have those set as constants.

@@ +489,5 @@
> +          if (!this.isEmpty(this.mHourField.value)) {
> +            hour = Number(this.mHourField.value);
> +            if (!this.is24Hour) {
> +              var ampm = this.mAMPMField.value;
> +              if (ampm == this.pmIndicator && hour < 12) {

These "12"'s should probably be constants set somewhere.

@@ +503,5 @@
> +            minute = Number(this.mMinuteField.value);
> +          }
> +
> +          // Picker only needs hour/minute.
> +          var time = { hour: hour, minute: minute };

As indicated elsewhere, you can do:

let time = { hour, minute };

@@ +506,5 @@
> +          // Picker only needs hour/minute.
> +          var time = { hour: hour, minute: minute };
> +
> +          this.log("getCurrentValue: " + JSON.stringify(time));
> +          return JSON.stringify(time);

Is the stringification a security measure?

@@ +513,5 @@
> +      </method>
> +    </implementation>
> +  </binding>
> +
> +  <binding id="datetime-input-base">

Out of curiosity, what made you split this datetime-input-base out? Do you plan for there to be more than one subclass?

@@ +586,5 @@
> +
> +      <method name="setValueFromPicker">
> +        <parameter name="aValue"/>
> +        <body>
> +          this.setFieldsFromPicker(aValue);

Missing <![CDATA[ bits.

@@ +594,5 @@
> +      <method name="advanceToNextField">
> +        <parameter name="aReverse"/>
> +        <body>
> +        <![CDATA[
> +        this.log("advanceToNextField");

The indentation in here is a little off.

@@ +696,5 @@
> +    </implementation>
> +
> +    <handlers>
> +      <handler event="focus">
> +        this.onFocus(event);

Why not put the onFocus contents in here? Is there a need for the indirection?

@@ +707,5 @@
> +      </handler>
> +
> +      <handler event="click">
> +      <![CDATA[
> +        //XXX: .originalTarget is not expected.

.originalTarget is not expected? Can you explain?

@@ +714,5 @@
> +          return;
> +        }
> +
> +        if (!(event.originalTarget instanceof HTMLButtonElement)) {
> +        this.mInputElement.openDateTimePicker(this.getCurrentValue());

Broken indentation.

::: toolkit/modules/DateTimePickerHelper.jsm
@@ +7,5 @@
> +const Cc = Components.classes;
> +const Ci = Components.interfaces;
> +const Cu = Components.utils;
> +
> +const DEBUG = true;

This should probably be set to false.

@@ +20,5 @@
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +Cu.import("resource://gre/modules/Services.jsm");
> +
> +this.DateTimePickerHelper = {
> +  picker: null,

Overall, I'm really pleased and impressed with the structure in this file. Good stuff!

One thing to note is that, as the initiator of a new file, you've got the opportunity to set the tone and conventions. I suggest adding documentation for each of the functions in here, to encourage future hackers in this file to do the same in order to stick to file convention.

@@ +44,5 @@
> +  receiveMessage: function(aMessage) {
> +    debug("receiveMessage: " + aMessage.name);
> +    switch (aMessage.name) {
> +      case "FormDateTime:OpenPicker":
> +        this.showPicker(aMessage.target, aMessage.data);

As a general tip, I find it's always useful to wrap these cases in a scope so that when using let, SpiderMonkey doesn't complain if we re-use some of the same variable names.

So, example:

case "FormDateTime:OpenPicker": {
  this.showPicker(aMessage.target, aMessage.data);
  break;
}

@@ +52,5 @@
> +        break;
> +      case "FormDateTime:InputBoxValueChanged":
> +        let value = JSON.parse(aMessage.data.value);
> +        debug("Input box value is now: " + value.hour + ":" + value.minute);
> +        // TODO: pass value to picker.

Still work to do here, or is this for a follow-up?

@@ +74,5 @@
> +    }
> +  },
> +
> +  updateInputBoxValue: function(aEvent) {
> +    // TODO: parse data based on input type.

You're getting the detail here... anything more TODO here?

@@ +81,5 @@
> +    let browser = this.weakBrowser ? this.weakBrowser.get() : null;
> +    if (browser) {
> +      browser.messageManager.sendAsyncMessage(
> +        "FormDateTime:PickerValueChanged",
> +        { hour: hour, minute: minute });

There's a nice shortcut in situations like this - ES6 lets makes the following equivalent:

{ hour: hour, minute: minute }

{ hour, minute }

So you can drop the <key>: bits.

@@ +102,5 @@
> +      return;
> +    }
> +
> +    this.weakBrowser = Cu.getWeakReference(aBrowser);
> +    this.picker = aBrowser.dateTimePicker;

Presumably, aBrowser.dateTimePicker could be false if we're dealing with a browser that's not in the <xul:tabbrowser>, no? In that case, we'll throw here. Maybe we should log instead, and exit intentionally.

Comment 30

[Jessica Jong [:jessica] (inactive)]

Comment on attachment 8787515 [details] [diff] [review]
patch, v2.

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

Thank you so much for the throughout review. Please see my answers below. Note that I've skipped the nits, but still thanks for catching them.

::: browser/base/content/browser.xul
@@ +157,5 @@
> +    <panel id="DateTimePickerPanel"
> +           hidden="true"
> +           noautofocus="true"
> +           consumeoutsideclicks="false"
> +           level="parent">

Will do.

::: dom/html/test/forms/mochitest.ini
@@ +63,5 @@
>  skip-if = (buildapp == 'b2g' && toolkit != 'gonk') #Bug 931116, b2g desktop specific, initial triage
>  [test_input_sanitization.html]
>  [test_input_textarea_set_value_no_scroll.html]
> +skip-if = os == "android" || appname == "b2g"
> +[test_input_time_key_events.html]

Right, thanks for catching this.

::: dom/html/test/forms/test_input_time_key_events.html
@@ +27,5 @@
> +    SimpleTest.finish();
> +  });
> +});
> +
> +var testData = [

Will do.

@@ +33,5 @@
> +    keys: ["1030", "VK_DOWN"],
> +    initialVal: "",
> +    expectedVal: "10:30"
> +  },
> +  // Type 3 in the hour field will automatically advance to the minute field.

Sure, will do.

@@ +136,5 @@
> +
> +function test() {
> +  var elem = document.getElementById("input");
> +  
> +  for (let i = 0; i < testData.length; i++) {

Neat, thanks for this.

::: toolkit/content/browser-content.js
@@ +1510,5 @@
>  
>  AutoCompletePopup.init();
> +
> +var DateTimePickerListener = {
> +  init: function() {

Will do.

@@ +1576,5 @@
> +  },
> +
> +  isDataAvailable: function(aData) {
> +    let obj = JSON.parse(aData);
> +    return Object.keys(obj).some(function(prop) {

Sure, I'll see how to refine this.

@@ +1583,5 @@
> +  },
> +
> +  receiveMessage: function(aMessage) {
> +    switch (aMessage.name) {
> +      case "FormDateTime:PickerClosed":

Will do.

@@ +1618,5 @@
> +                                               : this._inputElement.value,
> +            step: this._inputElement.step,
> +            min: this._inputElement.min,
> +            max: this._inputElement.max,
> +            locale: this.getLocale(),

Right, will remove this and will just stay in sync on which attribute/pref to use for getting the locale.

@@ +1638,5 @@
> +        if (this._inputElement &&
> +            this._inputElement.ownerDocument == aEvent.target) {
> +          sendAsyncMessage("FormDateTime:HidePicker");
> +          this.close();
> +        }

Will do.

::: toolkit/content/widgets/datetimebox.xml
@@ +3,5 @@
> +<!-- This Source Code Form is subject to the terms of the Mozilla Public
> +   - License, v. 2.0. If a copy of the MPL was not distributed with this
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +
> +<bindings id="datetimeboxBindings"

Will do.

@@ +20,5 @@
> +
> +    <implementation>
> +      <constructor>
> +      <![CDATA[
> +        // TODO: localization

We'll handle localization in another bug, for our first step, this is going to land (pref-off) without localization.

@@ +43,5 @@
> +        this.mMinPageUpDownInterval = 10;
> +
> +        this.mHourField =
> +          document.getAnonymousElementByAttribute(this, "anonid", "input-one");
> +        this.mHourField.classList.add("numeric");

Sorry for not explaining this part.
The idea is that we are going to implement the layout for type=date, time, month, week, datetime-local in the same bindings. "datetime-input-base" is the base binding with three text boxes and two separators. Each of the type will inherit this base binding, and customize the text boxes as needed, even adding/removing fields. For example, in type=time, the last text box is use as AM/PM field, so it's not numeric. But reconsidering, most of the fields are numeric, so maybe we should add it in the mark-up and remove the class for AM/PM field.

@@ +96,5 @@
> +          this.mMinuteSeparator.setAttribute("class", "datetime-separator");
> +          container.insertBefore(this.mMinuteSeparator, this.mSpaceSeparator);
> +
> +          this.mSecondField = document.createElementNS(HTML_NS, "input");
> +          this.mSecondField.classList.add("textbox-input");

I see, thanks.

@@ +127,5 @@
> +          this.mSecondSeparator.textContent = this.milliSeparator;
> +          this.mSecondSeparator.setAttribute("class", "datetime-separator");
> +          container.insertBefore(this.mSecondSeparator, this.mSpaceSeparator);
> +
> +          this.mMillisecondField = document.createElementNS(HTML_NS, "input");

Second and millisecond is optional in the HTML spec, so the fields only show up when needed, e.g. value set has second/millisecond part. I'll find a way to generalize this.

@@ +223,5 @@
> +
> +          var hour = Number(this.mHourField.value);
> +          if (!this.is24Hour) {
> +            var ampm = this.mAMPMField.value;
> +            if (ampm == this.pmIndicator && hour < 12) {

Sure, will do.

@@ +506,5 @@
> +          // Picker only needs hour/minute.
> +          var time = { hour: hour, minute: minute };
> +
> +          this.log("getCurrentValue: " + JSON.stringify(time));
> +          return JSON.stringify(time);

It's because the parameter for setValueFromDateTimeBox() and openDateTimePicker() is DOMString.

@@ +513,5 @@
> +      </method>
> +    </implementation>
> +  </binding>
> +
> +  <binding id="datetime-input-base">

Yes, as explained above.

@@ +586,5 @@
> +
> +      <method name="setValueFromPicker">
> +        <parameter name="aValue"/>
> +        <body>
> +          this.setFieldsFromPicker(aValue);

Oh, I thought <![CDATA[ bits are not always needed.

@@ +696,5 @@
> +    </implementation>
> +
> +    <handlers>
> +      <handler event="focus">
> +        this.onFocus(event);

Hmm, yes, I think we can move the contents back here.

@@ +707,5 @@
> +      </handler>
> +
> +      <handler event="click">
> +      <![CDATA[
> +        //XXX: .originalTarget is not expected.

The .originalTarget I get is different from the .originalTarget in focus event, when I click on one of the text boxes I get:

[DateTimeBox] focus on: [object HTMLInputElement]
[DateTimeBox] click on: [object HTMLDivElement]

And when I click on the button, the .originalTarget is not equal to 'document.getAnonymousElementByAttribute(this, "anonid", "reset-button")', so I only check if it's instanceof HTMLButtonElement.

Would you happen to know why?

::: toolkit/modules/DateTimePickerHelper.jsm
@@ +7,5 @@
> +const Cc = Components.classes;
> +const Ci = Components.interfaces;
> +const Cu = Components.utils;
> +
> +const DEBUG = true;

Sure, will set it back to false.

@@ +20,5 @@
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +Cu.import("resource://gre/modules/Services.jsm");
> +
> +this.DateTimePickerHelper = {
> +  picker: null,

Sure, thanks for the suggestion.

@@ +44,5 @@
> +  receiveMessage: function(aMessage) {
> +    debug("receiveMessage: " + aMessage.name);
> +    switch (aMessage.name) {
> +      case "FormDateTime:OpenPicker":
> +        this.showPicker(aMessage.target, aMessage.data);

Will do.

@@ +52,5 @@
> +        break;
> +      case "FormDateTime:InputBoxValueChanged":
> +        let value = JSON.parse(aMessage.data.value);
> +        debug("Input box value is now: " + value.hour + ":" + value.minute);
> +        // TODO: pass value to picker.

The value will be handled in the bug for picker (bug 1283384).

@@ +74,5 @@
> +    }
> +  },
> +
> +  updateInputBoxValue: function(aEvent) {
> +    // TODO: parse data based on input type.

Hmm, for type=time we are getting hour/minute from detail, and pass it to the input box. Is that what you mean?

@@ +81,5 @@
> +    let browser = this.weakBrowser ? this.weakBrowser.get() : null;
> +    if (browser) {
> +      browser.messageManager.sendAsyncMessage(
> +        "FormDateTime:PickerValueChanged",
> +        { hour: hour, minute: minute });

Neat, thanks for this.

@@ +102,5 @@
> +      return;
> +    }
> +
> +    this.weakBrowser = Cu.getWeakReference(aBrowser);
> +    this.picker = aBrowser.dateTimePicker;

Sure, will refine this part.

Comment 31

[Daniel Holbert [:dholbert]]

Comment on attachment 8787515 [details] [diff] [review]
patch, v2.

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

(Sorry for the long review turnaround time here.)

(At smaug's suggestion from comment 1288591, I've mainly focused on the reflow code for now, and I spot-checked the other parts of the frame class for nits, but didn't look at them too thoroughly.  Happily, the reflow code is pretty much identical to nsNumberFrame::Reflow, which I reviewed in the past, so it's all pretty straightforward & known-good.)

Review comments below, mostly nits, some of which aren't your fault but are from code that's already mis-indented/etc. elsewhere in the tree, which you were unlucky enough to inherit when you copypasted/cribbed that old code.  Better to fix those in your new copies here & not propagate old badness into this new code! :)

::: layout/forms/nsDateTimeControlFrame.cpp
@@ +131,5 @@
> +
> +  DO_GLOBAL_REFLOW_COUNT("nsDateTimeControlFrame");
> +  DISPLAY_REFLOW(aPresContext, this, aReflowInput, aDesiredSize, aStatus);
> +  NS_FRAME_TRACE(NS_FRAME_TRACE_CALLS,
> +                  ("enter nsDateTimeControlFrame::Reflow: availSize=%d,%d",

This line |("enter...| needs to be de-indented by 1 space -- it's indented too much right now.

(It's broken in nsVideoFrame.cpp as well, which is probably where you got this from. I've just pushed a patch to fix that & some other related whitespace issues in that file:
 https://hg.mozilla.org/integration/mozilla-inbound/rev/33e977bca774 )

@@ +132,5 @@
> +  DO_GLOBAL_REFLOW_COUNT("nsDateTimeControlFrame");
> +  DISPLAY_REFLOW(aPresContext, this, aReflowInput, aDesiredSize, aStatus);
> +  NS_FRAME_TRACE(NS_FRAME_TRACE_CALLS,
> +                  ("enter nsDateTimeControlFrame::Reflow: availSize=%d,%d",
> +                  aReflowInput.AvailableWidth(), aReflowInput.AvailableHeight()));

This line's a bit too long. Wrap after "AvailableWidth()," please.

@@ +154,5 @@
> +    borderBoxBSize = contentBoxBSize +
> +      aReflowInput.ComputedLogicalBorderPadding().BStartEnd(myWM);
> +  } // else, we'll figure out borderBoxBSize after we resolve contentBoxBSize.
> +
> +  nsIFrame* outerWrapperFrame = mFrames.FirstChild();

Does "outerWrapperFrame" make sense as this variable-name here?  In nsNumberFrame, it has this name because there's a member-variable called "mOuterWrapper", and this is the frame associated with that frame.

In this case, perhaps there's a better name for this frame that's clearer about what piece of content you're expecting it to represent?  See also my comment about possibly renaming mDateTimeInputBox. (This variable might want to have its name correspond to that variable somehow.)

@@ +155,5 @@
> +      aReflowInput.ComputedLogicalBorderPadding().BStartEnd(myWM);
> +  } // else, we'll figure out borderBoxBSize after we resolve contentBoxBSize.
> +
> +  nsIFrame* outerWrapperFrame = mFrames.FirstChild();
> +  if (outerWrapperFrame) {

In nsNumberControlFrame (where most of this reflow code seems to comes from), we instead do this logic with an "if/else" -- whereas you're only including the "else" clause, effectively.  Specifically, nsNumberControlFrame has:
  if (!outerWrapperFrame) { // display:none?
    ...[resolve intrinsically sized content-box & border-box to 0]...
  } else {
    ...
https://dxr.mozilla.org/mozilla-central/rev/938ce16be25f9c551c19ef8938e8717ed3d41ff5/layout/forms/nsNumberControlFrame.cpp#

And the first "if" chunk there zeroes out some variables that we use at the end of the function.  Without that, one of these variables (borderBoxBSize) is otherwise left uninitialized (and later used while still being uninitialized!), and the other (contentBoxBSize) is left with a bogus huge sentinel value that we don't actually want to use for sizing.

So -- I think you need to copy that bit of nsNumberControlFrame reflow code over here.

@@ +249,5 @@
> +
> +  aStatus = NS_FRAME_COMPLETE;
> +
> +  NS_FRAME_TRACE(NS_FRAME_TRACE_CALLS,
> +                  ("exit nsDateTimeControlFrame::Reflow: size=%d,%d",

As above, this line is indented 1 space to far. Please deindent the open-paren by 1 space.

@@ +259,5 @@
> +nsDateTimeControlFrame::CreateAnonymousContent(nsTArray<ContentInfo>& aElements)
> +{
> +  // Set up "datetimebox" XUL element which will be XBL-bound to the
> +  // actual controls.
> +  nsNodeInfoManager* nodeInfoManager = mContent->GetComposedDoc()->NodeInfoManager();

This line is a bit too long (should be maximum 80 characters).  Let's wrap it after the "=", like you do for the assignment on the next line.

@@ +312,5 @@
> +  if (aNameSpaceID == kNameSpaceID_None) {
> +    if (aAttribute == nsGkAtoms::value) {
> +      MOZ_ASSERT(mContent->IsHTMLElement(nsGkAtoms::input), "bad cast");
> +      bool typeIsDateTime =
> +        static_cast<dom::HTMLInputElement*>(mContent)->GetType() == NS_FORM_INPUT_TIME;

This line is a bit too long.

Since this bool is only used once anyway, let's just pull the static_cast out into a helper variable, and then just make the "==" comparison directly inside of the "if" check, and get rid of the bool entirely.
Like so:
      auto contentAsInputelem = static_cast<dom::HTMLInputElement*>(mContent);
      // If script changed [... SNIP ...]
      if (contentAsInputelem->->GetType() == NS_FORM_INPUT_TIME) {

::: layout/forms/nsDateTimeControlFrame.h
@@ +52,5 @@
> +
> +  void Reflow(nsPresContext*           aPresContext,
> +              ReflowOutput&            aDesiredSize,
> +              const ReflowInput&       aReflowState,
> +              nsReflowStatus&          aStatus) override;

Nit: drop unnecessary extra spaces between types & args here, please.  (Looks like there are 6 more spaces than are necessary right now.)

(The args lining up is nice, but the current minimum-of-7-spaces-between-typename-and-variable-name is unnecessary/awkward. Let's just drop that to minimum of 1 space, and then align the others as-needed. i.e. drop 6 spaces from each line.)

@@ +60,5 @@
> +  void AppendAnonymousContentTo(nsTArray<nsIContent*>& aElements,
> +                                uint32_t aFilter) override;
> +
> +  nsresult AttributeChanged(int32_t  aNameSpaceID, nsIAtom* aAttribute,
> +                                    int32_t  aModType) override;

Drop the extra space after "int32_t" in both lines of this function signature, and fix indentation before int32_t.

@@ +68,5 @@
> +  void HandleFocusEvent();
> +  void SetPickerState(bool aOpen);
> +
> +private:
> +  nsCOMPtr<nsIContent> mDateTimeInputBox;

This variable name is perhaps too vague.

This whole frame class is conceptually a "date time input box", and so is the <input type="[date|time]"> element itself.  And this variable "mDateTimeInputBox" isn't either of those things -- it's some subcomponent. So, consider renaming it to better correspond to what it represents, as a subcomponent.  (Or if it really needs its current name for some reason, please include a line or two of documentation explaining what the element is.)

Also, "box" is kind of a synonym for "frame" (in the sense of "frame tree" / "box tree").  So, perhaps better not to use the word "box" to name something that's really an element, in frame code.

@@ +71,5 @@
> +private:
> +  nsCOMPtr<nsIContent> mDateTimeInputBox;
> +};
> +
> +#endif // nsDateTimeControlFrame_h__
\ No newline at end of file

General nit: this added file (and 3 other added files in this patch) do not currently end in a newline. If you look at the text of the patch file itself, you'll see the patch warning you by adding this text: "\ No newline at end of file".

Please include a newline character at the end of the file (just jump to the end and hit "Enter" for any affected files) -- this is in the coding-style general practices here:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#General_C.2FC.2B.2B_Practices

(I think there are a total of 4 files in this patch that are currently missing a newline, according to this command:
curl -l https://bug1288591.bmoattachments.org/attachment.cgi?id=8787515 \
  | grep "No newline at end of file" | wc -l
)

Comment 32

[Daniel Holbert [:dholbert]]

Two self-corrections:
(In reply to Daniel Holbert [:dholbert] from comment #31)
> (At smaug's suggestion from comment 1288591, I've mainly focused on the
> reflow code for now

Sorry, I meant to say "comment 26" there. Mis-pasted the bug number instead of the comment number. :)

> Since this bool is only used once anyway, let's just pull the static_cast
> out into a helper variable, and then just make the "==" comparison directly
> inside of the "if" check, and get rid of the bool entirely.
> Like so:
>       auto contentAsInputelem =
> static_cast<dom::HTMLInputElement*>(mContent);

Sorry, my camel-casing was broken there -- I should've capitalized "Elem" at the end of the suggested variable-name. (not that you have to use that exact name)

Comment 33

[Jessica Jong [:jessica] (inactive)]

Comment on attachment 8787515 [details] [diff] [review]
patch, v2.

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

Thank you Daniel for the review feedback.

::: layout/forms/nsDateTimeControlFrame.cpp
@@ +131,5 @@
> +
> +  DO_GLOBAL_REFLOW_COUNT("nsDateTimeControlFrame");
> +  DISPLAY_REFLOW(aPresContext, this, aReflowInput, aDesiredSize, aStatus);
> +  NS_FRAME_TRACE(NS_FRAME_TRACE_CALLS,
> +                  ("enter nsDateTimeControlFrame::Reflow: availSize=%d,%d",

Will do.

@@ +132,5 @@
> +  DO_GLOBAL_REFLOW_COUNT("nsDateTimeControlFrame");
> +  DISPLAY_REFLOW(aPresContext, this, aReflowInput, aDesiredSize, aStatus);
> +  NS_FRAME_TRACE(NS_FRAME_TRACE_CALLS,
> +                  ("enter nsDateTimeControlFrame::Reflow: availSize=%d,%d",
> +                  aReflowInput.AvailableWidth(), aReflowInput.AvailableHeight()));

Will do.

@@ +154,5 @@
> +    borderBoxBSize = contentBoxBSize +
> +      aReflowInput.ComputedLogicalBorderPadding().BStartEnd(myWM);
> +  } // else, we'll figure out borderBoxBSize after we resolve contentBoxBSize.
> +
> +  nsIFrame* outerWrapperFrame = mFrames.FirstChild();

Sure, will rename this based on mDateTimeInputBox change.

@@ +155,5 @@
> +      aReflowInput.ComputedLogicalBorderPadding().BStartEnd(myWM);
> +  } // else, we'll figure out borderBoxBSize after we resolve contentBoxBSize.
> +
> +  nsIFrame* outerWrapperFrame = mFrames.FirstChild();
> +  if (outerWrapperFrame) {

Thanks for reminding me about this, will fix this in the upcoming patch.

@@ +249,5 @@
> +
> +  aStatus = NS_FRAME_COMPLETE;
> +
> +  NS_FRAME_TRACE(NS_FRAME_TRACE_CALLS,
> +                  ("exit nsDateTimeControlFrame::Reflow: size=%d,%d",

Will do.

@@ +259,5 @@
> +nsDateTimeControlFrame::CreateAnonymousContent(nsTArray<ContentInfo>& aElements)
> +{
> +  // Set up "datetimebox" XUL element which will be XBL-bound to the
> +  // actual controls.
> +  nsNodeInfoManager* nodeInfoManager = mContent->GetComposedDoc()->NodeInfoManager();

Will do.

@@ +312,5 @@
> +  if (aNameSpaceID == kNameSpaceID_None) {
> +    if (aAttribute == nsGkAtoms::value) {
> +      MOZ_ASSERT(mContent->IsHTMLElement(nsGkAtoms::input), "bad cast");
> +      bool typeIsDateTime =
> +        static_cast<dom::HTMLInputElement*>(mContent)->GetType() == NS_FORM_INPUT_TIME;

Will do.

::: layout/forms/nsDateTimeControlFrame.h
@@ +52,5 @@
> +
> +  void Reflow(nsPresContext*           aPresContext,
> +              ReflowOutput&            aDesiredSize,
> +              const ReflowInput&       aReflowState,
> +              nsReflowStatus&          aStatus) override;

I think I'll not align the args here, since I'm not doing it for other functions.

@@ +60,5 @@
> +  void AppendAnonymousContentTo(nsTArray<nsIContent*>& aElements,
> +                                uint32_t aFilter) override;
> +
> +  nsresult AttributeChanged(int32_t  aNameSpaceID, nsIAtom* aAttribute,
> +                                    int32_t  aModType) override;

Will do.

@@ +68,5 @@
> +  void HandleFocusEvent();
> +  void SetPickerState(bool aOpen);
> +
> +private:
> +  nsCOMPtr<nsIContent> mDateTimeInputBox;

Hmmm, that's why they say naming variables and classes is the hardest thing.. :)
I am thinking of "mSubInputContent", I don't think it's a good one but I can't think of any other right now, so please let me know if you've got one!

@@ +71,5 @@
> +private:
> +  nsCOMPtr<nsIContent> mDateTimeInputBox;
> +};
> +
> +#endif // nsDateTimeControlFrame_h__
\ No newline at end of file

Thanks for reminding me about this, will fix this in the upcoming patch.

Comment 34

[Jessica Jong [:jessica] (inactive)]

Attachment: patch, v3.

Addressed review comment 26, comment 29 and comment 31, really appreciate your time on this!
Some of the concerns from the reviewers were answered above, please let me know if you have any other question.
Also, tested on Firefox for Android, and it's working as expected (as a normal text box and picker from the OS).

Some variable/method names have changed, including:
- HTMLInputElement.webidl
inputBoxValue -> dateTimeInputBoxValue
setValueFromPicker() -> updateDateTimeInputBox()
setValueFromInputBox() -> updateDateTimePicker()

- nsDateTimeControlFrame.h
mDateTimeInputBox -> mInputAreaContent, and corresponding local variables in nsDataTimeControlFrame.cpp

- nsIDateTimeInputBox.idl -> nsIDateTimeInputArea.idl
nsIDateTimeInputBox -> nsIDateTimeInputArea

Comment 35

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8791073 [details] [diff] [review]
patch, v3.

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

The JS / XBL / CSS bits look fine to me. I also looked at the tests. This is really great stuff - just a few small corrections.

I'm certain you're doing this anyways, but do make sure to coordinate with scottwu in bug 1283384 to make sure that you're consistent about internal nomenclature (for example, I believe he's changing "ampm" to "dayperiod". We should be consistent here.)

Great work, Jessica!

::: dom/html/test/forms/test_input_time_key_events.html
@@ +28,5 @@
> +  });
> +});
> +
> +var testData = [
> +  /**

Busted indendation from 33-36

@@ +53,5 @@
> +    initialVal: "",
> +    expectedVal: "05:07"
> +  },
> +  {
> +    // Advnace to AM/PM field and change to PM.

Typo "Advance"

@@ +89,5 @@
> +    initialVal: "16:59",
> +    expectedVal: "16:00"
> +  },
> +  {
> +    // PageUp on hour field increments hout by 3.

typo: "hout" -> "hour"

@@ +95,5 @@
> +    initialVal: "05:00",
> +    expectedVal: "08:00"
> +  },
> +  {
> +    // PageDown on hour field decrements hout by 3.

typo: "hout" -> "hour"

::: layout/reftests/forms/input/datetime/from-time-to-other-type-unthemed.html
@@ +4,5 @@
> +             like that type (not like an input time element) -->
> +  <script type="text/javascript">
> +    function setToCheckbox()
> +    {
> +      document.getElementById("i").type="checkbox";

Spaces on either side of =

::: layout/reftests/forms/input/datetime/to-time-from-other-type-unthemed.html
@@ +3,5 @@
> +  <!-- Test: input element changed to time state doesn't look like checkbox state -->
> +  <script type="text/javascript">
> +    function setToTime()
> +    {
> +      document.getElementById("i").type="time";

Spaces on either side of =

::: toolkit/content/browser-content.js
@@ +1511,5 @@
>  AutoCompletePopup.init();
> +
> +/**
> + * DateTimePickerListener is the communication channel between the text box
> + * (content) for date/time input types and its picker (chrome). It works on

The note about it working in e10s and non-e10s mode can probably be removed, imo.

@@ +1529,5 @@
> +    });
> +  },
> +
> +  uninit: function() {
> +    removeEventListener("MozOpenDateTimePicker", this);

Probably best to make sure we've cleared out _inputElement while we're at it.

::: toolkit/content/widgets/datetimebox.xml
@@ +20,5 @@
> +
> +    <implementation>
> +      <constructor>
> +      <![CDATA[
> +        // TODO: Bug 1301312 - localization for input type=time input.

I'm worried about users seeing these unlocalized strings before we're ready to ship. Is the plan to have this feature be disabled until the localization is done? If so, how does the disabling work for this binding? (I understand how it works for the WebIDL binding method calls).

@@ +431,5 @@
> +      </method>
> +
> +      <method name="setFieldValue">
> +       <parameter name="aField"/>
> +        <parameter name="aValue"/>

Busted indentation here and down.

@@ +636,5 @@
> +      </method>
> +
> +      <method name="clearInputFields">
> +        <body>
> +          // Override me!

Perhaps these should throw Components.results.NS_ERROR_NOT_IMPLEMENTED?

::: toolkit/modules/DateTimePickerHelper.jsm
@@ +9,5 @@
> +const Cu = Components.utils;
> +
> +const DEBUG = false;
> +function debug(aStr) {
> +  if (DEBUG) { dump("-*- DateTimePickerHelper: " + aStr + "\n"); }

Nit: even though this is debug stuff, let's break this up over 3 lines instead of jamming it onto one.

@@ +22,5 @@
> +
> +/*
> + * DateTimePickerHelper receives message from content side (input box) and
> + * is reposible for opening, closing and updating the picker. Similary,
> + * DateTimePickerHelper listens for picker's events and notify parent side

Nit: It notifies the content side, not the parent side.

Comment 36

[Daniel Holbert [:dholbert]]

Comment on attachment 8791073 [details] [diff] [review]
patch, v3.

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

r=me on the frame class with nits below addressed.

(I skimmed over the non-Reflow()-related stuff, and I'm asking smaug to review those parts.)

::: layout/forms/moz.build
@@ +21,5 @@
>  UNIFIED_SOURCES += [
>      'nsButtonFrameRenderer.cpp',
>      'nsColorControlFrame.cpp',
>      'nsComboboxControlFrame.cpp',
> +    'nsDateTimeControlFrame.cpp',

This is good, but as a sanity-check, please be sure this builds if you add this new file to SOURCES instead of UNIFIED_SOURCES.  (This would just be a local tweak for testing purposes -- don't actually change this in the final patch here.)

If it builds with the file in SOURCES, that proves you aren't accidentally relying on another .cpp file's #includes / forward declarations (which is otherwise easy to do by accident).  Such a reliance would be fragile, since every time we add a new file, it shakes up the UNIFIED_SOURCES groupings.

(This would be a good thing to double-check for every new .cpp file in this patch, too.)

::: layout/forms/nsDateTimeControlFrame.h
@@ +4,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/**
> + * This frame type is used for input type=date, time, month, week, and
> + * datetime-local.

Nit: it looks like this comment isn't strictly true (not yet, at least).  According to this patch's nsCSSFrameConstructor.cpp changes, this frame is only used for <input type="time"> at the moment, and we use text-input frames for the other types.

Maybe add a (separate) comment to clarify this (which we'll update/shorten as we implement other types) -- something like this:
 /**
  * NOTE: some of the above-mentioned input types are still to-be-implemented.
  * See nsCSSFrameConstructor::FindInputData, as well as bug 1286182,
  * bug [...], bug [...].
  */

(I'm not sure what the bug numbers are for month/week/datetime-local.  If we don't have bugs for those yet, now might be a good time to file them, so we can reference them here.)

@@ +69,5 @@
> +  void SetPickerState(bool aOpen);
> +
> +private:
> +  // Anonymous child which is bound via XBL to the input area and reset button.
> +  nsCOMPtr<nsIContent> mInputAreaContent;

I'm not sure this comment makes sense ("bound to the input area and reset button") -- it's only bound to a single element, right?

(Is it bound to an element that *wraps* the input area and reset button? [which I presume are two other elements inside of this one?])

Comment 37

[Olli Pettay [:smaug][[e-mail]]]

Sorry about the delay with review. Trying to get it done still tonight.

Comment 38

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8791073 [details] [diff] [review]
patch, v3.

>+void HTMLInputElement::GetDateTimeInputBoxValue(nsAString& aValue)
>+{
>+  MOZ_ASSERT(IsDateTimeInputType(mType),
>+             "This should be called on datetime input types only");
>+
>+  if (mDateTimeInputBoxValue.EqualsLiteral("{}")) {
>+    aValue.SetIsVoid(true);
>+  } else {
>+    aValue = mDateTimeInputBoxValue;
>+  }
>+}
This is weird setup.
You could use a dictionary in the .webidl and return that here.
Something like
dictionary DateTimeValue
{
  long hour;
  ....
};

And then in C++ just set the values you need to set on it.


>+HTMLInputElement::UpdateDateTimeInputBox(const nsAString& aValue)
>+{
>+  MOZ_ASSERT(IsDateTimeInputType(mType),
>+             "This should be called on datetime input types only");
>+
>+  nsDateTimeControlFrame* frame = do_QueryFrame(GetPrimaryFrame());
>+  if (frame) {
>+    frame->SetValueFromPicker(aValue);
>+  }
>+}
I think same applies here too, and then if you need to JSON-fy a dictionary to pass it to some method, 
you can always use ToJSON. And a dictionary can be also initialized from a JSON string. They have Init(const nsAString& aJSON); automatically generated.


>+HTMLInputElement::GetOwnerDateTimeControl()
>+{
>+  if (IsInNativeAnonymousSubtree() &&
>+      mType == NS_FORM_INPUT_TEXT &&
>+      GetParent() &&
>+      GetParent()->GetParent() &&
>+      GetParent()->GetParent()->GetParent() &&
>+      GetParent()->GetParent()->GetParent()->GetParent()) {
>+    // Yes, this is very very deep.
>+    HTMLInputElement* grandparent =
Hmm, not grandparent, but some ancestor.
Perhaps just call the variable ownerDateTimeControl



>   /**
>+   * Current value in the input box value, in json format. Could be not
>+   * complete, e.g., if user only entered hour in type=time, then it is
>+   * { hour: 9 }.
>+   */
>+  nsString mDateTimeInputBoxValue;
So this could be UniquePtr<DateTimeValue> mDateTimeValue; where DateTimeValue is coming from webidl dictionary

>   // This is similar to set .value on nsIDOMInput/TextAreaElements, but handling
>   // of the value change is closer to the normal user input, so 'change' event
>   // for example will be dispatched when focusing out the element.
>-  [ChromeOnly]
>+  [Func="IsChromeOrXBL"]
>   void setUserInput(DOMString input);
On C++ side could we actually limit type=file case to ChromeOnly callers. So, somewhere there
check if nsContentUtils::LegacyIsCallerChromeOrNativeCode return null when type is file.
type=file just happens to be particularly security sensitive.
Bug 1297393 will make the setup a bit nicer eventually, and could get rid of *Legacy* part, but if that bug doesn't 
land soon, LegacyIsCallerChromeOrNativeCode should be fine until that.



>+partial interface HTMLInputElement {
>+  [Pref="dom.forms.datetime", ChromeOnly]
>+  readonly attribute DOMString dateTimeInputBoxValue;
>+
>+  [Pref="dom.forms.datetime", ChromeOnly]
>+  void updateDateTimeInputBox(DOMString value);
>+
>+  [Pref="dom.forms.datetime", ChromeOnly]
>+  void setDateTimePickerState(boolean open);
>+
>+  [Pref="dom.forms.datetime", Func="IsChromeOrXBL"]
>+  void openDateTimePicker(DOMString initialValue);
>+
>+  [Pref="dom.forms.datetime", Func="IsChromeOrXBL"]
>+  void updateDateTimePicker(DOMString value);
So I think using dictionary could make this easier to read and use. One can just pass around objects.
Sure, then when passing value to xbl some json handling is needed.
Though, there has to be some way to nicely pass json value there too... thinking.
One option is to make nsIDateTimeInputArea methods to take jsval and not DOMString.
Then in C++ when you have the dictionary from webidl layer, call
ToJSValue on it 
http://searchfox.org/mozilla-central/rev/5bdd2876aeb9dc97dc619ab3e067e86083dad023/dom/bindings/ToJSValue.h#223
I think you'd need to be in the compartment of the input element's window.
So before ToJSValue do something like
scope = xpc::GetXBLScope(aCx, inputElement->GetWrapper());
AutoJSAPI jsapi;
NS_ENSURE_STATE(jsapi.Init(scope));

(add some null checks)

I think that would make the code a bit simpler. Less manual JSON-fying.

Comment 39

[Olli Pettay [:smaug][[e-mail]]]

(and happy to do new reviews even this week. Just keeping the review queue closed for couple of days, so send email if you need review :) )

Comment 40

[Jessica Jong [:jessica] (inactive)]

(In reply to Mike Conley (:mconley) - (needinfo me!) from comment #35)
> Comment on attachment 8791073 [details] [diff] [review]
> patch, v3.
> 
> Review of attachment 8791073 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The JS / XBL / CSS bits look fine to me. I also looked at the tests. This is
> really great stuff - just a few small corrections.

Thanks a lot for the corrections. :)

> 
> I'm certain you're doing this anyways, but do make sure to coordinate with
> scottwu in bug 1283384 to make sure that you're consistent about internal
> nomenclature (for example, I believe he's changing "ampm" to "dayperiod". We
> should be consistent here.)

Sure, will sync with Scott.

> 
> Great work, Jessica!
> 
> ::: dom/html/test/forms/test_input_time_key_events.html
> @@ +28,5 @@
> > +  });
> > +});
> > +
> > +var testData = [
> > +  /**
> 
> Busted indendation from 33-36
> 
> @@ +53,5 @@
> > +    initialVal: "",
> > +    expectedVal: "05:07"
> > +  },
> > +  {
> > +    // Advnace to AM/PM field and change to PM.
> 
> Typo "Advance"
> 
> @@ +89,5 @@
> > +    initialVal: "16:59",
> > +    expectedVal: "16:00"
> > +  },
> > +  {
> > +    // PageUp on hour field increments hout by 3.
> 
> typo: "hout" -> "hour"
> 
> @@ +95,5 @@
> > +    initialVal: "05:00",
> > +    expectedVal: "08:00"
> > +  },
> > +  {
> > +    // PageDown on hour field decrements hout by 3.
> 
> typo: "hout" -> "hour"
> 
> :::
> layout/reftests/forms/input/datetime/from-time-to-other-type-unthemed.html
> @@ +4,5 @@
> > +             like that type (not like an input time element) -->
> > +  <script type="text/javascript">
> > +    function setToCheckbox()
> > +    {
> > +      document.getElementById("i").type="checkbox";
> 
> Spaces on either side of =
> 
> :::
> layout/reftests/forms/input/datetime/to-time-from-other-type-unthemed.html
> @@ +3,5 @@
> > +  <!-- Test: input element changed to time state doesn't look like checkbox state -->
> > +  <script type="text/javascript">
> > +    function setToTime()
> > +    {
> > +      document.getElementById("i").type="time";
> 
> Spaces on either side of =
> 
> ::: toolkit/content/browser-content.js
> @@ +1511,5 @@
> >  AutoCompletePopup.init();
> > +
> > +/**
> > + * DateTimePickerListener is the communication channel between the text box
> > + * (content) for date/time input types and its picker (chrome). It works on
> 
> The note about it working in e10s and non-e10s mode can probably be removed,
> imo.
> 
> @@ +1529,5 @@
> > +    });
> > +  },
> > +
> > +  uninit: function() {
> > +    removeEventListener("MozOpenDateTimePicker", this);
> 
> Probably best to make sure we've cleared out _inputElement while we're at it.
> 
> ::: toolkit/content/widgets/datetimebox.xml
> @@ +20,5 @@
> > +
> > +    <implementation>
> > +      <constructor>
> > +      <![CDATA[
> > +        // TODO: Bug 1301312 - localization for input type=time input.
> 
> I'm worried about users seeing these unlocalized strings before we're ready
> to ship. Is the plan to have this feature be disabled until the localization
> is done? If so, how does the disabling work for this binding? (I understand
> how it works for the WebIDL binding method calls).

This is going to be pref-off by default. When it is pref-off, the input element type falls back to "text".
http://hg.mozilla.org/mozilla-central/file/tip/dom/html/HTMLInputElement.cpp#l5398

> 
> @@ +431,5 @@
> > +      </method>
> > +
> > +      <method name="setFieldValue">
> > +       <parameter name="aField"/>
> > +        <parameter name="aValue"/>
> 
> Busted indentation here and down.
> 
> @@ +636,5 @@
> > +      </method>
> > +
> > +      <method name="clearInputFields">
> > +        <body>
> > +          // Override me!
> 
> Perhaps these should throw Components.results.NS_ERROR_NOT_IMPLEMENTED?
> 
> ::: toolkit/modules/DateTimePickerHelper.jsm
> @@ +9,5 @@
> > +const Cu = Components.utils;
> > +
> > +const DEBUG = false;
> > +function debug(aStr) {
> > +  if (DEBUG) { dump("-*- DateTimePickerHelper: " + aStr + "\n"); }
> 
> Nit: even though this is debug stuff, let's break this up over 3 lines
> instead of jamming it onto one.
> 
> @@ +22,5 @@
> > +
> > +/*
> > + * DateTimePickerHelper receives message from content side (input box) and
> > + * is reposible for opening, closing and updating the picker. Similary,
> > + * DateTimePickerHelper listens for picker's events and notify parent side
> 
> Nit: It notifies the content side, not the parent side.

Comment 41

[Jessica Jong [:jessica] (inactive)]

(In reply to Daniel Holbert [:dholbert] from comment #36)
> Comment on attachment 8791073 [details] [diff] [review]
> patch, v3.
> 
> Review of attachment 8791073 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me on the frame class with nits below addressed.
> 
> (I skimmed over the non-Reflow()-related stuff, and I'm asking smaug to
> review those parts.)
> 
> ::: layout/forms/moz.build
> @@ +21,5 @@
> >  UNIFIED_SOURCES += [
> >      'nsButtonFrameRenderer.cpp',
> >      'nsColorControlFrame.cpp',
> >      'nsComboboxControlFrame.cpp',
> > +    'nsDateTimeControlFrame.cpp',
> 
> This is good, but as a sanity-check, please be sure this builds if you add
> this new file to SOURCES instead of UNIFIED_SOURCES.  (This would just be a
> local tweak for testing purposes -- don't actually change this in the final
> patch here.)
> 
> If it builds with the file in SOURCES, that proves you aren't accidentally
> relying on another .cpp file's #includes / forward declarations (which is
> otherwise easy to do by accident).  Such a reliance would be fragile, since
> every time we add a new file, it shakes up the UNIFIED_SOURCES groupings.
> 
> (This would be a good thing to double-check for every new .cpp file in this
> patch, too.)

Thanks for the info, I've tried moving 'nsDateTimeControlFrame.cpp' to SOURCES, and it does build successfully. I'll try with other new .cpp files later.

> 
> ::: layout/forms/nsDateTimeControlFrame.h
> @@ +4,5 @@
> > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> > +
> > +/**
> > + * This frame type is used for input type=date, time, month, week, and
> > + * datetime-local.
> 
> Nit: it looks like this comment isn't strictly true (not yet, at least). 
> According to this patch's nsCSSFrameConstructor.cpp changes, this frame is
> only used for <input type="time"> at the moment, and we use text-input
> frames for the other types.
> 
> Maybe add a (separate) comment to clarify this (which we'll update/shorten
> as we implement other types) -- something like this:
>  /**
>   * NOTE: some of the above-mentioned input types are still
> to-be-implemented.
>   * See nsCSSFrameConstructor::FindInputData, as well as bug 1286182,
>   * bug [...], bug [...].
>   */
> 
> (I'm not sure what the bug numbers are for month/week/datetime-local.  If we
> don't have bugs for those yet, now might be a good time to file them, so we
> can reference them here.)

Just filed for month (Bug 1306215), week (Bug 1306216) and datetime-local (Bug 1306217), and date is already there (Bug 1286182). Will update the comment.

> 
> @@ +69,5 @@
> > +  void SetPickerState(bool aOpen);
> > +
> > +private:
> > +  // Anonymous child which is bound via XBL to the input area and reset button.
> > +  nsCOMPtr<nsIContent> mInputAreaContent;
> 
> I'm not sure this comment makes sense ("bound to the input area and reset
> button") -- it's only bound to a single element, right?
> 
> (Is it bound to an element that *wraps* the input area and reset button?
> [which I presume are two other elements inside of this one?])

You are right, "bound to an element that *wraps* the input area and reset button" makes more sense. Thanks!

Comment 42

[Jessica Jong [:jessica] (inactive)]

(In reply to Olli Pettay [:smaug] (reviewing overload) from comment #38)
> Comment on attachment 8791073 [details] [diff] [review]
> patch, v3.
> 
> >+void HTMLInputElement::GetDateTimeInputBoxValue(nsAString& aValue)
> >+{
> >+  MOZ_ASSERT(IsDateTimeInputType(mType),
> >+             "This should be called on datetime input types only");
> >+
> >+  if (mDateTimeInputBoxValue.EqualsLiteral("{}")) {
> >+    aValue.SetIsVoid(true);
> >+  } else {
> >+    aValue = mDateTimeInputBoxValue;
> >+  }
> >+}
> This is weird setup.
> You could use a dictionary in the .webidl and return that here.
> Something like
> dictionary DateTimeValue
> {
>   long hour;
>   ....
> };
> 
> And then in C++ just set the values you need to set on it.
> 
> 
> >+HTMLInputElement::UpdateDateTimeInputBox(const nsAString& aValue)
> >+{
> >+  MOZ_ASSERT(IsDateTimeInputType(mType),
> >+             "This should be called on datetime input types only");
> >+
> >+  nsDateTimeControlFrame* frame = do_QueryFrame(GetPrimaryFrame());
> >+  if (frame) {
> >+    frame->SetValueFromPicker(aValue);
> >+  }
> >+}
> I think same applies here too, and then if you need to JSON-fy a dictionary
> to pass it to some method, 
> you can always use ToJSON. And a dictionary can be also initialized from a
> JSON string. They have Init(const nsAString& aJSON); automatically generated.
> 
> 
> >+HTMLInputElement::GetOwnerDateTimeControl()
> >+{
> >+  if (IsInNativeAnonymousSubtree() &&
> >+      mType == NS_FORM_INPUT_TEXT &&
> >+      GetParent() &&
> >+      GetParent()->GetParent() &&
> >+      GetParent()->GetParent()->GetParent() &&
> >+      GetParent()->GetParent()->GetParent()->GetParent()) {
> >+    // Yes, this is very very deep.
> >+    HTMLInputElement* grandparent =
> Hmm, not grandparent, but some ancestor.
> Perhaps just call the variable ownerDateTimeControl
> 
> 
> 
> >   /**
> >+   * Current value in the input box value, in json format. Could be not
> >+   * complete, e.g., if user only entered hour in type=time, then it is
> >+   * { hour: 9 }.
> >+   */
> >+  nsString mDateTimeInputBoxValue;
> So this could be UniquePtr<DateTimeValue> mDateTimeValue; where
> DateTimeValue is coming from webidl dictionary
> 
> >   // This is similar to set .value on nsIDOMInput/TextAreaElements, but handling
> >   // of the value change is closer to the normal user input, so 'change' event
> >   // for example will be dispatched when focusing out the element.
> >-  [ChromeOnly]
> >+  [Func="IsChromeOrXBL"]
> >   void setUserInput(DOMString input);
> On C++ side could we actually limit type=file case to ChromeOnly callers.
> So, somewhere there
> check if nsContentUtils::LegacyIsCallerChromeOrNativeCode return null when
> type is file.
> type=file just happens to be particularly security sensitive.
> Bug 1297393 will make the setup a bit nicer eventually, and could get rid of
> *Legacy* part, but if that bug doesn't 
> land soon, LegacyIsCallerChromeOrNativeCode should be fine until that.
> 
> 
> 
> >+partial interface HTMLInputElement {
> >+  [Pref="dom.forms.datetime", ChromeOnly]
> >+  readonly attribute DOMString dateTimeInputBoxValue;
> >+
> >+  [Pref="dom.forms.datetime", ChromeOnly]
> >+  void updateDateTimeInputBox(DOMString value);
> >+
> >+  [Pref="dom.forms.datetime", ChromeOnly]
> >+  void setDateTimePickerState(boolean open);
> >+
> >+  [Pref="dom.forms.datetime", Func="IsChromeOrXBL"]
> >+  void openDateTimePicker(DOMString initialValue);
> >+
> >+  [Pref="dom.forms.datetime", Func="IsChromeOrXBL"]
> >+  void updateDateTimePicker(DOMString value);
> So I think using dictionary could make this easier to read and use. One can
> just pass around objects.
> Sure, then when passing value to xbl some json handling is needed.
> Though, there has to be some way to nicely pass json value there too...
> thinking.
> One option is to make nsIDateTimeInputArea methods to take jsval and not
> DOMString.
> Then in C++ when you have the dictionary from webidl layer, call
> ToJSValue on it 
> http://searchfox.org/mozilla-central/rev/
> 5bdd2876aeb9dc97dc619ab3e067e86083dad023/dom/bindings/ToJSValue.h#223
> I think you'd need to be in the compartment of the input element's window.
> So before ToJSValue do something like
> scope = xpc::GetXBLScope(aCx, inputElement->GetWrapper());
> AutoJSAPI jsapi;
> NS_ENSURE_STATE(jsapi.Init(scope));
> 
> (add some null checks)
> 
> I think that would make the code a bit simpler. Less manual JSON-fying.

Thanks for the suggestion. I decided to use DOMString because we're going to use these APIs for all date/time input types, that means we'll be passing hour, minute, year, month, day, week, and any of them could be optional. I can try to use dictionary instead, just wanted confirm some points:
- If we do not give a default value to a dictionary member, it is optional, right? Would it be simpler to use something like '-1'?
- When passing from DOM to layout, do we need to use JSON-fy the dictionary, can we use dictionary coming from webidl directly?
- Then, from layout to XBL (nsIDateTimeInputArea), I'll try jsval.

Thanks!

Comment 43

[Olli Pettay [:smaug][[e-mail]]]

dictionary properties could be used as optional, which is the default, or you could make them
nullable
dictionary FooBar {
  long? hour = null;
  ...
}
both should work.

I was thinking DOM to layout would pass the dictionary and then when layout passes to XBL, it would convert to jsval, so yes, what you were thinking.

Comment 44

[Daniel Holbert [:dholbert]]

(Note: when replying to review comments, please delete the pieces of the reply-quote that you're not directly replying to, so the comments aren't quite so huge for only a few lines of response -- e.g. comment 40, comment 42. Thanks!)

Comment 45

[Jessica Jong [:jessica] (inactive)]

(In reply to Daniel Holbert [:dholbert] from comment #44)
> (Note: when replying to review comments, please delete the pieces of the
> reply-quote that you're not directly replying to, so the comments aren't
> quite so huge for only a few lines of response -- e.g. comment 40, comment
> 42. Thanks!)

Noted, will keep that in mind in the future. Thanks.

Comment 46

[Jessica Jong [:jessica] (inactive)]

Attachment: patch, v4.

- Addressed review comment 35, comment 36 and comment 38. Major change is that I have changed the type for date/time value from DOMString (in json format) to dictionary
- Added icon provided by Visual
- Added reviewers name

Comment 47

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8797100 [details] [diff] [review]
patch, v4.

>+void HTMLInputElement::GetDateTimeInputBoxValue(DateTimeValue& aValue)
>+{
>+  MOZ_ASSERT(IsDateTimeInputType(mType),
>+             "This should be called on datetime input types only");

Tiny bit risky assertion. What if some script in the web page has just changes the type? Does the chrome js deal with such case and not call the method.
Could we rather just have 
if (IsDateTimeInputType(mType) && mDateTimeInputBoxValue) {
  aValue = *mDateTimeInputBoxValue
}


>+HTMLInputElement::UpdateDateTimeInputBox(const DateTimeValue& aValue)
>+{
>+  MOZ_ASSERT(IsDateTimeInputType(mType),
>+             "This should be called on datetime input types only");
Perhaps change MOZ_ASSERT to a warning. NS_WARN_IF(!IsDateTimeInputType(mType) ...)
Also GetDateTimeInputBoxValue could use similar warning.


>+HTMLInputElement::SetDateTimePickerState(bool aOpen)
>+{
>+  MOZ_ASSERT(IsDateTimeInputType(mType),
>+             "This should be called on datetime input types only");
This too could just have a warning, IMO.


>+HTMLInputElement::OpenDateTimePicker(const DateTimeValue& aInitialValue)
>+{
>+  MOZ_ASSERT(IsDateTimeInputType(mType),
>+             "This should be called on datetime input types only");
And this.

>+
>+  mDateTimeInputBoxValue = new DateTimeValue(aInitialValue);
>+  nsContentUtils::DispatchChromeEvent(OwnerDoc(),
>+                                      static_cast<nsIDOMHTMLInputElement*>(this),
>+                                      NS_LITERAL_STRING("MozOpenDateTimePicker"),
>+                                      true, true);
This stuff should be probably inside 
if (IsDateTimeInputType(mType)) {} so that we never end up creating a useless DateTimeValue or dispatch an event.

>+HTMLInputElement::UpdateDateTimePicker(const DateTimeValue& aValue)
>+{
>+  MOZ_ASSERT(IsDateTimeInputType(mType),
>+             "This should be called on datetime input types only");
warn


>+
>+  mDateTimeInputBoxValue = new DateTimeValue(aValue);
>+  nsContentUtils::DispatchChromeEvent(OwnerDoc(),
>+                                      static_cast<nsIDOMHTMLInputElement*>(this),
>+                                      NS_LITERAL_STRING("MozUpdateDateTimePicker"),
>+                                      true, true);
if (IsDateTimeInputType(mType)) { ... } around this



>+HTMLInputElement::CloseDateTimePicker()
>+{
>+  MOZ_ASSERT(IsDateTimeInputType(mType),
>+             "This should be called on datetime input types only");
warn

>+
>+  nsContentUtils::DispatchChromeEvent(OwnerDoc(),
>+                                      static_cast<nsIDOMHTMLInputElement*>(this),
>+                                      NS_LITERAL_STRING("MozCloseDateTimePicker"),
>+                                      true, true);
if (IsDateTimeInputType(mType)) {...}

>+HTMLInputElement::SetUserInput(const nsAString& aInput,
>+                               const mozilla::Maybe<nsIPrincipal*>& aPrincipal) {
>+  MOZ_ASSERT(aPrincipal.isSome());
>+
>+  if (mType == NS_FORM_INPUT_FILE &&
>+      !nsContentUtils::IsSystemPrincipal(aPrincipal.value())) {
>+    return;
>+  }
This new principal passing is nice.

>+  if (mType == NS_FORM_INPUT_TIME &&
>+      !IsExperimentalMobileType(mType) &&
>+      aVisitor.mEvent->mMessage == eFocus &&
>+      aVisitor.mEvent->mOriginalTarget == this) {
>+    // If original target is this and not the anonymous text control, we should
>+    // pass the focus to the anonymous text control.
>+    nsDateTimeControlFrame* frame = do_QueryFrame(GetPrimaryFrame());
>+    if (frame) {
>+      frame->HandleFocusEvent();
>+    }
>+  }
Random comment, need to test focus handling quite a bit: ensure that inputElement.focus()/.blur() work, 
tabbing to focus needs to work too, also with tabindex attribute, autofocus attribute. 
Also, what happens to the focus event when focus is move to be inside native anonymous content? Do we end up dispatching to focus events somehow?
Could you add some more tests, in addition to all the events you're already adding.


>+nsDateTimeControlFrame::SetValueFromPicker(const DateTimeValue& aValue)
>+{
>+  nsCOMPtr<nsIDateTimeInputArea> inputAreaContent =
>+    do_QueryInterface(mInputAreaContent);
>+  if (inputAreaContent) {
>+    AutoJSAPI api;
>+    if (!api.Init(mContent->OwnerDoc()->GetScopeObject())) {
>+      return;
>+    }
>+
>+    JSObject* scope = xpc::GetXBLScope(api.cx(), mContent->GetWrapper());
You should null check GetWrapper() (though, in practice we should always have a wrapper since something on the chrome JS is calling the method on HTMLInputElement)

>+    AutoJSAPI jsapi;
>+    if (!jsapi.Init(scope)) {
>+      return;
>+    }
>+
>+    JSContext* cx = jsapi.cx();
>+    JS::Rooted<JS::Value> jsValue(cx);
Nit, I would just pass jsapi.cx() as param



This looks really great, so r+ for DOM stuff. But I wouldn't be surprised if more focus testing reveals something odd.
Did you test the UI with different page zoom levels?

Comment 48

[Jessica Jong [:jessica] (inactive)]

(In reply to Olli Pettay [:smaug] from comment #47)
> Comment on attachment 8797100 [details] [diff] [review]
> patch, v4.
> 
> >+void HTMLInputElement::GetDateTimeInputBoxValue(DateTimeValue& aValue)
> >+{
> >+  MOZ_ASSERT(IsDateTimeInputType(mType),
> >+             "This should be called on datetime input types only");
> 
> Tiny bit risky assertion. What if some script in the web page has just
> changes the type? Does the chrome js deal with such case and not call the
> method.
> Could we rather just have 
> if (IsDateTimeInputType(mType) && mDateTimeInputBoxValue) {
>   aValue = *mDateTimeInputBoxValue
> }
> 

Will change it to "if (NS_WARN_IF(!IsDateTimeInputType(mType)) || mDateTimeInputBoxValue) { return; }"

> 
> >+HTMLInputElement::UpdateDateTimeInputBox(const DateTimeValue& aValue)
> >+{
> >+  MOZ_ASSERT(IsDateTimeInputType(mType),
> >+             "This should be called on datetime input types only");
> Perhaps change MOZ_ASSERT to a warning.
> NS_WARN_IF(!IsDateTimeInputType(mType) ...)
> Also GetDateTimeInputBoxValue could use similar warning.

Will change it to "if (NS_WARN_IF(!IsDateTimeInputType(mType))) { return; }" here and below.
 
> >+  if (mType == NS_FORM_INPUT_TIME &&
> >+      !IsExperimentalMobileType(mType) &&
> >+      aVisitor.mEvent->mMessage == eFocus &&
> >+      aVisitor.mEvent->mOriginalTarget == this) {
> >+    // If original target is this and not the anonymous text control, we should
> >+    // pass the focus to the anonymous text control.
> >+    nsDateTimeControlFrame* frame = do_QueryFrame(GetPrimaryFrame());
> >+    if (frame) {
> >+      frame->HandleFocusEvent();
> >+    }
> >+  }
> Random comment, need to test focus handling quite a bit: ensure that
> inputElement.focus()/.blur() work, 
> tabbing to focus needs to work too, also with tabindex attribute, autofocus
> attribute. 
> Also, what happens to the focus event when focus is move to be inside native
> anonymous content? Do we end up dispatching to focus events somehow?
> Could you add some more tests, in addition to all the events you're already
> adding.

While adding tests, I found that xbl:inherits is not working as expected, e.g. xbl:inherits=tabindex does not set the bound element's (input type=time) tabindex to the inner input=text, need to dig deeper, otherwise, will try to set the attributes manually.

> 
> >+nsDateTimeControlFrame::SetValueFromPicker(const DateTimeValue& aValue)
> >+{
> >+  nsCOMPtr<nsIDateTimeInputArea> inputAreaContent =
> >+    do_QueryInterface(mInputAreaContent);
> >+  if (inputAreaContent) {
> >+    AutoJSAPI api;
> >+    if (!api.Init(mContent->OwnerDoc()->GetScopeObject())) {
> >+      return;
> >+    }
> >+
> >+    JSObject* scope = xpc::GetXBLScope(api.cx(), mContent->GetWrapper());
> You should null check GetWrapper() (though, in practice we should always
> have a wrapper since something on the chrome JS is calling the method on
> HTMLInputElement)

Will do.

> 
> >+    AutoJSAPI jsapi;
> >+    if (!jsapi.Init(scope)) {
> >+      return;
> >+    }
> >+
> >+    JSContext* cx = jsapi.cx();
> >+    JS::Rooted<JS::Value> jsValue(cx);
> Nit, I would just pass jsapi.cx() as param

Will do.

> 
> 
> This looks really great, so r+ for DOM stuff. But I wouldn't be surprised if
> more focus testing reveals something odd.
> Did you test the UI with different page zoom levels?

Yes, different page zoom levels works fine, the icon size for the reset button does not change though.

Comment 49

[Jessica Jong [:jessica] (inactive)]

Attachment: patch, v5.

Added tests for focus, blur, autofocus and tabindex, and fixed some bugs while writing the tests.

Hi Olli, Daniel, since I made some changes in the DOM/Layout code, may I have your review again? Thanks a lot.

Comment 50

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8797946 [details] [diff] [review]
patch, v5.

>+nsDateTimeControlFrame::SetValueFromPicker(const DateTimeValue& aValue)
>+{
>+  nsCOMPtr<nsIDateTimeInputArea> inputAreaContent =
>+    do_QueryInterface(mInputAreaContent);
>+  if (inputAreaContent) {
>+    AutoJSAPI api;
>+    if (!api.Init(mContent->OwnerDoc()->GetScopeObject())) {
>+      return;
>+    }
>+
>+    JSObject* wrapper = mContent->GetWrapper();
>+    if (!wrapper) {
>+      return;
>+    }
>+
>+    JSObject* scope = xpc::GetXBLScope(api.cx(), wrapper);
Now I realized. scope should be null checked too.

>+    AutoJSAPI jsapi;
>+    if (!jsapi.Init(scope)) {
So perhaps just make this if (!scope || !jsapi.Init(scope))

>+  synthesizeKey("VK_TAB", {}); // jump to minute field
>+  synthesizeKey("VK_TAB", {}); // jump to AM/PM field
>+  synthesizeKey("VK_TAB", {}); // jump to next input
>+  is(document.activeElement, time3,
>+     "input element with tabindex=-1 is not tabbable");
Maybe you could check that document.activeElement points to the right element after each VK_TAB

Comment 51

[Daniel Holbert [:dholbert]]

Comment on attachment 8797946 [details] [diff] [review]
patch, v5.

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

r=me on updated version.

I didn't look too closely; I just compared it against the version I previously reviewed, v3, and it looks like the reflow implementation is unchanged, and that's the part that I'm focusing on from a review perspective. (I'm trusting smaug to review the rest.)

Comment 52

[Jessica Jong [:jessica] (inactive)]

Attachment: final patch. r=mconley,dholbert,smaug

Thank you all for your help! :)

Comment 53

[Jessica Jong [:jessica] (inactive)]

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

Comment 54

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ccff388a7ef4
Implement the layout for <input type=time>. r=mconley, r=dholbert, r=smaug

Comment 55

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ccff388a7ef4

Comment 56

[Andrei Vaida [:avaida]]

This looks like something that could benefit from manual QA. Jessica, what do you think?

Comment 57

[Sebastian Zartner [:sebo]]

Is the patch meant to contain the popup, too? Because on Windows 10 it's empty.
Should I open a follow-up bug?

Sebastian

Comment 58

[Daniel Holbert [:dholbert]]

@sebo, that might be covered by bug 1283384 (though I'm not sure).

Comment 59

[Sebastian Zartner [:sebo]]

(In reply to Daniel Holbert [:dholbert] from comment #58)
> @sebo, that might be covered by bug 1283384 (though I'm not sure).

Thank you for the note, Daniel! It looks like this bug builds the basis for bug 1283384, so I mark it as blocker of the other one. Unfortunately the summaries of the two bugs are more or less the same, so it's hard for me to distinguish their purpose.

Sebastian

Comment 60

[Jessica Jong [:jessica] (inactive)]

(In reply to Sebastian Zartner [:sebo] from comment #59)
> (In reply to Daniel Holbert [:dholbert] from comment #58)
> > @sebo, that might be covered by bug 1283384 (though I'm not sure).
> 
> Thank you for the note, Daniel! It looks like this bug builds the basis for
> bug 1283384, so I mark it as blocker of the other one. Unfortunately the
> summaries of the two bugs are more or less the same, so it's hard for me to
> distinguish their purpose.
> 
> Sebastian

Thanks Daniel and Sebastian! Yes, the popup is covered by bug 1283384. And please remember to turn on the pref "dom.forms.datetime".

Comment 61

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

(In reply to Andrei Vaida, QA [:avaida] – please ni? me from comment #56)
> This looks like something that could benefit from manual QA. Jessica, what
> do you think?

Yes. Brindusa is our QA contact to oversee the whole "date time input" project.
There's a WIKI[1] for the QA efforts.

[1] https://wiki.mozilla.org/QA/DateTimeInputTypes

Comment 62

[Sebastian Zartner [:sebo]]

I just saw two UX issues with the current implementation.

1. While the focus is already within the field, clicking into the field to select another part or clearing the value requires two clicks - with the first click you only close the popup. This action should only require one click.

2. When the parts are empty (showing "--"), the text cursor is set between the two dashes, while when they hold a value, it is always selected. It would be more consistent if the two dashes were also selected.

3. The icon to clear the value has no transparent background but a light gray tone. This is especially visible when you change the background.

4. Really a nit: It is not possible to select the whole value and copy it. In Chrome it's possible that you press Ctrl+A to select the whole value and then Ctrl+C to copy it.

Jessica, could you please tell me whether those issues will be fixed by bug 1283384 or whether I should file new bugs?

Sebastian

Comment 63

[Michał Gołębiowski-Owczarek [:mgol]]

(In reply to Sebastian Zartner [:sebo] from comment #62)
> 4. Really a nit: It is not possible to select the whole value and copy it.
> In Chrome it's possible that you press Ctrl+A to select the whole value and
> then Ctrl+C to copy it.

I've just tested it in Chrome and it doesn't behave there nicely either. You can press Cmd+A to select everything but it's displayed in a different shade than the selected time part which indicates it's completely separated from focus. Moreover, you can't remove this selection by clicking on hours/minutes, you have to actually click outside of the input and focus it again to get rid of the selection.

Comment 64

[Sebastian Zartner [:sebo]]

(In reply to Michał Gołębiowski [:m_gol] from comment #63)
> (In reply to Sebastian Zartner [:sebo] from comment #62)
> > 4. Really a nit: It is not possible to select the whole value and copy it.
> > In Chrome it's possible that you press Ctrl+A to select the whole value and
> > then Ctrl+C to copy it.
> 
> I've just tested it in Chrome and it doesn't behave there nicely either. You
> can press Cmd+A to select everything but it's displayed in a different shade
> than the selected time part which indicates it's completely separated from
> focus. Moreover, you can't remove this selection by clicking on
> hours/minutes, you have to actually click outside of the input and focus it
> again to get rid of the selection.

Yes, if that feature is something considered worth to implement, the UX should be better than in Chrome.

Sebastian

Comment 65

[Jessica Jong [:jessica] (inactive)]

Thank you Sebastian for your comment, we really appreciate the feedback we get from early adopters. The issues you mentioned are all improvements we can make for the input box to work better, so feel free to file new bug if there isn't one yet (Bug 1283384 is more about the picker, so it shouldn't fix anything related to the input box). Below are some comments from the implementation point of view. 

(In reply to Sebastian Zartner [:sebo] from comment #62)
> I just saw two UX issues with the current implementation.
> 
> 1. While the focus is already within the field, clicking into the field to
> select another part or clearing the value requires two clicks - with the
> first click you only close the popup. This action should only require one
> click.

The popup is a xul panel, so its default behavior is to hide itself whenever it's clicked outside of it. However, UX commented that we should not hide the popup if we click on the input box, so it seems that we can not rely on xul panel's default behavior. This should be covered by bug 1301310 thou.

> 
> 2. When the parts are empty (showing "--"), the text cursor is set between
> the two dashes, while when they hold a value, it is always selected. It
> would be more consistent if the two dashes were also selected.

This is because "--" is the placeholder, so it can not be selected. If we want consistency, we may give up placeholder and use normal text instead.

> 
> 3. The icon to clear the value has no transparent background but a light
> gray tone. This is especially visible when you change the background.

File bug 1310587, should be fixed soon.

> 
> 4. Really a nit: It is not possible to select the whole value and copy it.
> In Chrome it's possible that you press Ctrl+A to select the whole value and
> then Ctrl+C to copy it.

No, we don't allow copying/pasting for now. I still don't have idea how to achieve this, since the input box is composed by several text boxes, and I am not sure if they can be selected at the same time.

> 
> Jessica, could you please tell me whether those issues will be fixed by bug
> 1283384 or whether I should file new bugs?
> 
> Sebastian

Comment 66

[Sebastian Zartner [:sebo]]

Thank you for your feedback, Jessica!

(In reply to Jessica Jong [:jessica] from comment #65)
> (In reply to Sebastian Zartner [:sebo] from comment #62)
> > I just saw two UX issues with the current implementation.
> > 
> > 1. While the focus is already within the field, clicking into the field to
> > select another part or clearing the value requires two clicks - with the
> > first click you only close the popup. This action should only require one
> > click.
> 
> The popup is a xul panel, so its default behavior is to hide itself whenever
> it's clicked outside of it. However, UX commented that we should not hide
> the popup if we click on the input box, so it seems that we can not rely on
> xul panel's default behavior. This should be covered by bug 1301310 thou.

The point is that you cannot click the input field while the popup is open. But thank you for the hint! I'll follow bug 1301310 and see if the issue gets fixed by it.

> > 2. When the parts are empty (showing "--"), the text cursor is set between
> > the two dashes, while when they hold a value, it is always selected. It
> > would be more consistent if the two dashes were also selected.
> 
> This is because "--" is the placeholder, so it can not be selected. If we
> want consistency, we may give up placeholder and use normal text instead.

I see. I've filed bug 1310823 for it.

> > 3. The icon to clear the value has no transparent background but a light
> > gray tone. This is especially visible when you change the background.
> 
> File bug 1310587, should be fixed soon.

Great, thanks!

> > 4. Really a nit: It is not possible to select the whole value and copy it.
> > In Chrome it's possible that you press Ctrl+A to select the whole value and
> > then Ctrl+C to copy it.
> 
> No, we don't allow copying/pasting for now. I still don't have idea how to
> achieve this, since the input box is composed by several text boxes, and I
> am not sure if they can be selected at the same time.

Ok, with the current implementation that's probably not feasible that way. I suppose, what could be done instead is to implement the copy and paste actions on the wrapping input. I've filed bug 1310831 for that.

Sebastian

Comment 67

[Chris Mills [:cmills]]

I've set this to ddc for now, as this purely seems to be about implementing the layout, rather than turning it on in the browser. Let me know if I'm wrong. Is there a separate bug for turning it on in the browser (I just tested it in latest nightly, and it doesn't seem to be enabled yet)?

Comment 68

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #67)
> I've set this to ddc for now, as this purely seems to be about implementing
> the layout, rather than turning it on in the browser. Let me know if I'm
> wrong. Is there a separate bug for turning it on in the browser (I just
> tested it in latest nightly, and it doesn't seem to be enabled yet)?
It's currently pref-off because we're focusing on enabling more features. 
Pref-on plan is under discussion. Some information available in below WIKI.
https://wiki.mozilla.org/TPE_DOM/Date_time_input_types#Roadmap

Comment 69

[Sebastian Zartner [:sebo]]

(In reply to Wesley Huang [:wesley_huang] (EPM) (NI me) from comment #68)
> (In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #67)
> > I've set this to ddc for now, as this purely seems to be about implementing
> > the layout, rather than turning it on in the browser. Let me know if I'm
> > wrong. Is there a separate bug for turning it on in the browser (I just
> > tested it in latest nightly, and it doesn't seem to be enabled yet)?
> It's currently pref-off because we're focusing on enabling more features. 
> Pref-on plan is under discussion. Some information available in below WIKI.
> https://wiki.mozilla.org/TPE_DOM/Date_time_input_types#Roadmap

I've added an entry to https://developer.mozilla.org/en-US/Firefox/Experimental_features#HTML. (I've chosen 51 as version, because the preference is already available in the 51 Beta, even when the first UI was added in 52, as far as I can see.)

Sebastian