Implement as-you-type filtering #220

erayd · 2018-03-19T10:19:56Z

If there are search results present:

Typing in the search field will filter the results.
Pressing enter will autofill the first entry.
Pressing enter when there are no matching entries will run a normal search.

If there are search results present, no filter has been entered, and Backspace is pressed:

All search results are cleared, as is the search input box.
Pressing enter will run a normal search.

The reason for the Backspace feature is to allow the user to explicitly run a manual search instead of filtering, even when the automatic domain search returns matches.

If the search results are the outcome of a manual search, and no filter text has been entered, then pressing enter will not auto-fill the first entry, but will instead re-run the search. This is to prevent the user accidentally filling a login form by pressing enter twice when running a manual search.

This PR closes #40 and #211.

If there are search results present: * Typing in the search field will filter the results * Pressing <enter> will autofill the first entry * Pressing <enter> when there are no matching entries will run a normal search If there are search results present, no filter has been entered, and '\' is pressed: * All search results are cleared, as is the search input box * Pressing <enter> will run a normal search

maximbaz · 2018-03-19T10:35:33Z

Awesome stuff! My initial thoughts:

I'd like to have a visual indication of which mode we are in, filtering or searching. An interesting idea was suggested in Filter the list of already discovered passwords #211, show an indicator on the left side inside of the search box (and clear it with Backspace instead of \, because that's how Chromium behaves). Doesn't need to be fancy looking, but should be "not ugly" 🙂
Filtering should be fuzzy - again, no need for fancy external libraries, don't change the order of search results, but something very basic, like wk should find work.

erayd · 2018-03-19T10:47:46Z

@maximbaz

I'd like to have a visual indication of which mode we are in, filtering or searching.

No problem - I'll add this.

clear it with Backspace instead of \

Chrome doesn't appear to fire the onkeypress event for backspace in the search field, although it does for any other key. Do you know how to catch this?

Filtering should be fuzzy - again, no need for fancy external libraries, don't change the order of search results, but something very basic, like wk should find work.

I strongly dislike this idea. The outcome of fuzzy searches tend to be non-obvious, and when I'm wanting to quickly filter a list, I need it to be predictable. IMO, fuzzy searches belong in the main search feature, not in the filter.

The way it works now is substring matching for each word in the filter, separated by either whitespace or /. Is there a reason this is undesirable? I'm struggling to understand how fuzzy searching is useful in a quick-filter.

erayd · 2018-03-19T10:50:12Z

Chrome doesn't appear to fire the onkeypress event for backspace in the search field, although it does for any other key. Do you know how to catch this?

Never mind - onkeydown does catch it, so I'll just use that.

maximbaz · 2018-03-19T11:03:00Z

My experience is a lot different with fuzzy search, I find it very useful - also, when done properly, typing "work" should find the "work" entry as the top one, so you would not experience any difference comparing to the substring implementation. Try searching for full substrings in the domain search in v2.0.12, it is fuzzy by default, but nevertheless searching for substrings should put the most expected results on top. However when you get used to typing "wk", you will be pleased that it works too 🙂

However I realize that perhaps a good fuzzy search is not easy to implement and probably requires some dependency to be working acceptably well. So, a substring search is definitely preferred over a bad-working fuzzy search, but a good-working fuzzy search is preferred over the substring search 🙂

And if you are still not convinced, I'm fine with implementing a configuration option, whether filtering should be fuzzy or not. In that case, fuzzy filter and configuration option don't even have to be part of this PR.

erayd · 2018-03-19T11:11:51Z

However I realize that perhaps a good fuzzy search is not easy to implement and probably requires some dependency to be working acceptably well. So, a substring search is definitely preferred over a bad-working fuzzy search, but a good-working fuzzy search is preferred over the substring search.

Do you have a library to recommend for this, that you would be happy to have included in the extension?

And if you are still not convinced, I'm fine with implementing a configuration option, whether filtering should be fuzzy or not. Doesn't even have to be part of this PR.

If you're happy with a config option, then I have no problem with the filtering being fuzzy, even by default. Just so long as I can turn the fuzzy stuff off :-).

maximbaz · 2018-03-19T11:29:34Z

I'll try to test some libraries later today 👍

maximbaz · 2018-03-19T13:16:15Z

I'm using this PR and will be publishing new findings as I find something:

Open browserpass, it is filtering mode, type something so that no results are left - the indicator of "filter mode" is gone, but should stay.
Open browserpass, it is in filtering mode, press Backspace to switch to search mode, type "githud", press Enter, press Backspace once (in attempt to fix the typo and change "githud" to "github") - the entire search field is cleared, but only the last char should be removed.

erayd · 2018-03-19T13:26:29Z

Open browserpass, it is filtering mode, type something so that no results are left - the indicator of "filter mode" is gone, but should stay.

This is deliberate - if you filter out all results (i.e. to make the result list empty), it switches to regular search mode. That's why the hint vanishes. Can you explain what the desired behavior should be instead?

Typo fixing - good catch. Need to think about how to resolve that one.

I've swiched to fuzzy search using fuse.js - does how I've implemented it fit what you were expecting?

erayd · 2018-03-19T13:28:49Z

OK, I've sorted the typo thing. Is that now working as you expected it to work?

maximbaz · 2018-03-19T13:31:05Z

This is deliberate - if you filter out all results (i.e. to make the result list empty), it switches to regular search mode. That's why the hint vanishes. Can you explain what the desired behavior should be instead?

Aaaah, I see - no, keep it like this, very useful indeed. I just didn't realize that it switches to the search mode, I thought it is still in the filter mode 🙂

I'll test the fuzzy search later today 👍

maximbaz · 2018-03-19T13:43:39Z

Regarding making fuzzy filter configurable, there is already an option for fuzzy search, I'm wondering if it makes sense to reuse that same option for fuzzy filtering, or introduce a new option. Is it possible that a user would like to disable fuzzy search, but enable fuzzy filtering, or the other way around? Consider that it is also not obvious for people unfamiliar with the code to understand what's the difference between option "Fuzzy search" and "Fuzzy filter". What do you recommend?

erayd · 2018-03-19T13:47:39Z

I found that making the filter a little more strict was enough that I was happy with fuzzy filtering as the sole option. So I would be happy to leave it as-is, and not worry about substring matching.

If you think that it would be better to include both filter methods, then yes - using the existing option for fuzzy search seems like a good idea.

maximbaz · 2018-03-19T14:00:29Z

OK, let's keep the fuzzy filtering as the sole option for now.

I'm only unhappy with the library, I saw people complain about it, and now I see why:

ig should already find no result, and no additions to ig should add results.

I'm reviewing a few options now, what do you think about this library?

https://github.com/farzher/fuzzysort

If we choose it, let's disallow typos to make it strict (i.e. allowTypo: false if it's true by default).

I haven't tested it myself yet in much details, but the demo seems quite good. Also this library is maintained now (contrary to some other existing libraries), which I like.

erayd · 2018-03-19T14:02:39Z

I see what you mean - the behavior in your example is horrible! I'll take a look at the one you're suggesting now, hang on.

erayd · 2018-03-19T14:13:31Z

That library seems to be overly strict with filtering on patterns that aren't at the start of a word. For example, hub does not match github. This is problematic.

maximbaz · 2018-03-19T14:14:50Z

Hmm this is not happening on their demo page at least...

erayd · 2018-03-19T14:18:14Z

It happens as soon as you set a strictness threshold. Looks like it might be OK with no threshold set though - have just commited it so you can have a look. What do you think?

erayd · 2018-03-19T14:23:48Z

I might need to do an iterated version; seems it can't handle multi-word searches.

maximbaz · 2018-03-19T14:44:07Z

I don't mind at all, if you like it, let's keep it 👍

From the usage point of view, this looks pretty solid. I'll review the code a bit later, and will test more in meantime.

Found this on chrome://extensions, could you please have a look?

erayd · 2018-03-19T14:50:38Z

Found this on chrome://extensions, could you please have a look?

Could you please clarify how / where to reproduce that, if it's still an issue? I've just pushed a commit that fixes the type, but I would like to test to ensure that solved the issue.

maximbaz · 2018-03-19T14:53:18Z

I didn't see when exactly that happened, but I think you most probably did fix it with the last commit. I cleared all the errors and will be testing the code in its latest stage, if any errors appear I'll compose the repro steps.

erayd · 2018-03-19T15:02:39Z

Sounds good - thanks :-).

…s used

maximbaz · 2018-03-19T15:30:48Z

chrome/script.browserify.js

+
+function searchKeyHandler(e) {
+  // switch to search mode if '\' is pressed and no filter text has been entered
+  if (e.code == "Backspace" && logins && logins.length > 0 && (!e.target.value.length || e.target.value == domain)) {


I think && logins is not needed anymore since it can never be null

switch to search mode if '\' is pressed - needs to be Backspace

maximbaz · 2018-03-19T15:32:05Z

chrome/script.browserify.js

-  if (!this.s.value.length) {
-    return;
-  }
+  if (fillOnSubmit && logins && logins.length > 0) {


I think && logins is not needed anymore since it can never be null

maximbaz · 2018-03-19T15:35:06Z

chrome/script.browserify.js

-          logins = response;
+          logins = resultLogins = response ? response : [];
+          document.getElementById("filter-search").textContent = domain;
+          fillOnSubmit = useFillOnSubmit && logins && logins.length > 0;


&& logins

maximbaz · 2018-03-19T15:37:59Z

chrome/script.browserify.js

@@ -222,14 +298,16 @@ function keyHandler(e) {
      switchFocus("div.entry:first-child > .login", "nextElementSibling");
      break;
    case "c":
-      if (e.ctrlKey) {
+      if (e.target.id != "search-field" && e.ctrlKey) {


both copy shortcuts seem to not work for me anymore, do they work for you?

Investigating...

These seem to be working for me. Perhaps fixed by one of the other commits?

Are you still having issues, or are they working for you now?

Emm they actually don't even work for me in release version... okay, nevermind, I'll deal with them separately.

One thought - is the item actually selected when you use the copy shortcuts? I did change the behavior so that they did not run when the search box was selected, because they conflicted with entering filter text (i.e. typing a capital C resulted in a copy-username-and-close behavior, which is obviously undesirable). That's what the target.id check is for.

maximbaz · 2018-03-19T15:46:21Z

chrome/script.browserify.js

+function filterLogins(e) {
+  // remove executed search from input field
+  if (!fillOnSubmit && e.target.value.indexOf(domain) === 0) {
+    e.target.value = e.target.value.substr(domain.length);


could you please explain to me the purpose of this piece?

It works weirdly...:

Open browserpass, it is in filter mode

Press Backspace to switch to search mode

Type "gith", press Enter

Press Backspace, everything is cleared (difficult to fix typo - not because of this code block, but still a valid issue)

Slowly type "gith" again, when you enter "h" of "gith" the field gets cleared again (because of this code block)

It's a bug. Was half of a previous solution, which I have now removed.

maximbaz · 2018-03-19T15:50:41Z

chrome/script.browserify.js

+  // switch to search mode if '\' is pressed and no filter text has been entered
+  if (e.code == "Backspace" && logins && logins.length > 0 && (!e.target.value.length || e.target.value == domain)) {
+    e.preventDefault();
+    logins = resultLogins = [];


This is very minor thing and most probably we shouldn't do anything here, but posting in case you have some idea how to avoid the confusion. If not, don't worry about this.

I press Backspace and I am told that there are no passwords for github.com.

I will fix this.

maximbaz · 2018-03-19T15:58:55Z

chrome/script.browserify.js

+  m.redraw();
+
+  // show / hide the filter hint
+  showFilterHint(logins && logins.length);


one more place with logins &&

maximbaz · 2018-03-19T16:17:53Z

chrome/script.browserify.js

+          logins = resultLogins = response ? response : [];
+          document.getElementById("filter-search").textContent = domain;
+          fillOnSubmit = useFillOnSubmit && logins.length > 0;
+          if (logins && logins.length > 0) {


one more place with logins &&... sorry 😃

maximbaz · 2018-03-19T16:21:58Z

Is it intentional that now after every search the form is switched back in the filter mode? I didn't notice in which commit this appeared, but I actually think it is interesting 🙂

erayd · 2018-03-19T16:23:46Z

Is it intentional that now after every search the form is switched back in the filter mode?

Yes. It will switch to filter mode if the search returns one or more results, but will stay in search mode if no results are found.

If you hit backspace immediately after it switches to filter mode from a manual search, it will go back to editing the search query.

erayd · 2018-03-19T16:25:12Z

By the way, would you like me to squash this PR into a single commit? Or leave it as-is?

maximbaz · 2018-03-19T16:29:13Z

Very cool. Seems pretty solid to me, is there anything else you want to do or that is not done yet?

I just tested in Firefox, there are no noticeable issues as well.

Leave as it is, Github will squash when merging, but in PR we will have history of commits.

erayd · 2018-03-19T16:30:52Z

Very cool. Seems pretty solid to me, is there anything else you want to do or that is not done yet?

Nope, I'm pretty happy with it now. It covers all my use-cases, and I can't currently think of anything else it ought to do that others might want from it.

Leave as it is

Will do :-).

erayd · 2018-03-19T16:31:12Z

Is there anything else you'd like me to do to this PR before it's ready to merge?

maximbaz

Awesome improvement, thanks a lot!

erayd · 2018-03-19T16:36:11Z

Thank you too - your input resulted in it being a lot better than it was when I initially opened the PR!

jangari · 2018-03-20T02:43:11Z

Great work guys!

Question on usage: filter-as-you-type only seems to work on the search results, not where you have no matching domain, and so no automatically populated search results. Is that right?

When I saw this discussion yesterday I assumed it would add the ability to type to filter all password-store files when no search was active. So, the following should (under my assumptions) show me all passwords matching 'wor', i.e. everything in my Work folder, plus whatever else (fuzzily) matches that pattern:

Am I off the mark here?

sophacles · 2018-03-20T02:55:22Z

@maximbaz @erayd I just got around to pulling these changes and playing with them -- this is AWESOME! Well done :)

erayd · 2018-03-20T03:48:01Z

@jangari
That's how it's supposed to work. Once you search, it will then start filtering results.

If you want to filter your entire database using filter-while-typing, just search for / - unless you have a very weird database structure, that should match every entry you have.

maximbaz · 2018-03-20T11:43:44Z

@erayd found a tiny nitpick, not worth wasting time if the fix is not obvious, but maybe it is obvious to you?

Open browserpass, it is in filter mode, start typing something until no results appear, extension switches to the search mode, but the bottom bar with "No matching passwords for github.com" appears. Ideally it should not appear, just like if you press Backspace immediately after opening browserpass.

erayd · 2018-03-20T18:07:40Z

@maximbaz That was deliberate - i.e. "you have filtered everything, and there are no longer any results that match."

I can change it though, if you think that some other behaviour would be better?

maximbaz · 2018-03-20T18:16:30Z

Aaah, no, it makes total sense, it's just not instantly obvious that when you are in this state, you are at the same time in the search mode (you see, I've actually asked about this already...).

Let's keep it as it is now.

erayd mentioned this pull request Mar 19, 2018

Enter key to autocomplete #40

Closed

Use backspace instead of \ to clear the search results

5518806

erayd added 2 commits March 20, 2018 01:17

Add hint to indicate we're filtering rather than searching

e3e5e9f

Don't trigger click events from the search entry field

71d3115

Switch to fuzzy searching for filter

4aee57b

Allow fixing search typos

3543872

erayd added 2 commits March 20, 2018 02:39

Reset the list when the filter is empty

3d8c108

Fix backspace when there is only one letter in the filter

f2c9adb

Search length no longer matters here

1698187

Change placeholder text based on hint status

173e270

Change fuzzy filter library

160807e

Add comment

fb4718b

Ensure empty / default types are correct

587296a

Allow backspace to edit successful manual searches until the filter i…

07d6501

…s used

maximbaz suggested changes Mar 19, 2018

View reviewed changes

Don't check logins

e8b05b9

maximbaz reviewed Mar 19, 2018

View reviewed changes

erayd added 2 commits March 20, 2018 04:59

Remove "no matches" message when backspace-editing

66f0a93

Don't check logins

09a396b

maximbaz reviewed Mar 19, 2018

View reviewed changes

Don't check logins

9a8ad29

maximbaz approved these changes Mar 19, 2018

View reviewed changes

maximbaz merged commit 49bbfd5 into browserpass:master Mar 19, 2018

erayd deleted the quick-filter branch March 19, 2018 16:37

Implement as-you-type filtering #220

Implement as-you-type filtering #220

Conversation

erayd commented Mar 19, 2018 • edited Loading

maximbaz commented Mar 19, 2018

erayd commented Mar 19, 2018 • edited Loading

erayd commented Mar 19, 2018

maximbaz commented Mar 19, 2018 • edited Loading

erayd commented Mar 19, 2018

maximbaz commented Mar 19, 2018

maximbaz commented Mar 19, 2018

erayd commented Mar 19, 2018

erayd commented Mar 19, 2018

maximbaz commented Mar 19, 2018

maximbaz commented Mar 19, 2018

erayd commented Mar 19, 2018

maximbaz commented Mar 19, 2018

erayd commented Mar 19, 2018

erayd commented Mar 19, 2018

maximbaz commented Mar 19, 2018

erayd commented Mar 19, 2018

erayd commented Mar 19, 2018

maximbaz commented Mar 19, 2018

erayd commented Mar 19, 2018

maximbaz commented Mar 19, 2018

erayd commented Mar 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erayd Mar 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maximbaz commented Mar 19, 2018

erayd commented Mar 19, 2018

erayd commented Mar 19, 2018

maximbaz commented Mar 19, 2018

erayd commented Mar 19, 2018

erayd commented Mar 19, 2018

maximbaz left a comment

Choose a reason for hiding this comment

erayd commented Mar 19, 2018

jangari commented Mar 20, 2018

sophacles commented Mar 20, 2018

erayd commented Mar 20, 2018

maximbaz commented Mar 20, 2018

erayd commented Mar 20, 2018

maximbaz commented Mar 20, 2018

erayd commented Mar 19, 2018 •

edited

Loading

erayd commented Mar 19, 2018 •

edited

Loading

maximbaz commented Mar 19, 2018 •

edited

Loading

erayd Mar 19, 2018 •

edited

Loading