Web Console auto-complete search should be case-insensitive

ASSIGNED
Assigned to

Status

P1
normal
ASSIGNED
7 years ago
2 days ago

People

(Reporter: jaws, Assigned: nchevobbe)

Tracking

(Blocks: 2 bugs)

Trunk
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [boogaloo-mvp][autocomplete])

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

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?
Component: Developer Tools → Developer Tools: Console
Whiteboard: [autocomplete]
Priority: -- → P3
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
Blocks: 991806
No longer blocks: 682694
Sebo, would you be willing to port over the Firebug implementation?
Flags: needinfo?(sebastianzartner)
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
Flags: needinfo?(sebastianzartner)
Assignee: nobody → sole
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! :)
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 hidden (mozreview-request)
(Assignee)

Comment 7

a year 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 :)
Attachment #8855276 - Flags: review?(nchevobbe) → review+

Comment 8

a year 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
Attachment #8855276 - Flags: review?(poirot.alex) → review+

Comment 9

a year 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 :)
Soledad, are you still planning to finish your work on this or can blackwelllandis take it over?

Sebastian
Flags: needinfo?(sole)
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 :)
Flags: needinfo?(sole)

Comment 12

a year ago
Thanks Soledad.

I did read Alex's review and will definitely check out your patch!
And thanks for the offer of help.
(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. :)
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?
Flags: needinfo?(spenades)
I unfortunately don't have time to finish this now - I'll unassign myself.
Flags: needinfo?(spenades)
Assignee: spenades → nobody

Comment 16

4 months 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).

Updated

2 months ago
Product: Firefox → DevTools
(Assignee)

Updated

a month ago
Blocks: 1458831
Whiteboard: [autocomplete] → [boogaloo-reserve][autocomplete]
(Assignee)

Comment 17

a month 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.
(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
> 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?
Flags: needinfo?(nchevobbe)
(Assignee)

Comment 20

3 days 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.
Flags: needinfo?(nchevobbe)
Priority: P3 → P2
Whiteboard: [boogaloo-reserve][autocomplete] → [boogaloo-mvp][autocomplete]
(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
(Assignee)

Updated

2 days ago
Assignee: nobody → nchevobbe
Status: NEW → ASSIGNED
Priority: P2 → P1
You need to log in before you can comment on or make changes to this bug.