Closed Bug 260120 Opened 19 years ago Closed 17 years ago

improvements to datepicker and timepicker

Categories

(Calendar :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mvl, Unassigned)

References

Details

Attachments

(2 files, 6 obsolete files)

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.
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.
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.)
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>
+          <![CDATA[
+            var parsed = Date.parse(this.kTextBox.value)

Date parse appears to be defined here:
http://lxr.mozilla.org/seamonkey/source/js/src/jsdate.c#741
It calls date_parseString, defined here:
http://lxr.mozilla.org/seamonkey/source/js/src/jsdate.c#552
It is not localized, for example it looks for only the english strings defined here
http://lxr.mozilla.org/seamonkey/source/js/src/jsdate.c#440

The current code uses DateFormater (french spelling?)
http://lxr.mozilla.org/seamonkey/source/calendar/resources/content/datetimepickers/datepicker.xbl#230

The DateFormater constructor probes the current date format to find the order of
year month day and stores indexes.
http://lxr.mozilla.org/seamonkey/source/calendar/resources/content/dateUtils.js#156

DateFormater.parseShortDate uses the stored indexes to parse the date.
http://lxr.mozilla.org/seamonkey/source/calendar/resources/content/dateUtils.js#467

Does that help?
When is the estimated deadline?
Attached patch fixes as requested (obsolete) — Splinter Review
gekacheka - thanks for clarifying.. and sorry for being a bit dense. I've fixed
the date parsing issue (using dateUtil's code) and made the change for
preventBubble. I also did a bit of refactoring to move the date/time
parsing/formatting and common functionality into a datetimepicker-base. I
unfortunately don't have a specific time frame for 1.8b2 yet.
Attachment #182190 - Attachment is obsolete: true
Attachment #182327 - Flags: second-review?(mvl)
Attachment #182327 - Flags: first-review?(gekacheka)
Attached patch some (hopefully) helpful diffs (obsolete) — Splinter Review
hopefully this will help with review - timepicker in particular was heavily
modified, so I'm not sure how much good it will do, but I know this is a big
patch  and it's hard to grasp what all changed. if there's anything else I can
do to help make it easier, please let me know. Thanks!
Attachment #182328 - Attachment is patch: true
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.
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)
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.
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 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>
  +	     <![CDATA[
  +	       this.mParsedTime = false;
  +	       /* Parameter aValue may be a date or a date time. Dates are
  +		  read according to locale/OS setting (d-m-y or m-d-y or ...). 
  +		  (see initDateFormat). See parseTime for times. */

* strict javascript warning, looks like this.mParsedTime needs to be
  initialized in the constructor
    Warning: reference to undefined property this.mParsedTime
    Source File: chrome://calendar/content/datetimepickers/datetimepickers.xml
    Line: 0

* html looks inappropriate in datetimepickers.xml
  +<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" 
  +	     "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
  +

* Minor: In timepicker, &laquo; and &raquo; for "more" and "less" were a bit
  mysterious, particularly since they don't point the same direction picker
  expands/contracts.  Maybe &and; (^) and &or; (v) would be better
  since they point in vertical directions.  (&Delta; and &nabla; are
  also possible, though not as symmetrical in some fonts.)

* suggest fix quoting so it doesn't mess up string coloring in emacs
  -	      // '12H34M56Spm" for any characters H,M,S and any language's
"pm".
  +	      // "12H34M56Spm" for any characters H,M,S and any language's
"pm".

Thanks for the diff, it was helpful.   The timebox shortening is nicer.  Thanks
for the event cleanup.
(In reply to comment #14)
> Probe idea sounds ok.  Suggest that it also omit space in locales that do
> not use spaces.
I'll see what I can do on the spaces. If it's a common thing, we may want to
submit a bug on scriptabledateformat

> (I don't understand why using the date format service requires privileges.)
I can probe to see if the scriptabledateformat is available and use it instead
of toLocaleFormat if it is, but that means we could end up with different
behavior between privileged and unprivileged code on some platforms. It
shouldn't be much overhead to the code.

>  XForms is currently supplied as an extension,
>  but Calendar is also supplied as an extension.)
XForms itself isn't the issue, it's the xforms documents that are unprivileged.

> Other issues:
> 
> * skin/datetimepickers/datetimepickers.css doesn't seem to be loaded.
>   (time picker grids have no shading or borders)
I'll clean this up. <resource.. is the standard in /toolkit, so I'd prefer that
be the only thing needed. It looks like it's just a path problem. 

> * the documentation could be restored for
>   - Requires:
>       <?xml-stylesheet type="text/css"
> href="chrome://calendar/content/datetimepickers/datetimepickers.css" ?>  
I really didn't want this line. It'll only be required until it moves to toolkit
as then it will be "defined" in xul.css. And I didn't want to have to deal with
removing it when it went to toolkit.

> * html looks inappropriate in datetimepickers.xml
>   +<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" 
>   +	     "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
>   +
This is needed for the &laquo; and &raquo; entities. Are there alternatives?

> * Minor: In timepicker, &laquo; and &raquo; for "more" and "less" were a bit
>   mysterious, particularly since they don't point the same direction picker
>   expands/contracts.  Maybe &and; (^) and &or; (v) would be better
>   since they point in vertical directions.  (&Delta; and &nabla; are
>   also possible, though not as symmetrical in some fonts.)
I think I'll check with our resident UI experts on IRC and go by their decision.
I have no preference personally - just didn't want to have to deal with
localizing less/more.
 
> Thanks for the diff, it was helpful.   The timebox shortening is nicer.  
> Thanks for the event cleanup.
No problem. I'll get these changes out shortly. Thanks!
-Andrew

(In reply to comment #14)
> * Minor: In timepicker, &laquo; and &raquo; for "more" and "less" were a bit
>   mysterious, particularly since they don't point the same direction picker
>   expands/contracts.  Maybe &and; (^) and &or; (v) would be better
>   since they point in vertical directions.  (&Delta; and &nabla; are
>   also possible, though not as symmetrical in some fonts.)

The &laquo; and &raquo 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
                   >>
Attachment #182327 - Flags: second-review?(mvl)
Attachment #182327 - Flags: first-review?(gekacheka)
Attached patch with changes requested (obsolete) — Splinter Review
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!
Attachment #182328 - Attachment is obsolete: true
Attachment #182338 - Attachment is obsolete: true
Attachment #182658 - Flags: second-review?(mvl)
Attachment #182658 - Flags: first-review?(gekacheka)
Attachment #182338 - Flags: second-review?(mvl)
Attachment #182338 - Flags: first-review?(gekacheka)
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.
Attached patch probe done in init (obsolete) — Splinter Review
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)
Attachment #182658 - Flags: second-review?(mvl)
Attachment #182658 - Flags: first-review?(gekacheka)
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:");
Attachment #182774 - Attachment is obsolete: true
Attachment #182785 - Flags: second-review?(mvl)
Attachment #182785 - Flags: first-review?(gekacheka)
Attachment #182774 - Flags: second-review?(mvl)
Attachment #182774 - Flags: first-review?(gekacheka)
(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
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+
Attachment #182785 - Flags: second-review?(mvl)
QA Contact: gurganbl → general
Assignee: mvl → nobody
(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: 17 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.