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 It's needed base widget to create xforms widgets of various namespaces (like xul, xhtml, svg). Here base widgets will be exposed for widgets declared in xforms.xml file. Reproducible: Always

Patch doesn't expose base widgets for "label-accesskey", "upload" and "input-date" widgets. I want to support them with future patches.

Comment on attachment 208835 [details] [diff] [review] patch >+ >+ get readonly() { >+ return this.getAttribute('readonly') == 'true' ? true : false; >+ }, >+ set readonly(val) { >+ if (val) { >+ this.setAttribute('readonly', 'true'); >+ } else { >+ this.removeAttribute('readonly'); >+ } >+ } You should set the value of the "readonly" attribute to "readonly". There are similar problems with at least "disabled" attribute.

(In reply to comment #4) > You should set the value of the "readonly" attribute to "readonly". > There are similar problems with at least "disabled" attribute. > Other than that, I think this looks ok. So post a new patch and I'll r+.

fixed smaug's comment 4

fixed input[type="date"] issues, the patches of bug 323683 and bug 323829 are applied too.

(In reply to comment #7) > Created an attachment (id=210738) [edit] > patch > > fixed input[type="date"] issues, the patches of bug 323683 and bug 323829 are > applied too. > Reviewing is difficult if there is code from many patches. + <implementation implements="nsIXFormsUIWidget nsIXFormsLabelElement"> Why this line? nsIXFormsLabelElement is not scriptable atm. So I wonder if + <property name="textValue" readonly="true" + onget="return this.control.value;"/> really works.

(In reply to comment #9) > (In reply to comment #7) > > Created an attachment (id=210738) [edit] > > patch > > > > fixed input[type="date"] issues, the patches of bug 323683 and bug 323829 are > > applied too. > > > > Reviewing is difficult if there is code from many patches. I agree. But patches for mentioned bugs was reviewed and I guess they will be checked in soon. In that case I should update my patch too. I just thought if you reviewed these patches then you know issues of these bugs and probably it will not be a too diffucult for you. In general this problem is pecular for this bug: some bugs were fixed and you should review the updated patch again :). Thus I'm able to remove the patches supporting from my patch, just let me know me. > > + <implementation implements="nsIXFormsUIWidget nsIXFormsLabelElement"> > Why this line? nsIXFormsLabelElement is not scriptable atm. So I wonder if > + <property name="textValue" readonly="true" > + onget="return this.control.value;"/> > really works. > Since nsIXFormsLabelElement isn't scriptable then implements="nsIXFormsLabelElement" isn't needed. But for sure property textValue works. I use it for select controls realization (and I guess the property can be usefull). I guess it would be nice if nsIXFormsLabelElements (or its part) will be scriptable.

(In reply to comment #10) > > Since nsIXFormsLabelElement isn't scriptable then > implements="nsIXFormsLabelElement" isn't needed. But for sure property > textValue works. I use it for select controls realization (and I guess the > property can be usefull). I guess it would be nice if nsIXFormsLabelElements > (or its part) will be scriptable. > But you're not using textValue in this patch, right? And does it work in the same way as the nsIXFormsLabelElement::textValue See http://lxr.mozilla.org/seamonkey/source/extensions/xforms/nsXFormsLabelElement.cpp#194 and http://lxr.mozilla.org/seamonkey/source/extensions/xforms/nsXFormsLabelElement.cpp#402

(In reply to comment #11) > But you're not using textValue in this patch, right? And does it work in the > same way as the nsIXFormsLabelElement::textValue > See > http://lxr.mozilla.org/seamonkey/source/extensions/xforms/nsXFormsLabelElement.cpp#194 > and > http://lxr.mozilla.org/seamonkey/source/extensions/xforms/nsXFormsLabelElement.cpp#402 > Right. Yes, it works in the same way. I removed the property. I guess we should return to the discussion when patch for select control will be reviewed.

Comment on attachment 210838 [details] [diff] [review] patch Hmm, there was something to review after all ;) >+ <implementation> >+ <method name="getControlElement"> I'd like to see some documentation about getControlElement. What kind of object it is and why it different in different controls. For example in <output> __proto__ is the control element, but in <label> it is .ctrl. >+ <body> >+ // XXX changing from setting textContent to setting nodeValue of >+ // first child (text node created by space character initializer >+ // above). Workaround for bug 322975. Probably should be changed >+ // back after repeat is xbl-ized This comment seems to be in wrong place. I guess it should be just before the getter or setter. >+ return { >+ __proto__: this.ownerDocument. >+ getAnonymousElementByAttribute(this, 'anonid', 'control'), >+ >+ get value() { >+ return this.firstChild.nodeValue; >+ }, >+ set value(val) { >+ this.firstChild.nodeValue = val; >+ } >+ }; >+ </body> >+ </method> >+ >+ </implementation> >+ </binding> >+ >+ >+ <!-- LABEL: <DEFAULT> --> >+ <binding id="xformswidget-label" >+ extends="chrome://xforms/content/xforms.xml#xformswidget-label-base"> >+ <content> >+ <html:span anonid="control"><children/></html:span> >+ </content> >+ >+ <implementation> >+ <method name="getControlElement"> >+ <body> >+ return { >+ ctrl: this.ownerDocument. >+ getAnonymousElementByAttribute(this, 'anonid', 'control'), >+ >+ get value() { >+ if (this.hasbound) >+ return this.ctrl.textContent; >+ return this.ctrl.parentNode.textContent; >+ }, >+ set value(val) { >+ if (val != null) { >+ this.hasbound = true; >+ this.ctrl.textContent = val; >+ } else { >+ this.hasbound = false; >+ } >+ }, >+ >+ hasbound: false Should this be hasBound. >+ <!-- SECRET: <DEFAULT> --> >+ <binding id="xformswidget-secret" >+ extends="chrome://xforms/content/xforms.xml#xformswidget-secret-base"> >+ <content> >+ <children includes="label"/> >+ <html:input anonid="control" xbl:inherits="accesskey" type="password"/> >+ <children/> >+ </content> Can't you make this just extend normal input, but use <html:input type="password">? And couldn't you push a default getControlElement implementation up to xformswidget-base so that you don't have to copy-paste similar getControlElement implementation in many places?

(In reply to comment #13) > Hmm, there was something to review after all ;) Yea, there was something to rewrite for me and even I like more the new variant :). > I'd like to see some documentation about getControlElement. getControlElement() method is documented for each control. Though I don't sure comments aren't contains spelling or stylistic errors. > What kind of object it is and why it different in different controls. For > example in <output> __proto__ is the control element, but in <label> it is > .ctrl. Fixed. > This comment seems to be in wrong place. I guess it should be just before the > getter or setter. Fixed > Should this be hasBound. Fixed > Can't you make this just extend normal input, but use <html:input > type="password">? Fixed > And couldn't you push a default getControlElement implementation up to > xformswidget-base so that you don't have to copy-paste similar > getControlElement implementation in many places? I think I couldn't since getControlElement() allow to works with underlying control and xforms base binding shouldn't do any assumption about underlaying control. I can't see other way how it can be done.

Comment on attachment 211106 [details] [diff] [review] patch >+ <!-- INPUT: <DEFAULT> --> >+ <binding id="xformswidget-input" >+ extends="chrome://xforms/content/xforms.xml#xformswidget-input-base"> >+ <content> >+ <children includes="label"/> >+ <html:input class="xf-value" anonid="control" xbl:inherits="accesskey"/> >+ <children/> >+ </content> >+ >+ <implementation> >+ <method name="getControlElement"> >+ <body> >+ return { >+ __proto__: this.ownerDocument. >+ getAnonymousElementByAttribute(this, 'anonid', 'control'), >+ >+ get value() { >+ return this.__proto__.value; >+ }, >+ set value(val) { >+ this.__proto__.value = val; Is the .__proto__ really needed here? Shouldn't this.value do the same thing? You're using __proto__ also elsewhere, when you're accessing the control element, but not everywhere. I'd still like to think if there was a better way to handle getControlElement implementations. Copy-pasting code is just ugly. But fortunately getControlElement implementation is always quite small, so I don't mind too much if it is done in the current way. And this patch contains still code for those other bugs, meaning it can't be checked in as such.

(In reply to comment #15) > >+ return { > >+ __proto__: this.ownerDocument. > >+ getAnonymousElementByAttribute(this, 'anonid', 'control'), > >+ > >+ get value() { > >+ return this.__proto__.value; > >+ }, > >+ set value(val) { > >+ this.__proto__.value = val; > > Is the .__proto__ really needed here? Shouldn't this.value do the same thing? Er, what I mean here is that, because the __proto__ already has getter and setter for |value|, why you have to reimplement it.

(In reply to comment 16) > Er, what I mean here is that, because the __proto__ already has getter and > setter for |value|, why you have to reimplement it. I needn't. Fixed. (In reply to comment#15) > You're using __proto__ also elsewhere, when you're accessing the control > element, but > not everywhere. I didn't understand. > I'd still like to think if there was a better way to handle getControlElement > implementations. It would be fine, but getControlElement() implementations is to different from one control to other.

Comment on attachment 211239 [details] [diff] [review] patch >+ <!-- OUTPUT: <DEFAULT> >+ The output widget assumes successors bindings implement getElementControl() >+ method what returns the object: >+ { >+ set value(); // set stirng value Typo "stirng" And the patch still contains the fix for Bug 323829, right? Remove it, please. Unfortunately I couldn't test this because it doesn't apply cleanly to trunk anymore. But if you could update the patch and get a review from Allan tomorrow morning, I could review it later and check it in (unless there are some problems ofc). I think this looks good, but as I said, couldn't test.

(In reply to comment #18) > Typo "stirng" Fixed > And the patch still contains the fix for Bug 323829, right? Remove it, please. I guess now I shouldn't. Right? > Unfortunately I couldn't test this because it doesn't apply cleanly to trunk > anymore. Fixed.

Comment on attachment 211356 [details] [diff] [review] patch This breaks output's labels :(

Not too good test for output's labels. Sorry that I didn't notice this earlier.

(In reply to comment #20) > This breaks output's labels :( >

(In reply to comment #20) > This breaks output's labels :( > Fixed

Comment on attachment 211686 [details] [diff] [review] patch I think this is ok, r=me

(In reply to comment #24) > (From update of attachment 211686 [details] [diff] [review] [edit]) > I think this is ok, r=me > Thanks for valuable notes, they help me to make code cleaner and clearer. I think the first bug's issue fixing is just around the corner now. I have plans to make xforms controls for xul immediately after checking this one. I guess it will be easy bug (as making as reviewing) :).

Comment on attachment 211686 [details] [diff] [review] patch I only have some nits. Very good work! I would like to have a description in the start of xforms.xml, which explains the basic idea of the design. Something like: This file implements the "abstract" UI classes for all XForms controls. They all have "pure virtual" functions that they expect to be implemented by concrete application and returned in the getElementControl() call. An example is the controls for XHTML in xforms-xhtml.xml. A very brief description in xforms-xhtml.xml too. The only code-comment I have is: Why isn't focus implemented on xforms-widget-base, shouldn't we assume that all elements can be focused? > + <!-- OUTPUT: <DEFAULT> > + The output widget assumes successors bindings implement getElementControl() > + method what returns the object: > + { > + set value(); // set "stirng" value stirng -> string > + <!-- SECRET: <DEFAULT> > + We don't need in any special base binding for secret widget. All > + successors bindings should be extended from base binding for input widget. > + --> > + > + <!-- TEXTAREA: <DEFAULT> > + We don't need in any special base binding for secret widget. All > + successors bindings should be extended from base binding for input widget. > + --> secret -> textarea

(In reply to comment #27) > Created an attachment (id=211830) [edit] > patch Checked in on trunk.