Closed Bug 378172 Opened 17 years ago Closed 17 years ago

[Proto] Recurrence dialog: datepickers need a facelift

Categories

(Calendar :: General, defect)

Lightning 0.3.1
defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: berend.cornelius09, Assigned: berend.cornelius09)

Details

Attachments

(6 files, 8 obsolete files)

472 bytes, image/png
Details
472 bytes, image/png
Details
315 bytes, image/png
Details
839 bytes, application/zip
Details
3.90 KB, application/octet-stream
Details
44.43 KB, patch
Details | Diff | Splinter Review
the weekday and monthpickers in the recurrence dialog need a facelift. Chris Jansen provided me some imagelists to be used to emphasize the different states of a datepicker. By means of these images five different states can be provided:
- Normal
- MouseOver
- MouseDown
- Selected 
The state "disabled" can be achieved by setting the textcolor to geytext and by setting the dialogcolor with 50% transparency and by reducing the border transparency to 90%
Summary: Recurrence dialog: datepickers need a facelift → [Proto] Recurrence dialog: datepickers need a facelift
I tried to resolve the issue but came to a point, where I got stuck...
This is the current state of the patch that I attach:
I basically resolved the problem by setting up xul-stack elements for the datepickers with a textbox at the top and an image at the bottom. For some reason the rules in the css file "sun-calendar-event-dialog-recurrence-datepicker.css" are not applied to the image in the dialog's month view (in the week view it works fine). 
The patch that reflects the current state with a remaining unresolved problem about not applied .css rules for the image box in the datepickers
Attachment #262244 - Attachment is obsolete: true
Berend, please keep binary files and patches separate, since otherwise it may be possible that others won't be able to apply your patch. Furthermore, it screws up the diff view for a patch. So, please attach all binaries in an archive format of your choice and the patch as a separate file.
First of all, it would be great to have all the binary files in a single archive that already contains the files at the location in the tree where they are supposed to go. Otherwise one needs to first find out in the patch (by inspecting the jar.mn) where those files should go. Second, those images most probably want to go to m/c/base/themes/common. Third, is there any reason why the first image has been submitted twice?

And last but not least, is there any reason for having these images separately for the different controls? Wouldn't it be better to just use one image for both controls? This would have the additional benefit to prevent the order being mixed up (as it is already the case with those two images). Download size isn't the reason why I bring this up here, but css-rules get more complex and grow unnecessarily.
I modified the source code in such a way that only one image list is needed that is scaled down to the desired size.
Attachment #263254 - Attachment is obsolete: true
Attachment #265803 - Flags: review?
Attachment #265803 - Flags: review? → review?(michael.buettner)
the archive contains the new imagelist for the datepickers and only needs to be extracted to the calendar folder.
Comment on attachment 265803 [details] [diff] [review]
The new patch with only one image file

I'd prefer to keep the binary files and the patch separate, if you don't mind.

>--- mozilla_ref/calendar/prototypes/wcap/sun-calendar-event-dialog-recurrence-datepicker.css
>+++ mozilla/calendar/prototypes/wcap/sun-calendar-event-dialog-recurrence-datepicker.css
Regarding this file I have several comments in general. First, you introduce rules related to the 'stack-datebox'-class. I think it would be a good idea to name all datepicker related elements according to a common naming schema, like 'datepicker-daybox' or something like that. Having 'stack' in the name unnecessarily creates a relationship to the implementation. Second, please adhere to the style all other *.css files in the project use.

- *one* blank link between rules
- blank after name and before the opening brace
- two blanks in front of each css-statement
- colon directly following the name

please see calendar-views.css or any other *.css file as a reference.

>+.stack-datebox[bottomborder="true"]{
>+    border-bottom    : 1px solid ThreeDShadow;
>+}
>+
>+
>+.stack-datebox[rightborder="true"]{
>+    border-right      : 1px solid ThreeDShadow;
> }
these elements are arranged in a grid, that's why you selectively want to add borders. but wouldn't it be better to add attributes such as 'top', bottom', 'left', right' to the elements so that themes can freely decide which styles they want to add instead of suggesting the border style? that's just an idea while i was reading this file.

>+  list-style-image  : url("chrome://calendar/skin/datepicker-background.png");
This statement to repeated 4 times, just the first one is really necessary. All others are redundant.

>+  <binding id="datepicker-box" extends="xul:box">
>+    <content>
>+      <xul:stack anonid="stack-box" class="stack-datebox" 
As I mentioned above, I'd suggest to name this 'datepicker-box' and the binding itself 'datepicker'. I'm open to other suggestions, but 'stack-box' isn't a good name.

xbl:inherits="disabled,othermonth,mode,bottomborder,rightborder">
>+        <xul:image anonid="image-box" class="image-box"/>
>+        <xul:box class="container-box" orient="vertical" align="center" pack="center">
>+            <xul:text anonid="text-box" class="text-box" xbl:inherits="value"/>
Please stick to 2 blanks instead of switching between 2 and 4 blanks, this applies to all changes made to this file.

>+      <method name="BooleanPropertyToAttribute">
>+      	<parameter name="elementID"/>      
>+      	<parameter name="attributeName"/>      
>+      	<parameter name="val"/>      
wrong indentation.

>+          <body>
>+            <![CDATA[		   
>+		 var element = document.getAnonymousElementByAttribute(this, "anonid", elementID);
>+		 if (val) {
>+		     element.setAttribute(attributeName, "true");
>+		 } else {
>+		     element.removeAttribute(attributeName);
>+	     }
>+	     return val;
>+          ]]>
>+        </body>
wrong indentation. please align the cdata-tags and the body-tags properly.

>+      <method name="AttributeToBooleanProperty">
>+      	<parameter name="elementID"/>      
>+      	<parameter name="attributeName"/>      
>+          <body>
>+            <![CDATA[		   
>+       	    var elementbox = document.getAnonymousElementByAttribute(this, "anonid", elementID);
>+            return (elementbox.hasAttribute(attributeName));
please align this properly.

>+          ]]>
>+        </body>
>+      </method>
>+        <constructor>
>+          <![CDATA[
>+             var oStackBox = document.getAnonymousElementByAttribute(this, "anonid","stack-box");
>+             var oImageBox = document.getAnonymousElementByAttribute(this, "anonid", "image-box");
>+             oImageBox.setAttribute("style",
>+                                     "height: " +
>+                                   (oStackBox.boxObject.height) + "px;width: " +
>+                                   (oStackBox.boxObject.width) + "px");
'o' is not a commonly used prefix, would you mind naming these elements like 'stackbox' and 'imagebox'?

regarding BooleanPropertyToAttribute() and AttributeToBooleanProperty(), I'd prefer to move these methods to calUtils.js or calendar-dialog-utils.js. Propbably the latter is the better choice, but that's up to you. The reason is, that a binding is not the appropriate place for utility functions. If each and every binding adds utility functions we end up with a whole bunch of methods that do exactly the same while having bindings that are larger than necessary. My personal opinion is that such a small binding doesn't need to this at all. It would suffice to just add the code in a verbose fashion. It may be worth while for larger ones, though.

>   <binding id="datepicker-weekday" extends="xul:box">
...
>+        <xul:hbox anonid="mainbox" flex="1">
>+                <datepicker-box bottomborder="true" xbl:inherits="disabled,mode"/> 
>+                <datepicker-box bottomborder="true" xbl:inherits="disabled,mode"/>
>+                <datepicker-box bottomborder="true" xbl:inherits="disabled,mode"/>
>+                <datepicker-box bottomborder="true" xbl:inherits="disabled,mode"/>
>+                <datepicker-box bottomborder="true" xbl:inherits="disabled,mode"/>
>+                <datepicker-box bottomborder="true" xbl:inherits="disabled,mode"/>
>+                <datepicker-box bottomborder="true" rightborder="true" xbl:inherits="disabled,mode"/>
Your 'mode'-attribute lets the *.css rules decide about what kind of datepicker they apply to, right. But there's no need to instantiate a 'datepicker-weekday' xul-element with the additional need to add a 'mode=week' attribute. As long as we're in the datepicker-weekday binding it's crystal clear that the mode is 'week'. So there's no need to state xbl:inherits=mode.

>-              var index = val[i]-1-this.weekStartOffset;
>+              var index = val[i]-this.weekStartOffset;
Presumably this fixes some bug, could you please explain why you did this?

>+            <xul:vbox anonid="mainbox" class="datepicker-monthday-mainbox" flex="1" >
>+                <xul:hbox class="datepicker-row" flex="1">
>+                    <datepicker-box value="1" xbl:inherits="disabled,mode"/>
>+                    <datepicker-box value="2" xbl:inherits="disabled,mode"/>
Same comment as above regarding the mode attribute.

Since there are a ton of style nits (most of them in the css-file), the boolean-attribute methods that need to be moved and the mode-attribute (which appear to be most important), marking r-.
Attachment #265803 - Flags: review?(michael.buettner) → review-
(In reply to your comment)
>     <xul:stack anonid="stack-box" class="stack-datebox" 
> As I mentioned above, I'd suggest to name this 'datepicker-box' and the binding
> itself 'datepicker'. I'm open to other suggestions, but 'stack-box' isn't a
> good name.
As I named all boxes within the datepicker following their boxtype or their purpose ("image-box", "container-box", "text-box") I would like to keep this convention. I prefer self-explaning names and 'datepicker-box' does not give me a hint about the capabilities of this element.
Attached patch The newest patch (obsolete) — Splinter Review
(in reply to comment #11)
>>-              var index = val[i]-1-this.weekStartOffset;
>>+              var index = val[i]-this.weekStartOffset;
> Presumably this fixes some bug, could you please explain why you did this?

I removed this code out of the patch as it is part of a fix for issue #383272.


>>   <binding id="datepicker-weekday" extends="xul:box">
>...
>>+        <xul:hbox anonid="mainbox" flex="1">
>>+                <datepicker-box bottomborder="true" >xbl:inherits="disabled,mode"/> 
>>+                <datepicker-box bottomborder="true" >xbl:inherits="disabled,mode"/>
>>+                <datepicker-box bottomborder="true" >xbl:inherits="disabled,mode"/>
>>+                <datepicker-box bottomborder="true" >xbl:inherits="disabled,mode"/>
>>+                <datepicker-box bottomborder="true" >xbl:inherits="disabled,mode"/>
>>+                <datepicker-box bottomborder="true" >xbl:inherits="disabled,mode"/>
>>+                <datepicker-box bottomborder="true" rightborder="true" >xbl:inherits="disabled,mode"/>
>Your 'mode'-attribute lets the *.css rules decide about what kind of datepicker
>they apply to, right. But there's no need to instantiate a 'datepicker-weekday'
>xul-element with the additional need to add a 'mode=week' attribute. As long as
>we're in the datepicker-weekday binding it's crystal clear that the mode is
>'week'. So there's no need to state xbl:inherits=mode.

I changed the inheritance now that 'mode' inherits from 'id'. Attribute inheritance offered the advantage that I only had to define the attribute once instead of for each child element. 

>As I mentioned above, I'd suggest to name this 'datepicker-box' and the binding
>itself 'datepicker'. I'm open to other suggestions, but 'stack-box' isn't a
>good name.
The binding 'datepicker' is already in use and defined in 'datetimepickers.xml'. As "my" datepicker extend a xul:box but besides have a similar functionality I find the name appropriate.
Attachment #265803 - Attachment is obsolete: true
Attachment #267258 - Flags: review?(michael.buettner)
Status: NEW → ASSIGNED
OS: Solaris → All
Hardware: Sun → All
Attached file test harness (obsolete) —
This archive contains a small test harness that shows how to construct a button with an arbitrary background image based on a default toolkit button.
Comment on attachment 267258 [details] [diff] [review]
The newest patch

>+  <binding id="datepicker-box" extends="xul:box">
>     ...
>+      <handler event="mousedown" action="if (event.button == 0) this.dayPushed()"/>
>+      <handler event="mouseup" action="if (event.button == 0) this.dayUp()"/>
>+      <handler event="mouseout" action="if (event.button == 0) this.dayOut()"/>
In general the patch works as advertised. Although, one particular piece of code took my attention, namely the datepicker-box binding. It seems that we're re-inventing the wheel here, since we're building a button-like control from scratch. Wouldn't it be a smarter solution to inherit from the toolkit button-base and add the image-display feature that we're needing here?

Such a binding could look like this here:

  <binding id="image-button" display="xul:button"
           extends="chrome://global/content/bindings/button.xml#button-base">
    <resources>
      <stylesheet src="chrome://global/skin/toolbarbutton.css"/>
    </resources>
    <content>
      <children includes="observes|template|menupopup|tooltip"/>
      <xul:stack>
        <xul:image class="toolbarbutton-icon"/>
        <xul:label class="toolbarbutton-text" crop="right" flex="1"
                   xbl:inherits="value=label,accesskey,crop,toolbarmode,buttonstyle"/>
      </xul:stack>
    </content>
  </binding>

I attached a fully working example based on this binding to this bug in order to show how this solution could look like. I'd suggest to incorporate this into the patch since it will reduce the amount of code and make the final result much more elegant.
Attachment #267258 - Flags: review?(michael.buettner) → review-
Attached file test harness
oops, the previously submitted archive didn't contain all files, sorry for the bug spam.
Attachment #271645 - Attachment is obsolete: true
I modified the daypicker binding according to your suggestion so that it's now derived from a button. Indeed I could remove a lot of my prior implementation. Tested it under Windows, Solaris and MacOSX.
Attachment #262865 - Attachment is obsolete: true
Attachment #267258 - Attachment is obsolete: true
Attachment #283995 - Flags: ui-review?(christian.jansen)
Attachment #283995 - Flags: review?(michael.buettner)
Comment on attachment 283995 [details] [diff] [review]
patch with binding derived from button

Thanks for the patch, this looks much better than before.

ui+ with the following changes:

- please add a white background to the buttons
- on mac add a more visible border hex=808080
- Monthly:
  please disable the 4 buttons located right of 31
Attachment #283995 - Flags: ui-review?(christian.jansen) → ui-review+
Attached patch patch with UI changes (obsolete) — Splinter Review
I added the open issues that Christian mentioned to the patch. As one of them - the border under MacOS- was platform specific I now deal with two "calendar-daypicker.css" files below 'calendar/base/themes'. Consequently I also moved the binding file "sun-calendar-recurrence-daypicker.xml" to 'calendar/base' and renamed it to "calendar-daypicker"
Attachment #283995 - Attachment is obsolete: true
Attachment #284165 - Flags: review?(michael.buettner)
Attachment #283995 - Flags: review?(michael.buettner)
Comment on attachment 284165 [details] [diff] [review]
patch with UI changes

>+          <xul:label anonid="daytext" class="toolbarbutton-text" flex="1"
>+                   xbl:inherits="value=label"/>
Indentation wrong.

>+		</xul:hbox>
This line contains tabs.

>+			if (aEvent.attrName =="checked") {
>+				var event = document.createEvent('Events');
>+				event.initEvent('select', true, true);
>+				this.calendar.dispatchEvent(event);
>+			}
All of these lines contain tabs.

>+    	  this.setAttribute("autoCheck", "true");
This line contains tabs.

>diff -a -U 8 -pN -r -x '*.png' mozilla_ref/calendar/lightning/jar.mn mozilla/calendar/lightning/jar.mn
*lightning* jar.mn? This obviously doesn't work for sunbird, does it? The line that adds the image to the calendar.jar should be moved to c/b/jar.mn.

>+#expand    skin/classic/calendar/datepicker-background.png  (/calendar/base/themes/common/datepicker-background.png)
Please name the image *day* picker instead of *date* picker, as the binding itself has been renamed this does no longer fit.

Apart from these minor issues, everything looks great and works as expected. Thanks for your patience and thanks for taking care. r=mickey with the above mentioned issues being addressed.
Attachment #284165 - Flags: review?(michael.buettner) → review+
Whiteboard: [checkin-needed after 0.7]
Attached patch final patchSplinter Review
patch checked in on trunk and MOZILLA_1_8_BRANCH
Attachment #284165 - Attachment is obsolete: true
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Whiteboard: [checkin-needed after 0.7]
Target Milestone: --- → 0.8
Verified with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.9pre)
Gecko/20071028 Calendar/0.8pre and Lt 2007102805
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.