Closed Bug 323849 Opened 19 years ago Closed 18 years ago

Expose base bindings for xforms select widgets

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

(3 files, 9 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 up

Reproducible: Always
Blocks: 323851
Summary: Expose base widgrets fo xforms selects widgets → Expose base bindings for xforms select widgets
Attached file select test
I need in xforms:label's "textValue" property accessible from java script. I can see the utility for this property not only for the bug fixing. Now textValue property is realized by xtf binding in nsIXFormsLabelElement interface (the interface is not scriptable). I guess the property should be realized by xbl control in the context of nsIXFormsLabelUIElement interface.
Blocks: 327236
I maked new bug about 'textValue' property for a label (bug 327239)
Depends on: 327239
Attached patch first try (obsolete) — Splinter Review
Attachment #212566 - Flags: review?(allan)
Attachment #212566 - Flags: review?(smaug)
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee: aaronr → surkov
Status: NEW → ASSIGNED
Comment on attachment 212566 [details] [diff] [review]
first try

I think Doron's is the better reviwer than me for selects
Attachment #212566 - Flags: review?(allan) → review?(doronr)
Comment on attachment 212566 [details] [diff] [review]
first try

>+
>+  Binding 'xformwidget-select-base' assumes that it's successor have select
>+  control as a child node and it's implementation has next properties and
>+  methods:

"has following", not "has next"

>+  <binding id="xformswidget-select-compact"
>+           extends="chrome://xforms/content/select.xml#xformswidget-select-base">
>+    <content>
>+      <html:label>
>+        <html:span class="label-container">
>+           <children includes="label"/>
>+        </html:span>
>+        <html:selectwidget anonid="control" xbl:inherits="style, accesskey"/>
>+        <children/>
>+      </html:label>
>+    </content>
>+  </binding>

html:selectwidget is evil.  Perhaps define it in our own namespace?

>+      <method name="removeAllItems">
>+        <body>
>+          for (var i = this.control.childNodes.length; i > 0; i--) {
>+             this.control.removeChild(this.control.childNodes[i-1]);
>+          }
>+        </body>
>+      </method>

this.control.innerHTML = "" might be faster (batched changes vs node-by-node removal).

>+
>+      <property name="selectedIndex"
>+                onget="throw Error('not implemented');"
>+                onset="throw Error('not implemented');"/>

then why define it?

>+
>+      <method name="removeAllItems">
>+        <body>
>+         for (var i = this.control.childNodes.length; i > 0; i--) {
>+             this.control.removeChild(this.control.childNodes[i-1]);
>+          }
>+        </body>
>+      </method>

for's closing bracket not aligned

>@@ -561,18 +569,18 @@
>             } else {
>               // if some copyItems were selected by the user prior to the call
>               // to _getSelectedValues, then we would not have set up
>-              // _accessorValueCache.  Since the node we are bound to can't
>+              // _delegateValueCache.  Since the node we are bound to can't
>               // be set by copyItems (its not an ELEMENT_NODE), any copyItems
>               // in this select would have been deselected during
>               // _getSelectedValues.  Thus, anything in the contentEnvelope at
>               // this point should just be strings and so we can set
>-              // delegate.value directly and use _accessorValueCache after all.
>+              // delegate.value directly and use _delegateValueCache after all.

um, don't we call them accessors and not delegates anymore?

>+  <!-- WIDGET FOR SELECT CONTROLS -->
>+  <binding id="widget-base">

then shouldn't this be widget-select-base?

just a quick overview, don't have time today for a full review.
Attached patch next try (obsolete) — Splinter Review
(In reply to comment #6)

> "has following", not "has next"

Fixed

> html:selectwidget is evil.  Perhaps define it in our own namespace?

If we will use own namespace then I afraid to get some problems related with custom namespace elements in xhtml context. I guess the better to use just html:span element since we need in container only for control widget. What do you think?


> 
> >+      <method name="removeAllItems">
> >+        <body>
> >+          for (var i = this.control.childNodes.length; i > 0; i--) {
> >+             this.control.removeChild(this.control.childNodes[i-1]);
> >+          }
> >+        </body>
> >+      </method>
> 
> this.control.innerHTML = "" might be faster (batched changes vs node-by-node
> removal).

I got the strange behaviour when I use innerHTML for control. If test file doesn't contains html prefix in namespace declaration then I get an exception "prefix not bound to a namespace".

> >+      <property name="selectedIndex"
> >+                onget="throw Error('not implemented');"
> >+                onset="throw Error('not implemented');"/>
> 
> then why define it?

select[appearance="compact"] has selectIndex property threafore I added it to select[appearance="full"] but I didn't implement it.

> for's closing bracket not aligned

Fixed

> um, don't we call them accessors and not delegates anymore?

No, it's a mistake, fixed.

> >+  <!-- WIDGET FOR SELECT CONTROLS -->
> >+  <binding id="widget-base">
> 
> then shouldn't this be widget-select-base?

I called it as "controlwidget-base". Do you like such?

> just a quick overview, don't have time today for a full review.
> 

I look forward to further comments. :)
Attachment #212566 - Attachment is obsolete: true
Attachment #212705 - Flags: review?(doronr)
Attachment #212566 - Flags: review?(smaug)
Attachment #212566 - Flags: review?(doronr)
(In reply to comment #7)
> Created an attachment (id=212705) [edit]
> > html:selectwidget is evil.  Perhaps define it in our own namespace?
> 
> If we will use own namespace then I afraid to get some problems related with
> custom namespace elements in xhtml context. I guess the better to use just
> html:span element since we need in container only for control widget. What do
> you think?
> 

use a span then, probably better.
Attached patch updated (obsolete) — Splinter Review
Attachment #213227 - Flags: review?(doronr)
Attachment #213227 - Flags: review?(smaug)
Attachment #212705 - Attachment is obsolete: true
Attachment #212705 - Flags: review?(doronr)
Blocks: 326556
Comment on attachment 213227 [details] [diff] [review]
updated

>+
>+      <method name="appendGroup">
>+        <parameter name="aLabel"/>
>+        <parameter name="aGroup"/>
>+        <body>
>+          var item = document.createElementNS(this.XHTML_NS, "optgroup");
>+          if(aLabel) {
>+            item.appendChild(aLabel.cloneNode(true));
>+          }
>+          if(aGroup) {
>+            aGroup.appendChild(item);
>+          } else {
>+            this.control.appendChild(item);
>+          }

if (aFoo) {, not if(aFoo) :)

>+              controlitem = this.control.appendItem(
>+                label.nodeValue,
>+                item.value,
>+                group
>+              );
>             } else {
>-              var childLength = label.childNodes.length;
>-
>-              // text content only?
>-              if (childLength == 1 && label.firstChild.nodeType == Node.TEXT_NODE) {
>-                option.textContent = label.textContent;
>-              } else {
>-                // clone all children.  We don't simply clone the xforms:label
>-                // because xhtml:select will get confused by it and break
>-                // find-as-you-type.
>-                for (var i = 0; i < childLength; i++) {
>-                  option.appendChild(label.childNodes[i].cloneNode(true));
>+              controlitem = this.control.appendItem(
>+                null,
>+                item.value,
>+                group
>+              );
>+            }


why do both appendItem calls have their arguments on a new line each? kinda weird.

rest looks fine, tried my own select testcase and it worked fine.
Attachment #213227 - Flags: review?(doronr) → review+
I hate to be the bearer of bad news, but I found some issues with the 'updated' patch.

A change to xf:select in this patch causes https://bugzilla.mozilla.org/attachment.cgi?id=202441 to fail.  Should only get a select and deselect when selecting or deselecting the last item in the list.  We shouldn't generate xforms-select and xforms-deselect if selecting copyItems that are bound to non-element nodes (since this would be an illegal selection).  xf:select1 works fine in this regard, so some change that you made to xf:select is causing this.

A change to xf:select in this patch causes https://bugzilla.mozilla.org/attachment.cgi?id=200186 to fail.  If you load the testcase and select any single item in the list, the rest should deselect, but with this patch applied, this isn't the case.

For the above two bugs, you should probably make sure that you have my latest changes for select and select1 factored into your patch.  For bug 316895 I made some changes that maybe you missed?  I checked it in about the time you were started creating patches on this bug.

https://bugzilla.mozilla.org/attachment.cgi?id=203466 now fails.  Should have three items in the list.  I only see one with this patch.

Just as an FYI, I've personally found that when I make changes to select and select1 that running through the testcases on bug 316895 and bug 279063 helped me work out a lot of issues in my patches.
Attached patch patch [aaron's comment] (obsolete) — Splinter Review
(In reply to comment #11)
> For the above two bugs, you should probably make sure that you have my latest
> changes for select and select1 factored into your patch.  For bug 316895 I made
> some changes that maybe you missed?  I checked it in about the time you were
> started creating patches on this bug.

Patches of bug 328393 and bug 316895 are applied.

> Just as an FYI, I've personally found that when I make changes to select and
> select1 that running through the testcases on bug 316895 and bug 279063 helped
> me work out a lot of issues in my patches.
> 

Testcases are tested with my patch. I hope no one testcase isn't hidden from me :).
Attachment #213227 - Attachment is obsolete: true
Attachment #214146 - Flags: review?(aaronr)
Attachment #213227 - Flags: review?(smaug)
Comment on attachment 214146 [details] [diff] [review]
patch [aaron's comment]

The patch contains doron's commments too and my debug alert :)). I'll remove the alert after additional comments.
Attached patch updated (obsolete) — Splinter Review
Attachment #214146 - Attachment is obsolete: true
Attachment #215362 - Flags: review?(aaronr)
Attachment #214146 - Flags: review?(aaronr)
Comment on attachment 215362 [details] [diff] [review]
updated

Note: I gave up on trying to review the patch.  Just too big! :-)  So I'm reviewing the resulting files after the patch is applied.  So I might not be able to point to specific patch lines.  But I'll try to be as exact as I can.

You removed the _uiElement field and property, but the selectedIndex property in xformswidget-select-base still uses it.

You introduced some switch statements...please make sure the '{' for the switch is on the same line as the switch, just like an 'if'.  And please indent the 'case's.

More comments to come...
Attached patch patch2 [aaron] (obsolete) — Splinter Review
(In reply to comment #15)

> Note: I gave up on trying to review the patch.  Just too big! :-)  So I'm
> reviewing the resulting files after the patch is applied.  So I might not be
> able to point to specific patch lines.  But I'll try to be as exact as I can.
> 

Don't mind, just tell me what's wrong :). Though you can point lines in result files to me too.

> You removed the _uiElement field and property, but the selectedIndex property
> in xformswidget-select-base still uses it.
> 

For sure, I forgot about it. Fixed.

> You introduced some switch statements...please make sure the '{' for the switch
> is on the same line as the switch, just like an 'if'.  And please indent the
> 'case's.

Fixed. Though most of 'case's in xforms files isn't indented.

> More comments to come...
> 

I look forward them.
Attachment #215362 - Attachment is obsolete: true
Attachment #215472 - Flags: review?(aaronr)
Attachment #215362 - Flags: review?(aaronr)
In _buildChoices:

- you use label.nodeValue, but what if the label is bound using a ref attribute.
- parameter name is aChoise.  Could you please correct it to aChoice?
- can't you just call _buildItem instead of duplicating the code in 'case "item":'

In _buildItemset:

- why put interfaces into variables if you only use them once?  Other places you don't
- I'm not a JS guru by any means, but by using containers[y].ELEMENT_NODE, doesn't that re-evaluate to find containers[y]?  Couldn't you just use Node.ELEMENT_NODE when you need to?  I could be wrong, though.

In controlwidget-base:

- could you put a comment in here as to why you have to call this.parentControl?  Like mention what the 'this' pointer is inside the function.  All of the other code in this file has 'this' being the select.  And probably mention who uses controlwidget-base since it isn't used in select.xml.

Looks good so far.  I'm done with select.xml.  More comments probably on Sunday...
Attached patch patch3 [aaron] (obsolete) — Splinter Review
(In reply to comment #17)
> In _buildChoices:
> 
> - you use label.nodeValue, but what if the label is bound using a ref
> attribute.

label binding redefines 'nodeValue' property threafore it should works ok. Probably it's a not good to redefine it.

> - parameter name is aChoise.  Could you please correct it to aChoice?

Fixed

> - can't you just call _buildItem instead of duplicating the code in 'case
> "item":'
> 

Fixed

> In _buildItemset:
> 
> - why put interfaces into variables if you only use them once?  Other places
> you don't

Fixed

> - I'm not a JS guru by any means, but by using containers[y].ELEMENT_NODE,
> doesn't that re-evaluate to find containers[y]?  Couldn't you just use
> Node.ELEMENT_NODE when you need to?  I could be wrong, though.

Fixed

> In controlwidget-base:
> 
> - could you put a comment in here as to why you have to call
> this.parentControl?  Like mention what the 'this' pointer is inside the
> function.  All of the other code in this file has 'this' being the select.  And
> probably mention who uses controlwidget-base since it isn't used in select.xml.

I didn't understand what do you exactly mean. I just added a comment before 'controlwidget-base' binding.

> Looks good so far.  I'm done with select.xml.  More comments probably on
> Sunday...

I'm looking forward. Thanks for the deeply review.
Attachment #215472 - Attachment is obsolete: true
Attachment #215475 - Flags: review?(aaronr)
Attachment #215472 - Flags: review?(aaronr)
(In reply to comment #18)
> Created an attachment (id=215475) [edit]
> patch3 [aaron]
> 
> (In reply to comment #17)
> > In _buildChoices:
> > 
> > - you use label.nodeValue, but what if the label is bound using a ref
> > attribute.
> 
> label binding redefines 'nodeValue' property threafore it should works ok.
> Probably it's a not good to redefine it.
> 

Looking at the XBL code for label, though, it looks like that label.nodeValue will return the content of the label span, right?  Well, what if the xf:choices gets refreshed before the label does?  Then won't the choices element get the label's old data?  For example, the form via JS updates the instance data then calls recalc, revalidate and refresh on the model.  xf:choices will be in the model list before its label is, so it'll get refreshed first, I believe.
Comment on attachment 215475 [details] [diff] [review]
patch3 [aaron]

Inside select-xhtml.xml (for compact appearance):

- with your comment, "When appearance attribute is changed then we should call nsIXFormsDelegate.widgetAttached()", you should put a XXX before that if that is something that we need to do that we aren't doing, yet.

- for the 'readonly' property on widget-select-compact, I don't get why you use xbl:inherits="disabled=readonly".  Why not just set @disabled instead of setting @readonly and then having XBL forward the value on to @disabled?

- Please make some kind of comment in this file outlining how things are tied together...something helpful enough so that someone going through this code shouldn't have to also open select.xml, and xforms.css to get an idea of what xbl:binding is binding to which element.  For example, you have two elements with the same anonid bound together (html:span and html:select).  That is confusing to see without some kind of comment.

- you have a XXX comment: "XXX: since xforms:label can include arbitrary elements then 'focus',  'blur' and 'change' events should be listen from 'xhtml:select' element only."  Isn't that what you are already doing in this patch?  XXX should only be used in a comment to make us aware that there is more work to do here.  Otherwise use 'NOTE:' or 'FYI:'

Inside select-xhtml.xml (for full appearance):

- where you throw error, if the Error doesn't throw put some context around the error (like file and line number), could you please make the error message more specific.  Like throw Error('selectFull: selectedIndex not implemented');  something that will be simple to grep on in 5 months when someone finally runs into this problem and reports it to us :-)

- why are you setting an attribute for readonly?  Why not use a field?  That would be more efficient than doing DOM manipulation, I'd think.

- For the XXX's in the full select section, please make sure that there are bugs open for these.

Those are all my comments.  Pretty nice rewrite.  Some things are a little more confusing but this patch makes most everything else much easier to understand than it was before!
Attachment #215475 - Flags: review?(aaronr) → review-
Attached file testcase2
(In reply to comment #19)

> 
> Looking at the XBL code for label, though, it looks like that label.nodeValue
> will return the content of the label span, right?  Well, what if the xf:choices
> gets refreshed before the label does?  Then won't the choices element get the
> label's old data?  For example, the form via JS updates the instance data then
> calls recalc, revalidate and refresh on the model.  xf:choices will be in the
> model list before its label is, so it'll get refreshed first, I believe.
> 

You're complitely right. Testcase shows the problem. I can see only one solution at the moment. xf:label of xf:choices should refresh xf:select like it do xf:label of xf:item. Probably it's a bad solution. I need in ideas.
Attached patch patch4 (obsolete) — Splinter Review
(In reply to comment #19)
> Looking at the XBL code for label, though, it looks like that label.nodeValue
> will return the content of the label span, right?  Well, what if the xf:choices
> gets refreshed before the label does?  Then won't the choices element get the
> label's old data?  For example, the form via JS updates the instance data then
> calls recalc, revalidate and refresh on the model.  xf:choices will be in the
> model list before its label is, so it'll get refreshed first, I believe.

I do like current realization do now. It's just coping of xf:label control.

(In reply to comment #20)
> (From update of attachment 215475 [details] [diff] [review] [edit])
> Inside select-xhtml.xml (for compact appearance):
> 
> - with your comment, "When appearance attribute is changed then we should call
> nsIXFormsDelegate.widgetAttached()", you should put a XXX before that if that
> is something that we need to do that we aren't doing, yet.

No, it's no XXX. The comment is obsolete, I removed them.

> - for the 'readonly' property on widget-select-compact, I don't get why you use
> xbl:inherits="disabled=readonly".  Why not just set @disabled instead of
> setting @readonly and then having XBL forward the value on to @disabled?

In general you're right. It's no needed. But I interpret the control widget as a widget (sorry for a pun) and threafore I prefer to set attributes for it as it is for real widgets. But if you insist then I'll do as you propose.

> - Please make some kind of comment in this file outlining how things are tied
> together...something helpful enough so that someone going through this code
> shouldn't have to also open select.xml, and xforms.css to get an idea of what
> xbl:binding is binding to which element.  For example, you have two elements
> with the same anonid bound together (html:span and html:select).  That is
> confusing to see without some kind of comment.

I added some comments (for select controls and for control widgets).

> - you have a XXX comment: "XXX: since xforms:label can include arbitrary
> elements then 'focus',  'blur' and 'change' events should be listen from
> 'xhtml:select' element only."  Isn't that what you are already doing in this
> patch?  

No. 'focus' and 'blur' are handled on top binding but binding can contain focusable controls too and hence we will handle events from focusable controls too. It's not right.

> XXX should only be used in a comment to make us aware that there is
> more work to do here.  Otherwise use 'NOTE:' or 'FYI:'

For sure I know.

> Inside select-xhtml.xml (for full appearance):
> 
> - where you throw error, if the Error doesn't throw put some context around the
> error (like file and line number)

I can't find how can I put context info into Error. As usual Error() constructor do it ownself but it don't inside of binding.


> , could you please make the error message more
> specific.  Like throw Error('selectFull: selectedIndex not implemented'); 
> something that will be simple to grep on in 5 months when someone finally runs
> into this problem and reports it to us :-)

I did more specific message. I guess we should fix it before 5 month will be running out. :))

> - why are you setting an attribute for readonly?  Why not use a field?  That
> would be more efficient than doing DOM manipulation, I'd think.

The comment is like for xf:select appearance='compact'.

> 
> - For the XXX's in the full select section, please make sure that there are
> bugs open for these.

I guess it's better to open bugs after check in because I can link on lxr site in bug description :).
Attachment #215475 - Attachment is obsolete: true
Attachment #215877 - Flags: review?(aaronr)
(In reply to comment #21)

> I can see only one
> solution at the moment. xf:label of xf:choices should refresh xf:select like it
> do xf:label of xf:item. Probably it's a bad solution. I need in ideas.
> 

I did a workaround of the problem and I'll open new bug for it.
Comment on attachment 215877 [details] [diff] [review]
patch4

> > - for the 'readonly' property on widget-select-compact, I don't get why you use
> > xbl:inherits="disabled=readonly".  Why not just set @disabled instead of
> > setting @readonly and then having XBL forward the value on to @disabled?
> 
> In general you're right. It's no needed. But I interpret the control widget as
> a widget (sorry for a pun) and threafore I prefer to set attributes for it as
> it is for real widgets. But if you insist then I'll do as you propose.
> 

What you say makes sense if you expect someone to actually create an element that binds to widget-select-compact and widget-select-full.  But if that is the case, you probably should document/comment that you are setting a readonly attribute on these two bindings for the use of elements who bind to them.  Something near the top of the binding so that someone who is looking to bind to them see that they have @readonly available for their use.


> > - you have a XXX comment: "XXX: since xforms:label can include arbitrary
> > elements then 'focus',  'blur' and 'change' events should be listen from
> > 'xhtml:select' element only."  Isn't that what you are already doing in this
> > patch?  
> 
> No. 'focus' and 'blur' are handled on top binding but binding can contain
> focusable controls too and hence we will handle events from focusable controls
> too. It's not right.

I talked to surkov about this on IRC.  He explained that since you inserts the xf:choices' xf:label into the optgroup, it could get focus and blurred by the user, generating events that the control widget handler would handle.  Now I understand.  He's going to fix it in another bug.

> > , could you please make the error message more
> > specific.  Like throw Error('selectFull: selectedIndex not implemented'); 
> > something that will be simple to grep on in 5 months when someone finally runs
> > into this problem and reports it to us :-)
> 
> I did more specific message. I guess we should fix it before 5 month will be
> running out. :))

The change you made to throw a more specific error is great.  Thanks.

> > 
> > - For the XXX's in the full select section, please make sure that there are
> > bugs open for these.
> 
> I guess it's better to open bugs after check in because I can link on lxr site
> in bug description :).
> 

This patch looks good.  With the above comment added to the code about @readonly, r=me.
Attachment #215877 - Flags: review?(aaronr) → review+
Attached patch final patchSplinter Review
Doron, don't you want to look at the patch again after all Aaron's comments before check in?
Attachment #215877 - Attachment is obsolete: true
(In reply to comment #25)
> Created an attachment (id=215967) [edit]
> final patch
> 
> Doron, don't you want to look at the patch again after all Aaron's comments
> before check in?
> 

looks fine
Attached patch for checkin (obsolete) — Splinter Review
Attachment #215967 - Attachment is obsolete: true
Attachment #215967 - Attachment is obsolete: false
Comment on attachment 216384 [details] [diff] [review]
for checkin

Too late. I checked attachment 216384 [details] [diff] [review] in on trunk.
Attachment #216384 - Attachment is obsolete: true
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Whiteboard: xf-to-branch
It appears that xforms-select and xforms-deselect aren't surfacing, see test suite case 4.6.3.a ... perhaps a follow up bug is needed?

I'm currently fixing bug 330825 to suppress value change in select1 for incremental false and noticed select select/deselect missing.
No longer depends on: 327239
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: