329204 - pick out calendar widget

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect)

Description

[alexander :surkov (:asurkov)]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20051212 Firefox/1.6a1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20051212 Firefox/1.6a1

patch is coming

Reproducible: Always

Comment 1

[alexander :surkov (:asurkov)]

Attachment: first try

the patch adds calendar widget for xhtml, output[date][appearance="full"] and input[date][appearance="full"] widgets for xforms.

Comment 2

[alexander :surkov (:asurkov)]

Attachment: testcase

As regular comments as nitpickings (about names of properties/methods ans so on) are looked forward :)

Comment 3

[Doron Rosenberg (IBM)]

Comment on attachment 213871 [details] [diff] [review]
first try

>diff -uNra mozilla.orig/extensions/xforms/jar.mn mozilla/extensions/xforms/jar.mn
>--- mozilla.orig/extensions/xforms/jar.mn	2006-02-22 12:30:24.000000000 +0800
>+++ mozilla/extensions/xforms/jar.mn	2006-03-03 15:33:43.000000000 +0800
>@@ -7,6 +7,8 @@
> * content/xforms/xforms-prefs.xul              (resources/content/xforms-prefs.xul)
> * content/xforms/xforms-prefs-ui.xul           (resources/content/xforms-prefs-ui.xul)
> * content/xforms/xforms-prefs.js               (resources/content/xforms-prefs.js)
>+  content/xforms/toolkit.xml                   (resources/content/toolkit.xml)
>+  content/xforms/toolkit-xhtml.xml             (resources/content/toolkit-xhtml.xml)
>   content/xforms/xforms.xml                    (resources/content/xforms.xml)
>   content/xforms/xforms-xhtml.xml              (resources/content/xforms-xhtml.xml)
>   content/xforms/xforms-xul.xml                (resources/content/xforms-xul.xml)


why does this need a toolkit files?  Plus, if it is needed, probably better to call it widgets.xml.

>diff -uNra mozilla.orig/extensions/xforms/resources/content/input-xhtml.xml mozilla/extensions/xforms/resources/content/input-xhtml.xml
>--- mozilla.orig/extensions/xforms/resources/content/input-xhtml.xml	2006-02-24 12:46:49.000000000 +0800
>+++ mozilla/extensions/xforms/resources/content/input-xhtml.xml	2006-03-03 15:33:43.000000000 +0800
>@@ -183,7 +183,37 @@
>   </binding>
> 
> 
>-<!-- INPUT: Month -->
>+  <!-- INPUT: <DATE, APPEARANCE='FULL' -->
>+  <binding id="xformswidget-input-date-full"
>+           extends="chrome://xforms/content/input.xml#xformswidget-input-base">
>+    <content>
>+      <children includes="label"/>
>+      <html:calendar anonid="control" noinitialize="true"/>
>+      <children/>
>+    </content>

Also, html:calendar is evil, we should be either using our own namespace or doing html:span class="xforms-calendar".


>+          if (!this._isUIBuilded) {
>+            this.buildUI();
>+            this._isUIBuilded = true;
>+          }

english nit: .isUIBuilt

>+
>+          // set days for previous month
>+          var dayOffset = this.dayOffset;
>+          var prevDayCount = this.prevDaysCount;
>+          for (var i = 0; i < dayOffset; i++) {
>+            this._dayElements[i].textContent = prevDayCount + i - dayOffset + 1;
>+            this._dayElements[i].setAttribute("class", "prevMonth");
>+          }
>+
>+          // set days for current month
>+          var count = this.daysCount + dayOffset;
>+          for (; i < count; i++) {
>+            this._dayElements[i].textContent = i - dayOffset + 1;
>+            this._dayElements[i].setAttribute("class", "currentMonth");
>+          }
>+
>+          // set days for next month
>+          for (var day = 1; i < this._dayElements.length; i++, day++) {
>+            this._dayElements[i].textContent = day;
>+            this._dayElements[i].setAttribute("class", "nextMonth");
>+          }
>+
>+          this.refreshCurrentDate(this.currentDate.getDate(), aFocusedDay);
>+        ]]>
>+        </body>
>+      </method>

I wonder if using .className would be faster.

Comment 4

[alexander :surkov (:asurkov)]

Attachment: with doron's comments

(In reply to comment #3)

> why does this need a toolkit files? 

The controls described in toolkit files it's not xforms controls, it's toolkit or auxiliary widgets. I guess it would be not right if we put them in one of xforms widgets files.

> Plus, if it is needed, probably better to
> call it widgets.xml.
> 

I thought about names like 'toolkit', 'widgets', 'common', 'auxilary'. But I cannot decide what one of them is more appropriate. I don't like 'widgets' name a lot because I guess it can confuse somebody: what widgets? xforms or not? I really don't know. Do you still prefer 'widgets' name?

> Also, html:calendar is evil, we should be either using our own namespace or
> doing html:span class="xforms-calendar".
> 

Fixed, now it is used html:span[class="xforms-calendar"]

> >+          if (!this._isUIBuilded) {
> >+            this.buildUI();
> >+            this._isUIBuilded = true;
> >+          }
> 
> english nit: .isUIBuilt

Fixed

> I wonder if using .className would be faster.

I dunno thought I doubt whether because 'className' property sets 'class' attribute too.

Comment 5

[Allan Beaufour]

Comment on attachment 213967 [details] [diff] [review]
with doron's comments

* I'm not too happy about "toolkit" either because it is confused with the "toolkit/" dir. I would still prefer "widgets".

* If I get this right, you are basically creating the date picker from scratch. Why?

* I know Doron suggested "span class="xforms-calendar", but I think we should be careful about "poluting" the class namespace, and binding to spans. Maybe just use mozType:calendar? What do the rest of you say?
(another problem is that "noinitilize" is not a valid attribute on a html:span)

At least we should not create a generic binding to html|span[...], it should be limited to xf|input html|span[...].

* Nit: I think the calendars should have a border

* It seems weird for me that you can hover over dates in output/readonly controls. Also, that you can change focus to another day.

Comment 6

[alexander :surkov (:asurkov)]

(In reply to comment #5)
> (From update of attachment 213967 [details] [diff] [review] [edit])
> * I'm not too happy about "toolkit" either because it is confused with the
> "toolkit/" dir. I would still prefer "widgets".

Ok, let it will be "widgets" :)

> * If I get this right, you are basically creating the date picker from scratch.
> Why?

I didn't understand. Do you mean why do I create day elements dynamically (instead of placing them into xbl:content statically)?

> 
> * I know Doron suggested "span class="xforms-calendar", but I think we should
> be careful about "poluting" the class namespace, and binding to spans. Maybe
> just use mozType:calendar? What do the rest of you say?

> At least we should not create a generic binding to html|span[...], it should be
> limited to xf|input html|span[...].

If we will not exspose separate calendar widgets for xhtml then I guess we don't need in special classname or mozType:calendar. Thought mozType:calendar looks good for me.

> (another problem is that "noinitilize" is not a valid attribute on a html:span)
> 

Fine. Do you propose to put the attribute in special namespace? If it true then what namespace will be better? Thought I don't like a lot this attribute. If we will not expose a calendar widget for xhtml then I guess I can work without the "noinitialize" attribute.

> 
> * Nit: I think the calendars should have a border

Ok, I'll fix it.

> 
> * It seems weird for me that you can hover over dates in output/readonly
> controls. 

Probably you're right. Thought I can't see a something bad in that.

> Also, that you can change focus to another day.

I don't agree because focus setting alows keyboard navigation. Keyboard navigation is still usefull even if control is readonly. :) It's just a focus, current day isn't changed.

Comment 7

[Doron Rosenberg (IBM)]

Why not hide the dropdown widget if it is readonly, and grey out the input field?

Comment 8

[alexander :surkov (:asurkov)]

(In reply to comment #7)
> Why not hide the dropdown widget if it is readonly, and grey out the input
> field?
> 

There is only one reason. It's usefull to have a full calendar (not only days of current month) because I can work with it, I can see a last month, last year and so on. I cannot change current date and imo that's enough. I guess we should have for output a calendar widget but not specific appearance of date.

But if you and Allan insist on it then I will be forced to do it for sure :).

Comment 9

[alexander :surkov (:asurkov)]

(In reply to comment #8)
> (In reply to comment #7)
> > Why not hide the dropdown widget if it is readonly, and grey out the input
> > field?
> > 

I didn't understand your comment properly to all appearances and posted my previous comment. You shoudn't take it into acount.

What dropdown widget do you have in view?

Comment 10

[alexander :surkov (:asurkov)]

Attachment: next try

Comment 11

[alexander :surkov (:asurkov)]

(In reply to comment #5)
> (From update of attachment 213967 [details] [diff] [review] [edit])
> * I'm not too happy about "toolkit" either because it is confused with the
> "toolkit/" dir. I would still prefer "widgets".

Fixed
> 
> * I know Doron suggested "span class="xforms-calendar", but I think we should
> be careful about "poluting" the class namespace, and binding to spans. Maybe
> just use mozType:calendar? What do the rest of you say?
> (another problem is that "noinitilize" is not a valid attribute on a html:span)

@noinitialize was removed, instead on span[class] is used span[mozType|calendar]

> At least we should not create a generic binding to html|span[...], it should be
> limited to xf|input html|span[...].
> 

Fixed, now we are limited to xf|input[appearance="full"] html|span[mozType|calendar] and xf|output[appearance="full"] html|span[mozType|calendar]

> * Nit: I think the calendars should have a border
> 

Fixed

Comment 12

[Allan Beaufour]

Comment on attachment 215092 [details] [diff] [review]
next try

Could you put the @noinitialize in the mozType namespace too?

With that, r=me

Comment 13

[alexander :surkov (:asurkov)]

Attachment: patch [allan's comment]

Comment 14

[Allan Beaufour]

Checked into trunk