298993 - add FAYT (find as you type) for richlistbox for the EM

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, enhancement)

Description

[Przemyslaw Bialik]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b2) Gecko/20050626 Firefox/1.0+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b2) Gecko/20050626 Firefox/1.0+

Bring back FAYT (find as you type) for EM (FF pre 0.9).

Reproducible: Always

Steps to Reproduce:
1. Open EM with a few installed extensions
2. Try to find one by typing it's name

Actual Results:  
Nothing happens.

Expected Results:  
Extension matching FAYT is selected.

Comment 1

[Robert Strong (they/them - no direct email)]

If this were to be added I believe the proper place to do so would be in the
richlistbox widget... this would add fayt to the download manager as well. Also,
the product would be toolkit and the component would be xul widgets

Comment 2

[Nickolay_Ponomarev]

To do that, you'd need to make all richlistitems have common text
attribute/property which will be searched by fayt. There isn't one ATM.

Confirming as a valid enh request for EM, if someone actually starts
implementing this, he may move it to XUL widgets, if it's needed.

Comment 3

[Erik Arvidsson]

The richlistitem has a label getter which concatenates the labels inside it. Is
there a js implementation of fayt for lists in the codebase already? Otherwise
I'm interested in providing one.

Comment 4

[Simon Bünzli]

Attachment: move FAYT handler to listbox-base

This patch enables FAYT for all richlistboxes (including those of the Add-ons and Downloads managers).

For implementors who use a richlistitem's "label" property for extended accessibility and add e.g. a textual representation of images in front of the visible text, there's the option to implement a "fayt_label" property which will be matched instead.

Since there's currently no reliable way to tell which items are actually visible (and thus searchable), that property would have to be used also if the richlistbox is filterable.

See e.g. the Console² extension ( https://addons.mozilla.org/firefox/1815 ) for where this would be needed.

Comment 5

[Neil Deakin]

Comment on attachment 268608 [details] [diff] [review]
move FAYT handler to listbox-base

>+             // allow listitems to specify the string being searched for
>+             // (fayt = Find As You Type)
>+             var faytText = "fayt_label" in item ? item.fayt_label : item.label;

Using a property won't work for rows that aren't visible (scrolled out of view) because they have no XBL attached. (Bug 250123) This would be better as an attribute called "searchlabel".

Is there a way to disable the incremental search?

Comment 6

[Simon Bünzli]

Attachment: move FAYT handler to listbox-base (take II)

(In reply to comment #5)
> Using a property won't work for rows that aren't visible

OTOH using attributes won't work for richlistitems.

What about probing for the properties and falling back to the "label" attribute if no property is found? That should be compatible with all listitems (where label property and attribute are the same anyway) while allowing richlistitems to use properties only.

Not sure whether to check for a "searchlabel" attribute as well - since I don't see listitems containing rich content and thus needing it...

> Is there a way to disable the incremental search?

disableKeyNavigation="true"

Comment 7

[Shawn Wilsher :sdwilsh]

This won't end up working in the download manager because it doesn't use richlistitems in the richlistbox.  We might want to end up changing that.

Comment 8

[Simon Bünzli]

(In reply to comment #7)
Last I tested it worked in the DM as well - it just subclasses the richlistitem binding, but that's both fine and even expected.

Comment 9

[Neil Deakin]

Comment on attachment 268707 [details] [diff] [review]
move FAYT handler to listbox-base (take II)

>+        var start = l > 0 ? this.getIndexOfItem(this.selectedItems[l-1]) : -1;

Add spaces around the minus sign

>+          var searchText = "searchlabel" in listitem ? listitem.searchlabel :
>+                           "label" in listitem ? listitem.label :
>+                           listitem.getAttribute("label"); // for bug 250123

richlistitem should have a 'searchLabel' property declared that returns the value of the 'label' attribute (or the 'searchlabel' attribute but I don't see any reason why label can't be used)

Comment 10

[Simon Bünzli]

Attachment: move FAYT handler to listbox-base (nits fixed)

(In reply to comment #9)
> richlistitem should have a 'searchLabel' property declared that returns the
> value of the 'label' attribute

You surely mean the 'label' property (a label attribute on a richlistitem itself has never had any special meaning).

Comment 11

[Neil Deakin]

> You surely mean the 'label' property

No I don't. Because the label attribute isn't used, you can just use it here. You need to have the search text specified by an attribute otherwise you would need a script to set it.

Comment 12

[Simon Bünzli]

In that case I don't understand what you're saying. Why would I not want to use the label property per default which in many cases returns something quite reasonable (with this patch you get FAYT in both Add-ons Manager and Downloads Manager and for most other users which don't override the label property)?

Comment 13

[Neil Deakin]

Can you give some example XUL code showing how you expect the search label to be used in various cases? It might help me understand what you are expecting.

Comment 14

[Simon Bünzli]

Have a look at the following binding, especially the "label" and "fayt_label" properties:
http://www.mozdev.org/source/browse/console2/src/console2/chrome/content/console2/console2.xml?rev=1.2&content-type=text/x-cvsweb-markup

"label" is used for accessibility and is enhanced with explaining, non-visual information, whereas "fayt_label" either just returns the main string or nothing if the item isn't visible.

Using a separate "label" attribute would require IMO unneeded parallel code (always setting "label" when setting the attributes which are actually displayed) and would make hiding elements pretty much impossible.

In the above cited code, hiding is achieved through CSS rules and we just dynamically check the boxObject's height (slightly hacky, though, since that doesn't seem to be always available). Now, we could do that check ourselves, but as long as the other keyboard navigation related methods don't take hidden listitems into account, either, we could just continue to leave this to implementors (in a reasonable way).

Of course, the common case should be that you get FAYT for free without having to set any additional attribute (which you wouldn't if you require the "label" attribute).

Comment 15

[Neil Deakin]

I mean an example that just uses richlistbox. No XBL, just what is expected from a normal richlistbox with items in it.

Comment 16

[Simon Bünzli]

I still don't understand then. A normal richlistbox is supposed to behave as a normal listbox: you should get FAYT automatically (unless you opt out through "disableKeyNavigation").

You can't do that with a "label" attribute (which simply doesn't exist) but you can easily do that with the "label" property. As I said: applying this patch will give you FAYT for free in most if not all richlistbox implementations (pick any example from the net and it should work).

The only reason for the separate "searchLabel" property is that you can't override the FAYT handler itself and that the "label" property is used for accessibility as well and can thus be modified to contain more complete text-only content (as in the Console² case).

Comment 17

[Neil Deakin]

Yes, which is why I said that the searchLabel property needs to be defined for richlistitem by adding a declaration to its XBL. If the goal of this property is to allow the search label to override and be different than what the label property returns, then it needs to allow the XUL author to specify it using an attribute. After this discussion, it seems that searchlabel is a suitable name for this attribute.

If the author hasn't specified a searchlabel attribute then it should fall back to using the label.

Comment 18

[Simon Bünzli]

Attachment: use the searchlabel attribute (if present)

Thanks for the explanation. I originally thought that in the case where you didn't extend the XBL using the stock "label" property would be enough. That's been the case in all examples I've seen, but of course doesn't need to be so.

This patch now checks for a searchlabel attribute set from XUL before falling back to the "label" property which leads to the same default behavior I described above while allowing to change an item's search term both from XUL and XBL.

Comment 19

[Neil Deakin]

Comment on attachment 270060 [details] [diff] [review]
use the searchlabel attribute (if present)

Yes, this looks good.

>+
>+      <!-- override this property if the label property doesn't return the
>+           a string starting with the first visible label and a static
>+           searchlabel attribute isn't sufficient -->

Since overriding richlistitem isn't the expected way to use richlistbox, don't mention this in the comment.

Also, make sure to note this new change on MDC.

Comment 20

[Simon Bünzli]

Attachment: for check-in

Comment 21

[Phil Ringnalda (:philor)]

toolkit/content/widgets/listbox.xml 1.25
toolkit/content/widgets/richlistbox.xml 1.38

Comment 22

[Mark Finkle (:mfinkle) (use needinfo?)]

Added documentation for searchlabel attribute and searchLabel property. Also updated docs for label property.

see:
http://developer.mozilla.org/en/docs/XUL:richlistitem

Comment 23

[Shawn Wilsher :sdwilsh]

Is there a way to have this disabled?

Comment 24

[Simon Bünzli]

(In reply to comment #23)
Sure, see comment #6, but I hope you have good reason for wanting to do so.