Last Comment Bug 369077 - richlistbox is corrupted if "selected" is inside richlistitem
: richlistbox is corrupted if "selected" is inside richlistitem
Status: VERIFIED FIXED
: testcase
Product: Toolkit
Classification: Components
Component: XUL Widgets (show other bugs)
: Trunk
: All All
-- normal (vote)
: ---
Assigned To: alexander :surkov
:
: Neil Deakin
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2007-02-02 04:34 PST by O. Atsushi (Torisugari)
Modified: 2007-09-08 23:18 PDT (History)
7 users (show)
jwalden+bmo: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Testcase (1.58 KB, application/vnd.mozilla.xul+xml)
2007-02-02 04:34 PST, O. Atsushi (Torisugari)
no flags Details
patch (3.41 KB, patch)
2007-02-02 09:50 PST, alexander :surkov
neil: second‑review-
Details | Diff | Splinter Review
Testcase (listbox) (1.15 KB, application/vnd.mozilla.xul+xml)
2007-02-03 15:43 PST, O. Atsushi (Torisugari)
no flags Details
patch2 (3.53 KB, patch)
2007-02-06 09:40 PST, alexander :surkov
neil: first‑review+
enndeakin: second‑review+
Details | Diff | Splinter Review
patch3 (3.70 KB, patch)
2007-03-19 12:20 PDT, alexander :surkov
enndeakin: first‑review-
neil: second‑review-
Details | Diff | Splinter Review
patch4 (3.75 KB, patch)
2007-07-02 07:25 PDT, alexander :surkov
no flags Details | Diff | Splinter Review
patch5 (3.37 KB, patch)
2007-07-03 09:38 PDT, alexander :surkov
neil: review+
Details | Diff | Splinter Review

Description User image O. Atsushi (Torisugari) 2007-02-02 04:34:56 PST
Created attachment 253746 [details]
Testcase

Probably, this is a regression introduced by bug 298371.
Maybe
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/toolkit/content/widgets/richlistbox.xml&rev=1.33&mark=374-378#374
?

If a child node of a richlistitem has the property "selected",
|this.selectedItems| contains the node.

In the testcase, XUL "radio" element also has the property "selcted".
And this "slected" is a readonly property, while richlistitem's is settable.
So the behavior is pretty buggy, due to tons of script errors.

Note that the testcase works fine with Fx2.0 and GranParadisoA1. A trunk bug.
Comment 1 User image alexander :surkov 2007-02-02 09:50:02 PST
Created attachment 253762 [details] [diff] [review]
patch
Comment 2 User image O. Atsushi (Torisugari) 2007-02-02 13:52:37 PST
(In reply to comment #1)
> Created an attachment (id=253762) [details]
Then, listcol(s) sitll works?
Comment 3 User image alexander :surkov 2007-02-02 19:15:48 PST
(In reply to comment #2)
> (In reply to comment #1)
> > Created an attachment (id=253762) [details] [details]
> Then, listcol(s) sitll works?
> 

Sorry, listcols for listbox or richlistbox? Can you give an example?
Comment 4 User image O. Atsushi (Torisugari) 2007-02-02 19:47:07 PST
(In reply to comment #3)
> Sorry, listcols for listbox or richlistbox? Can you give an example?

I meant initialization of listcol for listbox, as it is moving from
listbox-base to listbox. But I realized that there's no problem. I'm
sorry for that.

BTW, you are not going to fix this bug for listbox? I guess  this part
would cause the same problem, if a <radio/> element is in a <listitem/>
element...
+      <constructor>
+      <![CDATA[
+        var els = this.getElementsByAttribute("selected", "true");
+        for (var i = 0; i < els.length; ++i)
+          this.selectedItems.push(els[i]);
+      ]]>
+      </constructor>
+

Or listbox is not *rich* enough to have any radio inside? I'm not too
sure about other XUL elements which has "selected" attribute.
Comment 5 User image alexander :surkov 2007-02-02 20:32:31 PST
(In reply to comment #4)

> Or listbox is not *rich* enough to have any radio inside? I'm not too
> sure about other XUL elements which has "selected" attribute.
> 

Generally listitem can have any elements but I'm not sure is it allowed. This question I guess to toolkit's guys.
Comment 6 User image O. Atsushi (Torisugari) 2007-02-03 15:43:02 PST
Created attachment 253894 [details]
Testcase (listbox)

(In reply to comment #5)
> 
> Generally listitem can have any elements but I'm not sure is it allowed. This
> question I guess to toolkit's guys.
>

So will you adress the listbox issue, by moving
"children" property to listbox-base if necessary?
Comment 7 User image alexander :surkov 2007-02-04 04:26:06 PST
(In reply to comment #6)
> So will you adress the listbox issue, by moving
> "children" property to listbox-base if necessary?
> 

np if it's fine with listbox
Comment 8 User image neil@parkwaycc.co.uk 2007-02-06 04:25:15 PST
Comment on attachment 253762 [details] [diff] [review]
patch

I think if you're going to fix richlistbox you might as well fix listbox too. (Note that an alternative children enumerator is available to listbox.)
Comment 9 User image alexander :surkov 2007-02-06 07:27:28 PST
Comment on attachment 253762 [details] [diff] [review]
patch

(In reply to comment #8)
> (From update of attachment 253762 [details] [diff] [review])
> I think if you're going to fix richlistbox you might as well fix listbox too.
> (Note that an alternative children enumerator is available to listbox.)
> 

Ok.
Comment 10 User image alexander :surkov 2007-02-06 09:40:10 PST
Created attachment 254178 [details] [diff] [review]
patch2
Comment 11 User image O. Atsushi (Torisugari) 2007-02-06 12:37:18 PST
(In reply to comment #8)
> (Note that an alternative children enumerator is available to listbox.)

I'm just curious about what an alternative enumerator was.
this.getIndexOfItem() (nsIListboxObject::getIndexOfItem()) ?
Comment 12 User image O. Atsushi (Torisugari) 2007-02-06 15:50:35 PST
(In reply to comment #11)
> (In reply to comment #8)
> > (Note that an alternative children enumerator is available to listbox.)
> 
> I'm just curious about what an alternative enumerator was.
> this.getIndexOfItem() (nsIListboxObject::getIndexOfItem()) ?
> 

Oh, s/getIndexOfItem/getItemAtIndex/ sorry for spamming.
Comment 13 User image Neil Deakin 2007-02-07 07:26:26 PST
Comment on attachment 254178 [details] [diff] [review]
patch2

Looks OK, although you can still use the same code you've used for listbox with richlistbox
Comment 14 User image neil@parkwaycc.co.uk 2007-02-13 06:14:44 PST
Comment on attachment 254178 [details] [diff] [review]
patch2

Actually my preference for listbox would be a while loop similar to the selectAll code.

Sorry for the delay.
Comment 15 User image alexander :surkov 2007-02-13 08:47:25 PST
(In reply to comment #13)
> (From update of attachment 254178 [details] [diff] [review])
> Looks OK, although you can still use the same code you've used for listbox with
> richlistbox
> 

I'd like to save 'children' properties using for richlistbox because it keeps the work with richlistitems in one place. Also, I guess xforms has usecase for this. XForms can use richlistbox binding to create xforms:select element for XUL documents. There children collection should overriden to allow hierarchical richlistitems. If you haven't strong preferences then I'd like to save that code of the patch.
Comment 16 User image alexander :surkov 2007-02-13 08:49:16 PST
(In reply to comment #14)
> (From update of attachment 254178 [details] [diff] [review])
> Actually my preference for listbox would be a while loop similar to the
> selectAll code.

I'm sorry do you prefer to use getItemAtIndex/getNextItem? If so, then I guess it makes sense for richlistbox too, that may allow to have common code. Right?

Comment 17 User image alexander :surkov 2007-03-19 12:20:39 PDT
Created attachment 259019 [details] [diff] [review]
patch3

I'm sorry I bother you again, I just want to be sure it goes with you.
Comment 18 User image Neil Deakin 2007-03-19 12:41:42 PDT
Comment on attachment 259019 [details] [diff] [review]
patch3

>+      <method name="setInitialSelection">
>+        <body>
>+          var item = this.getItemAtIndex(0);
>+          while (item) {
>+            if (item.hasAttribute("selected"))
>+              this.selectedItems.push(item);
>+            item = this.getNextItem(item, 1);
>+          }
>+        </body>
>+      </method>

This won't work for listboxes due to bug 250123, so the listitems that are scrolled out of view won't have xbl attached to them yet so they won't implement the nsIDOMXULSelectControlItemElement interface.
Comment 19 User image neil@parkwaycc.co.uk 2007-03-20 03:57:41 PDT
Comment on attachment 259019 [details] [diff] [review]
patch3

>+            item = this.getNextItem(item, 1);
I didn't realise richlistbox doesn't have getNextItem, so you'll have to loop over the indices anyway.
Comment 20 User image alexander :surkov 2007-03-20 11:36:55 PDT
So, Neil, Enn will it be fine to check in patch2 that you r+?
Comment 21 User image O. Atsushi (Torisugari) 2007-06-23 10:06:31 PDT
What is blocking this bug's progress for 3 months?
Comment 22 User image alexander :surkov 2007-06-23 12:28:18 PDT
(In reply to comment #21)
> What is blocking this bug's progress for 3 months?
> 

I didn't realize Neil and Enn aren't CC'ed on the bug.
Comment 23 User image neil@parkwaycc.co.uk 2007-07-02 01:59:14 PDT
What I'd prefer is the patch2 but with the listbox constructor changed to use the while loop from the patch3.
Comment 24 User image alexander :surkov 2007-07-02 07:25:37 PDT
Created attachment 270578 [details] [diff] [review]
patch4

(In reply to comment #23)
> What I'd prefer is the patch2 but with the listbox constructor changed to use
> the while loop from the patch3.
> 

I can't use that loop due to bug 250123. Is it ok?
Comment 25 User image neil@parkwaycc.co.uk 2007-07-03 09:27:11 PDT
(In reply to comment #24)
>I can't use that loop due to bug 250123. Is it ok?
Oh, I forgot about that. Maybe we should use getItemAtIndex?
Comment 26 User image alexander :surkov 2007-07-03 09:38:30 PDT
Created attachment 270745 [details] [diff] [review]
patch5

like this?
Comment 27 User image neil@parkwaycc.co.uk 2007-07-04 04:13:41 PDT
Comment on attachment 270745 [details] [diff] [review]
patch5

Yes, I like this, thanks.
Comment 28 User image alexander :surkov 2007-07-04 05:23:22 PDT
checked in
Comment 29 User image O. Atsushi (Torisugari) 2007-07-06 22:03:43 PDT
->v
Thank you all very much!

Note You need to log in before you can comment on or make changes to this bug.