Closed Bug 329204 Opened 19 years ago Closed 19 years ago

pick out calendar widget

Categories

(Core Graveyard :: XForms, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: surkov, Assigned: surkov)

References

Details

(Keywords: fixed1.8.0.4, fixed1.8.1)

Attachments

(2 files, 3 obsolete files)

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
Blocks: 327584
Attached patch first try (obsolete) — Splinter Review
the patch adds calendar widget for xhtml, output[date][appearance="full"] and input[date][appearance="full"] widgets for xforms.
Attachment #213871 - Flags: review?(allan)
Attachment #213871 - Flags: review?(doronr)
Attached file testcase
As regular comments as nitpickings (about names of properties/methods ans so on) are looked forward :)
Assignee: aaronr → surkov
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.
Attachment #213871 - Flags: review?(doronr) → review-
Attached patch with doron's comments (obsolete) — Splinter Review
(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.
Attachment #213871 - Attachment is obsolete: true
Attachment #213967 - Flags: review?(doronr)
Attachment #213871 - Flags: review?(allan)
Attachment #213967 - Flags: review?(allan)
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.
Attachment #213967 - Flags: review?(allan) → review-
(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.
Why not hide the dropdown widget if it is readonly, and grey out the input field?
(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 :).
(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?
Attached patch next try (obsolete) — Splinter Review
Attachment #213967 - Attachment is obsolete: true
Attachment #215092 - Flags: review?(allan)
Attachment #213967 - Flags: review?(doronr)
Attachment #215092 - Flags: review?(doronr)
(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
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Comment on attachment 215092 [details] [diff] [review]
next try

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

With that, r=me
Attachment #215092 - Flags: review?(allan) → review+
Attachment #215092 - Attachment is obsolete: true
Attachment #215107 - Flags: review?(doronr)
Attachment #215092 - Flags: review?(doronr)
Blocks: 330641
Blocks: 327236
Attachment #215107 - Flags: review?(doronr) → review+
Checked into trunk
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Whiteboard: xf-to-branch
Blocks: 332853
Whiteboard: xf-to-branch
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: