Closed
Bug 260120
Opened 20 years ago
Closed 18 years ago
improvements to datepicker and timepicker
Categories
(Calendar :: General, defect)
Calendar
General
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: mvl, Unassigned)
References
Details
Attachments
(2 files, 6 obsolete files)
|
113.16 KB,
patch
|
vlad
:
first-review+
|
Details | Diff | Splinter Review |
|
2.79 KB,
patch
|
Details | Diff | Splinter Review |
datepicker (and timepicker) should not use mDisabled, but just the disabled attribute Instead of checking for keycode 13, they could use the destructor to check the input.
|
||
Comment 1•19 years ago
|
||
but what used to work appears to work now but with no dependences on dateUtils, no localization support needed, no "evals", disabled has been cleaned up, and quite a bit of other little things thrown in here and there as well as standardize the naming and spacing conventions. it moved over to toolkit pretty smoothly too and worked in unprivileged xul code over there.
|
|
||
Comment 2•19 years ago
|
||
you need shaver's toLocaleFormat patch for the attached patch to work.
Depends on: 291494
This patch looks similar to bug 92174 comment 40 and afterward, though I see the css files have been consolidated. I assume the toLocalFormat patch you mentioned is from bug 291494 ? Looks like date format is localized but date parsing is not localized, so the date parser won't work in most locales. preventBubble method seems superfluous. (I think this bug was meant as a bug for miscellaneous tiny fixes, not major patches.) (This patch addresses several issues. Was the intent just to save a checkpoint "in preparation", or to generate comment and review? Generally, incremental bugs and patches are preferred, so it is easier to test them and later back out individual changes if it turns out one of the changes introduces a bug somewhere else.)
|
|
||
Comment 4•19 years ago
|
||
First, yes, this is similar to the work that I did for bug 92174, however because of the amount of cleanup that was required to get it "toolkit-ified", it was decided to first apply those changes to /calendar so that when we submit the final patch for 92174 it's a straight copy from what we have under /calendar. However my patch for 92174 was designed to be as "minimalistic" as possible and still be functional, so some clean up did not take place in that patch that we really needed to do (in particular to get it to work in remote xul) Also, yes.. the toLocaleFormat patch I mentioned is from bug 291494. This allows us to remove the dependency on nsIDateFormatter and the dateFormat.properties so it'll work in unprivileged code. I don't see why parsing is any more or less locale specific than it was before. My understanding of the code pulled from dateUtils.js into datepicker is that it's flexible enough to handle pretty much any format you throw at it. I can always change it out for a javascript Date.parse if we prefer not to maintain that parsing code, but we will lose flexibility. Other suggestions are welcome as well. preventBubble, I thought, was to address issues with our popup in a popup. I'll differ to others as to whether it can be dropped. mvl? as to if this is the right place for this patch, I verified it with mvl before submitting it.. Normally I would agree, smaller patches would be better in some ways, and if someone has suggestions for how to do that, I'm all ears. However, between all the formatting issues, file changes, etc.. I don't know how to get that done easily and quickly... and we have a very small window to get this into 92174 for 1.8b2 so time is of the essence. My intention is to verify that this is pretty close to what we want to move to toolkit. For example, that I've tackled all the things we wanted to accomplish before it moves to toolkit and that I didn't cause any major regressions. When we think we have a patch that does those things, I assume this will be reviewed/committed as one big patch just like I applied here and we'll then submit a very "small" patch to move these files to /toolkit and fix up the appropriate jar's to deploy it to both toolkit and xpfe. So yes, comments/suggestions welcome as to whether this fits the criteria listed above.. this certainly isn't a speak now or forever hold your peace as enhancements can still be made to this once it hits toolkit. If I misunderstood my marching orders (mostly from shaver), I apologize.. if there's something specific I can do to make life easier, please let me know. Thanks! -Andrew
This method seems superflouous: why not call preventBubble directly on the event instead of calling this method? (Maybe it was for testing, so you could turn off all the preventBubble's at once?) + <method name="preventBubble"> + <parameter name="anEvent"/> + <body> + <![CDATA[ + // avoid calling popup handlers of higher popups in DOM (datepicker) + anEvent.preventBubble(); + ]]> + </body> + </method>
This patch uses Date.parse + <method name="parseTextBoxDate"> + <parameter name="aRefresh"/> + <body> + <
|
||
Comment 10•19 years ago
|
||
Files need directory names. Several datetimepickers.css are defined only as --- /dev/null 2005-04-29 14:10:13.116000000 -0400 +++ datetimepickers.css 2005-04-29 13:50:39.558739200 -0400 So the later versions overwrite the earlier ones in the patch, and the patch does not put any of them in the correct place for testing.
|
|
||
Comment 11•19 years ago
|
||
Sorry. This is the same file but with the paths added. I justed tested this against a clean tree. It should resolve the issue.
Attachment #182327 -
Attachment is obsolete: true
Attachment #182338 -
Flags: second-review?(mvl)
Attachment #182338 -
Flags: first-review?(gekacheka)
|
|
||
Comment 12•19 years ago
|
||
A solution that continues to use the Mozilla ScriptableDateFormat
service would be better for users.
* The patch with toLocaleFormat uses strftime "%x" for date, which may
produce long format dates in many locales.
strftime just enables two formats: %x and an optional alternate %Ex.
It does not specify if one format is long or short.
Before Calendar was updated to use the scriptable date format service,
this was a source of many complaints from windows users, along the
lines of "it should just use my operating system settings like other
programs".
On windows, the user has two customizable settings for date, a long
format and short format. For the date picker, the current code uses
the short format so it takes up little space (no long spelled out
words to type), typically
"4/3/05" in the US,
"3/4/05" in many european countries, and
"2005-04-03" (ISO) in some other places,
but it is user-settable independent of locale. (The long format
typically spells out the month name and the weekday, so it is
typically much larger than fits in a small field, such as
"Wednesday, September 29, 2005".)
Mozilla ScriptableDateFormat provides dateFormatShort that the
current code uses. It makes clear that a short format
date is intended.
* The patch hardcodes a time format. Many locales use 24 hour time,
and time formats often have no spaces.
+ <method name="formatTime">
+ <parameter name="aValue"/>
+ <body>
+ <![CDATA[
+ // get H:M AM/PM - we need to strip the leading 0 out of %I
+ return aValue.toLocaleFormat("%I:%M %p").replace(/^0/, "");
Instead it should use the operating system time format. The
current code does so via the ScriptableDateFormat service.
|
|
||
Comment 13•19 years ago
|
||
Issues with ScriptableDateFormat: The reason we moved away from scriptabledateformat for this patch is because of issues with using it from unprivileged code. This is, of course, not an issue with calendar specifically, however in toolkit (and in particular if this code is ever used for XForms/WebForms 2), it's a big deal. Formatting Date: I understand the concern about what would happen on a locale that used a long date format as their preference, however I don't think we should ignore that setting either. Also note: unix uses the same code inside scriptabledateformat for short dates. See http://lxr.mozilla.org/seamonkey/source/intl/locale/src/unix/nsDateTimeFormatUnix.cpp#202 Formatting Time: on unix, strftime is used by the scriptabledateformat to generate the locale specific time using strftime in a similar fashion to what I did. See http://lxr.mozilla.org/seamonkey/source/intl/locale/src/unix/nsDateTimeFormatUnix.cpp#226 The only difference is that it first checks locale for 24 hour and if AM/PM should be first. Would an acceptable solution be to "probe" the locale format using toLocaleFormat("%X") to determine if 24H notation should be used and the position of AM/PM so that the output matches scriptabledateformat on unix? if that sounds like a plan, let me know and I'll implement it shortly. If there's anything else that needs to be updated while I'm at it, just let me know. Thanks!
|
|
||
Comment 14•19 years ago
|
||
Comment on attachment 182338 [details] [diff] [review] same as before but with paths for css files Formatting time: Probe idea sounds ok. Suggest that it also omit space in locales that do not use spaces. Formatting date: (FYI: A long date format is not acceptable to the date parser in most locales. Long dates often spell out month names, but the date parser specifically needs a short numerical date format so it can parse it in locales other than english (for example bug 257146 reports problems in vietnamese). It only accepts months in digits (global) or in ASCII letters (english) because Java RegExp currently does not support character classes for unicode letters, bug 258974. So may need to probe the date format and see if it is parsable, and if not, substitute a numerical date format. But it doesn't do this yet, so I don't expect you to add it.) (I don't understand why using the date format service requires privileges.) (Even if it does, I don't understand why a date field widget cannot access privileged services. Other widgets such as the file upload/download widget access privileged services to browse the filesystem. XForms is currently supplied as an extension, but Calendar is also supplied as an extension.) Other issues: * skin/datetimepickers/datetimepickers.css doesn't seem to be loaded. (time picker grids have no shading or borders) 1. problem seems to be content/datetimepickers/datetimepickers.css needs to import skin/datetimepickers/datetimepickers.css not skin/datetimepickers/datepicker.css 2. the <resources> <stylesheet src="skin/datetimepickers.css"> </resources> in timepicker-grids, datetimepickers-base, and minimonth seem have no effect, so maybe they should be removed since they are loaded from the content css. * Click date now takes an event reference to minimonth was removed, so a comment would be helpful, such as // aEvent.target is minimonth datepicker.update(new Date(aEvent.target.value), true); Alternatively, rename aEvent to aMinimonthClickEvent * the documentation could be restored for - Requires: <?xml-stylesheet type="text/css" href="chrome://calendar/content/datetimepickers/datetimepickers.css" ?> - May also dis/enable a datetimepicker's component datepicker or timepicker individually with document.getElementById("my-datetimepicker").datepickerdisabled = true; document.getElementById("my-datetimepicker").timepickerdisabled = true; * ".parsedTime" is a misnomer, it does not hold the parsed time. Suggest: ".lastDateParseIncludedTime" as a more accurate name. Maybe drop setter. * parseDateTime: suggest moving comment so it isn't after start of method. + <method name="parseDateTime"> + <parameter name="aValue"/> + <body> + <
|
||
Comment 16•19 years ago
|
||
(In reply to comment #14) > * Minor: In timepicker, « and » for "more" and "less" were a bit > mysterious, particularly since they don't point the same direction picker > expands/contracts. Maybe ∧ (^) and ∨ (v) would be better > since they point in vertical directions. (Δ and ∇ are > also possible, though not as symmetrical in some fonts.) The « and » symbols are fairly commonly used in UIs to indicate "expand " and "collapse", and also to indicate "see more options" (think of "advanced" controls in MS apps, for instance). I think that part of the problem is that they appear aligned to the bottom-left. It probably works better with them at the bottom-right: 1 2 3 4 5 6 7 8 9 10 11 12 :00 :15 :45 :55 >>
|
Reporter | |
Updated•19 years ago
|
Attachment #182327 -
Flags: second-review?(mvl)
Attachment #182327 -
Flags: first-review?(gekacheka)
|
|
||
Comment 17•19 years ago
|
||
This should address all the outstanding issues (unless already addressed in the bug comments). I'm a bit too tired to try to work out the diff's for the individual files. I'll try to get to that soon if you all need it. The only thing that's different than planned is that I decided I didn't like trying to check for ScriptableDateFormat because of the inconsistency issues mentioned. If we find that this is error prone and needs improving.. I'd rather improve it than hack in scriptabledate only when it's available. Thanks!
|
|
||
Updated•19 years ago
|
Attachment #182328 -
Attachment is obsolete: true
Attachment #182338 -
Attachment is obsolete: true
Attachment #182658 -
Flags: second-review?(mvl)
Attachment #182658 -
Flags: first-review?(gekacheka)
|
|
||
Updated•19 years ago
|
Attachment #182338 -
Flags: second-review?(mvl)
Attachment #182338 -
Flags: first-review?(gekacheka)
|
|
||
Comment 18•19 years ago
|
||
TimeFormat:
Use initTimeFormat probe code, remove probeDisplayFormat. It isn't as
cross-cultural (e.g., no 'M' in many cultures that don't use latin alphabet).
initTimeFormat should probe system format based on the new source of locale
time format, toLocaleFormat("%X"), not based on formatTime.
var amProbeString = amProbeTime.toLocaleString("%X");
...
var pmProbeString = pmProbeTime.toLocaleString("%X");
initTimeFormat should set an output format string for formatTime to use, so
formatTime doesn't have to recompute format each time. Add something like this
at the end of initTimeFormat:
var ampmSep = (pmProbeString.indexOf(' ') != -1? " " : "";
this.kTimeFormatString =
(this.ampmIndex == PRE_INDEX? "%p" + ampmSep + "%I:%M" :
this.ampmIndex == POST_INDEX? "%I:%M" + ampmSep + "%p" :
"%H:%M");
The formatTime method can be something like below to replace leading hour
zero even if preceded by ampm.
// remove leading zero in hours before colon in xx01:23xx
return aValue.toLocaleFormat(this.kTimeFormatString).replace(/0(\d):/,"$1:");
CSS:
Note: Calendar uses cyan/blue colors, but toolkit widgets default theme is
gray-based or O.S. preference based (e.g., selection color), so color might need
to be removed or change to gray when moved to toolkit so it matches other widgets.
I'd prefer to see this land ASAP and remaining localization/style issues worked out post-landing (assuming it doesn't break any current code), as this is a pretty big patch already -- I'm guessing due to the renaming of files, but still.
|
|
||
Comment 20•19 years ago
|
||
I agree we need to get it landed. I appreciate gekacheka's suggestion enough though that I went ahead and applied his code. The only "issue", and the only reason why I didn't do it that way initially was because it threw off my initialization a bit. But I like what I've got now, because of the slight reorg, a whole lot better than what I had before. Sorry for the delays in getting the suggestions into the patch. Thanks! -Andrew
Attachment #182658 -
Attachment is obsolete: true
Attachment #182774 -
Flags: second-review?(mvl)
Attachment #182774 -
Flags: first-review?(gekacheka)
|
|
||
Updated•19 years ago
|
Attachment #182658 -
Flags: second-review?(mvl)
Attachment #182658 -
Flags: first-review?(gekacheka)
|
|
||
Comment 21•19 years ago
|
||
Comment on attachment 182774 [details] [diff] [review] probe done in init 1. date picker doesn't work for me in calendar, it doesn't popup, and there is an error that this.kMinimonth.update is not defined. Problem seems to be that <minimonth/> is not expanded with its xbl definition. Looks like minimonth is not defined. It works if I add the following to the top of content/datetimepickers/datetimepickers.css: @import url("chrome://calendar/content/datetimepickers/minimonth.css"); 2. This version checks for initialization to the beginning of the parse and format methods. The previous version initialized in the constructor. The reason for this approach should be documented. (Previous version failed because <constructor> does not take a <body>, just put the code directly in <constructor>.) 3. Please add comment explaining need for kTimeFormatString, such as + + // build time display format that mimics "%x" format without seconds var ampmSep = (pmProbeString.indexOf(' ') != -1? " " : ""); 4. Please add comment explaining need to modify custom formatted time string: + // remove leading zero in hours before colon in xx01:23pmxx return aValue.toLocaleFormat(this.kTimeFormatString).replace(/0(\d):/,"$1:");
|
|
||
Comment 22•19 years ago
|
||
So close! Thanks!
|
|
||
Updated•19 years ago
|
Attachment #182774 -
Attachment is obsolete: true
Attachment #182785 -
Flags: second-review?(mvl)
Attachment #182785 -
Flags: first-review?(gekacheka)
|
|
||
Updated•19 years ago
|
Attachment #182774 -
Flags: second-review?(mvl)
Attachment #182774 -
Flags: first-review?(gekacheka)
|
|
||
Comment 23•19 years ago
|
||
(patch -l -p 2 -i file.patch) Patch newpickersall4a.patch added comment + Datetimepicker-base internally initializes + date/time formats so that descendents are not required to initialize the + formats they need in their constructor. So the if to call initDateFormat and initTimeFormat from the methods are there because the inits in the datetimepicker-base constructor weren't working? As mentioned earlier, that constructor had a <body> that shouldn't be there. In patch newpickersall3.patch, the formats were initialized in the datetimepickers-base constructor, which is simpler than the current code. If that constructor is fixed (no <body>, put code directly in <constructor>) then it appears there is no reason to call them from the subclass constructors, and no reason sprinkle the "if ... .initDateFormat()" and "if ... .initTimeFormat()" calls anywhere else. Property lastDateParseIncludedTime onget= doesn't need the if either. Suggest simplify as follows: * remove above comment * put initDateFormat and initTimeFormat calls in datetimepicker-base constructor (as in patch newpickersall3.patch, but without <body> tags) * remove "if ... .initDateFormat()" from parseDate * remove "if ... .initTimeFormat()" from parseTime * remove "if ... .initTimeFormat()" from formatTime * remove "if ... = false" from lastDateParseIncludedTime.onget
|
|
||
Comment 24•19 years ago
|
||
The reason the init was broken up between date and time was so a control that wanted to handle just one or the other didn't have to waste cycles init'ing and probing when it wasn't necessary. I realize the <body> tag was an issue, but I didn't really want to be calling init there anyway. A couple of if's is a lot less overhead the having to call init unnecessarily. I honestly don't have a strong preference but can we accept it as is?
Comment on attachment 182785 [details] [diff] [review] comments added and issue with css import addressed r=vladimir for checkin (with additional followup patch applied), please file followup bugs for any remaining problems
Attachment #182785 -
Flags: first-review?(gekacheka) → first-review+
|
|
Reporter | |
Updated•19 years ago
|
Attachment #182785 -
Flags: second-review?(mvl)
|
||
Updated•19 years ago
|
QA Contact: gurganbl → general
|
|
Reporter | |
Updated•18 years ago
|
Assignee: mvl → nobody
|
||
Comment 26•18 years ago
|
||
(In reply to comment #25) > (From update of attachment 182785 [details] [diff] [review] [edit]) > r=vladimir for checkin (with additional followup patch applied), please file > followup bugs for any remaining problems > This should have been closed a long time ago. :-)
Status: NEW → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•