672733 - Web Console auto-complete search should be case-insensitive

Status: RESOLVED FIXED

(DevTools :: Console, defect, P1)

Description

[Jared Wein [:jaws] (please needinfo? me)]

When using the web console, I expected that the auto-complete feature would continue to complete my typing even though I disregarded correct capitalization.

For example, I typed: "gBrowser.selectedt" and I expected that it would narrow down the auto-complete results to "gBrowser.selectedTab" so that I could just hit tab and continue typing. Instead, when I typed the lowercase t, there were no more results left in the auto-complete suggestion list.

There may be two variables declared with the same name but with different capitalization. Maybe in this case we could bubble the result with the closer capitalization towards the top of the suggestion list?

Comment 1

[Sebastian Zartner [:sebo]]

Firebug already implements this feature, which makes typing within the command line somewhat faster.
The implementation there is to show all capitalization variants when you type everything in lowercase but restricts to case-sensitive results in case you enter at least one uppercase letter.

Example:
When you type 'window.g', you get the following list of suggestions:

window.GainNode
window.Gamepad
window.GamepadAxisMoveEvent
window.GamepadButton
window.GamepadButtonEvent
window.GamepadEvent
window.getComputedStyle
window.getDefaultComputedStyle
window.getMaxGCPauseSinceClear
window.getSelection

But when you type 'window.G' you only get the uppercase suggestions:

window.GainNode
window.Gamepad
window.GamepadAxisMoveEvent
window.GamepadButton
window.GamepadButtonEvent
window.GamepadEvent

There's one special case in the Firebug implementation. When you type a small letter as the first letter, you will only get the lowercase suggestions of the 'window' object.

Sebastian

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

Sebo, would you be willing to port over the Firebug implementation?

Comment 3

[Sebastian Zartner [:sebo]]

I'd be happy to do so, though there's currently no spare time left for it.
So, feel free to take it over or let it unassigned for now and I'll try to do that at a later point.

Sebastian

Comment 4

[Soledad Penades [:sole] [:spenades]]

I was asking Alex for easy-ish bugs to dive deeper into DevTools and he suggested this one might be good. So taking it for when I finish with my existing queue. No ETA given yet! :)

Comment 5

[Alexandre Poirot [:ochameau]]

To avoid you trying to process the whole console codebase and learn about all of its components.
Here is what I think you be tweaked:
http://searchfox.org/mozilla-central/rev/0079c7adf3b329bff579d3bbe6ac7ba2f6218a19/devtools/client/webconsole/jsterm.js#1378-1613
_updateCompletionResults calls the actors (webconsoleClient.autocomplete()) that lives here:
http://searchfox.org/mozilla-central/rev/0079c7adf3b329bff579d3bbe6ac7ba2f6218a19/devtools/server/actors/webconsole.js#1003-1068

Comment 6

[Soledad Penades [:sole] [:spenades]]

Attachment: Bug 672733 - Web console autocomplete search should be case insensitive.

Review commit: https://reviewboard.mozilla.org/r/127154/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/127154/

Comment 7

[Nicolas Chevobbe [:nchevobbe]]

Comment on attachment 8855276 [details]
Bug 672733 - Web console autocomplete search should be case insensitive.

https://reviewboard.mozilla.org/r/127154/#review129902

This looks good to me, r+ if TRY is green :)

Comment 8

[Alexandre Poirot [:ochameau]]

Comment on attachment 8855276 [details]
Bug 672733 - Web console autocomplete search should be case insensitive.

https://reviewboard.mozilla.org/r/127154/#review129904

There is an edge case with this patch that you can address in a followup.
When you are not autocompleting after a dot, the first character is special.
If you start with a lowerase, you will only get autocompletion for symbols starting with lowercase,
same for the uppercase:
* start typing "A", I would expect to see addEventListener as a suggestion, but it doesn't appear
* start typing "o", this time "Object" doesn't appear

Please reask for review if you end up addressing that, otherwise it looks good with the following comments being addressed.

::: devtools/client/webconsole/jsterm.js:1504
(Diff revision 1)
>        // to filter the cache
>        if (lastNonAlpha) {
>          filterBy = input.substring(input.lastIndexOf(lastNonAlpha) + 1);
>        }
>  
> +      // Convert filter to lower case only once

I think a comment saying we do a case insensitive search would be more helpful than this comment.

::: devtools/client/webconsole/jsterm.js:1505
(Diff revision 1)
>        if (lastNonAlpha) {
>          filterBy = input.substring(input.lastIndexOf(lastNonAlpha) + 1);
>        }
>  
> +      // Convert filter to lower case only once
> +      let filterByLc = filterBy.toLowerCase();

I wouldn't introduce a new variable here.
You end up filter with this new lowercased version.
So, instead, I would override `filterBy` and use it for any code after that.

It is not really clear to me what do we do with matchProp after this function is called,
but I have not seen any difference while doing that change...

::: devtools/client/webconsole/jsterm.js:1678
(Diff revision 1)
> +      let dotPosition = cursor - matchLength;
> +      let newValue = value.substring(0, dotPosition) + completion
> +        + value.substring(cursor, value.length);
> +      let newCursor = dotPosition + completion.length;
> +
> +      this.setInputValue(newValue);

New variable names helps following what is going on, but a comment may help even more.

Note that, "dotPosition" isn't ideal. Yes, in many cases it will be a dot, but not always:
  new Prom<TAB>
  dotPosition will be 4, just before the 'P'
I think cursor was a better name.

Here is a possible comment:
Replace the already typed symbol prefix (=matchProp) with the full symbol name (=completion) and shift the cursor accordingly.

::: devtools/client/webconsole/test/browser_webconsole_bug_585991_autocomplete_keys.js:391
(Diff revision 1)
> +
> +    deferred.resolve();
> +  });
> +
> +  jsterm.setInputValue("window");
> +  EventUtils.synthesizeKey(".", {});

You may add a test to ensure autocompletion works fines on the first key stroke:

EventUtils.synthesizeKey("A", {});
EventUtils.synthesizeKey("d", {});
EventUtils.synthesizeKey("d", {});

Should end up autocompleting addEventListener.

::: devtools/server/actors/webconsole.js:1059
(Diff revision 1)
>          };
>          addWebConsoleCommands(helpers);
>          this._webConsoleCommandsCache =
>            Object.getOwnPropertyNames(helpers.sandbox);
>        }
> -      matches = matches.concat(this._webConsoleCommandsCache
> +      if(result.matchProp) {

Looks like you may even merge this `if(result.matchProp)` into `if (!lastNonAlphaIsDot)`
That, to prevent creating _webConsoleCommandsCache.

::: devtools/server/actors/webconsole.js:1062
(Diff revision 1)
>            Object.getOwnPropertyNames(helpers.sandbox);
>        }
> -      matches = matches.concat(this._webConsoleCommandsCache
> -          .filter(n => n.startsWith(result.matchProp)));
> +      if(result.matchProp) {
> +        // We want to match case-insensitively, but we don't need to convert on
> +        // every match, so we'll keep the result in a variable
> +        let matchPropLc = result.matchProp.toLowerCase();

I imagine it will already be lowercased if you do matchProp: filterBy from _updateCompletionResult

Comment 9

[blackwelllandis]

Is there an update on this? I see that someone started on it, but there hasn't been any activity in a few months?

I'm looking to contribute to Firefox and this particular bug would make my day job easier.

For what it's worth, I had already patched it before I came here and have been using it for a few days :)

Comment 10

[Sebastian Zartner [:sebo]]

Soledad, are you still planning to finish your work on this or can blackwelllandis take it over?

Sebastian

Comment 11

[Soledad Penades [:sole] [:spenades]]

Hello Sebastian, blackwellandis!

I am quite busy right now, so if you want to take over I'll be very happy! Agreed this makes our jobs easier!

Feel free to take anything in my patch that is useful!

Alex's review comments have lots of good improvements that are good to have in mind, specially the testing and naming bits, so I'd advice having a look at that if you want to make a SUPER GOOD patch.

Thank you for contributing. I will be happy to help with whatever I learned while attempting to fix this :)

Comment 12

[blackwelllandis]

Thanks Soledad.

I did read Alex's review and will definitely check out your patch!
And thanks for the offer of help.

Comment 13

[Gabriel Luong [:gl] (ΦωΦ)]

(In reply to blackwelllandis from comment #12)
> Thanks Soledad.
> 
> I did read Alex's review and will definitely check out your patch!
> And thanks for the offer of help.

Hi blackwellandis,

Just checking in if you would still like to finish this.

Thanks you for contributing. :)

Comment 14

[:Harald Kirschner :digitarald]

blackwellandis, feel free to jump in this bug again when you have time to re-claim it.

sole, could you check if you or somebody else has time to wrap this work up; or un-assign yourself?

Comment 15

[Soledad Penades [:sole] [:spenades]]

I unfortunately don't have time to finish this now - I'll unassign myself.

Comment 16

[blackwelllandis]

(In reply to Gabriel [:gl] (ΦωΦ) from comment #13)
> (In reply to blackwelllandis from comment #12)
> > Thanks Soledad.
> > 
> > I did read Alex's review and will definitely check out your patch!
> > And thanks for the offer of help.
> 
> Hi blackwellandis,
> 
> Just checking in if you would still like to finish this.
> 
> Thanks you for contributing. :)

Hi. I got bogged down with another project at work and plain out forgot about it.
I'm, again, working on a bit of JS at work and was reminded of the issue when using the console today.
I've re-synced with the sources and am re-acclimating. 
My SO is out of town so I will hopefully have time to test and submit it this weekend(fingers crossed).

Comment 17

[Nicolas Chevobbe [:nchevobbe]]

One thing to keep in mind when implementing this is how we deal with the autocompletion text.
What I call autocompletion text is the grey text that appear after the cursor to tell the user what will happen if they hit the Tab/Enter key.
e.g., if the user entered `win` we display a `dow` after it.
In the following, I'll refer to this as : `win|dow` , where the autocompletion is after the "|" char, which represents the caret.

now let's say we have a case-insensitive search and the user gets `WIN|dow` for example.
Is this still fine ? Should we have something that makes the text looks like `win|dow` ?

I'm not sure this is a bug deal, and this can probably be handled in a follow up.

Comment 18

[Sebastian Zartner [:sebo]]

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #17)
> One thing to keep in mind when implementing this is how we deal with the
> autocompletion text.
> What I call autocompletion text is the grey text that appear after the
> cursor to tell the user what will happen if they hit the Tab/Enter key.
> e.g., if the user entered `win` we display a `dow` after it.
> In the following, I'll refer to this as : `win|dow` , where the
> autocompletion is after the "|" char, which represents the caret.
> 
> now let's say we have a case-insensitive search and the user gets `WIN|dow`
> for example.
> Is this still fine ? Should we have something that makes the text looks like
> `win|dow` ?

Please see comment 1! I think Firebug provided a quite good UX for this case.

In short: If the entered text is lowercase, autocompletion text (I call it autosuggestion) matches should be case insensitive, otherwise they should be case sensitive.

The case should be corrected once you hit Tab. So if you entered `document.gete` you get `document.gete|lementById` and when you hit Tab it gets autocompleted to `document.getElementById`.

Sebastian

Comment 19

[:Harald Kirschner :digitarald]

> In short: If the entered text is lowercase, autocompletion text (I call it autosuggestion) matches should be case insensitive, otherwise they should be case sensitive.

Not sure what that exactly fixes. Did users get annoyed by the fuzzy matches and somehow learned the hidden magic that typing something uppercase made it more exact? This might yield to mixed results when a users starts guessing letter cases and triggers case sensitive matching.

> The case should be corrected once you hit Tab. So if you entered `document.gete` you get `document.gete|lementById` and when you hit Tab it gets autocompleted to `document.getElementById`.

That makes sense and doesn't seem too hard.

Nicolas, do you see any issues with the replace-word-with-correct-case approach?

Comment 20

[Nicolas Chevobbe [:nchevobbe]]

> Nicolas, do you see any issues with the replace-word-with-correct-case approach?

No, that's fine to me. I may give this a shot soon.

Comment 21

[Sebastian Zartner [:sebo]]

(In reply to :Harald Kirschner :digitarald from comment #19)
> > In short: If the entered text is lowercase, autocompletion text (I call it autosuggestion) matches should be case insensitive, otherwise they should be case sensitive.
> 
> Not sure what that exactly fixes. Did users get annoyed by the fuzzy matches
> and somehow learned the hidden magic that typing something uppercase made it
> more exact? This might yield to mixed results when a users starts guessing
> letter cases and triggers case sensitive matching.

Sorry, I missed your comment! The opposite is the case, users were asking for this feature (like here :-) ). The Firebug team considered this the best UX, because when people type everything lowercase we assumed that they don't care about the case, while when they type mixed case, they probably want the case to be considered.
Users weren't forced to learn the logic behind that because if they continued typing mixed case they got the same results like before. That the fuzziness showed more results than case sensitive matching in many cases was mitigated by preselecting the most commonly used item of the list of suggestions (for which there is bug 1270015).

I don't have numbers confirming that this is the best UX, though in several years of having the autocompletion implemented that way we didn't get any complaints about it.

Sebastian

Comment 22

[Nicolas Chevobbe [:nchevobbe]]

Attachment: Bug 672733 - Make autocomplete search case insensitive; r=Honza.

This patch adds a smarter heuristic for autocompletion results:
if the input first letter is lowercased, then we'll filter
matching properties case insensitively. But if the user starts
with an uppercase, we assume they know the property they want
and thus respect the casing.
For example: `win` will return both `window` and `Window`, but
`Win` will return `Window` only.
Due to this behavior, we change the order of the autocomplete
results so lowercased property are displayed before the uppercased
one.
If we take are example again, it's likely that if a user type `win`,
they want `window`, but the alphabetical order would return `Window`
first which would annoy user.

Now, since we return results that does not match exactly the user
input, we need to modify the frontend.

Usually, we only show the autocompletion popup if there are
at least 2 matching items, since 1 matching item will still
be displayed using the autocompletion text. But now, since the
input might not match, we want to still display the popup if
there is one matching item, but starts differentely than what
the user entered.
For example, the user typed `window.addeve`, which matches
`addEventListener`. The completion text will make it looks like
it will be completed to `window.addeventListener`, which would
be undefined. So showing the popup with the actual matching
property might avoid some confusion for the user.

A test was added to make sure the frontend works as expected.
Some test cases were added in the server test to make sure
the actor returns expected results. Other tests needed some
adjustement because of the insensitive case matches and the
new order of results.

Comment 23

[Sebastian Zartner [:sebo]]

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #22)
> This patch adds a smarter heuristic for autocompletion results:

That all sounds really great to me! Thank you so much for working on this!
Just one question for clarification: If the user types `window.adDE`, will they get `window.addEventListener` as result or not?

Sebastian

Comment 24

[Nicolas Chevobbe [:nchevobbe]]

(In reply to Sebastian Zartner [:sebo] from comment #23)
> (In reply to Nicolas Chevobbe [:nchevobbe] from comment #22)
> > This patch adds a smarter heuristic for autocompletion results:
> 
> That all sounds really great to me! Thank you so much for working on this!
> Just one question for clarification: If the user types `window.adDE`, will
> they get `window.addEventListener` as result or not?
> 
> Sebastian

Yes, we only check the first letter of what needs to be completed (here `adDE`) in order to decide if we want to be loosely filtering or not.

Comment 25

[Nicolas Chevobbe [:nchevobbe]]

I noticed a small popup positioning issue when typing `document.adde`, i'm investigating what's happening

Comment 26

[Nicolas Chevobbe [:nchevobbe]]

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #25)
> I noticed a small popup positioning issue when typing `document.adde`, i'm
> investigating what's happening

Fixed by first setting the autocompletionText, then showing the popup. It seems that there were some race which was causing the cursor to be at an erroneous position.

Comment 27

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 9003404 [details]
Bug 672733 - Make autocomplete search case insensitive; r=Honza.

Jan Honza Odvarko [:Honza] has approved the revision.

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4c603c11ce8c
Make autocomplete search case insensitive; r=Honza.

Comment 29

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/4c603c11ce8c

Comment 30

[Irene Smith]

Added a statement to the section of the console page (https://developer.mozilla.org/en-US/docs/Tools/Web_Console/The_command_line_interpreter#Autocomplete):

Console autocomplete suggestions are case-insensitive.

Let me know if that isn't sufficient.

Comment 31

[Nicolas Chevobbe [:nchevobbe]]

clearing needinfo as we discussed this in Slack