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

RESOLVED FIXED in mozilla1.9alpha8

Status

()

Toolkit
XUL Widgets
--
enhancement
RESOLVED FIXED
12 years ago
10 years ago

People

(Reporter: Przemyslaw Bialik, Assigned: Simon Bünzli)

Tracking

({dev-doc-complete})

unspecified
mozilla1.9alpha8
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

12 years ago
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.
Severity: minor → enhancement
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

12 years ago
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 3

12 years ago
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.
Component: Extension/Theme Manager → XUL Widgets
Product: Firefox → Toolkit
QA Contact: extension.manager → xul.widgets
Summary: bring back FAYT (find as you type) for EM → add FAYT (find as you type) for richlistbox for the EM
(Assignee)

Comment 4

10 years ago
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.
Assignee: nobody → zeniko
Status: NEW → ASSIGNED
Attachment #268608 - Flags: review?(enndeakin)

Comment 5

10 years ago
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?
Attachment #268608 - Flags: review?(enndeakin) → review-
(Assignee)

Comment 6

10 years ago
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"
Attachment #268608 - Attachment is obsolete: true
Attachment #268707 - Flags: review?(enndeakin)
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.
(Assignee)

Comment 8

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

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

Comment 10

10 years ago
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).
Attachment #268707 - Attachment is obsolete: true
Attachment #270018 - Flags: review?(enndeakin)
Attachment #268707 - Flags: review?(enndeakin)

Comment 11

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

Comment 12

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

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

Comment 14

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

10 years ago
I mean an example that just uses richlistbox. No XBL, just what is expected from a normal richlistbox with items in it.
(Assignee)

Comment 16

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

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

Comment 18

10 years ago
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.
Attachment #270018 - Attachment is obsolete: true
Attachment #270060 - Flags: review?(enndeakin)
Attachment #270018 - Flags: review?(enndeakin)

Comment 19

10 years ago
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.
Attachment #270060 - Flags: review?(enndeakin) → review+
(Assignee)

Comment 20

10 years ago
Created attachment 270137 [details] [diff] [review]
for check-in
Attachment #270060 - Attachment is obsolete: true
(Assignee)

Updated

10 years ago
Whiteboard: [checkin needed]
Target Milestone: --- → mozilla1.9beta1
(Assignee)

Updated

10 years ago
Keywords: dev-doc-needed
(Assignee)

Updated

10 years ago
Blocks: 317422
toolkit/content/widgets/listbox.xml 1.25
toolkit/content/widgets/richlistbox.xml 1.38
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Whiteboard: [checkin needed]
Added documentation for searchlabel attribute and searchLabel property. Also updated docs for label property.

see:
http://developer.mozilla.org/en/docs/XUL:richlistitem
Keywords: dev-doc-needed
(Assignee)

Updated

10 years ago
Blocks: 386482

Updated

10 years ago
Flags: in-testsuite?

Updated

10 years ago
Keywords: dev-doc-complete
Is there a way to have this disabled?
(Assignee)

Comment 24

10 years ago
(In reply to comment #23)
Sure, see comment #6, but I hope you have good reason for wanting to do so.
You need to log in before you can comment on or make changes to this bug.