Closed Bug 686575 Opened 13 years ago Closed 13 years ago

Filtering sites on about:permissions feels slow

Categories

(Firefox :: Settings UI, defect)

6 Branch
defect
Not set
normal

Tracking

()

RESOLVED WONTFIX

People

(Reporter: felix, Assigned: felix)

References

Details

Attachments

(1 file, 2 obsolete files)

Because we're using a textbox type="search", there is always a small lag before filtering the sites list. This is OK for filtering large lists, but feels unnecessarily slow in the following flow:

type "mo" -> wait -> list is filtered to 25 items -> append 'z' for "moz" -> wait -> list is filtered to 1-2 items

Appending the letter filtering by the additional letter 'z' should be much faster.
Assignee: nobody → ffung
Depends on: 665527
Attached patch Optimizing some filters (obsolete) — Splinter Review
For users with a small about:permissions list, (which is most users), we can do filtering of the sites-list as soon as key-down and not lock-up the UI.

Note: This is a much simpler diff than what I originally had in mind. Comments below.
Attachment #566402 - Flags: review?(margaret.leibovic)
The above diff only improves things for users with short sites-lists. I opted for this because it'll fix things for most people and the following solution is a lot of work for an uncertain amount of benefit. Warning, most of this comment is left as a note for 'maybe we might want to do this'.

One of the bigger problems is that it might take possibly forever for certain sites to be added to the sites-list. Filtering won't even find that site until the sites-list is finished building. We can first load up all the sites-data into an array without building anything in the DOM. Then incrementally build as regular. If a filter happens, we can prioritize certain sites being added to the DOM.

If we want to improve things further, we can make use of some pretty simple data-structures. One of the worst things we're doing is iterating through the entire sites-list for every single filter. Say your sites-list was an array. After a filter, we can swap elements so that visible elements are in front, hidden elements in the back. Refining a filter (appending characters) means we only need to iterate through the front of the visible array (tracked by some count of visible elements). For every added character, we can keep a number of visible elements (pushing onto a stack) and for every deleted character we just pop back off the stack. Now we can reduce a lot of filters to a subset of sites-list that won't choke the UI and perform them onkeydown.
Comment on attachment 566402 [details] [diff] [review]
Optimizing some filters

This does seem more responsive, but I found a problem with the patch's functionality. If you click on the (x) to clear the search field, the list does not re-filter to show all sites again.

Could you also explain the need for mustFilterSitesList? In what cases is the command event fired but the input event not fired?
Attachment #566402 - Flags: review?(margaret.leibovic) → review-
mustFilterSitesLists does always accompany at least one input event. What is happening is that, in some cases, we won't do anything oninput. If the sites-list is too long and we try to filter on input, the input kinda locks up and key presses are jerky which is really gross. Naturally, oncommand takes care of this by deferring the filter until the user stops typing. So, basically this diff is a hybrid approach to sometimes using onsearch and sometimes oninput.
Attached patch Optimizing some filters (obsolete) — Splinter Review
- Got rid of _hasFiltered
- Made clearing work
Attachment #566402 - Attachment is obsolete: true
Attachment #566998 - Flags: review?(margaret.leibovic)
Comment on attachment 566998 [details] [diff] [review]
Optimizing some filters

>   /**
>+   * Optimization to allow for more responsive filtering
>+   */
>+  FAST_FILTER_THRESHOLD: 300,

How did you come up with this value? I understand you want a limit to prevent the search from becoming unresponsive if there are too many items, but I'm concerned that testing on your (powerful) laptop might not give a value that's good for all users. Perhaps there is a more robust way to come up with a cutoff? I don't want to make this overly complicated, but I also don't want us to accidentally make filtering worse for some users.

>+  mustFilterSitesList: function() {
>+    let filterValue = document.getElementById("sites-filter").value.toLowerCase();
>+    if (filterValue != this._lastFilteredValue) {
>+      this.filterSitesList(filterValue);
>+    }
>+  },
>+

I don't think you need to make this extra function. I think it would be cleaner/clearer if you just stick this logic in filterSitesList, since then you can just get filterValue in one place (you'd also be setting and getting _lastFilteredValue in the same place, which also reduces confusion). The real information you want to be passing to filterSitesList is that it's being triggered by the command event, so that could just passed in as a boolean from the oncommand function call.
Attachment #566998 - Flags: review?(margaret.leibovic) → review-
- Addressed above comments
Attachment #566998 - Attachment is obsolete: true
Attachment #567195 - Flags: review?(margaret.leibovic)
The problem described in comment 0 doesn't seem to be that serious to me (at least not serious enough to fix at the cost of the code complication). Isn't that just the default behavior of <textbox type="search">?

Seems to me like we should WONTFIX this.
Yes it is the default behavior of <textbox type="search"> and honestly, it isn't that great. This change is going to make it feel a lot more responsive for > 90% of users and this probably won't affect any future changes that would be made to filtering logic. It is the death by a thousand cuts, user experience heavily outweighs the additional code complexity. (and yes, I understand it can go the other way as well, but I lean towards UX).
If <textbox type="search"> is not UX-friendly by default, we should just fix that in general (rather than working around it here).
Based on Gavin's comments, I agree that we should WONTFIX this. We can file a new bug to improve the performance of <textbox type="search">, but until that affects more visible parts of the UI, this is still a low priority.
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → WONTFIX
Attachment #567195 - Flags: review?(margaret.leibovic)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: