377686 - listbox.xml/richlistbox.xml cleanup

Status: NEW

(Toolkit :: UI Widgets, defect)

Description

[Neil Deakin]

- clean up inheritance
- setters should return values
- set value attribute when selecting an item
- add listitem.checked property

Comment 1

[Neil Deakin]

Attachment: fix issues as described above

Comment 2

[Simon Bünzli]

Index: toolkit/content/widgets/richlistbox.xml
===================================================================
-      <property name="label" readonly="true">
+      <property name="label">
+        <setter>
+          return val;
+        </setter>

Wouldn't it be more appropriate to throw an error (NS_ERROR_NOT_AVAILABLE or NOT_IMPLEMENTED) instead of silently failing to do anything?

Besides: any chance of getting keypress handling into the base class? There really aren't any differences between http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/toolkit/content/widgets/listbox.xml&rev=1.23&mark=710-721#709 and http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/toolkit/content/widgets/richlistbox.xml&rev=1.35&mark=464-487#462 (except for the phase, but that one shouldn't really matter to a listbox anyway)?

With that done, the following two handlers could also be shared between listbox and richlistbox: http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/toolkit/content/widgets/listbox.xml&rev=1.23&mark=740-795#740 which would finally make fixing bug 298993 a piece of cake.

Comment 3

[Neil Deakin]

(In reply to comment #2)
> Index: toolkit/content/widgets/richlistbox.xml
> ===================================================================
> -      <property name="label" readonly="true">
> +      <property name="label">
> +        <setter>
> +          return val;
> +        </setter>
> 
> Wouldn't it be more appropriate to throw an error (NS_ERROR_NOT_AVAILABLE or
> NOT_IMPLEMENTED) instead of silently failing to do anything?
> 

No, it is implemented, and just does nothing. Or instead it could just create a child label and set it to the value.

> Besides: any chance of getting keypress handling into the base class? There
> really aren't any differences between
> 

Yes. If you can create a patch, that would be good.

Comment 4

[Simon Bünzli]

Attachment: move keyboard navigation handling to list-base

When this patch is checked in, I'll tackle bug 298993...

(In reply to comment #3)
> No, it is implemented, and just does nothing. Or instead it could just create a
> child label and set it to the value.

Creating a child label would have the unexpected consequence of adding to the .label instead of replacing it. As it currently stands, .label is designed as a read-only property and should IMO remain as such until there's a reasonable way of implementing it so that it behaves as you might expect from listboxes (which is neither having a no-op setter nor an appending setter).

Comment 5

[Neil Deakin]

(In reply to comment #4)
> Created an attachment (id=266497) [details]
> move keyboard navigation handling to list-base
> 
> When this patch is checked in, I'll tackle bug 298993...
> 
> (In reply to comment #3)
> > No, it is implemented, and just does nothing. Or instead it could just create a
> > child label and set it to the value.
> 
> Creating a child label would have the unexpected consequence of adding to the
> .label instead of replacing it. 

It would naturally remove eveything else that was there too.

Comment 6

[Simon Bünzli]

(In reply to comment #5)
> It would naturally remove eveything else that was there too.

Makes sense... probably still not what I'd expect, but at least in line with how richlistbox's appendItem / insertItemAt already work (as a glorified listbox).

Comment 7

[Neil Deakin]

Comment on attachment 266497 [details] [diff] [review]
move keyboard navigation handling to list-base

The phase attributes should still be there.

Comment 8

[Simon Bünzli]

Attachment: move keyboard navigation handling to list-base (target="phase" only)

Comment 9

[Neil Deakin]

Comment on attachment 266502 [details] [diff] [review]
move keyboard navigation handling to list-base (target="phase" only)

Looks good. You may want to informally ask Neil Rashbrook about it too since he'd know about the phase stuff.

Comment 10

[Simon Bünzli]

I guess restricting cursor movement to the target phase makes even more sense for richlistboxes where you might want to use them for navigating between in-item-controls... Currently the focus in the Add-ons manager goes haywire when you tab to one of the buttons and then hit [Up] and [Down] a few times.

BTW: Would you mind checking this patch in, once the Alpha 5 freeze is over?

Comment 11

[Michael Wu [:mwu]]

Checking in toolkit/content/widgets/listbox.xml;
/cvsroot/mozilla/toolkit/content/widgets/listbox.xml,v  <--  listbox.xml
new revision: 1.24; previous revision: 1.23
done
Checking in toolkit/content/widgets/richlistbox.xml;
/cvsroot/mozilla/toolkit/content/widgets/richlistbox.xml,v  <--  richlistbox.xml
new revision: 1.36; previous revision: 1.35
done

Comment 12

[Simon Bünzli]

Reopening since Neil's own clean-up patch for this bug hasn't been landed yet.

Comment 13

[Michael Wu [:mwu]]

Sorry, didn't know there were two patches that needed to be checked in.

Comment 15

[Michael Wu [:mwu]]

Is the first patch going to get any review?

Comment 16

[Georg Maaß]

Hi,

are these modifications here responsible for the error message:
(TypeError): setting a property that has only a getter

in XulRunner Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; de-DE; rv:1.9a7pre) Gecko/2007071308 and also on the Windows platform when I try to set the selected property of a list item? In 1.9a6pre an earlier this did not occur. Or should I look for an other bug for this issue?

Comment 17

[Simon Bünzli]

Neil: please note that bug 386886 has introduced an additional reference to #listbox-base in mailnews/base/resources/content/mailWidgets.xml