Bug 149505 – Web Inspector: Console SearchBar should work more like ContentBrowser FindBanner

• Component: Web Inspector
• Status: RESOLVED FIXED

[Joseph Pecoraro]

* SUMMARY
Console SearchBar should work more like ContentBrowser FindBanner.

The Console's search has a poorer experience than the ContentBrowser search.

* STEPS TO REPRODUCE
1. Evaluate "123", "1234", "12345" in the console
2. Focus the Console's search bar
3. Type "123" followed by Enter/Return
  => Search bar should still have focus, but it does not have focus
  => Expected # of results and next/previous buttons, but there are none

[Radar WebKit Bug Importer]

<rdar://problem/22821142>

[João Oliveira]

Created attachment 261915 [details]
patch

[Devin Rousso]

Comment on attachment 261915 [details]
patch

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

> Source/WebInspectorUI/UserInterface/Views/ConsoleFindBanner.css:2
> + * Copyright (C) 2013 Apple Inc. All rights reserved.

2013?

> Source/WebInspectorUI/UserInterface/Views/FindBanner.js:34
> +        this._element.classList.add(className);

I would recommend adding an if statement/fallback in case className is ever falsy.

> Source/WebInspectorUI/UserInterface/Views/FindBanner.js:73
> +            this._doneButton = document.createElement("button");

We like to name our DOM element variables with *Element.  Also, this variable isn't used outside of this loop, so it's really not necessary to save it:
let doneButtonElement = ...

> Source/WebInspectorUI/UserInterface/Views/FindBanner.js:202
> +        this._delegate.searchBarWantsToLoseFocus();

You should add a check here to ensure that the delegate exists and has a function called searchBarWantsToLoseFocus.

> Source/WebInspectorUI/UserInterface/Views/FindBanner.js:205
> +

NIT: remove extra newlines.

> Source/WebInspectorUI/UserInterface/Views/LogContentView.js:896
> +        var numberOfResults = 0;

We are using "let" instead of "var" now.

> Source/WebInspectorUI/UserInterface/Views/LogContentView.js:904
> +                numberOfResults ++;

NIT: remove extra space.
numberOfResults++;

> Source/WebInspectorUI/UserInterface/Views/LogContentView.js:924
> +        this._searchBar.numberOfResults =  numberOfResults;

NIT: remove extra space.
= numberOfResults;

[João Oliveira]

Created attachment 261928 [details]
patch

[João Oliveira]

Created attachment 261931 [details]
patch

[Devin Rousso]

Comment on attachment 261931 [details]
patch

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

> Source/WebInspectorUI/UserInterface/Main.html:85
> +    <link rel="stylesheet" href="Views/ConsoleFindBanner.css">

We organize CSS files by alphabetical order.  Please move this to its appropriate place.

> Source/WebInspectorUI/UserInterface/Views/LogContentView.js:918
> +        this._findBanner.numberOfResults = numberOfResults;

You have a findBanner parameter for this function.  Is there a reason that you don't use it?  If so, why not remove the parameter?

[João Oliveira]

Comment on attachment 261931 [details]
patch

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

>> Source/WebInspectorUI/UserInterface/Views/LogContentView.js:918
>> +        this._findBanner.numberOfResults = numberOfResults;
> 
> You have a findBanner parameter for this function.  Is there a reason that you don't use it?  If so, why not remove the parameter?

you are right, no need to use it on this function, going to remove it

[João Oliveira]

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

>> Source/WebInspectorUI/UserInterface/Views/LogContentView.js:918
>> +        this._findBanner.numberOfResults = numberOfResults;
> 
> You have a findBanner parameter for this function.  Is there a reason that you don't use it?  If so, why not remove the parameter?

you are right, no need to use it on this function, going to remove it

[João Oliveira]

Created attachment 261961 [details]
patch

[Devin Rousso]

Comment on attachment 261961 [details]
patch

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

> Source/WebInspectorUI/UserInterface/Views/ConsoleFindBanner.css:58
> +.console-find-banner > input[type="search"] {

Is this CSS rule the only reason that you created ConsoleFindBanner.css?  If so, please delete this file and just add this rule to FindBanner.css like so:

.find-banner.console-find-banner > input[type="search"] {
    ...
}

> Source/WebInspectorUI/UserInterface/Views/FindBanner.js:28
> +    constructor(delegate, className = "find-banner", fixed = false)

I would actually change how you are currently doing the className to always add "find-banner" and, if the user specifies a className in the constructor, add another classname:

constructor(delegate, className, fixed = false)

> Source/WebInspectorUI/UserInterface/Views/FindBanner.js:34
> +        this._element.classList.add(className);

From line 28:

this._element.classList.add("find-banner");
if (className && className.length)
    this._element.classList.add(className):

This ensures that all FindBanners have the same default styling specified in FindBanner.css but also have the ability to have extra styling applied through this className arg (use this for my comment in ConsoleFindBanner.css).

[João Oliveira]

Created attachment 262131 [details]
patch

[Devin Rousso]

Comment on attachment 262131 [details]
patch

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

> Source/WebInspectorUI/ChangeLog:13
> +        * UserInterface/Views/ConsoleFindBanner.css: Added.

You forgot to update the changelog.

[João Oliveira]

Created attachment 262181 [details]
patch

[João Oliveira]

Created attachment 262203 [details]
patch

[Matt Baker]

Comment on attachment 262203 [details]
patch

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

> Source/WebInspectorUI/UserInterface/Views/LogContentView.js:93
> +        var navigationItems = [this._findBanner, this._scopeBar, this._clearLogNavigationItem];

Use let rather than var in new code.

> Source/WebInspectorUI/UserInterface/Views/LogContentView.js:219
> +    findBannerRevealPreviousResult() {

Opening brace should be on its own line.

[João Oliveira]

Created attachment 262208 [details]
patch

[Matt Baker]

Comment on attachment 262208 [details]
patch

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

> Source/WebInspectorUI/UserInterface/Views/FindBanner.js:83
> +

Extra newline.

> Source/WebInspectorUI/UserInterface/Views/FindBanner.js:90
> +

See above.

[João Oliveira]

Created attachment 262211 [details]
patch

[Joseph Pecoraro]

Comment on attachment 262211 [details]
patch

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

This certainly improves the experience in the console. Some more tweaks and it should be good to land.

> Source/WebInspectorUI/UserInterface/Views/FindBanner.js:34
>          this._element.classList.add("find-banner");

Now that the super class is creating the element this code should use "this.element" instead of "this._element".

> Source/WebInspectorUI/UserInterface/Views/FindBanner.js:36
> +        if (className && className.length)

The first condition is enough.

> Source/WebInspectorUI/UserInterface/Views/FindBanner.js:-69
> -        this._inputField.addEventListener("keydown", this._inputFieldKeyDown.bind(this), false);
> -        this._inputField.addEventListener("keyup", this._inputFieldKeyUp.bind(this), false);

I don't understand why these are being removed.

> Source/WebInspectorUI/UserInterface/Views/FindBanner.js:202
> +    loseFocus()

The name "loseFocus" could be improved, as this is both clearing the input and attempting to lose focus. It also doesn't need to be public.

How about "_clearAndBlur".

> Source/WebInspectorUI/UserInterface/Views/FindBanner.js:209
> +        if (this._delegate && typeof this._delegate.findBannerWantsToLoseFocus === "function") {
> +            this._inputField.value = "";
> +            this._previousSearchValue = "";
> +            this._delegate.findBannerSearchCleared();
> +            this._delegate.findBannerWantsToLoseFocus();
> +        }

You are calling two delegate methods but only checking for one. This should probably be rewritten as:

    this._inputField.value = "";
    this._previousSearchValue = "";
    if (this._delegate.findBannerSearchCleared)
        this._delegate.findBannerSearchCleared();
    if (this._delegate.findBannerWantsToLoseFocus)
        this._delegate.findBannerWantsToLoseFocus();

> Source/WebInspectorUI/UserInterface/Views/FindBanner.js:-275
> -            } else if (this._searchKeyPressed && this._numberOfResults > 0) {
> -                if (this._searchBackwards) {
> -                    if (this._delegate && typeof this._delegate.findBannerRevealPreviousResult === "function")
> -                        this._delegate.findBannerRevealPreviousResult(this);
> -                } else {
> -                    if (this._delegate && typeof this._delegate.findBannerRevealNextResult === "function")
> -                        this._delegate.findBannerRevealNextResult(this);
> -                }

This seems to break hitting "Enter" and "Shift+Enter" from searching forwards and backwards. We still want that!

> Source/WebInspectorUI/UserInterface/Views/LogContentView.js:60
> -        this._searchBar = new WebInspector.SearchBar("log-search-bar", WebInspector.UIString("Filter Console Log"), this);
> -        this._searchBar.addEventListener(WebInspector.SearchBar.Event.TextChanged, this._searchTextDidChange, this);
> +        this._findBanner = new WebInspector.FindBanner(this, "console-find-banner", true);

This used to include styles:

.search-bar.log-search-bar > input[type="search"] {
    width: 150px;
    border: 1px solid hsla(0, 0%, 0%, 0.35);
    padding-left: 4px;
}

I think we at least want to keep the border coloring (the others seem less important / poor).

> Source/WebInspectorUI/UserInterface/Views/LogContentView.js:874
> +    _performSearch(searchTerms = "")

I know it was an earlier comment, but I find this confusing / magical to have a default value here. I would think _performSearch() would repeat a search, not clear a search.

[João Oliveira]

Created attachment 262733 [details]
patch

[Joseph Pecoraro]

Comment on attachment 262733 [details]
patch

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

r=me.

> Source/WebInspectorUI/UserInterface/Views/FindBanner.js:77
> +            this.element.classList.add(className);

This statement looks redundant, as it was already one above on line 36.

> Source/WebInspectorUI/UserInterface/Views/LogContentView.js:-60
> -        this._searchBar = new WebInspector.SearchBar("log-search-bar", WebInspector.UIString("Filter Console Log"), this);

You have removed the only use of "log-search-bar". You should remove the CSS that is no longer used:

Views/LogContentView.css
188:.search-bar.log-search-bar > input[type="search"] {

> Source/WebInspectorUI/UserInterface/Views/LogContentView.js:874
> +    _performSearch(searchTerms)

Nit: A pre-existing bug is that the search should re-run when the filter bar changes (All | Errors | Warnings | Logs). That can be filed and fixed separately though.

[Joseph Pecoraro]

<http://trac.webkit.org/changeset/191071>

Made some additional changes while landing.
1. Styled the <input type="search"> more like it was (Search Bar) for appearances
2. Made "Escape" also set numberOfResults to null to not leave dangling "No Results Found" text
3. Restored the Placeholder in the Console Search bar
4. Removed some :placeholder-shown CSS that didn't seem to be changing behavior, unless I'm missing what it was doing.

[Timothy Hatcher]

(In reply to comment #22)
> <http://trac.webkit.org/changeset/191071>
> 
> 4. Removed some :placeholder-shown CSS that didn't seem to be changing
> behavior, unless I'm missing what it was doing.

The point of the :placeholder-shown selectors was to style the sidebar inputs with a white background when they had user typed text, not just when focused. So if you click/tab away it will stay white. That way the user has an indication a filter is applied and it won't recess into the sidebar background.

[Timothy Hatcher]

(In reply to comment #23)
> (In reply to comment #22)
> > <http://trac.webkit.org/changeset/191071>
> > 
> > 4. Removed some :placeholder-shown CSS that didn't seem to be changing
> > behavior, unless I'm missing what it was doing.
> 
> The point of the :placeholder-shown selectors was to style the sidebar
> inputs with a white background when they had user typed text, not just when
> focused. So if you click/tab away it will stay white. That way the user has
> an indication a filter is applied and it won't recess into the sidebar
> background.

That use to match Xcode's behavior, but they don't do that anymore. They now style the search box's filter icon blue when a text filter is applied. I'm ambivalent on the new Xcode design. (They also make blue icons have a 2px stroke when the grey/black icons have 1px. No like.)

[Joseph Pecoraro]

Patch restoring them is on:
<https://webkit.org/b/150452> Web Inspector: Restore :not(:placeholder-shown) behavior on search bars with comments