Last Comment Bug 298993 - add FAYT (find as you type) for richlistbox for the EM
: add FAYT (find as you type) for richlistbox for the EM
Status: RESOLVED FIXED
: dev-doc-complete
Product: Toolkit
Classification: Components
Component: XUL Widgets (show other bugs)
: unspecified
: All All
-- enhancement with 5 votes (vote)
: mozilla1.9alpha8
Assigned To: Simon Bünzli
:
: Neil Deakin
Mentors:
Depends on:
Blocks: 317422 386482
  Show dependency treegraph
 
Reported: 2005-06-28 03:09 PDT by Przemyslaw Bialik
Modified: 2007-07-12 10:29 PDT (History)
12 users (show)
sdwilsh: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
move FAYT handler to listbox-base (6.96 KB, patch)
2007-06-16 07:43 PDT, Simon Bünzli
enndeakin: review-
Details | Diff | Splinter Review
move FAYT handler to listbox-base (take II) (7.42 KB, patch)
2007-06-17 14:21 PDT, Simon Bünzli
no flags Details | Diff | Splinter Review
move FAYT handler to listbox-base (nits fixed) (8.59 KB, patch)
2007-06-27 09:13 PDT, Simon Bünzli
no flags Details | Diff | Splinter Review
use the searchlabel attribute (if present) (8.85 KB, patch)
2007-06-27 14:13 PDT, Simon Bünzli
enndeakin: review+
Details | Diff | Splinter Review
for check-in (8.64 KB, patch)
2007-06-27 23:08 PDT, Simon Bünzli
no flags Details | Diff | Splinter Review

Description User image Przemyslaw Bialik 2005-06-28 03:09:21 PDT
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 User image Robert Strong [:rstrong] (use needinfo to contact me) 2005-06-28 17:12:01 PDT
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 User image Nickolay_Ponomarev 2005-08-06 17:50:25 PDT
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 User image Erik Arvidsson 2005-08-21 10:53:38 PDT
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 User image Simon Bünzli 2007-06-16 07:43:25 PDT
Created attachment 268608 [details] [diff] [review]
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 User image Neil Deakin 2007-06-17 12:12:11 PDT
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 User image Simon Bünzli 2007-06-17 14:21:26 PDT
Created attachment 268707 [details] [diff] [review]
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 User image Shawn Wilsher :sdwilsh 2007-06-17 14:34:33 PDT
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 User image Simon Bünzli 2007-06-17 14:46:40 PDT
(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 User image Neil Deakin 2007-06-27 08:08:53 PDT
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 User image Simon Bünzli 2007-06-27 09:13:14 PDT
Created attachment 270018 [details] [diff] [review]
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 User image Neil Deakin 2007-06-27 09:40:07 PDT
> 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 User image Simon Bünzli 2007-06-27 09:49:20 PDT
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 User image Neil Deakin 2007-06-27 10:21:12 PDT
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 User image Simon Bünzli 2007-06-27 11:15:03 PDT
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 User image Neil Deakin 2007-06-27 11:41:54 PDT
I mean an example that just uses richlistbox. No XBL, just what is expected from a normal richlistbox with items in it.
Comment 16 User image Simon Bünzli 2007-06-27 13:27:58 PDT
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 User image Neil Deakin 2007-06-27 13:35:00 PDT
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 User image Simon Bünzli 2007-06-27 14:13:37 PDT
Created attachment 270060 [details] [diff] [review]
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 User image Neil Deakin 2007-06-27 14:25:57 PDT
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 User image Simon Bünzli 2007-06-27 23:08:09 PDT
Created attachment 270137 [details] [diff] [review]
for check-in
Comment 21 User image Phil Ringnalda (:philor) 2007-06-30 19:17:08 PDT
toolkit/content/widgets/listbox.xml 1.25
toolkit/content/widgets/richlistbox.xml 1.38
Comment 22 User image Mark Finkle (:mfinkle) (use needinfo?) 2007-06-30 21:39:30 PDT
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 User image Shawn Wilsher :sdwilsh 2007-07-12 07:42:00 PDT
Is there a way to have this disabled?
Comment 24 User image Simon Bünzli 2007-07-12 10:29:09 PDT
(In reply to comment #23)
Sure, see comment #6, but I hope you have good reason for wanting to do so.

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