XForms Input control, when bound to a date value, should render as a datetimepicker

RESOLVED FIXED

Status

Core Graveyard
XForms
--
enhancement
RESOLVED FIXED
12 years ago
9 months ago

People

(Reporter: Andrew Douglas, Assigned: Doron Rosenberg (IBM))

Tracking

({fixed1.8.0.2, fixed1.8.1})

Trunk
fixed1.8.0.2, fixed1.8.1

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 12 obsolete attachments)

1.46 KB, application/xhtml+xml
Details
197 bytes, image/png
Details
28.04 KB, patch
smaug
: review+
Details | Diff | Splinter Review
(Reporter)

Description

12 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050219 Firefox/1.0+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050219 Firefox/1.0+

The XForms input control should determine the datatype of the field that it is
bound, and dynamically display the appropriate datetimepicker - either date,
time or date/time. We'll also have to address Month/Year, Month, Year as well
(either build into the datetimepicker control or maybe use the localized strings
pulled from the datetimepicker's dateFormat.properties file to populate a select
list?). 

Reproducible: Always
(Reporter)

Updated

12 years ago
Depends on: 92174

Updated

12 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true

Updated

12 years ago
OS: Windows XP → All
Hardware: PC → All
(Assignee)

Comment 1

12 years ago
Created attachment 196250 [details] [diff] [review]
current progress

what I currently have.	Works for xsd:date only, all strings are gotten from
the user's locale (well, other than the choose date string on the button :), is
pretty accessible (you can use the keyboard to navigate the dates, but you have
to tab around to get back at the back/fwd buttons).

for gmonth/gday, we can easily use a select1.

for gmonthyear, spinbutton probably as well.

time would be trickier, probably need spinbuttons.

datetime would be both date and time in one widget
Assignee: aaronr → doronr
Status: NEW → ASSIGNED
(Assignee)

Comment 2

12 years ago
Created attachment 196354 [details] [diff] [review]
uses a dropdown arrow like select1

I ended up coming up with my own dropdown marker code, since the select1 one
was pretty complex.  I ended up using html:button with an html:img, which works
pretty fine.
Attachment #196250 - Attachment is obsolete: true
(Assignee)

Comment 3

12 years ago
Created attachment 196798 [details] [diff] [review]
now looks like a regular select, ctrl-left/right moves back/fwd a month

Review comments welcome, this can be tested.
Attachment #196354 - Attachment is obsolete: true
(Assignee)

Comment 4

12 years ago
Created attachment 196914 [details] [diff] [review]
as andrew pointed out, my leap year code wasn't complete
Attachment #196798 - Attachment is obsolete: true
(Assignee)

Comment 5

12 years ago
Created attachment 201044 [details] [diff] [review]
patch - date, month, year

supports date, gMonth and gDay.
Attachment #196914 - Attachment is obsolete: true
Attachment #201044 - Flags: review?(smaug)
(Assignee)

Comment 6

12 years ago
Created attachment 201134 [details]
testcase for all 3 types

Comment 7

12 years ago
Comment on attachment 201044 [details] [diff] [review]
patch - date, month, year

>+                <html:input type="button" class="-moz-date-back-button"
>+                             onclick="this.parentNode.parentNode.parentNode.parentNode.parentNode.parentNode.goBack();"/>

indentation (extra space)

>+              </html:td>
>+              <html:td colspan="5" align="center">
>+                <html:span anonid="date"/>
>+              </html:td>
>+              <html:td colspan="1">
>+                <html:input type="button" class="-moz-date-fwd-button"
>+                             onclick="this.parentNode.parentNode.parentNode.parentNode.parentNode.parentNode.goForward();"/>

indentation (extra space)


>+      <field name="_uibuilt">false</field>
>+      <field name="_isPickerVisibile">false</field>

Spelling



>+
>+input[mozType|type="http://www.w3.org/2001/XMLSchema#date"] html|td.prevMonth,
>+input[mozType|type="http://www.w3.org/2001/XMLSchema#date"] html|td.nextMonth {
>+  color: lightgrey;

Is the color right? Maybe it should be GrayText
(http://www.w3.org/TR/CSS21/ui.html#system-colors)


>+}
>+
>+input[mozType|type="http://www.w3.org/2001/XMLSchema#date"] html|td.prevMonth:hover,
>+input[mozType|type="http://www.w3.org/2001/XMLSchema#date"] html|td.nextMonth:hover {
>+  background-color: grey;

I'm not sure about this color. Perhaps it is ok.


>+
>+input[mozType|type="http://www.w3.org/2001/XMLSchema#date"] html|td.currentMonth:hover {
>+  background-color: blue;
>+  color: white;

color: HighlightText;
background-color: Highlight;


>+}
>+
>+html|input.-moz-date-back-button {
>+  width: 20px;
>+  background-image: url("chrome://global/skin/arrow/arrow-lft.gif") !important;
>+  background-repeat: no-repeat !important;
>+  background-position: center !important;
>+}
>+
>+html|input.-moz-date-fwd-button {
>+  width: 20px;
>+  background-image: url("chrome://global/skin/arrow/arrow-rit.gif") !important;
>+  background-repeat: no-repeat !important;
>+  background-position: center !important;
>+}
>+

I guess (didn't test) these buttons don't look right in WinXP default theme.
Would it be possible to fix the problem?


>+html|span.-moz-date-container {
>+  margin: 0px;
>+  margin-bottom: 1px;
>+  border-color: ThreeDFace;
>+  background-color: -moz-Field;
>+  font: -moz-list;
>+  -moz-appearance: menulist;
>+  border-style: inset;
>+  padding-right: 0px;
>+  border-width: 2px;
>+  padding-bottom: 0px;
>+  padding-top: 1px;
>+}

This is quite similar to html|span.-moz-select1-container.
Could you merge the CSS rules? (Just use different selectors)

>+
>+html|input.-moz-xforms-date-dropdown {
>+  width: 12px;
>+  height: 1.3em;
>+  background-image: url("data:image/gif;base64,R0lGODlhBwAEAIAAAAAAAP%2F%2F%2FyH5BAEAAAEALAAAAAAHAAQAAAIIhA%2BBGWoNWSgAOw%3D%3D") !important;
>+  background-repeat: no-repeat !important;
>+  background-position: center !important;
>+  vertical-align: text-top;
>+  margin: 0px !important;
>+  margin-top: -1px !important;
>+  -moz-appearance: menulist-button;
>+  -moz-user-select: none !important;
>+  -moz-user-focus: ignore !important;
>+}

And this is almost like html|input.-moz-xforms-select1-dropdown
Attachment #201044 - Flags: review?(smaug) → review-
(Assignee)

Comment 8

12 years ago
Created attachment 201446 [details] [diff] [review]
new patch
Attachment #201044 - Attachment is obsolete: true
Attachment #201446 - Flags: review?(smaug)

Comment 9

12 years ago
Comment on attachment 201446 [details] [diff] [review]
new patch


>+      <html:span class="-moz-date-container">
>+        <html:input anonid="control" class="-moz-xforms-date-input"
>+                    onblur="this.parentNode.parentNode.delegate.value = this.value;"
>+                    onclick="this.parentNode.parentNode._change();"

_change() method is no-op in this binding, so do you need this for anything?

>+
>+      <method name="_change">
>+        <body>
>+        </body>
>+      </method>

Actually, why do you need this method here at all?


>+
>+          var value = this.inputField.value;
>+          // js date likes YYYY/MM/DD, schema's is YYYY-MM-DD
>+          value = value.replace(/-/g, "/");

Could we support both styles, js and schema, so that
if user prefers to write the date, both styles can be used...
But using only schema is ok too.

>+
>+      <method name="_buildUI">
>+        <parameter name="aDate"/>
>+        <body>
>+          <![CDATA[
>+          var xhtmlNS = "http://www.w3.org/1999/xhtml";
>+
>+          // shortname defaults
>+          var dayShort = ["Sun", "Mon", "Tue", "Wed", "Thu", "Fri", "Sat"];
>+
>+          // try to get localized short names.
>+          // May 2005's first day is a Sunday - also, month is 0-indexed in JS
>+          var day;
>+          for (var i = 0; i < 7; i++) {
>+            day = new Date(2005, 4, i+1).toLocaleFormat("%a");
>+            if (day)
>+              dayShort[i] = day;
>+          }

It would be great to able to use the locale 'first-day-of-the-week'
(which for me should be Monday), but I think Javascript doesn't provide
such feature. So for now this is ok too.

The widget looks good to me and seems to even work and is localized, so
with those comments, r=me

One thing we might want to add (later) is an icon for calendar.
So not to use dropdown button but some icon which looks a bit like a
calendar.
Attachment #201446 - Flags: review?(smaug)
Attachment #201446 - Flags: review?(aaronr)
Attachment #201446 - Flags: review+

Comment 10

12 years ago
aaronlev, could you try this patch out from a accessibility/usability perspective, please?  I can provide you with a .xpi with this patch in it if you don't build xforms anymore.  Thanks.

Comment 11

12 years ago
Usability/behavior/visual issues:

1) If the calendar dropdown is down, should the user be able to enter in the input field part of the combo box?
2) If you enter 2005-12-45, it doesn't default back to today's date, but rather goes to 2006-01-14.
3) If change value of control bound to gMonth and then hit tab, the select1 input field loses the selected value (even though it is still in the instance data).
4) nit: bottom border of the input field part of the select1 for a xsd:date input is lost.  Maybe ask smaug about how to fix this...seems to me this happened with select1's while he was developing them.
5) if you click to get the popdown for the xsd:date input and then click to get the popdown for the xsd:gDay, the calendar doesn't hide itself.
6) I personally think that if you click on the left or right month buttons on the calendar, then focus should stay there until a day is clicked on or the user hits the up/down key.  Hitting enter key should when focus is on a left/right month button on the calendar should advance/go back a month yet keep the focus on the same button.  Also if the focus is on the left month button and the user hits the right key, then focus should go to the right month button, not a day on the calendar.
7) Please update the testcase to use pseudoclass instead of attribute.
8) you should probably put alternate text on the left/right month buttons anonymous content for screen readers, shouldn't you?

We can fix/resolve these issues on a seperate bug if you want.  Just let me know and I'll give this code a review if you want to do that.

Comment 12

12 years ago
Comment on attachment 201446 [details] [diff] [review]
new patch

removing review request.  Doron is fixing some of my UI critiques in this bug.
Attachment #201446 - Flags: review?(aaronr)
(Assignee)

Comment 13

12 years ago
Created attachment 202146 [details] [diff] [review]
fix aaron's comments

The only thing I couldn't get to work is if you focus away from the picker, it won't hide the dropdown, since it is hard to get the current focused element during a blur event.
Attachment #201446 - Attachment is obsolete: true
Attachment #202146 - Flags: review?(aaronr)

Comment 14

12 years ago
Comment on attachment 202146 [details] [diff] [review]
fix aaron's comments

removing the review request.  Doron's going to try xforms-previous, xforms-next to see if he can use them to resolve the dropdown issue.
Attachment #202146 - Flags: review?(aaronr)
(Assignee)

Comment 15

12 years ago
Created attachment 202420 [details] [diff] [review]
new patch
Attachment #202146 - Attachment is obsolete: true
Attachment #202420 - Flags: review?(aaronr)
(Assignee)

Comment 16

12 years ago
Created attachment 202421 [details]
calendar img (belongs in extensions/xforms/resources/content/

Comment 17

12 years ago
Comment on attachment 202420 [details] [diff] [review]
new patch

not updating the instance data, so removing review request while waiting for new patch.
Attachment #202420 - Flags: review?(aaronr)
(Assignee)

Comment 18

12 years ago
Comment on attachment 202420 [details] [diff] [review]
new patch

accessor stuff broke this.
Attachment #202420 - Attachment is obsolete: true
(Assignee)

Comment 19

12 years ago
Created attachment 202692 [details] [diff] [review]
with accessor stuff added
(Assignee)

Comment 20

12 years ago
Created attachment 203023 [details] [diff] [review]
fix windows alignment issue for the calendar image.
Attachment #202692 - Attachment is obsolete: true
(Assignee)

Updated

12 years ago
Attachment #203023 - Flags: review?(aaronr)

Comment 21

12 years ago
Comment on attachment 203023 [details] [diff] [review]
fix windows alignment issue for the calendar image.

>+input[mozType|type="http://www.w3.org/2001/XMLSchema#date"] html|input[anonid="dropmarker"] {
>+  min-width:27px;
>+  min-height: 1.3em;
>+  background-image: url(chrome://xforms/content/calendar.png) !important;
>+  background-position: center !important;
>+  background-repeat: no-repeat !important;
>+}

Perhaps some alt text or @title for the dropmarker button since this is a .png?  Something for the accessibility agents to chew on.

>+      <method name="_showPicker">
>+        <body>
>+          <![CDATA[
>+          // show the picker
>+          var picker = this.picker;
>+
>+          if (this._isPickerVisible) {
>+            this._hidePicker(true);
>+            return;
>+          }

If we have a showPicker and a hidePicker, shouldn't we just return if showPicker is called when the picker is already showing?  showPicker probably shouldn't be a toggle if there is a hidePicker.

>+
>+          picker.style.display = "block";
>+          this._isPickerVisible = true;
>+
>+          var value = this.inputField.value;
>+          // js date likes YYYY/MM/DD, schema's is YYYY-MM-DD
>+          value = value.replace(/-/g, "/");
>+
>+          // we check if the delgate is valid since javascript Date()
>+          // returns a valid date for 2005-04-56.
>+          var tmpDate = new Date(value);
>+          if (!this.accessors.isValid() || tmpDate == "Invalid Date")
>+            this._date = new Date();
>+          else
>+            this._date = tmpDate;
>+
>+          if (!this._uibuilt)
>+            this._buildUI(this._date);
>+
>+          // position the dropdown, aligning it witht the calendar button

nit: spelling

>+          var dropmarker = document.getAnonymousElementByAttribute(this, "anonid", "dropmarker");
>+          var dropmarkerBox = document.getBoxObjectFor(dropmarker);
>+          var width = document.getBoxObjectFor(picker).width;
>+
>+          var position = dropmarkerBox.x - width + dropmarkerBox.width;
>+
>+          // reset position if it will bleed to the left or right
>+          if (position < 0) {
>+            position = 0;
>+          } else if ((position + width) > window.document.width) {
>+            position = window.document.width - width;
>+          }

in the 'else if' test, what if the result is position < 0?  Is it more important that we see the left of the calendar, or the right?  Test this out on your own.  If you size the width of the browser with your testcase smaller, you'll see that picker creeps left away from the dropmarker button way before the dropmarker is out of the browser window.  Maybe allow the drop down to move left, but only as far left as the right edge of the drop down matching the right edge of the input field?

Gotta run.  I'll finish the review a little later this afternoon.

Comment 22

12 years ago
Comment on attachment 203023 [details] [diff] [review]
fix windows alignment issue for the calendar image.

>+++ extensions/xforms/resources/content/xforms.xml	14 Nov 2005 19:44:35 -0000
>@@ -88,11 +88,11 @@
> 
>       <property name="accessors" readonly="true">
>         <getter>
>-	   <![CDATA[
>+        <![CDATA[
>           if (!this._accessors && this.delegate)
>             this._accessors = this.delegate.getXFormsAccessors();
>           return this._accessors;
>-	   ]]>
>+        ]]>
>         </getter>
>       </property>

nit: indentation.  We need to decide on our indentation w.r.t. this.  xforms.xml does it differently than select1.xml which does it differently from select.xml.  Which is right as far as Mozilla styling?  Should we always indent <![CDATA[ and should we indent the contents of the CDATA?  This also applies to the rest of the code below.


>+      <method name="selectCell">
>+        <parameter name="aCellNum"/>
>+        <body>
>+          <![CDATA[
>+          this._cells[this._currentCellIndex].node.setAttribute("tabindex", "-1");
>+
>+          this._currentCellIndex = aCellNum;
>+
>+          this._cells[this._currentCellIndex].node.setAttribute("tabindex", "0");
>+          this._cells[this._currentCellIndex].node.focus();
>+          ]]>
>+        </body>
>+      </method>

Shouldn't you check to see if aCellNum == this._currentCellIndex before you do all of this work?

>+    <handlers>
>+      <handler event="keypress">
>+        <![CDATA[
>+        if (this._isPickerVisible) {
>+          var index = this._currentCellIndex;
>+          var currentElement = event.originalTarget;
>+
>+          if (event.keyCode == event.DOM_VK_DOWN) {
>+            if (currentElement.localName == "input") {
>+              // if we are on the button, down should focus the current selected
>+              // cell
>+              this.selectCell(this._currentCellIndex);
>+            } else if ((index + 7) < this._cells.length) {
>+              this.selectCell(index + 7);
>+            }
>+          } else if (event.keyCode == event.DOM_VK_UP) {
>+            // td means we are on a cell
>+            if (currentElement.localName == "td" && (index - 7) >= 0) {
>+              this.selectCell(index - 7);
>+            } else {
>+              // focus the back button
>+              document.getAnonymousElementByAttribute(this, "anonid", "back-button").focus();
>+            }
>+          } else if (event.keyCode == event.DOM_VK_LEFT) {
>+            // ctrl-left goes back a month
>+            if (event.ctrlKey) {
>+              this.goBack();
>+            } else if (currentElement.localName == "input") {
>+              // input means we are on one of the back/fwd buttons
>+              document.getAnonymousElementByAttribute(this, "anonid", "back-button").focus();
>+            } else if ((index - 1) >= 0) {
>+              this.selectCell(index - 1);
>+            }
>+          } else if (event.keyCode == event.DOM_VK_RIGHT) {
>+            // ctrl-right goes forward a month
>+            if (event.ctrlKey) {
>+              this.goForward();
>+            } else if (currentElement.localName == "input") {
>+              // input means we are on one of the back/fwd buttons
>+              document.getAnonymousElementByAttribute(this, "anonid", "fwd-button").focus();
>+            } else if ((index + 1) < this._cells.length) {
>+              this.selectCell(index + 1);
>+            }
>+          } else if (event.keyCode == event.DOM_VK_RETURN &&
>+                     event.originalTarget.localName == "td") {
>+            var type = event.originalTarget.className;
>+            if (type == "currentMonth") {
>+              this.selectCell(event.originalTarget.getAttribute("num"));
>+              this._valueSet();
>+            } else if (type == "prevMonth") {
>+              this.goBack();
>+            } else if (type == "nextMonth") {
>+              this.goForward();
>+            }
>+          } else if (event.keyCode == event.DOM_VK_ESCAPE) {
>+            this._hidePicker(true);
>+          }
>+        } else {
>+          if (event.keyCode == event.DOM_VK_DOWN) {
>+            // show picker when the user presses down
>+
>+            // first set the accessor value, since the input's value hasn't
>+            // been validated yet, and forcing this will.
>+            this.accessors.setValue(this.inputField.value);
>+
>+            this._showPicker()
>+          }
>+        }
>+        ]]>
>+      </handler>

Please add code to test for F4, alt+down and alt+up so that this control behaves like a select1.  Or open up a seperate bug (with 'a11y:' at the start of the subject) to address this problem.

>+ <!-- INPUT: Month -->
>+  <binding id="xformswidget-input-month"
>+           extends="chrome://xforms/content/xforms.xml#xformswidget-base">
>+    <content>
>+      <children/>
>+      <html:select anonid="control" xbl:inherits="style"
>+                   onchange="this.parentNode._change();"
>+                   onblur="this.parentNode._setValue()">
>+        <html:option value=""></html:option>
>+      </html:select>
>+    </content>

Should this inherit @accesskey?

>+  <!-- INPUT: Day -->
>+  <binding id="xformswidget-input-day"
>+           extends="chrome://xforms/content/xforms.xml#xformswidget-base">
>+    <content>
>+      <children/>
>+      <html:select anonid="control" xbl:inherits="style"
>+                   onchange="this.parentNode._change();"
>+                   onblur="this.parentNode._setValue()">
>+        <html:option value=""></html:option>
>+      </html:select>
>+    </content>

Should this inherit @accesskey?


r-'ing to see what you are going to do with the position comments I made earlier.  Other than the other minor stuff, looks great!
Attachment #203023 - Flags: review?(aaronr) → review-
(Assignee)

Comment 23

12 years ago
Created attachment 203417 [details] [diff] [review]
fix all nits/issues
Attachment #203023 - Attachment is obsolete: true
Attachment #203417 - Flags: review?(aaronr)
(Assignee)

Updated

12 years ago
Attachment #203417 - Attachment is obsolete: true
Attachment #203417 - Flags: review?(aaronr)
(Assignee)

Comment 24

12 years ago
Created attachment 203418 [details] [diff] [review]
forgot a file
Attachment #203418 - Flags: review?(aaronr)

Comment 25

12 years ago
Comment on attachment 203418 [details] [diff] [review]
forgot a file

alt+up, alt+down and F4 should toggle the dropdown.  Not just show it.

Rest of this looks great.

With that fixed, r=me.
Attachment #203418 - Flags: review?(aaronr) → review+
(Assignee)

Comment 26

12 years ago
Created attachment 203821 [details] [diff] [review]
with final nits

asking for re-review since a lot has changed.
Attachment #203418 - Attachment is obsolete: true
Attachment #203821 - Flags: review?(smaug)

Comment 27

12 years ago
Comment on attachment 203821 [details] [diff] [review]
with final nits


>+           <html:tbody anonid="tbody">
>+            <html:tr>
>+              <html:td colspan="1">
>+                <html:input type="button" anonid="back-button"
>+                            class="-moz-date-back-button" title="Previous Month"
>+                            onclick="this.parentNode.parentNode.parentNode.parentNode.parentNode.parentNode.goBack(true);"/>
>+              </html:td>
>+              <html:td colspan="5" align="center">
>+                <html:span anonid="date"/>
>+              </html:td>
>+              <html:td colspan="1">
>+                <html:input type="button" anonid="fwd-button"
>+                            class="-moz-date-fwd-button" title="Next Month"

If we really need the title attribute for previous and next month, the value
of it should be localizable (sp?).
So either remove title or make it localizable.
With either one r=me
Attachment #203821 - Flags: review?(smaug) → review+
(Assignee)

Comment 28

12 years ago
checked into trunk
Whiteboard: xf-to-branch

Comment 29

11 years ago
checked into MOZILLA_1_8_BRANCH via bug 323691.  Leaving open for now until it gets into 1.8.0

Updated

11 years ago
Whiteboard: xf-to-branch

Updated

11 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Keywords: fixed1.8.0.2
Resolution: --- → FIXED

Comment 30

11 years ago
verfied fixed on MOZILLA_1_8_BRANCH
Keywords: fixed1.8.1
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.