richlistbox is corrupted if "selected" is inside richlistitem

VERIFIED FIXED

Status

()

defect
VERIFIED FIXED
13 years ago
12 years ago

People

(Reporter: torisugari, Assigned: surkov)

Tracking

({testcase})

Trunk
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 4 obsolete attachments)

Reporter

Description

13 years ago
Posted file 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.
Keywords: testcase
Assignee

Comment 1

13 years ago
Posted patch patch (obsolete) — Splinter Review
Assignee: nobody → surkov.alexander
Status: NEW → ASSIGNED
Attachment #253762 - Flags: second-review?(neil)
Attachment #253762 - Flags: first-review?(enndeakin)
Reporter

Comment 2

13 years ago
(In reply to comment #1)
> Created an attachment (id=253762) [details]
Then, listcol(s) sitll works?
Assignee

Comment 3

13 years ago
(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?
Reporter

Comment 4

13 years ago
(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.
Assignee

Comment 5

13 years ago
(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.
Reporter

Comment 6

13 years ago
Posted file 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?
Assignee

Comment 7

13 years ago
(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 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.)
Attachment #253762 - Flags: second-review?(neil) → second-review-
Assignee

Comment 9

13 years ago
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.
Attachment #253762 - Flags: first-review?(enndeakin)
Assignee

Comment 10

13 years ago
Posted patch patch2 (obsolete) — Splinter Review
Attachment #253762 - Attachment is obsolete: true
Attachment #254178 - Flags: second-review?(enndeakin)
Attachment #254178 - Flags: first-review?(neil)
Reporter

Comment 11

13 years ago
(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()) ?
Reporter

Comment 12

13 years ago
(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

13 years ago
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
Attachment #254178 - Flags: second-review?(enndeakin) → second-review+
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.
Attachment #254178 - Flags: first-review?(neil) → first-review+
Assignee

Comment 15

13 years ago
(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.
Assignee

Comment 16

13 years ago
(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?

Assignee

Comment 17

12 years ago
Posted patch patch3 (obsolete) — Splinter Review
I'm sorry I bother you again, I just want to be sure it goes with you.
Attachment #254178 - Attachment is obsolete: true
Attachment #259019 - Flags: second-review?(neil)
Attachment #259019 - Flags: first-review?(enndeakin)

Comment 18

12 years ago
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.
Attachment #259019 - Flags: first-review?(enndeakin) → first-review-
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.
Attachment #259019 - Flags: second-review?(neil) → second-review-
Assignee

Comment 20

12 years ago
So, Neil, Enn will it be fine to check in patch2 that you r+?
Reporter

Comment 21

12 years ago
What is blocking this bug's progress for 3 months?
Assignee

Comment 22

12 years ago
(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.
What I'd prefer is the patch2 but with the listbox constructor changed to use the while loop from the patch3.
Assignee

Comment 24

12 years ago
Posted patch patch4 (obsolete) — Splinter Review
(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?
Attachment #259019 - Attachment is obsolete: true
Attachment #270578 - Flags: review?(neil)
(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?
Assignee

Comment 26

12 years ago
Posted patch patch5Splinter Review
like this?
Attachment #270578 - Attachment is obsolete: true
Attachment #270745 - Flags: review?(neil)
Attachment #270578 - Flags: review?(neil)
Comment on attachment 270745 [details] [diff] [review]
patch5

Yes, I like this, thanks.
Attachment #270745 - Flags: review?(neil) → review+
Assignee

Comment 28

12 years ago
checked in
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Reporter

Comment 29

12 years ago
->v
Thank you all very much!
Status: RESOLVED → VERIFIED

Updated

12 years ago
Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.