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

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

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

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.

(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 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.

(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?

(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 on attachment 215092 [details] [diff] [review] next try Could you put the @noinitialize in the mozType namespace too? With that, r=me

Checked into trunk