Scott Wu [:scottwu] Assignee
	Description • 8 years ago

      No description provided.

	Scott Wu [:scottwu] Assignee
	Comment 1 • 8 years ago

(1/2) [WIP] Temporary patch for date time input frame (from Jessica: Bug 1288591)

	Scott Wu [:scottwu] Assignee
	Comment 2 • 8 years ago

(2/2) [WIP] Time picker pop up UI

In browser.xul, DateTimePickerPanel is added, waiting to be displayed. DateTimeContentHelper.jsm would use sendAsyncMessage to call DateTimeParentHelper.jsm via remote-browser. The parent helper then loads the picker and opens it up.

Here are some technical decisions we've made so far:

1. The communication between input box and pop up UI is through messageManager.sendAsyncMessage via helper jsms.
2. The actual picker is written in xhtml and loaded in an iframe inside the picker XUL element (which binds to the panel). Rather than building the pickers in XUL, we want to try building them in standard html, which generally has less quirks and more predictable to work with. But I'm not sure if there are pitfalls I should watch out for.
3. Related to #2, currently the iframe page is loaded when panel is opened, so there's a slight loading time involved. An alternative is having it loaded beforehand, but it's going to take up more resources.
4. The communication between the XUL panel and iframe picker is done by listening and dispatching custom event. I listen to changes by attaching listeners to the iframe's contentDocument.

The pop up UI still has quite a bit work left to do, including AM/PM switching, proper dragging, l10n, a11y and keyboard event handling, and more.

Any feedback is appreciated! Thanks.

	Tim Guan-tin Chien [:timdream] (please needinfo)
	Comment 3 • 8 years ago

Mike, would this be something you could be the reviewer?

	Tim Guan-tin Chien [:timdream] (please needinfo)
	Comment 4 • 8 years ago

Neil, you were working on e10s-select and this patch is modelled after that. Would you be able the reviewer of this patch?

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

Comment on attachment 8777400 [details] [diff] [review]
0001-Temporary-patch-for-date-time-input-frame.patch

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

Just gave this a high-level look - and mostly looking at organization. I didn't look at the more complex things going on in nsDateTimeControlFrame (Enn might be better for feedback there).

In general, I think this is the right idea. See notes below - let me know if you have questions!

Thanks!

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

noautohide="false" is the default, and can be removed.

@@ +167,5 @@
> +           consumeoutsideclicks="true"
> +           noautohide="false"
> +          level="parent">
> +      <vbox>
> +        <iframe id="myFrame" width="200" height="200"/>

These hardcoded dimensions concern me - probably should be done in CSS, and perhaps with em units... We'll want to test different zoom levels too.

::: layout/forms/nsDateTimeControlFrame.cpp
@@ +71,5 @@
> +nsDateTimeControlFrame::ShowPicker()
> +{
> +  if (XRE_IsContentProcess()) {
> +    nsContentUtils::DispatchChromeEvent(mContent->OwnerDoc(), mContent,
> +                                        NS_LITERAL_STRING("mozshowdatetimepicker"),

We should probably do this for the non-content process case too, no?

::: toolkit/content/datetime-child.js
@@ +7,5 @@
> +XPCOMUtils.defineLazyModuleGetter(this, "DateTimeContentHelper",
> +                                  "resource://gre/modules/DateTimeContentHelper.jsm");
> +
> +addEventListener("mozshowdatetimepicker", event => {
> +  if (!event.isTrusted) {

This stuff can probably be safely moved into browser-content.js under toolkit/content. browser-content.js will be used by both remote and non-remote browsers.

::: toolkit/modules/DateTimeContentHelper.jsm
@@ +2,5 @@
> + * 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/. */
> +
> +"use strict";
> +

This file seems pretty similar to SelectContentHelper.jsm.

I wonder if we should combine the two in the same JSM (to avoid the memory overhead of new JSM compartments) and have a single FormControlContentHelper.jsm?

::: toolkit/modules/DateTimeParentHelper.jsm
@@ +1,4 @@
> +/* 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/. */
> +

Similarly, this maybe could be combined into the same JSM as SelectParentHelper.jsm as FormControlParentHelper.jsm.

	Scott Wu [:scottwu] Assignee
	Comment 6 • 8 years ago

Hello Mike, thank you so much for going over the patch! I wonder if I could get your feedback again for the 2nd part of the patch? It deals with the message passing mechanism between the picker panel and input box, as detailed in comment 2. The picker itself is still being worked on and subject to refactoring, so I'm more concerned about whether the communication aspect looks fine or not.

Really appreciate your help :)

	Scott Wu [:scottwu] Assignee
	Comment 7 • 8 years ago

Hi David, this is the bug for the scroller problem I asked you about. I have a time picker in xhtml within a xul:iframe, and I'd like to hide the scrollbars that appear in the picker. When I use the browser toolbox inspector, I can see that there are "scrollbar" elements, but I can't find ways to select them (with CSS selector or DOM API). I'm thinking if I could select them somehow, then I could just use display: none to hide them. Maybe I'm missing something or is there another way to hide the scrollbars?

Thanks a lot!

You can see it in action if you apply both patches, and use 'data:text/html, <input type="time">' as URL.

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

(In reply to Mike Conley (:mconley) - (Needinfo me!) from comment #5)
> Comment on attachment 8777400 [details] [diff] [review]
> 0001-Temporary-patch-for-date-time-input-frame.patch
> 
> Review of attachment 8777400 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Just gave this a high-level look - and mostly looking at organization. I
> didn't look at the more complex things going on in nsDateTimeControlFrame
> (Enn might be better for feedback there).

Thanks Mike for the feedback, actually this is a WIP taken from Bug 1288591, which is the bug for the layout part. :dholbert has given us some suggestions about nsDateTimeControlFrame, feedback for this part was just needed. :) 

> 
> In general, I think this is the right idea. See notes below - let me know if
> you have questions!
> 
> Thanks!
> 
> ::: browser/base/content/browser.xul
> @@ +164,5 @@
> > +    <panel id="DateTimePickerPanel"
> > +           hidden="true"
> > +           noautofocus="true"
> > +           consumeoutsideclicks="true"
> > +           noautohide="false"
> 
> noautohide="false" is the default, and can be removed.

Will do, and I plan to set consumeoutsideclicks to 'false', I think we shouldn't consume the clicks.

> 
> @@ +167,5 @@
> > +           consumeoutsideclicks="true"
> > +           noautohide="false"
> > +          level="parent">
> > +      <vbox>
> > +        <iframe id="myFrame" width="200" height="200"/>
> 
> These hardcoded dimensions concern me - probably should be done in CSS, and
> perhaps with em units... We'll want to test different zoom levels too.

Oh, this was just for testing, Part 2 in this bug will override this.

> 
> ::: layout/forms/nsDateTimeControlFrame.cpp
> @@ +71,5 @@
> > +nsDateTimeControlFrame::ShowPicker()
> > +{
> > +  if (XRE_IsContentProcess()) {
> > +    nsContentUtils::DispatchChromeEvent(mContent->OwnerDoc(), mContent,
> > +                                        NS_LITERAL_STRING("mozshowdatetimepicker"),
> 
> We should probably do this for the non-content process case too, no?

Yes, the if condition will be removed once we start working on non-e10s.

> 
> ::: toolkit/content/datetime-child.js
> @@ +7,5 @@
> > +XPCOMUtils.defineLazyModuleGetter(this, "DateTimeContentHelper",
> > +                                  "resource://gre/modules/DateTimeContentHelper.jsm");
> > +
> > +addEventListener("mozshowdatetimepicker", event => {
> > +  if (!event.isTrusted) {
> 
> This stuff can probably be safely moved into browser-content.js under
> toolkit/content. browser-content.js will be used by both remote and
> non-remote browsers.

Sure, browser-content.js sounds good. So, for this to work in e10s and non-e10s, here is where we check in which process we are, and act accordingly, right? Maybe load DateTimeContentHelper in content process case, and loads DateTimeParentHelper directly in parent process, and let them have common methods. Do you have any other suggestions or sample that we could follow?
> 
> ::: toolkit/modules/DateTimeContentHelper.jsm
> @@ +2,5 @@
> > + * 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/. */
> > +
> > +"use strict";
> > +
> 
> This file seems pretty similar to SelectContentHelper.jsm.
> 
> I wonder if we should combine the two in the same JSM (to avoid the memory
> overhead of new JSM compartments) and have a single
> FormControlContentHelper.jsm?
> 
> ::: toolkit/modules/DateTimeParentHelper.jsm
> @@ +1,4 @@
> > +/* 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/. */
> > +
> 
> Similarly, this maybe could be combined into the same JSM as
> SelectParentHelper.jsm as FormControlParentHelper.jsm.

Sure, we'll see how to combine them into one.

	Scott Wu [:scottwu] Assignee
	Comment 9 • 8 years ago

Update WIP

	Scott Wu [:scottwu] Assignee
	Comment 10 • 8 years ago

We found a way to select the scrollbar, but it involves adding a rule to xul.css, and wrapping the picker in a xul element. It does the trick, but I see that xul.css is locked down (Neil?), so there might be a better place to put it, or maybe a different solution altogether.

	Tim Guan-tin Chien [:timdream] (please needinfo)
	Comment 11 • 8 years ago

(In reply to Scott Wu [:scottwu] from comment #10)
> Created attachment 8780035 [details] [diff] [review]
> 0003-Hide-scrollbar.patch
> 
> so there might be a better place to
> put it, or maybe a different solution altogether.

A second thought, maybe put this in browser/base/content/browser.css since it's part of browser chrome?

That said, I sincerely hope we have alternative solution here...

	Tim Guan-tin Chien [:timdream] (please needinfo)
	Comment 12 • 8 years ago

(In reply to Scott Wu [:scottwu] from comment #10)
> Created attachment 8780035 [details] [diff] [review]

Allow me to try to explain what we are doing here: We need to hide the scroll bar, but interestingly we have found no way to get a references of the <scrollbar> element, and set |display: none| on it.

And no, it's not something you can get to with |doc.getAnonymousNodes(divEl)|.

Given DevTools can find them and show them in Browser Toolbox, I've spent some time looking into it's code base. I realized it calls into an inIDeepTreeWalker interface and walk the tree.

http://searchfox.org/mozilla-central/rev/d83f1528dbcb1e837d99cf5b6c36a90b03bf0e77/devtools/server/actors/inspector.js#2842

What's also interesting is that DevTools classify these elements as "anonymous content" element", which according the logic here is simply, neither a shadow DOM element or XBL anonymous element.

http://searchfox.org/mozilla-central/rev/d83f1528dbcb1e837d99cf5b6c36a90b03bf0e77/devtools/shared/layout/utils.js#594-651

I don't think this is proper way to locate and hide the scroll bar either, but this is an interesting finding.

	Neil Deakin
	Comment 13 • 8 years ago

Scrollbar visibility is controlled by the overflow css property. You never manipulate the scrollbars directly.

	Tim Guan-tin Chien [:timdream] (please needinfo)
	Comment 14 • 8 years ago

(In reply to Neil Deakin from comment #13)
> Scrollbar visibility is controlled by the overflow css property. You never
> manipulate the scrollbars directly.

We can't. The goal here is to leverage native scroll and scroll snap to implement a browser UI in HTML, without having a visible scroll bar. The alternative would be completely rebuild these behavior with JavaScript and CSS transform/transition, which makes no sense given we are working on the browser itself.

Would you mind look at attachment 8780035 [details] [diff] [review] and see if it's an acceptable workaround, if the XUL-specific style is put elsewhere than xul.css? Would you be able to tell us any alternative workaround?

	Tim Guan-tin Chien [:timdream] (please needinfo)
	Comment 15 • 8 years ago

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #11)
> A second thought, maybe put this in browser/base/content/browser.css since
> it's part of browser chrome?

This won't work. The rule has to be in a UA stylesheet to affect HTML documents.

	Tim Guan-tin Chien [:timdream] (please needinfo)
	Comment 16 • 8 years ago

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #15)
> This won't work. The rule has to be in a UA stylesheet to affect HTML
> documents.

With that, I've found the minimal workaround that I am happy with:

var domWinUtls = window.QueryInterface(Components.interfaces.nsIInterfaceRequestor).
                getInterface(Components.interfaces.nsIDOMWindowUtils);
domWinUtls.loadSheetUsingURIString('data:text/css,@namespace url("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"); scrollbar { display: none; }', domWinUtls.AGENT_SHEET);

This loads a small UA stylesheet into the window in question, that would remove the scrollbar.

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

Sorry for the delay - I'll have a review for this tomorrow.

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

I'm greatly confused by what I'm reading in this bug. Can somebody explain the scrollbar problem to me?

(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #14)
> (In reply to Neil Deakin from comment #13)
> > Scrollbar visibility is controlled by the overflow css property. You never
> > manipulate the scrollbars directly.
> 
> We can't. The goal here is to leverage native scroll and scroll snap to
> implement a browser UI in HTML, without having a visible scroll bar. The
> alternative would be completely rebuild these behavior with JavaScript and
> CSS transform/transition, which makes no sense given we are working on the
> browser itself.
> 

Can somebody show me a screenshot or something illustrating the problem, and the expected result? Messing around with scrollbars like this is pretty unusual, and (as Neil pointed out), we usually tell something that it doesn't have scrollbars using the overflow property - probably set to hidden if you don't want to see the things that are clipped by the area that was scrollable.

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

Comment on attachment 8780028 [details] [diff] [review]
0002-Bug-1283384-WIP-Time-picker-w-message-passing.patch

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

Out of curiosity, you appear to be creating the timepicker dynamically. For example, each spinner is a class that you're instantiating and then you're doing <template> cloning and insertion... why is this?

	Tim Guan-tin Chien [:timdream] (please needinfo)
	Comment 20 • 8 years ago

Scott will post screenshots here. Basically we are told to implement a spinning UI with visual hints of it's own, so there is no need to show the scroll bar.

	Scott Wu [:scottwu] Assignee
	Comment 21 • 8 years ago

Thanks a lot for looking over this bug, Mike. I've attached a screenshot to help explain what we've done so far, and here's a link to the UX spec: https://mozilla.invisionapp.com/share/237UTNHS8#/screens/171580963

Basically the UX team wants spinners much like what we see on iOS and Android, and they specifically want us to hide the scrollbar. Users can find a value by scrolling up and down with trackpad or mouse wheel. However if we use |overflow: hidden|, the spinner would become unscrollable, and we would need to re-implement things like scroll momentum and scroll snap ourselves, and it won't be as smooth as native scrolling.

	Scott Wu [:scottwu] Assignee
	Comment 22 • 8 years ago

Comment on attachment 8780028 [details] [diff] [review]
0002-Bug-1283384-WIP-Time-picker-w-message-passing.patch

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

Yeah this is something I would like your input for. The spinners are created dynamically because there might be 2 or 3 spinners depending on the format (12hr/24hr). I thought it's easier to see the final markup using template, instead of create the elements in JS. On the other hand it makes spinner.js dependent on the markup on the xhtml picker, so I'm not sure if this is a good way either.

	Tim Guan-tin Chien [:timdream] (please needinfo)
	Comment 23 • 8 years ago

(In reply to Scott Wu [:scottwu] from comment #22)
> On the other hand it makes spinner.js
> dependent on the markup on the xhtml picker, so I'm not sure if this is a
> good way either.

This is not really a concern. We don't expect spinner.js to be used independently of the XHTML file.

	Scott Wu [:scottwu] Assignee
	Comment 24 • 8 years ago

Here's an updated WIP. I've extracted the time state management to a separate file, and cleaned up some dead code. The message passing mechanism remains the same.

	Scott Wu [:scottwu] Assignee
	Comment 27 • 8 years ago

Comment on attachment 8782844 [details]
Bug 1283384 - (1/2) Rename form validation anchor to make it more generic

Pushed WIP to mozreview for better readability. Please let me know if you need any clarification. Thanks a lot! You can find me on IRC too.

	Tim Guan-tin Chien [:timdream] (please needinfo)
	Comment 28 • 8 years ago

Comment on attachment 8782844 [details]
Bug 1283384 - (1/2) Rename form validation anchor to make it more generic

I am working with Scott to get his OO pattern corrected. Will ask for feedback again once there is a new patch.

The goal here is to get an early feedback on the technical direction -- how events are routed, and how JavaScript classes are setup to show the UI.

	Scott Wu [:scottwu] Assignee
	Comment 30 • 8 years ago

Hello Mike,

The latest WIP depends on Bug 1288591, and this patch depends on WIP 3: https://bug1288591.bmoattachments.org/attachment.cgi?id=8785231.

I've added some more comments and hopefully makes it easier to read.

Thanks a lot!

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

(In reply to Scott Wu [:scottwu] from comment #30)
> Hello Mike,
> 
> The latest WIP depends on Bug 1288591, and this patch depends on WIP 3:
> https://bug1288591.bmoattachments.org/attachment.cgi?id=8785231.
> 
> I've added some more comments and hopefully makes it easier to read.
> 
> Thanks a lot!

Yes, the patch in bug 1288591 makes a lot of sense - I commented over there, and I think it's a solid foundation.

	Scott Wu [:scottwu] Assignee
	Comment 33 • 8 years ago

Hello Mike,

Wonder if you could give me some feedback on my patch?
https://reviewboard.mozilla.org/r/72878/diff/2-3/ (depends on WIP5 from bug 1288591)

It's for the time picker UI. I've made some modification since I last posted due to UX changes, but the architecture stays the same.

I know this patch is kinda big, so please let me know if you find any part confusing. Thanks a lot!

Implemented features:
- Creates time picker based on attributes (value, min, max, step) on the input element
- If one or more value is changed, all the value on picker will be passed to the input box when close
- When min/max is specified, invalid items are given grey color.
- When step is specified, only the valid items are shown.
- Value can be selected by scrolling with trackpad or mousewheel, clicking the up and down arrow keys, clicking on items, dragging on items.
- Selected value are highlighted, except if the value is invalid

What each file does:
- DateTimePickerHelper.jsm
Is responsible for opening and closing the pickers. It calls methods on datetimepopup.
- datetimepopup.xml:
Binds to the DateTimePickerPanel. It creates an iframe for the picker based on the input types, and listens to the changes. It then passes the values to input boxes.
- timepicker.xhtml:
It is loaded in an iframe. It contains the HTML template, js, and stylesheet for the time picker.
- timepicker.js:
Is responsible for creating the time picker. It initializes TimeKeeper, which keeps track of the time states.
- spinner.js:
The spinner object only cares about displaying the values given by timepicker. It has no knowledge of whether hour or minute is being displayed.

Known issues:
- Highlight state on AM/PM and hour is buggy
- Clicking on an already centered item should select it
- Warning box should be shown when the time state is out of range or off step (visual spec is still being discussed)

	Mike Conley (:mconley) (:⚙️)
	Comment 34 • 8 years ago
mozreview-review

Comment on attachment 8782844 [details]
Bug 1283384 - (1/2) Rename form validation anchor to make it more generic

https://reviewboard.mozilla.org/r/72878/#review75200

I'll continue my review tomorrow, but here are my initial comments.

::: browser/base/content/browser.xul:163
(Diff revision 3)
> +           type="arrow"
>             hidden="true"
> +           orient="vertical"
>             noautofocus="true"
>             consumeoutsideclicks="false"
> -           level="parent">
> +           animate="false">

Probably should leave the "level" attribute where it is instead of removing it.

::: toolkit/content/widgets/datetimepopup.xml:14
(Diff revision 3)
> +    <content flip="both" side="top" position="bottomcenter topleft" consumeoutsideclicks="false">
> +      <xul:vbox anonid="container" class="panel-arrowcontainer" flex="1"
> +               xbl:inherits="side,panelopen">
> +        <xul:box anonid="arrowbox" class="panel-arrowbox">
> +          <xul:image anonid="arrow" class="panel-arrow" xbl:inherits="side"/>
> +        </xul:box>
> +        <xul:box class="panel-arrowcontent" xbl:inherits="side,align,dir,orient,pack" flex="1">
> +          <xul:vbox align="top">
> +            <xul:iframe id="dateTimePopupFrame" anonid="dateTimePopupFrame"/>
> +          </xul:vbox>
> +        </xul:box>
> +      </xul:vbox>
> +    </content>

I don't think it's necessary to re-implement the contents of this binding.

[Check out this line from the current binding definition of arrowpanel.](http://searchfox.org/mozilla-central/rev/95b0ecf4b59a01e0524ca02f6c96ecaabe38f4d5/toolkit/content/widgets/popup.xml#365)

That <children/> makes it so that any nodes that are children of the bound node get automatically injected into that portion of the anonymous content.

What I suggest is that you get rid of the <content> block, and inherit it from arrowpanel instead. Then, in browser.xul, where you have the DateTimePickerPanel, put your iframe in there.

::: toolkit/content/widgets/datetimepopup.xml:42
(Diff revision 3)
> +              this.dateTimePopupFrame.style.width = "14em";
> +              this.dateTimePopupFrame.style.height = "14em";

We should probably have this 14em string be a constant somewhere.

::: toolkit/content/widgets/datetimepopup.xml:50
(Diff revision 3)
> +          }
> +        ]]></body>
> +      </method>
> +      <method name="closePicker">
> +        <body><![CDATA[
> +          this.hidden = true;

Seems a bit odd that we set hidden to true internally, but set it to false externally. We should probably make sure the same thing has the responsibility of setting this property.

::: toolkit/content/widgets/datetimepopup.xml:53
(Diff revision 3)
> +      <method name="closePicker">
> +        <body><![CDATA[
> +          this.hidden = true;
> +          this.setInputBoxValue(true);
> +          this.pickerState = {};
> +          this.dateTimePopupFrame.removeEventListener("load", this, true);

Should we also clear out the type?

::: toolkit/content/widgets/datetimepopup.xml:54
(Diff revision 3)
> +        <body><![CDATA[
> +          this.hidden = true;
> +          this.setInputBoxValue(true);
> +          this.pickerState = {};
> +          this.dateTimePopupFrame.removeEventListener("load", this, true);
> +          this.dateTimePopupFrame.setAttribute("src", "");

Is it necessary to unload the document? Perhaps we should open a follow-up such that if the next field also happens to be of type "time", we can re-use the last picker loaded in the iframe, rather than loading it again.

::: toolkit/content/widgets/datetimepopup.xml:62
(Diff revision 3)
> +      <method name="setPopupValue">
> +        <parameter name="data"/>
> +        <body><![CDATA[
> +          switch (this.type) {
> +            case "time":
> +              const { hour, minute } = data.value ? JSON.parse(data.value) : {};

Out of curiosity, why is this coming in as a JSON string? Why can we not just pass in an object with the values we want?

::: toolkit/content/widgets/datetimepopup.xml:63
(Diff revision 3)
> +        <parameter name="data"/>
> +        <body><![CDATA[
> +          switch (this.type) {
> +            case "time":
> +              const { hour, minute } = data.value ? JSON.parse(data.value) : {};
> +              this.dateTimePopupFrame.contentDocument.dispatchEvent(new CustomEvent("TimePickerSetValue", {

I wonder if postMessage might make more sense.

::: toolkit/content/widgets/datetimepopup.xml:127
(Diff revision 3)
> +        <parameter name="aEvent"/>
> +        <body><![CDATA[
> +          switch (aEvent.type) {
> +            case "load":
> +              this.initPicker(this.detail);
> +              this.dateTimePopupFrame.contentDocument.addEventListener("TimePickerPopupChanged", (evt) => {

We never remove this. Because the function is anonymous, we'll add more and more of them for each load event.

We should probably use message listeners instead, and just set up the listeners once - perhaps in the <constructor>, and then remove them in the <destructor>.

::: toolkit/content/widgets/spinner.js:13
(Diff revision 3)
> +// not care what the values represent. The setValue function is called
> +// when it detects a change in value triggered by scroll event.
> +// Supports scrolling, clicking on prev or next, clicking on item, and
> +// dragging.
> +
> +const Spinner = function ctor_Spinner(props, context) {

The cto_Spinner naming pattern seems a bit odd. SpiderMonkey will do the "right thing" if you name the function like so:

function Spinner(props, context) {
  // ...
};

Unless there's a reason you're setting it like this?

::: toolkit/content/widgets/spinner.js:19
(Diff revision 3)
> +  this.context = context;
> +  this._init(props);
> +};
> +
> +Spinner.prototype = {
> +  _init(props) {

Each new method that's being added here needs to have proper documentation. Particularly, we need documentation on what the function does, what its expected inputs are, return values, and any assumptions it makes.

See BrowserTestUtils.jsm as an example.

::: toolkit/content/widgets/spinner.js:20
(Diff revision 3)
> +  this._init(props);
> +};
> +
> +Spinner.prototype = {
> +  _init(props) {
> +    const { setValue, getDisplayString, itemHeight = 20, viewportSize = 5 } = props;

Let's pull out these magic numbers as documented constants please.

::: toolkit/content/widgets/timepicker.js:41
(Diff revision 3)
> +      format: format || "12"
> +    });
> +    timeKeeper.setState({ hour: timerHour, minute: timerMinute });
> +
> +    this.state = { timeKeeper };
> +    window.t = timeKeeper;

What is window.t used for?

::: toolkit/content/widgets/timepicker.js:66
(Diff revision 3)
> +        setValue: wrapSetValueFn(value => {
> +          timeKeeper.setHour(value);
> +          this.state.isHourSet = true;
> +        }),
> +        getDisplayString: hour => {
> +          if (format == "24") {

We should try to reduce the number of magic strings and numbers where possible, and use constants if we can.

::: toolkit/content/widgets/timepicker.xhtml:32
(Diff revision 3)
> +  // capability, so using |overflow: hidden| is not an option.
> +  // Instead, we are inserting a user agent stylesheet that is
> +  // capable of selecting scrollbars, and do |display: none|.
> +  var domWinUtls = window.QueryInterface(Components.interfaces.nsIInterfaceRequestor).
> +                  getInterface(Components.interfaces.nsIDOMWindowUtils);
> +  domWinUtls.loadSheetUsingURIString('data:text/css,@namespace url("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"); scrollbar { display: none; }', domWinUtls.AGENT_SHEET);

Out of curiosity, why is it necessary to load the stylesheet dynamically like this, instead of in the markup, like:

http://searchfox.org/mozilla-central/rev/95b0ecf4b59a01e0524ca02f6c96ecaabe38f4d5/browser/components/feeds/content/subscribe.xhtml#19

	Tim Guan-tin Chien [:timdream] (please needinfo)
	Comment 35 • 8 years ago

(In reply to Mike Conley (:mconley) - (Digging through needinfos and reviews) from comment #34)
> ::: toolkit/content/widgets/timepicker.xhtml:32
> (Diff revision 3)
> > +  // capability, so using |overflow: hidden| is not an option.
> > +  // Instead, we are inserting a user agent stylesheet that is
> > +  // capable of selecting scrollbars, and do |display: none|.
> > +  var domWinUtls = window.QueryInterface(Components.interfaces.nsIInterfaceRequestor).
> > +                  getInterface(Components.interfaces.nsIDOMWindowUtils);
> > +  domWinUtls.loadSheetUsingURIString('data:text/css,@namespace url("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"); scrollbar { display: none; }', domWinUtls.AGENT_SHEET);
> 
> Out of curiosity, why is it necessary to load the stylesheet dynamically
> like this, instead of in the markup, like:
> 
> http://searchfox.org/mozilla-central/rev/
> 95b0ecf4b59a01e0524ca02f6c96ecaabe38f4d5/browser/components/feeds/content/
> subscribe.xhtml#19

Is it possible to declare a page-specific user-agent stylesheet in the markup? I think Scott tried it and it didn't work.

	Scott Wu [:scottwu] Assignee
	Comment 36 • 8 years ago
mozreview-review-reply

Comment on attachment 8782844 [details]
Bug 1283384 - (1/2) Rename form validation anchor to make it more generic

https://reviewboard.mozilla.org/r/72878/#review75200

Thanks a lot! You've been super helpful :)

> Probably should leave the "level" attribute where it is instead of removing it.

Ok!

> I don't think it's necessary to re-implement the contents of this binding.
> 
> [Check out this line from the current binding definition of arrowpanel.](http://searchfox.org/mozilla-central/rev/95b0ecf4b59a01e0524ca02f6c96ecaabe38f4d5/toolkit/content/widgets/popup.xml#365)
> 
> That <children/> makes it so that any nodes that are children of the bound node get automatically injected into that portion of the anonymous content.
> 
> What I suggest is that you get rid of the <content> block, and inherit it from arrowpanel instead. Then, in browser.xul, where you have the DateTimePickerPanel, put your iframe in there.

Yes you are right! Originally I ran into problem when I had <iframe> in the <content> of datetimepopup.xml, the arrowpanel wouldn't work without its markup, but the problem is solved now that I move the <iframe> to browser.xul

> We should probably have this 14em string be a constant somewhere.

Ok. I've moved them to fields with readonly=true.

> Seems a bit odd that we set hidden to true internally, but set it to false externally. We should probably make sure the same thing has the responsibility of setting this property.

Yes set false should also be set internally.

> Should we also clear out the type?

For now type should be cleared out just to keep things clean. But we would need it if we were to reuse the iframe.

> Is it necessary to unload the document? Perhaps we should open a follow-up such that if the next field also happens to be of type "time", we can re-use the last picker loaded in the iframe, rather than loading it again.

Yes it is possible to reuse the document if the input type is the same, and it will save the initial loading time. I'll try it in a follow-up bug.

> Out of curiosity, why is this coming in as a JSON string? Why can we not just pass in an object with the values we want?

From what I understand, some DOM operations in webidl can only take string, so it's easier for Jessica if the data is in string form. However I'll probably parse the string earlier, before it's passed to the panel.

> I wonder if postMessage might make more sense.

Do you think there's any drawback currently? Or advantages of using postMessage?

> We never remove this. Because the function is anonymous, we'll add more and more of them for each load event.
> 
> We should probably use message listeners instead, and just set up the listeners once - perhaps in the <constructor>, and then remove them in the <destructor>.

I'll look into using messageListener, but prob not in constructor & destructor because the panel is in browser.xul and the listener would be added even if it's not needed.

> The cto_Spinner naming pattern seems a bit odd. SpiderMonkey will do the "right thing" if you name the function like so:
> 
> function Spinner(props, context) {
>   // ...
> };
> 
> Unless there's a reason you're setting it like this?

Ok. I was following a pattern I've seen from the Gaia code base. I'll just change it to simply using function declaration if that's the pattern in Firefox.

> Each new method that's being added here needs to have proper documentation. Particularly, we need documentation on what the function does, what its expected inputs are, return values, and any assumptions it makes.
> 
> See BrowserTestUtils.jsm as an example.

Got it. I'll add proper documentation to each function.

> Let's pull out these magic numbers as documented constants please.

Ok I'll declare them as const on the top.

> What is window.t used for?

Sorry that shouldn't be there!

> We should try to reduce the number of magic strings and numbers where possible, and use constants if we can.

Ok I'll pull out those magic strings/numbers.

> Out of curiosity, why is it necessary to load the stylesheet dynamically like this, instead of in the markup, like:
> 
> http://searchfox.org/mozilla-central/rev/95b0ecf4b59a01e0524ca02f6c96ecaabe38f4d5/browser/components/feeds/content/subscribe.xhtml#19

We haven't been able to select the scrollbar xul element from the xhtml file or in a css file. The only other way we've found was by changing the stylesheets for xul, which seems less than ideal.

	Mike Conley (:mconley) (:⚙️)
	Comment 37 • 8 years ago
mozreview-review

Comment on attachment 8782844 [details]
Bug 1283384 - (1/2) Rename form validation anchor to make it more generic

https://reviewboard.mozilla.org/r/72878/#review75524

I apologize for reviewing this in bursts. There's a lot here, and it's quite complicated. I'll have more feedback tomorrow.

::: toolkit/content/widgets/spinner.js:23
(Diff revision 3)
> +Spinner.prototype = {
> +  _init(props) {
> +    const { setValue, getDisplayString, itemHeight = 20, viewportSize = 5 } = props;
> +
> +    const spinnerTemplate = document.getElementById("spinner-template");
> +    const spinner = document.importNode(spinnerTemplate.content, true);

It's a bit strange to have the whole thing called the spinner, and for there to be a subelement called spinner. Is there a better name for either of these?

::: toolkit/content/widgets/spinner.js:31
(Diff revision 3)
> +      items: [],
> +      isScrolling: false
> +    };
> +    this.props = {
> +      setValue, getDisplayString, itemHeight, viewportSize,
> +      viewportTopOffset: (viewportSize - 1) / 2

Can you add some documentation about this calculation? Why ` - 1` for example?

::: toolkit/content/widgets/spinner.js:41
(Diff revision 3)
> +      next: spinner.querySelector(".next"),
> +      itemsViewElements: []
> +    };
> +
> +    this.context.appendChild(spinner);
> +    this._attachEventListners();

Typo: "Listners" -> "Listeners"

::: toolkit/content/widgets/spinner.js:44
(Diff revision 3)
> +
> +    this.context.appendChild(spinner);
> +    this._attachEventListners();
> +  },
> +  // Only the parent component calls setState on the spinner.
> +  // It checks if the items has changed and updates the DOM.

Typo: "items has changed" -> "items have changed"

::: toolkit/content/widgets/spinner.js:46
(Diff revision 3)
> +    this._attachEventListners();
> +  },
> +  // Only the parent component calls setState on the spinner.
> +  // It checks if the items has changed and updates the DOM.
> +  // If only the value has changed, smooth scrolls to the new value.
> +  setState(newState) {

We need to document the format of newState.

::: toolkit/content/widgets/spinner.js:83
(Diff revision 3)
> +      this.props.setValue(value);
> +    }
> +
> +    // Do infinite scroll when items length is bigger or equal to viewport
> +    // and isInfiniteScroll is not false.
> +    if (items.length >= viewportSize && isInfiniteScroll != false) {

Another place where was could use:

```JavaScript
if (items.length >= viewportSize && isInfiniteScroll) {
 //... 
}
```
?

::: toolkit/content/widgets/spinner.js:102
(Diff revision 3)
> +    const { items, isInfiniteScroll } = this.state;
> +
> +    // Prepends null elements so the selected value is centered in scroller
> +    let itemsView = new Array(viewportTopOffset).fill({}).concat(items);
> +
> +    if (items.length >= viewportSize && isInfiniteScroll != false) {

What are the possible values for isInfiniteScroll? Can we just use coercsion, and do:

```JavaScript
if (items.length >= viewportSize && isInfiniteScroll) {
  // ...
}
```

::: toolkit/content/widgets/spinner.js:106
(Diff revision 3)
> +
> +    if (items.length >= viewportSize && isInfiniteScroll != false) {
> +      // Calculate how many sets of items to create to fill 5 viewports up and down.
> +      // If items is greater than 5 viewports, create 2 sets only, if not, create as
> +      // many sets as needed.
> +      let count = Math.ceil(viewportSize * 5 / items.length) * 2;

More magic numbers in here to extract.

::: toolkit/content/widgets/spinner.js:108
(Diff revision 3)
> +      // Calculate how many sets of items to create to fill 5 viewports up and down.
> +      // If items is greater than 5 viewports, create 2 sets only, if not, create as
> +      // many sets as needed.
> +      let count = Math.ceil(viewportSize * 5 / items.length) * 2;
> +      for (let i = 0; i < count; i += 1) {
> +        Array.prototype.push.apply(itemsView, items);

Is this usage of .apply not equivalent to itemsView.push(...items) ? Or is there something else clever going on here?

Also... I'm just not understanding what's happening here. We created an Array on line 100 that might have some {} items at the top, followed by our items... and now for count times, we're putting more copies of items into itemsView? Why?

::: toolkit/content/widgets/spinner.js:129
(Diff revision 3)
> +
> +    if (diff > 0) {
> +      let count = diff;
> +      let frag = document.createDocumentFragment();
> +
> +      while (count) {

Why a while loop for this? Wouldn't a for-loop do?

::: toolkit/content/widgets/spinner.js:130
(Diff revision 3)
> +    if (diff > 0) {
> +      let count = diff;
> +      let frag = document.createDocumentFragment();
> +
> +      while (count) {
> +        let el = document.createElement('div');

Nit - using single quotes here, but double elsewhere.

::: toolkit/content/widgets/spinner.js:139
(Diff revision 3)
> +      while (count) {
> +        parent.removeChild(parent.lastChild);
> +        count += 1;
> +      }
> +      this.elements.itemsViewElements.splice(diff);

If this list is likely to change in length many times over the lifetime of the panel, should we have a list of nodes that we can recycle instead of creating new ones each time? Or am I prematurely optimizing?

::: toolkit/content/widgets/spinner.js:146
(Diff revision 3)
> +        count += 1;
> +      }
> +      this.elements.itemsViewElements.splice(diff);
> +    }
> +  },
> +  _setDisplayString(items, elements) {

This method is doing more than setting the string - it's setting a class. We should mention that.

::: toolkit/content/widgets/spinner.js:152
(Diff revision 3)
> +    const { getDisplayString } = this.props;
> +
> +    items.forEach((item, index) => {
> +      elements[index].textContent =
> +        item.value != undefined ? getDisplayString(item.value) : "";
> +      elements[index].className = item.valid ? "" : "invalid";

What does it mean to be valid or invalid? Should it be enabled and disabled instead?

::: toolkit/content/widgets/spinner.js:155
(Diff revision 3)
> +      elements[index].textContent =
> +        item.value != undefined ? getDisplayString(item.value) : "";
> +      elements[index].className = item.valid ? "" : "invalid";
> +    });
> +  },
> +  _attachEventListners() {

Typo, should be: `_attachEventListeners`

::: toolkit/content/widgets/spinner.js:170
(Diff revision 3)
> +    const { mouseState = {}, index } = this.state;
> +    const { viewportTopOffset } = this.props;
> +    const { spinner, prev, next } = this.elements;
> +
> +    switch (event.type) {
> +      case "scroll":

It's generally a good idea to wrap each case in a scope in case you end up using some common variable names and use `let`.

::: toolkit/content/widgets/spinner.js:212
(Diff revision 3)
> +        break;
> +      case "mouseleave":
> +        if (event.target == spinner) {
> +          // Stop listening to drag event if mouse is out of the spinner
> +          this._smoothScrollToIndex(this._getIndexFromScrollTop(spinner.scrollTop) - viewportTopOffset);
> +          spinner.removeEventListener("mousemove", this, { passive: true });

We should probably remove the mouseleave one too, right?

::: toolkit/content/widgets/spinner.js:263
(Diff revision 3)
> +      }
> +    });
> +
> +    return closestIndex - viewportTopOffset;
> +  },
> +  _scrollTo(focus, centering) {

I don't think "focus" is the right name for this argument. Perhaps "valueToFocus" or just "value".

::: toolkit/content/widgets/spinner.js:267
(Diff revision 3)
> +  },
> +  _scrollTo(focus, centering) {
> +    this.state.index = this._getScrollIndex(focus, centering);
> +    this.elements.spinner.scrollTop = this.state.index * this.props.itemHeight;
> +  },
> +  _smoothScrollTo(focus) {

Here's another place where I think "focus" is the wrong name.

::: toolkit/content/widgets/spinner.js:300
(Diff revision 3)
> +        this.elements.selected.classList.add("selection");
> +      }
> +    }
> +  },
> +  _isArrayDiff(a, b) {
> +    if (a.length != b.length) return true;

We tend to put even one-liners like this on several lines. Also, you have a convention established in this file that one-liners get braced, so let's do it like:

```JavaScript
if (a.length != b.length) {
  return true;
}
```

::: toolkit/content/widgets/spinner.js:302
(Diff revision 3)
> +    }
> +  },
> +  _isArrayDiff(a, b) {
> +    if (a.length != b.length) return true;
> +
> +    for (let i = 0; i < a.length; i += 1) {

I think `++i` or `i++` is more common as the incrementer.

::: toolkit/content/widgets/spinner.js:302
(Diff revision 3)
> +    for (let i = 0; i < a.length; i += 1) {
> +      for (let prop in a[i]) {

Nested for-loops, eh? Is there an upper bound on a and b length and property count?

::: toolkit/content/widgets/spinner.js:304
(Diff revision 3)
> +  _isArrayDiff(a, b) {
> +    if (a.length != b.length) return true;
> +
> +    for (let i = 0; i < a.length; i += 1) {
> +      for (let prop in a[i]) {
> +        if (a[i][prop] != b[i][prop]) return true;

Here's another one-liner that should be broken out over two lines and braced.

::: toolkit/content/widgets/timekeeper.js:43
(Diff revision 3)
> +  },
> +  get millisecond() {
> +    return this.state.time.getUTCMilliseconds();
> +  },
> +  get ampm() {
> +    return this.state.time.getUTCHours() < 12 ? 0 : 1;

Should this be true or false?

::: toolkit/content/widgets/timekeeper.js:72
(Diff revision 3)
> +    this.state.isInvalid = this.state.isOutOfRange || this.state.isOffStep;
> +
> +    this._setRanges();
> +  },
> +  setAmpm(ampm) {
> +    if (ampm == this.ampm) return;

Please split out and brace

::: toolkit/content/widgets/timekeeper.js:74
(Diff revision 3)
> +    if (ampm == 0) {
> +      this.setState({ hour: this.hour - 12 });
> +    } else {
> +      this.setState({ hour: this.hour + 12 });
> +    }

Again, should this be using true / false instead of 1 / 0? I know they're equivalent after coersion, but might as well be explicit for clarity.

::: toolkit/content/widgets/timekeeper.js:103
(Diff revision 3)
> +      minutes: this._getMinutes(hour),
> +      seconds: this._getSeconds(hour, minute),
> +      millisecond: this._getMilliseconds(hour, minute, second)
> +    };
> +  },
> +  _getAmpm() {

This all needs documentation - particularly, I find it confusing that `_getAmpm` is distinct from `get ampm()` above.

::: toolkit/content/widgets/timekeeper.js:104
(Diff revision 3)
> +      seconds: this._getSeconds(hour, minute),
> +      millisecond: this._getMilliseconds(hour, minute, second)
> +    };
> +  },
> +  _getAmpm() {
> +    if (this.props.format == "24") return [];

Split and brace.

	Scott Wu [:scottwu] Assignee
	Comment 38 • 8 years ago
mozreview-review-reply

Comment on attachment 8782844 [details]
Bug 1283384 - (1/2) Rename form validation anchor to make it more generic

https://reviewboard.mozilla.org/r/72878/#review75524

That's totally fine :) Thank you so much for taking the time to go over this with me. I'm working on addressing the issues and will make some comments on the new patch.

> It's a bit strange to have the whole thing called the spinner, and for there to be a subelement called spinner. Is there a better name for either of these?

Ok I think I'll just name is spinnerElement instead

> Can you add some documentation about this calculation? Why ` - 1` for example?

I'll put this in the comment:

Assume that the viewportSize is an odd number. Calculate how many items we need to insert on top of the spinner so that the selected is at the center. Ex: if viewportSize is 5, we need 2 items on top.

> What are the possible values for isInfiniteScroll? Can we just use coercsion, and do:
> 
> ```JavaScript
> if (items.length >= viewportSize && isInfiniteScroll) {
>   // ...
> }
> ```

You are right. It's just a boolean value. Originally the value was optional, but I've changed that.

> Is this usage of .apply not equivalent to itemsView.push(...items) ? Or is there something else clever going on here?
> 
> Also... I'm just not understanding what's happening here. We created an Array on line 100 that might have some {} items at the top, followed by our items... and now for count times, we're putting more copies of items into itemsView? Why?

Yes it didn't occur to me to use the spread syntax. Nothing clever here!

Line 100 is a standard list of items, but when infinite scroll is enabled, we need at least 2 more sets of items to act as buffer: one for the top and one for the bottom. But if the number of items is small, we need more sets. I'll revise the comment to make it clearer.

	Mike Conley (:mconley) (:⚙️)
	Comment 39 • 8 years ago
mozreview-review

Comment on attachment 8782844 [details]
Bug 1283384 - (1/2) Rename form validation anchor to make it more generic

https://reviewboard.mozilla.org/r/72878/#review76656

Okay, I think I've completed my first pass of this commit.

::: toolkit/content/widgets/timekeeper.js:69
(Diff revision 3)
> +
> +    this.state.isOffStep = this._isOffStep(this.state.time);
> +    this.state.isOutOfRange = (this.state.time < min || this.state.time > max);
> +    this.state.isInvalid = this.state.isOutOfRange || this.state.isOffStep;
> +
> +    this._setRanges();

So any time the time is changed, we have to re-calculate the steps? That doesn't seem right... why do we need to re-calculate the steps anytime outside initialization?

::: toolkit/content/widgets/timekeeper.js:185
(Diff revision 3)
> +  },
> +  stepAmpmBy(offset) {
> +    const current = this.ampm;
> +    const ampm = this._step(current, offset, this.state.ranges.ampm);
> +
> +    if (current == ampm) return current;

Splice and brace. I won't mention this any more for the one-liners, but assume it applies to the rest of them in this commit.

::: toolkit/content/widgets/timekeeper.js:187
(Diff revision 3)
> +    if (this.hour < 12) {
> +      this.setState({ hour: this.hour + 12 });
> +    } else {
> +      this.setState({ hour: this.hour - 12 });

More magic numbers to pull out

::: toolkit/content/widgets/timepicker.js:33
(Diff revision 3)
> +      min: this._parseTimeString(min) || new Date(0),
> +      max: this._parseTimeString(max) || new Date(24*60*60*1000 - 1),
> +      stepInMs: step ? step * 1000 : 60 * 1000,

More magic numbers and math to pull out. Also, spaces between *.

	Zibi Braniecki [:zbraniecki][:gandalf]
	Comment 40 • 8 years ago
mozreview-review

Comment on attachment 8782844 [details]
Bug 1283384 - (1/2) Rename form validation anchor to make it more generic

https://reviewboard.mozilla.org/r/72878/#review77008

::: toolkit/content/widgets/timepicker.js:48
(Diff revision 3)
> +  _parseTimeString(timeString) {
> +    let time = new Date("1970-01-01T" + timeString + "Z");
> +    return time.toString() == "Invalid Date" ? false : time;
> +  },
> +  _createComponents() {
> +    const { locale, step, format } = this.props;

Where is the locale being set?

::: toolkit/content/widgets/timepicker.js:67
(Diff revision 3)
> +          timeKeeper.setHour(value);
> +          this.state.isHourSet = true;
> +        }),
> +        getDisplayString: hour => {
> +          if (format == "24") {
> +            return new Intl.NumberFormat(locale).format(hour);

Generally speaking, if you're not caching the formatter, there's no reason to use Intl.NumberFormat, Number.toLocaleString is enough.

In this case, since you only use it once per code branch, you can do:

`
return hour.toLocaleString(locale);
`

::: toolkit/content/widgets/timepicker.js:88
(Diff revision 3)
> +    };
> +
> +    // The AM/PM spinner is only available in 12hr mode
> +    // TODO: Replace AM & PM string with localized string
> +    if (format == "12") {
> +      this.components.ampm = new Spinner({

I recommend renaming all "ampm" to "dayperiod". It's the more accurate term and also, not all countries use ampm for dayperiods (which is something we may want/have to tackle in the future).

	Scott Wu [:scottwu] Assignee
	Comment 42 • 8 years ago
mozreview-review-reply

Comment on attachment 8782844 [details]
Bug 1283384 - (1/2) Rename form validation anchor to make it more generic

https://reviewboard.mozilla.org/r/72878/#review76656

> So any time the time is changed, we have to re-calculate the steps? That doesn't seem right... why do we need to re-calculate the steps anytime outside initialization?

I want to make TimeKeeper so that it could be reused by input boxes in the future. It's possible to calculate all the steps for time picker because we only need to display hour and minutes, so the maximum number of steps is only 24 * 60 = 1440. The step number become much larger if we need to take it to milliseconds: 24 * 60 * 60 * 1000 = 86400000.

So instead of doing the calculation on initialization, I only calculate the steps for the time range being displayed. For example, at 5:30 AM, I'll need the hours between 0:00 to 11:59, and minutes between 5:00 to 5:59.

	Scott Wu [:scottwu] Assignee
	Comment 43 • 8 years ago
mozreview-review-reply

Comment on attachment 8782844 [details]
Bug 1283384 - (1/2) Rename form validation anchor to make it more generic

https://reviewboard.mozilla.org/r/72878/#review77008

> Where is the locale being set?

Right now it's not being set yet. Do you know what's the best way to get the user preferred locale? Normally I would use navigator.language in web pages, but I don't see it being used much in dxr.

> Generally speaking, if you're not caching the formatter, there's no reason to use Intl.NumberFormat, Number.toLocaleString is enough.
> 
> In this case, since you only use it once per code branch, you can do:
> 
> `
> return hour.toLocaleString(locale);
> `

OK! I wasn't aware of this method. I'll use this instead.

> I recommend renaming all "ampm" to "dayperiod". It's the more accurate term and also, not all countries use ampm for dayperiods (which is something we may want/have to tackle in the future).

That's cool. I was struggling with coming up with a good name for that. I'll change them to dayperiod.

	Scott Wu [:scottwu] Assignee
	Comment 44 • 8 years ago
mozreview-review-reply

Comment on attachment 8782844 [details]
Bug 1283384 - (1/2) Rename form validation anchor to make it more generic

https://reviewboard.mozilla.org/r/72878/#review75524

> Why a while loop for this? Wouldn't a for-loop do?

Yes a for-loop would do. Is while loop not preferred?

> If this list is likely to change in length many times over the lifetime of the panel, should we have a list of nodes that we can recycle instead of creating new ones each time? Or am I prematurely optimizing?

The list of nodes are only added/removed when the item size changes, which happens rarely if at all under the current UX spec (with disabled items greyed out rather than hidden). If the item size has not changed, the same nodes are reused.

> What does it mean to be valid or invalid? Should it be enabled and disabled instead?

Yeah enabled/disabled is better. Valid/invalid was intended to describe time, but the spinner should be more generic.

> Nested for-loops, eh? Is there an upper bound on a and b length and property count?

There's no hard length upper bound for a/b, but for the time picker the max is 60, and property count is now 3. For the date picker, the length of year spinner will be limited to less than 200.

> Should this be true or false?

Since the other time units are all numeric, I think it's more consistent to have am/pm represented as numbers as well, with 0 as AM and 1 as PM.

	Wesley Huang [:wesley_huang] (EPM) (NI me)
	Comment 45 • 8 years ago

A gentle reminder as comment 44 are likely for Mike to feedback.

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

(In reply to Wesley Huang [:wesley_huang] (EPM) (NI me) from comment #45)
> A gentle reminder as comment 44 are likely for Mike to feedback.

My apologies! I definitely recommend using the needinfo flag in the future to help me make sure these don't slip off my radar.

Answering now...

	Mike Conley (:mconley) (:⚙️)
	Comment 47 • 8 years ago
mozreview-review-reply

Comment on attachment 8782844 [details]
Bug 1283384 - (1/2) Rename form validation anchor to make it more generic

https://reviewboard.mozilla.org/r/72878/#review75524

> Yes it didn't occur to me to use the spread syntax. Nothing clever here!
> 
> Line 100 is a standard list of items, but when infinite scroll is enabled, we need at least 2 more sets of items to act as buffer: one for the top and one for the bottom. But if the number of items is small, we need more sets. I'll revise the comment to make it clearer.

Ah, okay - I understand. Thanks for explaining. :)

> Yes a for-loop would do. Is while loop not preferred?

While loops are certainly more rare in the Firefox front-end codebase, in my experience. For counting, we generally use for-loops. It's not a big deal, just something I noticed.

> The list of nodes are only added/removed when the item size changes, which happens rarely if at all under the current UX spec (with disabled items greyed out rather than hidden). If the item size has not changed, the same nodes are reused.

Alright, thanks for explaining.

> There's no hard length upper bound for a/b, but for the time picker the max is 60, and property count is now 3. For the date picker, the length of year spinner will be limited to less than 200.

Alright, I guess it's not too much of a worry then at this point. Maybe a good idea to document, however.

> Since the other time units are all numeric, I think it's more consistent to have am/pm represented as numbers as well, with 0 as AM and 1 as PM.

Thinking back, `ampm` being a boolean doesn't make much sense either, since the value doesn't indicate whether or not we're in the AM or PM case. That's true for the numeric representation as well.

Perhaps it'd be clearer if, when switching to dayPeriod, we have the two values 12 and 24 instead?

	Mike Conley (:mconley) (:⚙️)
	Comment 48 • 8 years ago
mozreview-review-reply

Comment on attachment 8782844 [details]
Bug 1283384 - (1/2) Rename form validation anchor to make it more generic

https://reviewboard.mozilla.org/r/72878/#review76656

> I want to make TimeKeeper so that it could be reused by input boxes in the future. It's possible to calculate all the steps for time picker because we only need to display hour and minutes, so the maximum number of steps is only 24 * 60 = 1440. The step number become much larger if we need to take it to milliseconds: 24 * 60 * 60 * 1000 = 86400000.
> 
> So instead of doing the calculation on initialization, I only calculate the steps for the time range being displayed. For example, at 5:30 AM, I'll need the hours between 0:00 to 11:59, and minutes between 5:00 to 5:59.

Hm. I'll be interested in the performance characteristics for the millisecond case. That sounds potentially problematic.

	Scott Wu [:scottwu] Assignee
	Comment 50 • 8 years ago

Hi Mike,

Thank a lot for your feedback. I've addressed most of the issues in the latest patch: https://reviewboard.mozilla.org/r/72878/diff/5-6/

There's a reference to FormValidationAnchor that I made use of to anchor the popup, but I found the name might cause confusion, so I made a separate patch to rename it.
https://reviewboard.mozilla.org/r/72878/diff/4-6/

I'm not sure what I did wrong with mozreivew but the commit diff looks a bit weird.. I'll see if I can fix that.

And I have a smaller one coming up that fixes other bugs & design changes, but I got a few questions based on your comments:

1. You mentioned I should probably use postMessage for communication with iFrame. I wonder what advantage it has and drawback of using addEventListener in this context?
https://reviewboard.mozilla.org/r/72878/#comment97320

2. You suggested using message listeners for listening to changes in the iFrame. I'm not sure how that would work? I've changed my code so that the event listener is removed when picker is closed. I wonder if this would be good enough?
https://reviewboard.mozilla.org/r/72878/#comment97326

3. I've renamed ampm to dayPeriod, but I don't think 12/24 should be the values either. Since AM and PM represents 0 and 12 respectively in terms of hours, maybe 0/12 would be more fitting if not 0/1.
https://reviewboard.mozilla.org/r/72878/#comment97880

Thanks again :)

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

Sorted this out with scottwu over Vidyo, but recording here for posterity:

(In reply to Scott Wu [:scottwu] from comment #50)
> 
> 1. You mentioned I should probably use postMessage for communication with
> iFrame. I wonder what advantage it has and drawback of using
> addEventListener in this context?
> https://reviewboard.mozilla.org/r/72878/#comment97320

Here's what I was commenting on:

```JavaScript
this.dateTimePopupFrame.contentDocument.dispatchEvent(new CustomEvent("TimePickerSetValue", {
```

On this line, the patch is accessing the contentDocument of the <iframe> and dispatching a custom event. Since e10s came along, there's been a tendency to avoid accessing contentDocument or contentWindow directly, and that if communication with the content is required, to use messages instead.

I'm aware that the iframe is not running out of process, but it's probably best to be consistent where possible, and to avoid confusion in the future, where someone might look at the datetimepopup.xml file, and assume that accessing contentDocument and contentWindow is advisable in general.

I know that in https://developer.mozilla.org/en-US/docs/Web/API/Window/postMessage, the "Using window.postMessage in extensions" section mentions the hazard of communicating with chrome:// pages using postMessage if the iframe can move to web URLs, but since the datetimepicker iframe is staying at our own internal URL, we might be okay here.

We might want security to double-check and bang the tires before exposing this to our release channel.

> 
> 2. You suggested using message listeners for listening to changes in the
> iFrame. I'm not sure how that would work? I've changed my code so that the
> event listener is removed when picker is closed. I wonder if this would be
> good enough?
> https://reviewboard.mozilla.org/r/72878/#comment97326

I guess I was just referring to using the "message" event listener to listen for messages sent back up from the iframe.

> 
> 3. I've renamed ampm to dayPeriod, but I don't think 12/24 should be the
> values either. Since AM and PM represents 0 and 12 respectively in terms of
> hours, maybe 0/12 would be more fitting if not 0/1.
> https://reviewboard.mozilla.org/r/72878/#comment97880

Sure, 0 and 12 makes sense to me, thanks.

> 
> Thanks again :)

No problem - thanks for your work!

	Scott Wu [:scottwu] Assignee
	Comment 54 • 8 years ago
mozreview-review

Comment on attachment 8796500 [details]
Bug 1283384 - Implement time picker UI w/ message passing,

https://reviewboard.mozilla.org/r/82346/#review80938

::: toolkit/content/widgets/datetimepopup.xml:46
(Diff revisions 1 - 2)
>            this.hidden = true;
>            this.setInputBoxValue(true);
>            this.pickerState = {};
> +          this.type = undefined;
>            this.dateTimePopupFrame.removeEventListener("load", this, true);
> +          this.dateTimePopupFrame.contentDocument.removeEventListener("TimePickerPopupChanged", this, false);

Remove the event listener when picker is closed.

::: toolkit/content/widgets/datetimepopup.xml:56
(Diff revisions 1 - 2)
>          <parameter name="data"/>
>          <body><![CDATA[
>            switch (this.type) {
> -            case "time":
> +            case "time": {
>                const { hour, minute } = data.value ? JSON.parse(data.value) : {};
> -              this.dateTimePopupFrame.contentDocument.dispatchEvent(new CustomEvent("TimePickerSetValue", {
> +              this.dateTimePopupFrame.contentWindow.postMessage({

Is this the way you would use postMessage?

Since the listener in the iFrame listens to "message", I need to come up with an event name to distinguish which event is it. Right now I use { name, detail } to look more like customEvent. I see a few examples of { msg, data }, but it seems everyone uses it slightly differently.

::: toolkit/content/widgets/datetimepopup.xml:126
(Diff revisions 1 - 2)
>          <parameter name="aEvent"/>
>          <body><![CDATA[
>            switch (aEvent.type) {
> -            case "load":
> +            case "load": {
>                this.initPicker(this.detail);
> -              this.dateTimePopupFrame.contentDocument.addEventListener("TimePickerPopupChanged", (evt) => {
> +              this.dateTimePopupFrame.contentDocument.addEventListener("TimePickerPopupChanged", this, false);

This listens to changes in the iframe. Is it okay to use contentDocument.addEventListener here?

If I were to use postMessage, I would need to do window.addEventListener("message", ...), and in the iframe use window.parent.postMessage. But window is browser.xul here, and attaching a message listener there seems like a bad idea.

::: toolkit/content/widgets/timepicker.js:174
(Diff revisions 1 - 2)
> -  _dispatchState() {
> +    _dispatchState() {
> -    const { hour, minute } = this.state.timeKeeper;
> +      const { hour, minute } = this.state.timeKeeper;
> -    const { isHourSet, isMinuteSet, isAmpmSet } = this.state;
> +      const { isHourSet, isMinuteSet, isDayPeriodSet } = this.state;
> -    // Dispatch a CustomEvent for the panel, and it will be passed
> +      // Dispatch a CustomEvent for the panel, and it will be passed
> -    // to input boxes
> +      // to input boxes
> -    document.dispatchEvent(new CustomEvent("TimePickerPopupChanged", {
> +      document.dispatchEvent(new CustomEvent("TimePickerPopupChanged", {

Send state changes from iframe to datetimepopup.

::: toolkit/content/widgets/timepicker.js:179
(Diff revisions 1 - 2)
> -    document.dispatchEvent(new CustomEvent("TimePickerPopupChanged", {
> +      document.dispatchEvent(new CustomEvent("TimePickerPopupChanged", {
> -      detail: { hour, minute, isHourSet, isMinuteSet, isAmpmSet }
> +        detail: { hour, minute, isHourSet, isMinuteSet, isDayPeriodSet }
> -    }));
> +      }));
> -  },
> +    },
> -  _attachEventListeners() {
> +    _attachEventListeners() {
> -    document.addEventListener("TimePickerSetValue", this, false);
> +      window.addEventListener('message', this);

Listens to postMessage from datetimepopup.xml

::: toolkit/content/widgets/timepicker.js:187
(Diff revisions 1 - 2)
> +     * Handle events.
> +     *
> +     * @param  {Event} event
> +     */
> -  handleEvent(event) {
> +    handleEvent(event) {
> -    switch (event.type) {
> +      if (event.type == "message") {

Listens to the "message" event, and use "name" to find its handler.

::: toolkit/modules/DateTimePickerHelper.jsm:93
(Diff revisions 1 - 2)
>      let rect = aData.rect;
>      let dir = aData.dir;
>      let type = aData.type;
>      let detail = aData.detail;
>  
> +    this._anchor = aBrowser.ownerGlobal.gBrowser.formValidationAnchor;

Bug 1306251 will rename formValidationAnchor to popupAnchor.

	Scott Wu [:scottwu] Assignee
	Comment 55 • 8 years ago

Hello Mike,

In a failed attempt to remove irrelevant mozreview requests, I mistakenly closed the review we were working on.. and I couldn't find a way to undo it. So I've started a new mozreview thread with the first patch being the one you looked at, and the 2nd one being the most recent version so that you can still see the diffs. Unfortunately, the issues you commented on is still on the old thread, but I'll add comments on the issues we discussed about so you can find them more easily.

Here's the diff: https://reviewboard.mozilla.org/r/82346/diff/1-2/
The thread I accidentally closed: https://reviewboard.mozilla.org/r/72878/

What's in this patch:
- Fixed style issues, small mistakes, nits
- Added comments for each method
- Moved xhtml to /toolkit/content, css to /toolkit/themes/shared
- Pulled out magic numbers to const to the top of each files
- Use postMessage to send data to the iframe
- Changed unit to em and rem, will add zooming capability in a follow up bug

My biggest question is on the use of postMessage, and I added comments on mozreview. Please let me know what you think and if it makes sense. I'm going working on browser tests, and hope to have something to show you next week. Thank you so much!

	Scott Wu [:scottwu] Assignee
	Comment 56 • 8 years ago

Still working on browser tests. Initially I had trouble telling EventUtils to click on elements I want, but I got past that. The tests will check initial states, click on up/down arrows, scroll on spinners, and click & drag on spinner items.

If I don't face more issues with dragging, I should be able to have the test ready in a day or two.

	Mike Conley (:mconley) (:⚙️)
	Comment 57 • 8 years ago
mozreview-review

Comment on attachment 8796500 [details]
Bug 1283384 - Implement time picker UI w/ message passing,

https://reviewboard.mozilla.org/r/82346/#review82302

Hey scottwu,

Sorry again that this took so long to review. It's a big patch!

I think it looks good. I think it's in need of a rebase after your popup anchor rename. I've also got some minor corrections and suggestions below.

I don't think I'll be able to find anything in another round of review. So let's land this thing and see how it behaves.

Thanks for your work here.

::: browser/base/content/browser.css:528
(Diff revision 2)
>  
>  #PopupAutoCompleteRichResult.showSearchSuggestionsNotification > richlistbox {
>    transition: none;
>  }
>  
> +#DateTimePickerPanel[type="arrow"] {

Not sure you need the type attribute selector here, since we'll want to use this panel regardless of whether or not it's an arrow panel, right?

::: browser/base/content/browser.xul:164
(Diff revision 2)
>             hidden="true"
> +           orient="vertical"
>             noautofocus="true"
>             consumeoutsideclicks="false"
> -           level="parent">
> +           level="parent"
> +           animate="false">

Why animate=false, out of curiosity? Are you sure UX doesn't want this to animate open and closed?

::: browser/base/content/browser.xul:165
(Diff revision 2)
> +           orient="vertical"
>             noautofocus="true"
>             consumeoutsideclicks="false"
> -           level="parent">
> +           level="parent"
> +           animate="false">
> +      <iframe id="dateTimePopupFrame" anonid="dateTimePopupFrame"/>

I don't think you need this anonid.

::: toolkit/content/timepicker.xhtml:25
(Diff revision 2)
> +        <div class="spinner"></div>
> +      </div>
> +      <button class="next">▼</button>
> +    </div>
> +  </template>
> +  <script type="application/javascript"><![CDATA[

I'm not sure you need the <![CDATA[ stuff here. We definitely use it in XBL, but I've not seen it elsewhere.

::: toolkit/content/widgets/datetimepopup.xml:15
(Diff revision 2)
> +      <field name="dateTimePopupFrame">
> +        document.getElementById("dateTimePopupFrame");
> +      </field>

Can we use this.querySelector instead? That way, we're making it clear that our assumption is that the dateTimePopupFrame exists inside the datetime-popup.

::: toolkit/content/widgets/datetimepopup.xml:75
(Diff revision 2)
> +                detail: { hour, minute, format,
> +                  min: detail.min, max: detail.max, step: detail.step
> +                }

Nit: can you please format like:

```JavaScript
detail: {
  hour,
  minute,
  format,
  min: detail.min,
  max: detail.max,
  step: detail.step,
},
```

::: toolkit/content/widgets/datetimepopup.xml:85
(Diff revision 2)
> +            }
> +          }
> +        ]]></body>
> +      </method>
> +      <method name="setInputBoxValue">
> +        <parameter name="passAll"/>

Mind documenting what passAll means exactly? The difference between the sendPickerValueChanged calls is kinda subtle.

::: toolkit/content/widgets/datetimepopup.xml:126
(Diff revision 2)
> +        <parameter name="aEvent"/>
> +        <body><![CDATA[
> +          switch (aEvent.type) {
> +            case "load": {
> +              this.initPicker(this.detail);
> +              this.dateTimePopupFrame.contentDocument.addEventListener("TimePickerPopupChanged", this, false);

We should be listening for messages from the iframe here. We should also double-check that the contentDocument has a system principal. This is mostly paranoia, but helps ensure that if somehow the panel gets tricked into navigating to another site, that we don't listen to messages from it anymore.

::: toolkit/content/widgets/spinner.js:56
(Diff revision 2)
> +        items: [],
> +        isScrolling: false
> +      };
> +      this.props = {
> +        setValue, getDisplayString, itemHeight, viewportSize,
> +        // Assume that the viewportSize is an odd number. Calculate how many items

Should we ensure that the viewportSize is odd? That seems probably reasonable given that we want to show something "selected". We might want to mention that in the viewportSize documentation, and perhaps round down or up to the nearest odd number.

::: toolkit/content/widgets/spinner.js:71
(Diff revision 2)
> +      };
> +
> +      this.context.appendChild(spinnerElement);
> +      this._attachEventListeners();
> +    },
> +    /**

Nit: Please put newlines before these comment blocks for easier readability.

::: toolkit/content/widgets/spinner.js:73
(Diff revision 2)
> +      this.context.appendChild(spinnerElement);
> +      this._attachEventListeners();
> +    },
> +    /**
> +     * Only the parent component calls setState on the spinner.
> +     * It checks if the items have changed and updates the DOM.

To be clear, "DOM" maybe should be "the spinner", so we don't confuse this with the DOM that hosts the input field.

::: toolkit/content/widgets/spinner.js:109
(Diff revision 2)
> +        }
> +      }
> +    },
> +    /**
> +     * Whenever scroll event is detected:
> +     * - Updated the index state

"Update the index state"

::: toolkit/content/widgets/spinner.js:110
(Diff revision 2)
> +      }
> +    },
> +    /**
> +     * Whenever scroll event is detected:
> +     * - Updated the index state
> +     * - If smooth scroll has reached its destination, and set [isScrolling]

"If a smooth scroll has reached its destination, set [isScrolling] state to false"

::: toolkit/content/widgets/spinner.js:235
(Diff revision 2)
> +      prev.addEventListener("mousedown", this);
> +      next.addEventListener("mousedown", this);

Alternatively, put a single mousedown event listener on the spinner element, instead of having three of them.

::: toolkit/content/widgets/spinner.js:308
(Diff revision 2)
> +          }
> +          break;
> +        }
> +        case "mousemove": {
> +          // Change spinner position on drag
> +          event.target.setCapture();

Should we not be adding this in the mousedown handler instead? See https://developer.mozilla.org/en-US/docs/Web/API/Element/setCapture

::: toolkit/content/widgets/spinner.js:327
(Diff revision 2)
> +    /**
> +     * Find the index of a value that is the closest to the current position.
> +     * If centering is true, find the index closest to the center.
> +     *
> +     * @param {Number/String} value: The value to find
> +     * @param {Boolean} centering: Whether or not to find the value closest to center

Please document the return value.

::: toolkit/content/widgets/spinner.js:438
(Diff revision 2)
> +     * Compares arrays of objects. It assumes the structure is an array of
> +     * objects, and objects in a and b have the same number of properties.
> +     *
> +     * @param  {Array<Object>} a
> +     * @param  {Array<Object>} b
> +     * @return {Boolean}  Returns true if a and b is different

"is different" -> "are different"

::: toolkit/content/widgets/timekeeper.js:18
(Diff revision 2)
> + * @param {Object} props
> + *        {
> + *          {Date} min
> + *          {Date} max
> + *          {Number} stepInMs
> + *          {String} format

Can you show examples for this one? Like:

{String} format, either "24" or "12".

::: toolkit/content/widgets/timekeeper.js:34
(Diff revision 2)
> +  const DAY_PERIOD_IN_HOURS = 12,
> +        SECOND_IN_MS = 1000,
> +        MINUTE_IN_MS = 60000,
> +        HOUR_IN_MS = 3600000,
> +        DAY_PERIOD_IN_MS = 43200000,
> +        DAY_IN_MS = 86400000;

Should probably have consts for "24" and "12" as well.

::: toolkit/content/widgets/timekeeper.js:57
(Diff revision 2)
> +    },
> +    get dayPeriod() {
> +      // 0 stands for AM and 12 for PM
> +      return this.state.time.getUTCHours() < DAY_PERIOD_IN_HOURS ? 0 : 12;
> +    },
> +    /**

Newlines before these big comment blocks please.

::: toolkit/content/widgets/timepicker.js:144
(Diff revision 2)
> +      this.components.hour.setState({
> +        value: setToMinValue ? timeKeeper.ranges.hours[0].value : timeKeeper.hour,
> +        items: timeKeeper.ranges.hours,
> +        isInfiniteScroll: true,
> +        isValueSet: isHourSet,
> +        isInvalid: isInvalid

Can do just:

```
isInvalid,
```

Since the property name and variable name match.

::: toolkit/content/widgets/timepicker.js:152
(Diff revision 2)
> +      this.components.minute.setState({
> +        value: setToMinValue ? timeKeeper.ranges.minutes[0].value : timeKeeper.minute,
> +        items: timeKeeper.ranges.minutes,
> +        isInfiniteScroll: true,
> +        isValueSet: isMinuteSet,
> +        isInvalid: isInvalid

Same as above, re: isInvalid.

::: toolkit/content/widgets/timepicker.js:162
(Diff revision 2)
> +        this.components.dayPeriod.setState({
> +          value: setToMinValue ? timeKeeper.ranges.dayPeriod[0].value : timeKeeper.dayPeriod,
> +          items: timeKeeper.ranges.dayPeriod,
> +          isInfiniteScroll: false,
> +          isValueSet: isDayPeriodSet,
> +          isInvalid: isInvalid

Same as above, re: isInvalid.

::: toolkit/content/widgets/timepicker.js:169
(Diff revision 2)
> +    _dispatchState() {
> +      const { hour, minute } = this.state.timeKeeper;
> +      const { isHourSet, isMinuteSet, isDayPeriodSet } = this.state;
> +      // Dispatch a CustomEvent for the panel, and it will be passed
> +      // to input boxes
> +      document.dispatchEvent(new CustomEvent("TimePickerPopupChanged", {
> +        detail: { hour, minute, isHourSet, isMinuteSet, isDayPeriodSet }
> +      }));
> +    },

This is another place where we could use postMessage to talk to the binding.

I believe you can do:

postMessage({
  name: "TimePickerPopupChanged",
  detail: {
  },
, "*");

::: toolkit/modules/DateTimePickerHelper.jsm:98
(Diff revision 2)
> +    this._anchor = aBrowser.ownerGlobal.gBrowser.formValidationAnchor;
> +    this._anchor.left = rect.left;
> +    this._anchor.top = rect.top;
> +    this._anchor.width = rect.width;
> +    this._anchor.height = rect.height;
> +    this._anchor.hidden = false;

Why do we need to unhide the anchor here?

::: toolkit/modules/DateTimePickerHelper.jsm:104
(Diff revision 2)
> +
>      debug("Opening picker with details: " + JSON.stringify(detail));
>      this.weakBrowser = Cu.getWeakReference(aBrowser);
>      this.picker = aBrowser.dateTimePicker;
> -    this.picker.hidden = false;
> -    this.picker.openPopupAtScreenRect("after_start", rect.left, rect.top,
> +    this.picker.loadPicker(type, detail);
> +    this.picker.openPopup(this._anchor, "after_start", rect.left, rect.top);

Can you add some documentation here explaining that we need the anchor in order for the arrow to be properly lined up (context in bug 691601).

	Zibi Braniecki [:zbraniecki][:gandalf]
	Comment 58 • 8 years ago
mozreview-review

Comment on attachment 8796500 [details]
Bug 1283384 - Implement time picker UI w/ message passing,

https://reviewboard.mozilla.org/r/82346/#review82990

::: toolkit/content/widgets/timepicker.js:104
(Diff revision 2)
> +            if (format == "24") {
> +              return hour.toLocaleString(locale);
> +            } else {
> +              // Hour 0 in 12 hour format is displayed as 12.
> +              const hourIn12 = hour % DAY_PERIOD_IN_HOURS;
> +              return hourIn12 == 0 ? Number(12).toLocaleString(locale)

Since you're going to do this for all values, you may benefit from creating one "Intl.NumberFormat" object instead of doing it for each value separately.

Same for minute spinner.

	Scott Wu [:scottwu] Assignee
	Comment 59 • 8 years ago
mozreview-review-reply

Comment on attachment 8796500 [details]
Bug 1283384 - Implement time picker UI w/ message passing,

https://reviewboard.mozilla.org/r/82346/#review82302

Hi Mike,

I've rebased and addressed the issues you pointed out.

Thank you so much for your review :)

> We should be listening for messages from the iframe here. We should also double-check that the contentDocument has a system principal. This is mostly paranoia, but helps ensure that if somehow the panel gets tricked into navigating to another site, that we don't listen to messages from it anymore.

Ok I've changed the listener to listen to "message", and the message handler checks if the origin is "chrome://global".

> Should we ensure that the viewportSize is odd? That seems probably reasonable given that we want to show something "selected". We might want to mention that in the viewportSize documentation, and perhaps round down or up to the nearest odd number.

Ok. I now check if the viewportSize is odd, otherwise the default size will be used just to keep things simple.

> Should probably have consts for "24" and "12" as well.

Added TIME_FORMAT_24 = "24" only because "12" is not used.

> Why do we need to unhide the anchor here?

The anchor can't be hidden or it will return (0,0) as position. I've also set .hidden = true when picker is closed.

	Scott Wu [:scottwu] Assignee
	Comment 61 • 8 years ago
mozreview-review-reply

Comment on attachment 8796500 [details]
Bug 1283384 - Implement time picker UI w/ message passing,

https://reviewboard.mozilla.org/r/82346/#review82990

Thanks Zibi! I'm also using navigator.language for locale for now as you mentioned.

> Since you're going to do this for all values, you may benefit from creating one "Intl.NumberFormat" object instead of doing it for each value separately.
> 
> Same for minute spinner.

Got it!

	Scott Wu [:scottwu] Assignee
	Comment 62 • 8 years ago

I wonder if this is how you would use postMessage in this context? Now both the panel and the iframe do postMessage to the iframe's contentWindow, and they both listen to the "message" event: https://reviewboard.mozilla.org/r/82346/diff/2-3#4

Just want to make sure it's good before I do checkin-needed.

Thanks again!

	Zibi Braniecki [:zbraniecki][:gandalf]
	Comment 63 • 8 years ago
mozreview-review

Comment on attachment 8796500 [details]
Bug 1283384 - Implement time picker UI w/ message passing,

https://reviewboard.mozilla.org/r/82346/#review83476

::: toolkit/content/widgets/timepicker.js:94
(Diff revisions 2 - 3)
>            setTimeFunction(value);
>            this._setComponentStates();
>            this._dispatchState();
>          };
>        };
> +      const numberFormat = new Intl.NumberFormat(locale).format;

Here's a catch. If you cache it here, you need to listen to `langaugeschange` event to react to language changes and recreate the formatter.

	Scott Wu [:scottwu] Assignee
	Comment 65 • 8 years ago

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #63)

> > +      const numberFormat = new Intl.NumberFormat(locale).format;
> 
> Here's a catch. If you cache it here, you need to listen to
> `langaugeschange` event to react to language changes and recreate the
> formatter.

Ah I see. I don't think it's an issue at this point, since whenever user clicks outside of the picker (ex. to change language settings), it closes. And when the picker is closed and reopened, it basically reloads and states from before is not reused. The only way for this to be an issue is when language preference is changed in the background while picker remains opened. Even if it happens, user can see the change by reopening the picker.

	Scott Wu [:scottwu] Assignee
	Comment 66 • 8 years ago

Hi Mike, wonder if you have time to take a quick look at my postMessage implementation? Just want to make sure I did it correctly. Thanks :)

> I wonder if this is how you would use postMessage in this context? Now both
> the panel and the iframe do postMessage to the iframe's contentWindow, and
> they both listen to the "message" event:
> https://reviewboard.mozilla.org/r/82346/diff/2-3#4

	Zibi Braniecki [:zbraniecki][:gandalf]
	Comment 67 • 8 years ago

(In reply to Scott Wu [:scottwu] from comment #65)
> (In reply to Zibi Braniecki [:gandalf][:zibi] from comment #63)
> 
> > > +      const numberFormat = new Intl.NumberFormat(locale).format;
> > 
> > Here's a catch. If you cache it here, you need to listen to
> > `langaugeschange` event to react to language changes and recreate the
> > formatter.
> 
> Ah I see. I don't think it's an issue at this point, since whenever user
> clicks outside of the picker (ex. to change language settings), it closes.
> And when the picker is closed and reopened, it basically reloads and states
> from before is not reused. The only way for this to be an issue is when
> language preference is changed in the background while picker remains
> opened. Even if it happens, user can see the change by reopening the picker.

Oh, that's perfect then. Great! :)

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

ni? for self, comment 66.

	Mike Conley (:mconley) (:⚙️)
	Comment 69 • 8 years ago
mozreview-review-reply

Comment on attachment 8796500 [details]
Bug 1283384 - Implement time picker UI w/ message passing,

https://reviewboard.mozilla.org/r/82346/#review80938

> Is this the way you would use postMessage?
> 
> Since the listener in the iFrame listens to "message", I need to come up with an event name to distinguish which event is it. Right now I use { name, detail } to look more like customEvent. I see a few examples of { msg, data }, but it seems everyone uses it slightly differently.

Yeah, the data that you can pass through postMessage can be anything serializable. Setting a name like this seems like a fine idea.

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

Hey scottwu. wesley_huang contacted me and said you're still missing some information here. What do you need?

	Zibi Braniecki [:zbraniecki][:gandalf]
	Comment 71 • 8 years ago
mozreview-review

Comment on attachment 8796500 [details]
Bug 1283384 - Implement time picker UI w/ message passing,

https://reviewboard.mozilla.org/r/82346/#review85942

::: toolkit/content/widgets/datetimepopup.xml:81
(Diff revision 4)
> +                  minute,
> +                  format,
> +                  min: detail.min,
> +                  max: detail.max,
> +                  step: detail.step,
> +                  locale: navigator.language

I'm sorry for adding you more work, but it seems that we can't go with navigator.languages just yet for the chrome UI.

Can you use: 

Cc["@mozilla.org/chrome/chrome-registry;1"].getService(Ci.nsIXULChromeRegistry).getSelectedLocale("global")

to retrieve the UI locale?

I'll search&replace everywhere once we have more sane preferences.

	Scott Wu [:scottwu] Assignee
	Comment 72 • 8 years ago
mozreview-review

Comment on attachment 8796500 [details]
Bug 1283384 - Implement time picker UI w/ message passing,

https://reviewboard.mozilla.org/r/82344/#review86102

::: toolkit/content/widgets/datetimepopup.xml:72
(Diff revision 4)
> +          switch (this.type) {
> +            case "time": {
> +              const { hour, minute } = detail.value;
> +              const format = detail.format || "12";
> +
> +              this.dateTimePopupFrame.contentWindow.postMessage({

I'm using postMessage on the iframe's contentWindow to send message from the panel to the iframe.

::: toolkit/content/widgets/datetimepopup.xml:134
(Diff revision 4)
> +        <parameter name="aEvent"/>
> +        <body><![CDATA[
> +          switch (aEvent.type) {
> +            case "load": {
> +              this.initPicker(this.detail);
> +              this.dateTimePopupFrame.contentWindow.addEventListener("message", this, false);

Listen to data from iframe. Instead of using window.addEventListener("message"), because window is browser.xul

::: toolkit/content/widgets/timepicker.js:180
(Diff revision 4)
> +    _dispatchState() {
> +      const { hour, minute } = this.state.timeKeeper;
> +      const { isHourSet, isMinuteSet, isDayPeriodSet } = this.state;
> +      // Dispatch a CustomEvent for the panel, and it will be passed
> +      // to input boxes
> +      window.postMessage({

This is the part I have question about: Here I need to send data from the iframe out to the panel. Normally I would find window.parent, and do postMessage on the parent. However, since the parent is actually browser.xul, sending/listening to event there seems like a bad idea. So instead I just do a postMessage on the itself. Which means both the parent and iframe listen to the iframe contentWindow, and they just filter what they need.

	Scott Wu [:scottwu] Assignee
	Comment 73 • 8 years ago

Hey Mike,

Sorry I wasn't been more clear about my question. I've written my question on mozreview as issues so it's more clear for you. I'll make sure I do that next time :)

	Scott Wu [:scottwu] Assignee
	Comment 74 • 8 years ago
mozreview-review-reply

Comment on attachment 8796500 [details]
Bug 1283384 - Implement time picker UI w/ message passing,

https://reviewboard.mozilla.org/r/82346/#review85942

> I'm sorry for adding you more work, but it seems that we can't go with navigator.languages just yet for the chrome UI.
> 
> Can you use: 
> 
> Cc["@mozilla.org/chrome/chrome-registry;1"].getService(Ci.nsIXULChromeRegistry).getSelectedLocale("global")
> 
> to retrieve the UI locale?
> 
> I'll search&replace everywhere once we have more sane preferences.

Don't be sorry, I should thank you for reading through my patch!
So getSelectedLocale("global") returns the chrome UI locale and not the user preferred language right? I remember there was a discussion about which to use but I can't find it now. Are we only using the chrome UI locale temporarily because navigator.languages is being worked on?

	Zibi Braniecki [:zbraniecki][:gandalf]
	Comment 75 • 8 years ago

> So getSelectedLocale("global") returns the chrome UI locale and not the user preferred language right?

Yes.

> I remember there was a discussion about which to use but I can't find it now. Are we only using the chrome UI locale temporarily because navigator.languages is being worked on?

navigator.languages returns the languages selected by the user to display content in.
the getSelectedLocale returns the locales we display our browser UI in.

We're still discussing whether we want to blend those two settings, but at this point we want to unify all of our UI to use the chrome UI.

	Scott Wu [:scottwu] Assignee
	Comment 76 • 8 years ago

ni for comment 72
https://bugzilla.mozilla.org/show_bug.cgi?id=1283384#c72

	Scott Wu [:scottwu] Assignee
	Comment 78 • 8 years ago

(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #75)
> We're still discussing whether we want to blend those two settings, but at
> this point we want to unify all of our UI to use the chrome UI.

Thanks for clearing that up! I've made the change to use getSelectedLocale: https://reviewboard.mozilla.org/r/82344/diff/4-5/

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

If I understand correctly, comment 72 is asking whether or not the approach you're currently taking is the right one. I've responded here: https://reviewboard.mozilla.org/r/82344/#comment111142

(Oddly, MozReview hasn't copied that comment into this bug. *sigh*)

Does that answer your question? Do you need anything else?

	Scott Wu [:scottwu] Assignee
	Comment 81 • 8 years ago
mozreview-review

Comment on attachment 8796500 [details]
Bug 1283384 - Implement time picker UI w/ message passing,

https://reviewboard.mozilla.org/r/82344/#review86468

::: toolkit/content/widgets/datetimepopup.xml:148
(Diff revisions 5 - 6)
>          ]]></body>
>        </method>
>        <method name="handleMessage">
>          <parameter name="aEvent"/>
>          <body><![CDATA[
>            if (aEvent.origin != "chrome://global") {

Check if message is coming from privileged content.

	Scott Wu [:scottwu] Assignee
	Comment 82 • 8 years ago

Yes you have answered my question :)

I've added the isSystemPrincipal check as you mentioned before doing postMessage to the iframe. As for receiving messages from the iframe, I check if origin is "chrome://global" or not.

https://reviewboard.mozilla.org/r/82344/diff/5-6/

I'll go ahead and land the code if this looks alright. Thanks!

	Mike Conley (:mconley) (:⚙️)
	Comment 83 • 8 years ago
mozreview-review

Comment on attachment 8796500 [details]
Bug 1283384 - Implement time picker UI w/ message passing,

https://reviewboard.mozilla.org/r/82346/#review86738

Yes, with the nit below fixed, I think this can be landed. Let's make sure it's preffed off until we're happy with the behaviour. We need to test it and start filing bugs against it (perhaps we should start thinking about getting SoftVision to test it once you think it's ready).

::: toolkit/content/widgets/datetimepopup.xml:148
(Diff revision 6)
> +          if (aEvent.origin != "chrome://global") {
> +            return;
> +          }

Should check for system principal here instead.

	Scott Wu [:scottwu] Assignee
	Comment 84 • 8 years ago
mozreview-review-reply

Comment on attachment 8796500 [details]
Bug 1283384 - Implement time picker UI w/ message passing,

https://reviewboard.mozilla.org/r/82346/#review86738

> Should check for system principal here instead.

I tried checking the source of the postMessage event but it's null because of security restriction as mentioned in https://developer.mozilla.org/en-US/docs/Web/API/Window/postMessage#Using_window.postMessage_in_extensions.

My current implementation is similar to what's being used at devtool: http://searchfox.org/mozilla-central/source/devtools/client/responsive.html/manager.js#444

Or do you know any other way we can check if the event comes from chrome?

	Scott Wu [:scottwu] Assignee
	Comment 85 • 8 years ago

ni? for :mconley, comment 84. (hopefully the last one for this bug!)

	Mike Conley (:mconley) (:⚙️)
	Comment 86 • 8 years ago
mozreview-review-reply

Comment on attachment 8796500 [details]
Bug 1283384 - Implement time picker UI w/ message passing,

https://reviewboard.mozilla.org/r/82346/#review86738

> I tried checking the source of the postMessage event but it's null because of security restriction as mentioned in https://developer.mozilla.org/en-US/docs/Web/API/Window/postMessage#Using_window.postMessage_in_extensions.
> 
> My current implementation is similar to what's being used at devtool: http://searchfox.org/mozilla-central/source/devtools/client/responsive.html/manager.js#444
> 
> Or do you know any other way we can check if the event comes from chrome?

I think from here we only care about whether or not `this.dateTimePopupFrame` is pointed at the chrome:// page, right?

I think we can probably just do the same check we do on line 164 here - check `this.dateTimePopupFrame.contentDocument.nodePrincipal.isSystemPrincipal`.

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

Responded!

	Scott Wu [:scottwu] Assignee
	Comment 89 • 8 years ago

(In reply to Mike Conley (:mconley) - (high latency on reviews and needinfos) from comment #87)
> Responded!

Got it. Thanks a lot for your help Mike!

	Pulsebot
	Comment 90 • 8 years ago

Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b20258d9a61b
Implement time picker UI w/ message passing, r=mconley

	Pulsebot
	Comment 91 • 8 years ago

Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/autoland/rev/67b4815aa406
Implement time picker UI w/ message passing: Fix eslint errors. r=eslint

	Phil Ringnalda (:philor)
	Comment 92 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/b20258d9a61b
https://hg.mozilla.org/mozilla-central/rev/67b4815aa406

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

I think this should be mentioned at https://developer.mozilla.org/en-US/Firefox/Experimental_features.

Sebastian

	Chris Mills [:cmills]
	Comment 94 • 8 years ago

Setting to ddc for now, as this is preffed off for the time being. We will document it when it is preffed on.