Closed Bug 92174 Opened 23 years ago Closed 18 years ago

create new calendar and datepicker widgets

Categories

(Core :: XUL, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla1.9alpha1

People

(Reporter: hewitt, Assigned: enndeakin)

References

Details

(Keywords: dev-doc-complete)

Attachments

(6 files, 5 obsolete files)

Long long ago I created a calendar widget and a type-in/dropdown datepicker widget. These have sat idle for a long time, but I see a need for them in the mail search and filter dialogs and would like to get em' checked in.
Hewitt, any reason you filed this when there is already a bug I am working on to fix a simple datepicker? We agreed in that bug not to use a calendar.
Everyone I've talked to agrees that a calendar interface would be clearer than the spinbuttons interface. Jen/German, what is your opinion?
Status: NEW → ASSIGNED
mpt recommended the simple datepicker. it's also 4xp.
Attached patch patch (obsolete) (deleted) — Splinter Review
joe, can you link to a screen shot? my only requirement (for the date picker in search and filters) is that the user can quickly enter in a date (by typing), without having to use the calendar picker. jglick may have other requirements. an additional warning about adding an elaborate calendar picker is, once it's in you'll have to support it, (so there is a cost to your time.) make sure to talk to the i18n team about any potential i18n / L10n issues with having a calendar picker.
first warning is that sunday isn't always the first column for calendars. Monday is for Spanish.
Why do I have the feeling that my comments/suggestions are ignored? bug 90289 is still a duplicate.
hwaara: your bug isn't a dupe. there's nothing wrong w/ having multiple widgets in the tree. We had 3 color pickers at one time iirc. Ideally people should be able to switch from one widget to another effortlessly... Mind you i'd prefer this kind of calendar over the plain one(s) from nc4, but some other project might want to use the simpler model.
Inconsistency (i.e., having different widgets for the same thing) is the root of all evil.
I agree with Seth, that the user should be able to also quickly enter in a date (by typing), without having to use the calendar picker. The visual cue of the calendar picker is available for those users who want it (I want this filter to run the until the last Friday of the month of September, what is that date...), while users who already know the exactly date they want are not hindered in anyway (they can quickly type in the date manually).
What would this datepicker look like? Would you attach a screen shot or mock-up?
This patch introduces 2 tags: <calendar/> which displays a standard grid calendar, or <datepicker/> which looks like a combo box: you can type the date, or drop down a calendar to pick it. See following screen shot..
Attached image screen shot (deleted) —
Marlon suggested that instead of having an open-ended textbox where the user types the date in, there should be three textboxes (for month, day, year). Currently, the format of the open-ended textbox is dependent on the string-to-date parsing abilities of the Javascript date object. You can type in like this "5/10/2001" or "May 10, 2001" or a number of other subtle varieties.
*** Bug 90289 has been marked as a duplicate of this bug. ***
I don't like reinventing the wheel, so I will give you, Joe, the responsibility to implement this widget. However, I would have appreciated some kind of notification before you started this -- or at least you could've responded to my comments in this bug... Comments: 1) Oh well... Joe, the new widget looks great. But let's not rush this in the tree. It's a new, advanced widget and I would appreciate if you could have some people testing it -- how it behaves (jglick, mpt & friends). 2) One concern of mine is that the text-only version of the date (where you can edit it like a textbox) needs to be localizable. For instance, in Sweden we use the following "syntax": 2001-12-02. 3) Please make the buttons to switch months look beveled, so they look "pushable" like regular buttons. 4) Please make it so this widget looks like a native part of both Skins, so the modern3 version looks fancy. :) That's all for now. Good work.
For all those L10N people listening, what issues should I be aware of for making localized calendars for regions like asia or the middle east that have different calendars?
i'd suggest ignoring Hebrew and perhaps certain other calendars for places (eg Israel) where normal dates are handled using the American (or British) system instead. It would be interesting if someone implemented the hebrew (lunar) calendar at a later date, but afaik it's used mainly for religious functions which don't really relate to web browsing or mail searches.
Note that even in localized Hebrew UI calendars have left-to-right direction, i.e. Sunday Monday Tuesday Wednesday.... and not ...Wednesday Tuesday Monday Sunday
I noticed that the date format is of "mm/dd/yy" while European users might be used to "dd/mm/yy". In Asia, "yyyy/mm/dd" is a popular format. Is this concern covered already?
Tao, that was what I was talking about in my previous post (see point #2). I remember when I was working on localizable date stuff and there was an interface nsIDateTimeFormatter (?)... You might want to check that one. I believe you can query it for a short date version and it will give you whatever is most appropriate on the system.
Right, I overlooked your previous comments. I also forwarded this bug report to mozilla-l10n for comments.
May God have mercy on us all. The 212 bug spam-o-rama is Now!
QA Contact: aegis → jrgm
Priority: -- → P3
Target Milestone: --- → mozilla0.9.6
Target Milestone: mozilla0.9.6 → Future
Target Milestone: Future → mozilla1.1
OEone has some date and time picker widgets that perhaps we can contribute along with the calendar source code. This bug is marked assigned, does that mean someone is working on it already? If someone can let me know how this is progressing, I'll find out about contributing out date and time pickers to Mozilla. Email at mikep@oeone.com.
If I am not mistaken, the OEOne calendar/time picker widget is not written using XBL. If you guys are willing to convert it to XBL, that would be great. Are there any features in your widgets that aren't present in the ones I've written?
Are the screen shots shown the actual implementations of the widgets, or just what we'd like them to look like? The OEone calendar widgets are not yet XBL, but are implemented as a XUL overlay. It wouldn't be hard to move them to XBL, but if the present elements are already done, I think we would be more likely to convert ours to them rather than vice versa. So, are the date and time pickers complete? Can we implement them at the same time as moving our calendar source into Mozilla?
Yes, the screen shots are of an actual working implementation. Note that I don't have a time picker, just a date picker.
Joe: have you been keeping this patch in sync with syntax changes, or would an update be needed? I don't know if you have a Linux box to build and run the Calendar on, but if you do, perhaps you'd like to take a look at the OEone widgets and see what you think. I very much like their interface. Gerv
The patch doesn't apply to my tree anymore, so I'm going to try and fix it to make it work. I think I've already found a bug in the date picker though. If you set the value of the date picker, and then click to open the picker part, it doesn't set the pickers date to the right value. It sets it to today. I don't know what the problem is exactly, but var val = this.getAttribute("value"); is always returning nothing for me, so the date defaults to today. I see no reason that this shouldn't go into the tree, even with issue with internationalization issues. Its a good widget that people will find useful, and just because its in the tree doesn't necessarily mean that Mozilla has to use it. If someone can let me know how to set the value properly, or verify that bug, I'll get the fix in when I fix the patch.
Attached file Modified Datepicker (deleted) —
This is a .zip file containing 4 files for the datepicker.
Attachment #43477 - Attachment is obsolete: true
The patch I just attached is the working datepicker that is now being used in the calendar application. It is in the tree (calendar only for now) in mozilla/calendar/resources/content/datepicker/ and I believe needs an image from the calendar skin/ directory. For what its worth, I think it would be a good idea to get this into the tree so people can start using it. Its a valuable widget, and by getting people to use it, hopefully they can help customize it for whatever locale they are in. There are still a few bugs in this version, mostly with typing dates and then selecting them. When you do that, normally the calendar picker doesn't show the right date.
Calendar already has a datepicker, and has quite some testing by now. It lives at http://lxr.mozilla.org/seamonkey/source/calendar/resources/content/datetimepickers/ It would be nice to have them in all builds, for example to use for a birthday field in addressbook. Also, webforms needs a datepicker one day. I can port the datepicker from calendar, but i'm looking for some 'ok' first. cc-ing, in hoping to have found someone who can decide on this.
I support it.
mconnor, who is the UI-czar for the new toolkit? I want this in XUL2. mvl, can you prepare a patch which moves this from calendar/ to toolkit/ so we can push this through?
I don't think we've reached that point where we have a specific toolkit UI-czar. We're getting to the point where we need to possibly split toolkit into its own component separate from the browser component for administration purposes. Maybe its time to bring this up to aviary (its been on my mind for a while now).
These would also be useful for XForms.
asked ben about this, until we need to split things off, we'll handle things like this under the normal toolkit review process. mvl, if you want to whip up that patch, I'd be more than happy to review it.
I would like to use this widget in seamonkey too. Are we planning on allowing toolkit widgets in seamonkey somehow?
mvl: at this point in time, the *location* of the seamonkey and new-toolkit widget bindings are forked, for no good reason I can see. I would suggest not forking the new binding file(s), but intead make toolkit/content/widgets the canonical location and use jar.mn to ship the same files to xpfe/global/resources/content/bindings . jar.mn files can now use absolute paths relative to topsrcdir, so xpfe/global/jar.mn can contain lines like this: content/global/bindings/datepicker.xml (/toolkit/content/widgets/datepicker.xml)
Patch coming soon... made blocker of xforms bug 283344
Blocks: 283344
this patch doesn't work in unprivileged pages - the calendar's datetimepicker's code relied on services for formatting the date. I figured that would either be a separate bug or addressed during review. The zip file contains the .gif's.. Sorry, I couldn't find a better way to attach them... extract the zip to mozilla\toolkit\themes\winstripe\global\ The has been tested in firefox and seamonkey on windows only. calendar has not be tested. a separate bug will be filed to get it to use the toolkit version of the datetimepicker. This is my first patch, so go easy on me .. or rather.. don't go easy on me :) -Andrew
Blocks: 283497
The diff provided adds most of the files as new. Could you provide a diff of the calendar version and the toolkit version of this code? Glancing over the existing code, one obvious major difference is the code for parsing dates and times has been copied directly into some constructors and methods, where it is less reusable and maintainable than if it is kept separate. (It also may run more often than necessary, especially if there are many date fields in a form, but I'm not sure how to avoid that using XBL.)
I apologize for not providing updates in the patch.. I'll work on creating those tomorrow. The functionality was pretty much copy paste, fixing of paths and following the conventions from the toolkit.. unless documented otherwise. But yes, moving the dateUtils code into the xbl was the biggest change. It was necessary because of bug 58757 - I don't have an <xbl:script> element. So unless developers referenced the dateUtils.js in their pages that used the datetimepickers (which is what the calendar code has been doing), or unless I created the dateUtil code as a service, I didn't see a lot of options. As far as reuse.. I'm open to suggestions for how to improve it. As far as efficiency - it's actually more efficient than the original because both DatePicker and TimePicker created DateFormatter objects in their constructor, which, in the DateFormatter constructor was having to analyze both Date and Time formats. In this version, the DatePicker only has to analyze Date formats in the constructor, the TimePicker sets up Time formatting - granted, it's still duplication of effort if you have more than one of the same type of control on the page - but no more so than the previous code. The method for parsing of the date and time is pretty much identical between the old and new so shouldn't be any different in terms of performance/duplication than before. But obviously I'm open to suggestions - I was sticking to the "copy and paste" mentality in order to stay in scope of what everyone wanted, based on the comments in this bug. I hope this helps clear up what my approach was. Thanks!
iirc, webforms will need some way to parse dates too, but also other formats. Would it be possible to have a <parsableinput/> (or whatever is gets called) for this? That tag could maybe open a script with a callback. (just shouting ideas, not requirements for this bug)
The address book would also like to use the datepicker widget for anniversaries & birthdays, which will help with Lightning integration.
Blocks: 13595
Taking. After discussion with various folks including mconnor as toolkit owner, we have a tentative plan to refactor the existing widgetry slightly so that it's entirely field-based, rather than jsDate based. There will then be wrappers created to coalesce the fields into jsDate (which will live in toolkit/) and calDateTime (which will live in calendar/).
Assignee: hewitt → dmose
Status: ASSIGNED → NEW
Priority: P3 → P2
Target Milestone: mozilla1.1alpha → mozilla1.9alpha
Assignee: dmose → enndeakin
Depends on: 348614
Attached patch date and time pickers (obsolete) (deleted) — Splinter Review
Here is the time and date pickers I came up with. In the end, they only ended up being loosely based on the calendar ones. Documentation is at http://wiki.mozilla.org/XUL:Specs:DateTimePickers Tests and examples are at http://xulplanet.com/ndeakin/tests/xts/dtpicker/ Let me know if these suit the needs for calendar or other places where they might be used. Note that a build with bug 348614 fixed is needed for the dropdown datepicker. Also, there is still an issue with focus navigation in the datepicker grid.
(In reply to comment #48) > > Documentation is at http://wiki.mozilla.org/XUL:Specs:DateTimePickers > Tests and examples are at http://xulplanet.com/ndeakin/tests/xts/dtpicker/ > > Let me know if these suit the needs for calendar or other places where they > might be used. Note that a build with bug 348614 fixed is needed for the > dropdown datepicker. Neil, can you show some tests for xhtml documents since it is important for xforms and webforms2?
It works in XHTML apart for clicking the arrow buttons to adjust the values or month. I'll investigate that.
Attached patch time and date pickers for toolkit (obsolete) (deleted) — Splinter Review
Fixed some event handling issues. The issues in non-xul documents were caused by bug 349477.
Attachment #236273 - Attachment is obsolete: true
Attachment #239258 - Flags: superreview?(neil)
Attachment #239258 - Flags: review?(mano)
Comment on attachment 239258 [details] [diff] [review] time and date pickers for toolkit Some drive-by comments, I didn't review most of the implementation itself (will do for the next patch). >Index: toolkit/content/widgets/datetimepicker.xml >=================================================================== >RCS file: toolkit/content/widgets/datetimepicker.xml >diff -N toolkit/content/widgets/datetimepicker.xml >--- /dev/null 1 Jan 1970 00:00:00 -0000 ... >+ <field name="_fieldOneCached">null</field> >+ <field name="_fieldTwoCached">null</field> >+ <field name="_fieldThreeCached">null</field> >+ <field name="_fieldAMPMCached">null</field> >+ <field name="_lastFocusedField">null</field> >+ <field name="attachedControl">null</field> >+ <field name="_hasEntry">true</field> >+ <property name="_fieldOne" readonly="true"> >+ <getter><![CDATA[ >+ if (!this._fieldOneCached) >+ this._fieldOneCached = document.getAnonymousElementByAttribute(this, "anonid", "input-one"); >+ return this._fieldOneCached; >+ ]]></getter> >+ </property> Cannot we do <field name="_fieldOne">document.getAnonymousElementByAttribute(this, "anonid", "input-one");</field> instead (as we do in various places in toolkit/content/widgets)? >+ <property name="_currentField" readonly="true"> >+ <getter> >+ var focusedInput; >+ try { >+ focusedInput = document.commandDispatcher.focusedElement; This will not work in xhtml documents, maybe use document.activeElement? >+ if (focusedInput == this._fieldOne || >+ focusedInput == this._fieldTwo || >+ focusedInput == this._fieldThree || >+ focusedInput == this._fieldAMPM) >+ return focusedInput; >+ } catch(ex) { } When would this throw? >+ <binding id="timepicker" >+ extends="chrome://global/content/bindings/datetimepicker.xml#datetimepicker-base"> >+ >+ <implementation> >+ <field name="is24HourClock">false</field> >+ <field name="hourLeadingZero">false</field> >+ <field name="minuteLeadingZero">true</field> >+ <field name="secondLeadingZero">true</field> >+ <field name="amIndicator">"AM"</field> >+ <field name="pmIndicator">"PM"</field> Those should be readonly if they're supposed to be public. >+ <property name="isPM"> >+ <getter> >+ <![CDATA[ >+ return (this.hour >= 11); >+ ]]> >+ </getter> >+ <setter> >+ <![CDATA[ >+ if (this.hour < 12) >+ this.hour += 12; >+ else >+ this.hour -= 12; >+ ]]> I'm not sure I understand the logic here, what would happen if I set isPM twice to the same value? >+ </setter> >+ </property> >+ <property name="hideseconds"> >+ <getter> >+ return (this.getAttribute("hideseconds") == "true"); >+ </getter> >+ <setter> >+ if (val) >+ this.setAttribute("hideseconds", "true"); >+ else >+ this.removeAttribute("hideseconds"); >+ if (this._secondField) >+ this._secondField.parentNode.collapsed = val; >+ document.getAnonymousElementByAttribute(this, >+ "anonid", "sep-second").collapsed = val; nit: add a field. >+ <method name="_init"> >+ <body> >+ <![CDATA[ >+ var numberOrder = /^(\D*)\s*(\d+)(\D*)(\d+)(\D*)(\d+)\s*(\D*)$/; >+ >+ var dt = new Date(2002,9,4).toLocaleFormat("%x"); >+ var numberFields = dt.match(numberOrder); >+ if (numberFields) { >+ document.getAnonymousElementByAttribute( >+ this, "anonid", "sep-first").value = numberFields[3]; >+ document.getAnonymousElementByAttribute( >+ this, "anonid", "sep-second").value = numberFields[5]; fields (and in other places i didn't comment on too) :) >+ <property name="selectedItem" onget="return this._selectedItem"> >+ <setter> >+ <![CDATA[ >+ if (!val.value) >+ return; >+ var datebox = document.getAnonymousElementByAttribute(this, "anonid", "datebox"); >+ if (val.parentNode.parentNode != datebox) >+ return; Setters should always return val >+ <method name="_init"> >+ <body> >+ <![CDATA[ >+ this._yearField = document.getAnonymousElementByAttribute(this, "anonid", "yearlabel"); >+ this._monthField = document.getAnonymousElementByAttribute(this, "anonid", "monthlabel"); >+ this._dateField = document.getAnonymousElementByAttribute(this, "anonid", "datebox"); Again, cannot we simply inline this in the <field> itself? >Index: toolkit/locales/en-US/chrome/global/datetimepicker.dtd >=================================================================== >+<!ENTITY firstdayofweek.default "0"> Maybe we should use a real string here. At least add a comment explaining what to set for Sunday/Monday. >Index: toolkit/themes/pinstripe/global/datetimepicker.css >=================================================================== >+@namespace url("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"); >+@namespace html url("http://www.w3.org/1999/xhtml"); >+ >+datepicker, timepicker { >+ -moz-appearance: none !important; nit: why !important? >+ padding: 0; >+ padding-bottom: 1px; nit: merge. >Index: toolkit/themes/winstripe/global/datetimepicker.css >=================================================================== >+@namespace url("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"); >+@namespace html url("http://www.w3.org/1999/xhtml"); >+ >+datepicker, timepicker { >+ -moz-appearance: none; >+ margin: 2px 4px; >+ padding: 0; >+ border: none; >+ background: none; >+ cursor: default; >+} >+ >+.datetimepicker-input-box { >+ -moz-appearance: textfield; >+ cursor: text; >+ margin-right: 2px; -moz-margin-end >+ padding: 2px 2px 3px 4px; -moz-padding-start/-moz-padding-end (these are all winstripe only, neither suite themes, nor pinstripe supoort RTL UI). >+html|*.datetimepicker-input { >+ text-align: right; >+} I do not remember if |text-align: end;| works; if not use the chromedir attribute and add an overriding selector. >+.datepicker-gridlabel[today="true"] { >+ background-color : darkgrey; >+ color : white; >+} >+ >+.datepicker-gridlabel[selected="true"] { >+ background-color : Highlight; >+ color : HighlightText; >+} nit: "color: ..." >+.datepicker-previous { >+ list-style-image: url("chrome://global/skin/arrow/arrow-lft.gif"); >+} >+ >+.datepicker-next { >+ list-style-image: url("chrome://global/skin/arrow/arrow-rit.gif"); >+} >+ >+.datepicker-previous:hover { >+ list-style-image: url("chrome://global/skin/arrow/arrow-lft-hov.gif"); >+} >+ >+.datepicker-next:hover { >+ list-style-image: url("chrome://global/skin/arrow/arrow-rit-hov.gif"); >+} >+ >+.datepicker-previous[disabled="true"] { >+ list-style-image: url("chrome://global/skin/arrow/arrow-lft-dis.gif"); >+} >+ >+.datepicker-next[disabled="true"] { >+ list-style-image: url("chrome://global/skin/arrow/arrow-rit-dis.gif"); >+} Set the chromedir attribute in the binding and flip those in the RTL case.
Attachment #239258 - Flags: review?(mano) → review-
Can you add some pix of this so that we can give UI feedback?
Some comments from reading code: o welcome improvement over calendar datetimepicker * can increment/decrement individual numerals * may be more accessible? (e.g., to see month before selecting date) o some regressions compared to calendar datetimepicker: * cannot paste/copy a time (must paste/copy each field individually?) * cannot paste/copy a date (must paste/copy each field individually?) * am/pm not localizable * am/pm rigid parse, does not allow variation (am, a.m., AM, A.M.; CJK, cyrillic, arabic) * requires numeric month format (No 31-Aug-2006) * no datetimepicker (so caller has to combine/decombine date & time) o inconveniences: * not possible for user to change time increment; may take up to 30 clicks to change time if set to 1min increment. * cannot type a date or time normally (must tab between subfields) o nits: * assumes localized 'a.m.' and 'p.m.' are same length (should use max). * this needs a comment to explain what it is trying to do: + if (aValue < min) + return aEntered ? min : max; + if (aValue > max) + return aEntered ? max : min; * increaseOrDecreaseMonth works differently depending on this.readOnly * value setter does not set time for non-Date instances.
(In reply to comment #51) > Created an attachment (id=239258) [edit] > time and date pickers for toolkit > > Fixed some event handling issues. The issues in non-xul documents were caused > by bug 349477. > Can you put some testcases for non-xul documents on your page?
> * am/pm not localizable The AM and PM strings come from the locale. > * am/pm rigid parse, does not allow variation > (am, a.m., AM, A.M.; CJK, cyrillic, arabic) Can you give an example of a variation? The strings come from the locale so whatever it uses will displayed. > * not possible for user to change time increment; > may take up to 30 clicks to change time if set to 1min increment. Or, just hold down the button. > * cannot type a date or time normally (must tab between subfields) > > * increaseOrDecreaseMonth works differently depending on this.readOnly Indeed. That's because the datepicker doesn't allow modification in this case. > * value setter does not set time for non-Date instances. > Do you mean you want a setValueAsString?
(In reply to comment #49) > (In reply to comment #48) > > Let me know if these suit the needs for calendar or other places where they > > might be used. Neil, perhaps you missed my email to you. On thinking about it, the date picker would be only really be useful to the Address Book (and integration with Calendar), if we could actually allow the user not to enter a day/month/year - for instances where they aren't known, especially with the address book where someone may not know the date at all (yes they could set it to some random date all the time but that's not exactly nice) or they just know the day and month and not the year. It could probably be worked around with extra checkboxes but that's a bit sad when we could allow them just to leave the field blank.
(In reply to comment #56) > > * am/pm not localizable > > The AM and PM strings come from the locale. Ok, I see, the fields are intialized with "AM" and "PM" (non-localizable), but then overwritten in _init() with a suffix parsed from Date.toLocalFormat("%X"). > > > * am/pm rigid parse, does not allow variation > > (am, a.m., AM, A.M.; CJK, cyrillic, arabic) > > Can you give an example of a variation? The strings come from the locale so > whatever it uses will displayed. The goal in the calendar datetimepicker is to make it easy for someone to paste a date/time, say one selected and copied from a (different) web page or an email message. Copying and pasting is always less typo-prone than re-entering dates and times. In many countries web pages may appear in more than one language, and dates and times may appear in more than one format, so the parser needs to accept multiple formats. The calendar time picker parses various forms of latin a.m./p.m. plus japanese, chinese, cyrillic, and arabic regardless of locale, so it covers the G8 countries' languages (biggest trading partners) and the U.N. languages (most widespread 2nd languages). I gave you some examples: am, a.m., AM, A.M., the patch under review only matches at most one form. If you're looking for non-latin examples, there are a few in the calendar datetimepicker code, encoded in \uNNNN unicode notation (see link below). Also see bug 270724 comment 3 and bug 270724 comment 5. See datetimepickers.xml#datetimepicker-base.parseTime http://lxr.mozilla.org/mozilla/source/calendar/resources/content/datetimepickers/datetimepickers.xml#1449 and .initTimeFormat http://lxr.mozilla.org/mozilla/source/calendar/resources/content/datetimepickers/datetimepickers.xml#1598 esp. am/pm regexes http://lxr.mozilla.org/mozilla/source/calendar/resources/content/datetimepickers/datetimepickers.xml#1679 The patch under review parses the current locales' am/pm formats from the toLocalFormat("%X") string and matches them exclusively (or at least first character). Instead, suggest these should be added to the am/pm regexes, especially if they don't already match it. (nit in _init(): Note that in numberOrder = /^(\D*)\s*.../ the \s* will never match because spaces will match \D, so if you expect spaces here, then they will need to be trimmed from the end of the (\D*) match.) (nit in timepicker keypress handler: code assumes am/pm strings differ in the first character, which is not true in all languages, e.g., not in japanese &#21320;&#21069;/&#21320;&#24460; .) > > * increaseOrDecreaseMonth works differently depending on this.readOnly > > Indeed. That's because the datepicker doesn't allow modification in this case. So shouldn't it behave the same as disabled if this.readonly is true? I don't understand this: + <method name="_increaseOrDecreaseMonth"> + <parameter name="aDir"/> + <body> + <![CDATA[ + if (this.disabled) + return; + + if (this.readOnly) { + this._displayedDate.setMonth(this._displayedDate.getMonth() + aDir); + this._updateUI(this._monthField, this._displayedDate.getMonth()); + } ... > > > * value setter does not set time for non-Date instances. > > > > Do you mean you want a setValueAsString? > I mean timepicker.value setter has code that does not set time if val is not a Date: + <property name="value" onget="return new Date(this._value);"> + <setter> + <![CDATA[ + if (!(val instanceof Date)) { + var dt = new Date(); + var dtstr = dt.getDate() + " " + dt.getMonth() + " " + + dt.getFullYear() + " " + val; + val = new Date(dtstr); + if (!(val instanceof Date)) + throw "Invalid Date"; + } + else { + val = new Date(val); + } + ... > > Do you mean you want a setValueAsString? > Some sort of set value as string is needed to support pasting dates and times copied from else where, so yes. The calendar datetimepicker supports pasting into the date field a date followed by a time as one string --- it parses the string and fills both fields. A similar strategy might be used to fill all later fields from a string pasted into one. Hope this is clearer, sorry for the ambiguity.
(In reply to comment #58) > The goal in the calendar datetimepicker is to make it easy for someone to paste > a date/time, say one selected and copied from a (different) web page or an > email message. Actually, I'd think the goal was to allow someone to pick a date and/or time, and thus the primary focus for its UI design. Unless you have evidence to the contrary, I'd think that pasting dates and times is significantly less likely. However, I can certainly add support for parsing dates and times while pasting values in the fields. > So shouldn't it behave the same as disabled if this.readonly is true? I don't > understand this: > A readonly element can still be examined and have its text selected. A disabled element cannot be interacted with at all. > I mean timepicker.value setter has code that does not set time if val is not a > Date: > > + <property name="value" onget="return new Date(this._value);"> > + <setter> > + <![CDATA[ > + if (!(val instanceof Date)) { > + var dt = new Date(); > + var dtstr = dt.getDate() + " " + dt.getMonth() + " " + > + dt.getFullYear() + " " + val; The line above sets the time.
(In reply to comment #59) > (In reply to comment #58) > > The goal in the calendar datetimepicker is to make it easy for someone to paste > > a date/time, say one selected and copied from a (different) web page or an > > email message. > > Actually, I'd think the goal was to allow someone to pick a date and/or time, > and thus the primary focus for its UI design. Unless you have evidence to the > contrary, I'd think that pasting dates and times is significantly less likely. Doubtful. I'd say pasting is quite common. Then again, with full integration of Lightning, it would be even nicer to click a date/time in an email and create a task/event/appointment from that with a right click. This is something to plan towards and shoot for.
Comment on attachment 239258 [details] [diff] [review] time and date pickers for toolkit Just looking at the base and timepicker bindings... >+ <field name="_value">new Date();</field> No ; please, it looks odd. >+ <body> >+ </body> <body/> ? >+ var dtstr = dt.getDate() + " " + dt.getMonth() + " " + >+ dt.getFullYear() + " " + val; This fails because a) because getMonth() returns 0-11 b) new Date seems to want m/d/y or y/m/d style dates >+ val = new Date(dtstr); >+ if (!(val instanceof Date)) new Date is always instanceof Date. However its .getTime may be NaN. >+ else { >+ val = new Date(val); But val already is a Date... >+ if (this.hour < 12) >+ this.hour += 12; >+ else >+ this.hour -= 12; This only works in the isPM = !isPM case. >+ if (this._secondField) >+ this._secondField.parentNode.collapsed = val; >+ document.getAnonymousElementByAttribute(this, >+ "anonid", "sep-second").collapsed = val; You should use xbl:inherits="collapsed=hideseconds" where appropriate. >+ var oldval; >+ if (field == this._hourField) >+ oldval = this.hour; >+ else if (field == this._minuteField) >+ oldval = this.minute; >+ else if (field == this._secondField) >+ oldval = this.second; >+ >+ newval = this._constrainValue(field, oldval + aDir, false); >+ >+ if (field == this._hourField) >+ this.hour = newval; >+ else if (field == this._minuteField) >+ this.minute = newval; >+ else if (field == this._secondField) >+ this.second = newval; Can't you write oldval = field.value; and field.value = newval; ? >+ <parameter name="aEntered"/> aWrap? Seeing as this is what it affects? >+ this._hourField = this._fieldOne; >+ this._minuteField = this._fieldTwo; >+ this._secondField = this._fieldThree; It hardly seems worth caching them in the base binding.
Comment on attachment 239258 [details] [diff] [review] time and date pickers for toolkit >+ var dt = new Date(aValue); >+ if (!isNaN(dt)) { Heh, I didn't know isNaN worked on Date objects :-) >+ aField.value = (aValue < 10 ? "000" + aValue : >+ (aValue < 100 ? "00" + aValue : >+ (aValue < 1000 ? "0" + aValue : aValue))); Could use aField.value = ("000" + aValue).slice(-4); >+ if (target.localName == "label") { I'm not sure that's quite right, there are other labels in the picker. >+ var rows = event.detail; >+ if (rows == NSUIEvent.SCROLL_PAGE_UP) { >+ rows = -1; >+ } else if (rows == NSUIEvent.SCROLL_PAGE_DOWN) { >+ rows = 1; >+ } else { >+ // In this case event.detail contains the default number of lines >+ // to scroll. We always want to only scroll 1 month though >+ rows = (rows > 0) ? 1 : -1; >+ } >+ >+ this._increaseOrDecreaseMonth(rows); Since SCROLL_PAGE_UP < 0 and SCROLL_PAGE_DOWN > 0 you only need this._increaseOrDecreaseMonth(event.detail < 0 ? -1 : 1); I couldn't keyboard navigate out of the current month using the grid style picker. Typically the navigation increments at the end of the month, plus PgUp/PgDn navigates a whole month. Because of the way popups work, keyboard navigation is ineffective for the grid inside a datepicker's popup. Some of the month names caused the grid to resize. This was particularly noticable when the grid was inside a popup. I was expecting the popup to close automatically when a date was clicked.
Attachment #239258 - Flags: superreview?(neil) → superreview-
By the way, where's the code that range-checks the day of month when you change the month (e.g. to Feburary)? I couldn't follow the logic.
> >+ else { > >+ val = new Date(val); > But val already is a Date... Need to copy the object so it doesn't get reused. > >+ if (this._secondField) > >+ this._secondField.parentNode.collapsed = val; > >+ document.getAnonymousElementByAttribute(this, > >+ "anonid", "sep-second").collapsed = val; > You should use xbl:inherits="collapsed=hideseconds" where appropriate. I considered this but then as this would go in the base binding, hideseconds would also work on the datepicker, which I don't want. > Can't you write oldval = field.value; and field.value = newval; ? The displayed value may not be the same as the actual value. > Because of the way popups work, keyboard navigation is ineffective > for the grid inside a datepicker's popup. This will be fixed by the large popup reworking code. > Some of the month names caused the grid to resize. > This was particularly noticable when the grid was inside a popup. Don't see this. Which platform/theme?
Status: NEW → ASSIGNED
The navigation is the grid is now better. It moves between months properly. Also, the value/dateValue properties get a string and jsdate forms of the date.
Attachment #246469 - Flags: superreview?(neil)
Attachment #246469 - Flags: review?(mano)
(In reply to comment #64) >>>+ if (this._secondField) >>>+ this._secondField.parentNode.collapsed = val; >>>+ document.getAnonymousElementByAttribute(this, >>>+ "anonid", "sep-second").collapsed = val; >>You should use xbl:inherits="collapsed=hideseconds" where appropriate. >I considered this but then as this would go in the base binding, hideseconds >would also work on the datepicker, which I don't want. Sorry, I'm not following you. It would go in the binding(s) in which you want hideseconds to work. >>Can't you write oldval = field.value; and field.value = newval; ? >The displayed value may not be the same as the actual value. Then constrain the display value perhaps? >>Some of the month names caused the grid to resize. >>This was particularly noticable when the grid was inside a popup. >Don't see this. Which platform/theme? Linux/Modern.
Comment on attachment 246469 [details] [diff] [review] rework date/time pickers a bit and address review comments >Index: toolkit/content/widgets/datetimepicker.xml >=================================================================== >+ <property name="_currentField" readonly="true"> >+ <getter> >+ var focusedInput = document.activeElement; >+ if (focusedInput == this._fieldOne || >+ focusedInput == this._fieldTwo || >+ focusedInput == this._fieldThree || >+ focusedInput == this._fieldAMPM) >+ return focusedInput; >+ return this._lastFocusedField ? this._lastFocusedField : this._fieldOne; nit: return this._lastFocusedField || this._fieldOne; >+ <constructor> >+ this._init(); >+ >+ var cval = this.getAttribute("value"); >+ if (cval) { >+ try { >+ this.value = cval; >+ return; >+ } catch (ex) { } >+ } >+ this.dateValue = new Date(); So, why is _dateValue initialized with "new Date()" if we override it here? >+ <binding id="timepicker" >+ extends="chrome://global/content/bindings/datetimepicker.xml#datetimepicker-base"> >+ >+ <implementation> >+ <field name="is24HourClock" readonly="true">false</field> >+ <field name="hourLeadingZero" readonly="true">false</field> >+ <field name="minuteLeadingZero" readonly="true">true</field> >+ <field name="secondLeadingZero" readonly="true">true</field> >+ <field name="amIndicator" readonly="true">"AM"</field> >+ <field name="pmIndicator" readonly="true">"PM"</field> I couldn't recall whether we actually have support for readonly fields. >+ this.hideSeconds = (this.getAttribute("hideseconds") == "true"); nit: this.hideSeconds = this.hideSeconds. >+ <property name="year" onget="return this._dateValue.getFullYear();"> >+ <setter> >+ <![CDATA[ >+ val = Number(val); >+ if (isNaN(val) || val < 1 || val > 9999) >+ throw "Invalid Year"; >+ this._setFieldValue(this.yearField, val); >+ return val; >+ ]]> >+ </setter> >+ </property> |val| should be returned the way it was passed. >+ <property name="month" onget="return this._dateValue.getMonth();"> >+ <property name="date" onget="return this._dateValue.getDate();"> ditto. >+ <property name="open"" onget="return false;" onset="return val;"/> Shouldn't you only declare this in the popup binding? >Index: toolkit/themes/*stripe/global/datetimepicker.css >=================================================================== >+datepicker, timepicker { >+ -moz-appearance: none; nit: already inherited.
Also, I don't think we should fire a change event when the same date is re-selected (as a result of double-click, etc.).
Attachment #246469 - Flags: review?(mano)
Attached patch fix issues (deleted) — Splinter Review
(In reply to comment #66) > Sorry, I'm not following you. It would go in the binding(s) in which you want > hideseconds to work. The same <content> is used for both the date and time pickers > > >>Can't you write oldval = field.value; and field.value = newval; ? > >The displayed value may not be the same as the actual value. > Then constrain the display value perhaps? Not following what you mean here. >>+ <property name="open"" onget="return false;" onset="return val;"/> > Shouldn't you only declare this in the popup binding? No, because then the api would be different for each type. I've fixed all of the other issues.
Attachment #239258 - Attachment is obsolete: true
Attachment #246469 - Attachment is obsolete: true
Attachment #246469 - Flags: superreview?(neil)
Attachment #250852 - Flags: superreview?(neil)
Attachment #250852 - Flags: review?(mano)
(In reply to comment #69) >>>>Can't you write oldval = field.value; and field.value = newval; ? >>>The displayed value may not be the same as the actual value. >>Then constrain the display value perhaps? >Not following what you mean here. I think you're getting the internal month value, updating it, constraining it, then setting it again, adding one, and updating the displayed value. What I thought you could do was to get the displayed month value, update it, constrain it (1-12), set it, subtract one, and update the internal value.
(In reply to comment #69) >I've fixed all of the other issues. I find the "September" picker is still noticably wider than most of the others. Also, when I clicked on one of the weekday headers of a dropdown picker, it seemed to confuse it as it no longer updated when I then clicked a real date.
(In reply to comment #70) >I think you're getting the internal month value, updating it, constraining it, >then setting it again, adding one, and updating the displayed value. Actually I now see you're getting the displayed value if it's been modified, and constraining it, otherwise using the internal month value; then updating it, constraining it, setting the internal month value, adding one, and updating the displayed value. I still feel that it would be easier if up/down manipulated the displayed value. It might not even be necessary to update the internal value immediately because you should subsequently see a change event.
(In reply to comment #61) >(From update of attachment 239258 [details] [diff] [review]) >>+ <body> >>+ </body> ><body/> ? Ah, that was broken until recently...
Comment on attachment 250852 [details] [diff] [review] fix issues Are PageUp/PageDown supposed to change the current date, or just scroll? >+ <method name="_constrainValue"> >+ <parameter name="aField"/> >+ <parameter name="aValue"/> >+ <parameter name="aEntered"/> Should be aNoWrap as above. >+ <row anonid="dayofweekbox"> >+ <label class="datepicker-gridlabel"/> >+ <label class="datepicker-gridlabel"/> >+ <label class="datepicker-gridlabel"/> >+ <label class="datepicker-gridlabel"/> >+ <label class="datepicker-gridlabel"/> >+ <label class="datepicker-gridlabel"/> >+ <label class="datepicker-gridlabel"/> >+ </row> These labels aren't clickable - change the class name perhaps? It would be good if you could do something about September ;-)
Attachment #250852 - Flags: superreview?(neil) → superreview+
Per the toolkit review rules, this kind of code especially needs unit tests before it lands.
Tests for this will be included in the XUL test suite which I'm in the process of figuring out how to incorporate into the 'make check' phase. You can run most of the tests from http://www.xulplanet.com/ndeakin/tests/xts/scripts/datepicker-api.xul and http://www.xulplanet.com/ndeakin/tests/xts/scripts/timepicker-api.xul The remaining tests that don't work are dependent on bug 364508.
(In reply to comment #74) > (From update of attachment 250852 [details] [diff] [review]) > Are PageUp/PageDown supposed to change the current date, or just scroll? > The way it works now for grid datepickers: - cursor keys navigate to different days - page up / page down change the displayed month without affecting the selection. The mouse wheel also cycles through the months in this way. For field datepickers, there are three textboxes which can be typed into, and up and down cursor keys can cycle up and down. The spinbuttons affect the currently focused field. Could use some thoughts on accessibility.
I'll let Alexander Surkov give you more detailed feedback on the accessibility. He just finished making the XForms datepicker accessible. Comment 77 sounds pretty good but aren't there any buttons in the drop down that need to be in the tab order? There must be, because it's not very discoverable to rely on pageup/pagedown. When the drop down is open the tab key should cycle locally between the currently selected day in grid and those buttons.
Yes, the tab order switches to the the grid, the previous month button and the next month button. There isn't any focus outline on the elements though, but that should be a simple css change. The popup form doesn't handle keyboard navigation currently due to issues with popups.
Comment on attachment 250852 [details] [diff] [review] fix issues (In reply to comment #78) > I'll let Alexander Surkov give you more detailed feedback on the accessibility. > He just finished making the XForms datepicker accessible. I didn't test yet the patch. But I'll look closer a bit later. Now I have one comment. >+ <grid class="datepicker-grid"> Grid should have attribute xhtml:role="wairole:grid" where wairole is http://www.w3.org/2005/01/wai-rdf/GUIRoleTaxonomy#. >+ <columns> >+ <column class="datepicker-gridrow" flex="1"/> >+ <column class="datepicker-gridrow" flex="1"/> >+ <column class="datepicker-gridrow" flex="1"/> >+ <column class="datepicker-gridrow" flex="1"/> >+ <column class="datepicker-gridrow" flex="1"/> >+ <column class="datepicker-gridrow" flex="1"/> >+ <column class="datepicker-gridrow" flex="1"/> >+ </columns> >+ <rows anonid="datebox"> >+ <row anonid="dayofweekbox"> >+ <label class="datepicker-gridlabel"/> This label should have attribute xhtml:role="wairole:columnheader". >+ <label class="datepicker-gridlabel"/> >+ <label class="datepicker-gridlabel"/> >+ <label class="datepicker-gridlabel"/> >+ <label class="datepicker-gridlabel"/> >+ <label class="datepicker-gridlabel"/> >+ <label class="datepicker-gridlabel"/> >+ </row> >+ <row> >+ <label class="datepicker-gridlabel"/> This label should have attribute xhtml:role="wairole:gridcell".
(In reply to comment #77) > The way it works now for grid datepickers: > - cursor keys navigate to different days > - page up / page down change the displayed month without affecting the > selection. The mouse wheel also cycles through the months in this way. But focused day should be changed. > For field datepickers, there are three textboxes which can be typed into, and > up and down cursor keys can cycle up and down. The spinbuttons affect the > currently focused field. Should left/right arrow focus previous/next textfield? For example, if caret is in begin of month textfield then left arrow focuses day textfield and move caret to the end of typed value. (In reply to comment #78) > Comment 77 sounds pretty good but aren't there any buttons in the drop down > that need to be in the tab order? There must be, because it's not very > discoverable to rely on pageup/pagedown. When the drop down is open the tab key > should cycle locally between the currently selected day in grid and those > buttons. And tab+shift too I guess. (In reply to comment #79) > The popup form doesn't handle keyboard navigation currently due to issues with > popups. > May you be more detailed?
(In reply to comment #81) > But focused day should be changed. The grid only supports single selection, so the focused value should be the same as the selected value, no? > Should left/right arrow focus previous/next textfield? For example, if caret is > in begin of month textfield then left arrow focuses day textfield and move > caret to the end of typed value. Possibly. The windows date/time changing widget doesn't, but the Mac one does. But this could be a follow up bug. > > The popup form doesn't handle keyboard navigation currently due to issues with > > popups. > May you be more detailed? Because the menu key listeners attached to the popup capture the keyboard events. Whe bug 279703 is fixed, we'll have a way to create popups that aren't menus.
(In reply to comment #82) > (In reply to comment #81) > > But focused day should be changed. > > The grid only supports single selection, so the focused value should be the > same as the selected value, no? Ah, right. I just got accustomed to XForms calendar that makes difference. > > Should left/right arrow focus previous/next textfield? For example, if caret is > > in begin of month textfield then left arrow focuses day textfield and move > > caret to the end of typed value. > > Possibly. The windows date/time changing widget doesn't, but the Mac one does. > But this could be a follow up bug. I'd like to have a follow up bug.
Comment on attachment 250852 [details] [diff] [review] fix issues r=mano, sorry for latency. Please add a license header to datetimepicker.xml.
Attachment #250852 - Flags: review?(mano) → review+
> It would be good if you could do something about September ;-) Does it work if the min-width for the datepicker-monthbox class is increased?
(In reply to comment #85) >>It would be good if you could do something about September ;-) >Does it work if the min-width for the datepicker-monthbox class is increased? It works in English... not sure about other languages :-/
alexander, can you look at the accessibility attributes here? Neil, do you think I should add an entity for the width here?
(In reply to comment #87) >Neil, do you think I should add an entity for the width here? Well, you'd need two entities, because the Mac defaults to bold font face so that it needs everything about 10% wider. The only other suggestion I have is to have separate month labels in a deck.
Attached patch I like the deck idea (deleted) — Splinter Review
I removed the min-width lines in the css, but maybe I should leave them in?
Attachment #252609 - Attachment is obsolete: true
Attachment #252790 - Flags: review?(neil)
(In reply to comment #87) > Created an attachment (id=252609) [details] > fix issues described above and add accessibility attributes > > alexander, can you look at the accessibility attributes here? It looks fine excepting you should declare wairole namespace inside your binding. If you declare it on bindings element and bound document doesn't declare it then wairole prefix isn't recognized.
Comment on attachment 252790 [details] [diff] [review] I like the deck idea Very neat :-)
Attachment #252790 - Flags: review?(neil) → review+
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Keywords: dev-doc-needed
Flags: in-testsuite?
tests added by bug 386772
Flags: in-testsuite? → in-testsuite+
Tagging as documentation complete.
sheppy: could you link to the created documents when marking a bug as dev-doc-complete? It helps those of us who're using bugzilla as a documentation source :) In this case it's the following pages, I suppose: http://developer.mozilla.org/en/docs/Firefox_3#New_elements http://developer.mozilla.org/en/docs/XUL:datepicker http://developer.mozilla.org/en/docs/XUL:timepicker
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: