Jared Wein [:jaws] (please needinfo? me) Reporter
	Description • 13 years ago

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?

	Sebastian Zartner [:sebo]
	Comment 1 • 8 years ago

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

	Jared Wein [:jaws] (please needinfo? me) Reporter
	Comment 2 • 8 years ago

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

	Sebastian Zartner [:sebo]
	Comment 3 • 8 years ago

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

	Soledad Penades [:sole] [:spenades]
	Comment 4 • 7 years ago

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! :)

	Alexandre Poirot [:ochameau]
	Comment 5 • 7 years ago

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

	Nicolas Chevobbe [:nchevobbe] Assignee
	Comment 7 • 7 years ago
mozreview-review

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 :)

	Alexandre Poirot [:ochameau]
	Comment 8 • 7 years ago
mozreview-review

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

	blackwelllandis
	Comment 9 • 7 years ago

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 :)

	Sebastian Zartner [:sebo]
	Comment 10 • 7 years ago

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

Sebastian

	Soledad Penades [:sole] [:spenades]
	Comment 11 • 7 years ago

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 :)

	blackwelllandis
	Comment 12 • 7 years ago

Thanks Soledad.

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

	Gabriel Luong [:gl] (ΦωΦ)
	Comment 13 • 7 years ago

(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. :)

	:Harald Kirschner :digitarald
	Comment 14 • 7 years ago

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?

	Soledad Penades [:sole] [:spenades]
	Comment 15 • 7 years ago

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

	blackwelllandis
	Comment 16 • 6 years ago

(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).

	Nicolas Chevobbe [:nchevobbe] Assignee
	Comment 17 • 6 years ago

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.

	Sebastian Zartner [:sebo]
	Comment 18 • 6 years ago

(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

	:Harald Kirschner :digitarald
	Comment 19 • 6 years ago

> 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?

	Nicolas Chevobbe [:nchevobbe] Assignee
	Comment 20 • 6 years ago

> 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.

	Sebastian Zartner [:sebo]
	Comment 21 • 6 years ago

(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

	Nicolas Chevobbe [:nchevobbe] Assignee
	Comment 22 • 6 years ago

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.

	Sebastian Zartner [:sebo]
	Comment 23 • 6 years ago

(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

	Nicolas Chevobbe [:nchevobbe] Assignee
	Comment 24 • 6 years ago

(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.

	Nicolas Chevobbe [:nchevobbe] Assignee
	Comment 25 • 6 years ago

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

	Nicolas Chevobbe [:nchevobbe] Assignee
	Comment 26 • 6 years ago

(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.

	Jan Honza Odvarko [:Honza] (always need-info? me)
	Comment 27 • 6 years ago

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

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

	Pulsebot
	Comment 28 • 6 years ago

Pushed by nchevobbe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4c603c11ce8c
Make autocomplete search case insensitive; r=Honza.

	Andrei Ciure[:aciure]
	Comment 29 • 6 years ago
bugherder

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