The default bug view has changed. See this FAQ.

richlistbox is corrupted if "selected" is inside richlistitem

VERIFIED FIXED

Status

()

Toolkit
XUL Widgets
VERIFIED FIXED
10 years ago
10 years ago

People

(Reporter: O. Atsushi (Torisugari), Assigned: surkov)

Tracking

({testcase})

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

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 4 obsolete attachments)

1.58 KB, application/vnd.mozilla.xul+xml
Details
1.15 KB, application/vnd.mozilla.xul+xml
Details
3.37 KB, patch
neil@parkwaycc.co.uk
: review+
Details | Diff | Splinter Review
(Reporter)

Description

10 years ago
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.

Updated

10 years ago
Keywords: testcase
(Assignee)

Comment 1

10 years ago
Created attachment 253762 [details] [diff] [review]
patch
Assignee: nobody → surkov.alexander
Status: NEW → ASSIGNED
Attachment #253762 - Flags: second-review?(neil)
Attachment #253762 - Flags: first-review?(enndeakin)
(Reporter)

Comment 2

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

Comment 3

10 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

10 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

10 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

10 years ago
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?
(Assignee)

Comment 7

10 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 8

10 years ago
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

10 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

10 years ago
Created attachment 254178 [details] [diff] [review]
patch2
Attachment #253762 - Attachment is obsolete: true
Attachment #254178 - Flags: second-review?(enndeakin)
Attachment #254178 - Flags: first-review?(neil)
(Reporter)

Comment 11

10 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

10 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

10 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 14

10 years ago
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

10 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

10 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

10 years ago
Created attachment 259019 [details] [diff] [review]
patch3

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

10 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 19

10 years ago
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

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

Comment 21

10 years ago
What is blocking this bug's progress for 3 months?
(Assignee)

Comment 22

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

Comment 23

10 years ago
What I'd prefer is the patch2 but with the listbox constructor changed to use the while loop from the patch3.
(Assignee)

Comment 24

10 years ago
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?
Attachment #259019 - Attachment is obsolete: true
Attachment #270578 - Flags: review?(neil)

Comment 25

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

10 years ago
Created attachment 270745 [details] [diff] [review]
patch5

like this?
Attachment #270578 - Attachment is obsolete: true
Attachment #270745 - Flags: review?(neil)
Attachment #270578 - Flags: review?(neil)

Comment 27

10 years ago
Comment on attachment 270745 [details] [diff] [review]
patch5

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

Comment 28

10 years ago
checked in
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
(Reporter)

Comment 29

10 years ago
->v
Thank you all very much!
Status: RESOLVED → VERIFIED
Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.