Bug 65511 – Web Inspector: autocomplete combobox for Styles sidebar and Console.

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Pavel Feldman]

Iterating through possible style values with up/down is cool, could a autocomplete combobox work? Good for discoverability.

[Alexander Pavlov (apavlov)]

Let's handle the CSS properties suggestions in this bug, and I've filed bug 66044 for the console prompt.

[Alexander Pavlov (apavlov)]

Created attachment 103635 [details]
[PATCH] Suggested solution

[Pavel Feldman]

Comment on attachment 103635 [details]
[PATCH] Suggested solution

View in context: https://bugs.webkit.org/attachment.cgi?id=103635&action=review

> Source/WebCore/inspector/front-end/StylesSidebarPane.js:956
> +        if (!curSection) {

Looks like too much copypaste.

> Source/WebCore/inspector/front-end/SuggestBox.js:92
> +        var bodyElement = this.inputElement.ownerDocument.body;

I've seen this in Popover helper. Is there anything to reuse?

> Source/WebCore/inspector/front-end/suggestBox.css:57
> +/* Vertical Scrollbar Styles */

Can we reuse existing ones?

[Alexander Pavlov (apavlov)]

(In reply to comment #3)
> (From update of attachment 103635 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=103635&action=review
> 
> > Source/WebCore/inspector/front-end/StylesSidebarPane.js:956
> > +        if (!curSection) {
> 
> Looks like too much copypaste.

Removed dead code.

> > Source/WebCore/inspector/front-end/SuggestBox.js:92
> > +        var bodyElement = this.inputElement.ownerDocument.body;
> 
> I've seen this in Popover helper. Is there anything to reuse?

Yes, extracted into utilities.js.

> > Source/WebCore/inspector/front-end/suggestBox.css:57
> > +/* Vertical Scrollbar Styles */
> 
> Can we reuse existing ones?

Done (created 2 separate custom scrollbar styles (for vertical and horizontal ones) in inspector.css and used them both in Popover and SuggestBox).

A fixed patch is to be attached shortly.

[Alexander Pavlov (apavlov)]

Created attachment 104048 [details]
[PATCH] Comments addressed

[Pavel Feldman]

Comment on attachment 104048 [details]
[PATCH] Comments addressed

View in context: https://bugs.webkit.org/attachment.cgi?id=104048&action=review

Too much copypaste.

> Source/WebCore/inspector/front-end/StylesSidebarPane.js:930
> +        if (!curSection) {

Could you submit this as a separate change?

> Source/WebCore/inspector/front-end/StylesSidebarPane.js:952
> +        if (!curSection) {

same other change

> Source/WebCore/inspector/front-end/StylesSidebarPane.js:1139
> +        this._selectorElement.scrollIntoViewIfNeeded(false);

same other change

> Source/WebCore/inspector/front-end/StylesSidebarPane.js:2047
> +            else if (moveDirection === "backward") {

same other change

> Source/WebCore/inspector/front-end/StylesSidebarPane.js:2248
> +        //this._clearAutoComplete();

Should this code be commented?

> Source/WebCore/inspector/front-end/SuggestBox.js:32
> +    this.parentElement = inputElement.ownerDocument.body;

Please make these fields private

> Source/WebCore/inspector/front-end/SuggestBox.js:43
> +    window.addEventListener("scroll", this._boundOnScroll, true);

No need for scroll / resize listeners.

> Source/WebCore/inspector/front-end/SuggestBox.js:85
> +        const anchorBox = this.inputElement.boxInWindow(window, this.parentElement);

Could you style it using CSS instead?

> Source/WebCore/inspector/front-end/SuggestBox.js:135
> +        if (event.handled)

event.handled is not standard. how can this happen? The one setting it should stop propagation instead as you do below.

> Source/WebCore/inspector/front-end/SuggestBox.js:146
> +                this.tabKeyPressed(event);

These all should be private.

> Source/WebCore/inspector/front-end/SuggestBox.js:192
> +        this.inputElement.pruneEmptyTextNodes();

This looks a lot like copypaste from the TextPrompt.

> Source/WebCore/inspector/front-end/SuggestBox.js:222
> +    acceptAutoComplete: function(event)

This method looks exactly as the one in the TextPrompt. I think the way you should implement it is to extend TextPrompt instead. It could be TextPrompt + drop box.

> Source/WebCore/inspector/front-end/SuggestBox.js:378
> +    _completionsReady: function(selection, completions)

Again giant copypaste?

[Alexander Pavlov (apavlov)]

Created attachment 112319 [details]
[PATCH] A major rewrite, which extends the TextPrompt functionality by aggregating SuggestBox

[Pavel Feldman]

Comment on attachment 112319 [details]
[PATCH] A major rewrite, which extends the TextPrompt functionality by aggregating SuggestBox

View in context: https://bugs.webkit.org/attachment.cgi?id=112319&action=review

> Source/WebCore/inspector/front-end/ConsoleView.js:353
> +        if (force && !expressionString)

You should nuke the prefix check below instead.

> Source/WebCore/inspector/front-end/StylesSidebarPane.js:110
> +WebInspector.StylesSidebarPane.StyleValueDelimiters = " \xA0\t\n\"':;,/(";

You should not suggest upon ")" type.

> Source/WebCore/inspector/front-end/StylesSidebarPane.js:1878
> +                if (isEnterKey(event)) {

It would be nice if TextPrompt could be configured to handle it internally. Worst case, you should do:

if (this._prompt.handleEvent(event))
  return;

in all entry points.

> Source/WebCore/inspector/front-end/TextPrompt.js:605
> +    this.completions = completions;

please make things private.

> Source/WebCore/inspector/front-end/inspector.css:2507
> +.suggest-box {

please move this to the newly added textPrompt.css and require it from ConsoleView and ElementsPanel. Alternatively you could make TextPrompt a view, but I am not sure it is worth it.

[Alexander Pavlov (apavlov)]

Created attachment 112344 [details]
[PATCH] Comments addressed

[Alexander Pavlov (apavlov)]

Created attachment 112494 [details]
[PATCH] Got rid of FocusRestorer

[Alexander Pavlov (apavlov)]

Created attachment 113170 [details]
[PATCH] Rebased on the refactored TextPrompt

[Alexander Pavlov (apavlov)]

Created attachment 113459 [details]
[PATCH] Rebased on the committed SuggestBox

[Pavel Feldman]

Comment on attachment 113459 [details]
[PATCH] Rebased on the committed SuggestBox

View in context: https://bugs.webkit.org/attachment.cgi?id=113459&action=review

> Source/WebCore/inspector/front-end/ConsoleView.js:114
> +    const suggestBoxConfig = new WebInspector.TextPrompt.SuggestBoxConfig("generic-suggest");

We should not use configs, please add corresponding events to the TextPrompt. SuggestBox should be entirely hidden.

[Alexander Pavlov (apavlov)]

Created attachment 113835 [details]
[PATCH] Got rid of SuggestBoxConfig

[Pavel Feldman]

Comment on attachment 113835 [details]
[PATCH] Got rid of SuggestBoxConfig

View in context: https://bugs.webkit.org/attachment.cgi?id=113835&action=review

> Source/WebCore/inspector/front-end/ConsoleView.js:115
> +    this.prompt.setSuggestBoxEnabled("generic-suggest");

enableSuggestBox?

[Alexander Pavlov (apavlov)]

Committed r99407: <http://trac.webkit.org/changeset/99407>

[Alexander Pavlov (apavlov)]

*** Bug 18651 has been marked as a duplicate of this bug. ***