Bug 145536 – Web Inspector: Add a filter for CSS properties in the Styles sidebar

• Component: Web Inspector
• Status: RESOLVED FIXED

[Matt Baker]

Add a filter bar to the computed and rules sidebar panels to allow filtering of the CSS properties in those lists. In the computed panel, matching properties will have the specific matching text highlighted (instead of the entire CSS property), while all other non-matching properties will not be displayed. In the rules panel, matching properties and selectors will be highlighted in their entirety (the whole CSS property/selector), while nonmatching properties will be slightly opaque.

[Matt Baker]

rdar://problem/13658649

[Devin Rousso]

Created attachment 254039 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 254039 [details]
Patch

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

I included some mostly style comments / nits for a first pass. I haven't done a full review yet though.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationSection.js:277
> +        var selectors = this._selectorElement.querySelectorAll(".header .selector span"),

Hmm, this is brittle and could break accidentally. For instance someone could change the markup for a selector and unintentionally break this. We may want to look into using higher level objects.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationSection.js:278
> +            hasMatchingSelector = false;

Style: One `var` per variable declaration avoids the weird indentation.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationSection.js:283
> +            if (needle && selectors[i].textContent.indexOf(needle) >= 0) {

Nit: String.prototype.includes?

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:227
> +        var propertiesList = (this._style.visibleProperties.length) ? this._style.visibleProperties : this._style.properties;

Style: Unnecessary parenthesis.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:233
> +                    property._filterResultClassName = null;

This is a case for "__foo" or using a Symbol(). Here, CSSStyleDeclarationTextEditor is adding a property onto another object. Using a name "_foo" might conflict with a member variable of that class. So we use "__foo" to denote this is an external property someone else has placed on this object.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:234
> +                else if (property.text.indexOf(needle) < 0)

Nit: String.prototype.includes?

  else if (!property.text.includes(needle))

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:250
> +        var propertiesList = (this._style.visibleProperties.length) ? this._style.visibleProperties : this._style.properties;

Style: Unnecessary parenthesis.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:283
> +                property._filterResultNeedlePosition = {start: indexesOfNeedle, length: needle.length};

Style: "__".

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:291
> +        return (matchingPropertyNames.length > 0);

Style: Unnecessary parenthesis.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDetailsSidebarPanel.css:82
> +    background-color: #fff;

Style: We would use the nickname "white" here.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDetailsSidebarPanel.css:92
> +    background-color: #fff;

Style: "white".

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDetailsSidebarPanel.js:88
> +        this._filterBar = new WebInspector.FilterBar();

Style: We drop constructor parenthesis if there are no arguments:

    this._filterBar = new WebInspector.FilterBar;

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDetailsSidebarPanel.js:89
> +        this._filterBar.placeholder = "Filter Property List";

This should be a localizable string:

    this._filterBar.placeholder = WebInspector.UIString("Filter Property List");

We can then re-generate the localizable strings file like so:

    shell> ./Tools/Scripts/extract-localizable-js-strings ./Source/WebInspectorUI/Localizations/en.lproj/localizedStrings.js ./Source/WebInspectorUI/UserInterface

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDetailsSidebarPanel.js:95
> +        this._filterBar._noResultsFoundLabel.textContent = "No Results Found";

Another case for WebInspector.UIString.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDetailsSidebarPanel.js:242
> +        if (typeof this._selectedPanel.filterDidChange === "function")
> +            this.contentElement.classList.add("has-filter-bar");
> +        else
> +            this.contentElement.classList.remove("has-filter-bar");

Style: I think we have been simplifying these "if add, else remove" to just:

    var hasFilter = typeof this._selectedPanel.filterDidChange === "function";
    this.contentElement.classList.toggle("has-filter-bar", hasFilter);

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDetailsSidebarPanel.js:266
> +    _filterDidChange(override) {

Style: Brace on newline.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDetailsSidebarPanel.js:273
> +        if (this._filterBar.filters.text)
> +            this.contentElement.classList.add("filter-in-progress");
> +        else 
> +            this.contentElement.classList.remove("filter-in-progress");

Style: Ditto regarding classList.toggle.

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:159
> +        // Ensures that the filterBar will only reset when a new node is selected (it doesn't refresh on window resize)

Style: Comments are sentences and should end in a period.

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:160
> +        if (oldStyle && typeof this._delegate.resetFilterBar === "function")

Style: Multi-line if statements should include braces.

> Source/WebInspectorUI/UserInterface/Views/FilterBar.js:106
> +        return (this._lastFilterValue !== this._inputField.value);

Style: No need for the parenthesis here.

> Source/WebInspectorUI/UserInterface/Views/FilterBar.js:130
>          this.dispatchEventToListeners(WebInspector.FilterBar.Event.FilterDidChange);
> +        this._lastFilterValue = this._inputField.value;

I'd expect the event to be dispatched after the internal logic.

> Source/WebInspectorUI/UserInterface/Views/RulesStyleDetailsPanel.js:249
> +        if (oldStyle && newSections.length && typeof this._delegate.resetFilterBar === "function")

Style: Braces

> Source/WebInspectorUI/UserInterface/Views/RulesStyleDetailsPanel.js:271
> +    filterDidChange(filterBar) {

Style: Opening brace on its own line.

> Source/WebInspectorUI/UserInterface/Views/RulesStyleDetailsPanel.js:273
> +        for (var section of this._sections) 

Style: Braces

> Source/WebInspectorUI/UserInterface/Views/RulesStyleDetailsPanel.js:278
> +            filterBar._noResultsFoundLabel.style.display = (!matchFound) ? "block" : "none";

Style: Parenthesis not needed.

> Source/WebInspectorUI/UserInterface/Views/RulesStyleDetailsPanel.js:293
> +            if(section.needsToUpdate) {

Style: "if ("

[Nikita Vasilyev]

Since it is a UI feature, it would be nice to have an animated GIF (I use http://www.cockos.com/licecap/ for capturing), a few seconds screencast (can be recorded with QuickTime) or at least a screenshot attached.

[Devin Rousso]

Created attachment 254074 [details]
Patch

[Timothy Hatcher]

Comment on attachment 254074 [details]
Patch

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

Not a full review, so refactoring needed still. Looking great though!

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDetailsSidebarPanel.js:269
> +        if (!this._filterBar.hasFilterChanged() && !override)
> +            return;

When is _filterDidChange called and hasFilterChanged() not true?

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:165
> +        // Ensures that the filterBar will only reset when a new node is selected (it doesn't refresh on window resize).
> +        if (oldStyle && typeof this._delegate.resetFilterBar === "function") {
> +            if (oldStyle.node === this.nodeStyles.node)
> +                this._delegate._filterDidChange(true);
> +            else
> +                this._delegate.resetFilterBar();
> +        }

I would expect this to be done at a higher level. CSSStyleDetailsSidebarPanel knows when the node changes or not. Dealing with the filter bar here breaks some layering and leaks some knowledge into these classes.

> Source/WebInspectorUI/UserInterface/Views/FilterBar.js:107
> +    hasFilterChanged()
> +    {
> +        return this._lastFilterValue !== this._inputField.value;
> +    }

FilterBar has more than just a text filter sometimes. So this function isn't accurate when button filters are used.

> Source/WebInspectorUI/UserInterface/Views/RulesStyleDetailsPanel.js:254
> +        // Ensures that the filterBar will only reset when a new node is selected (it doesn't refresh on window resize).
> +        if (oldStyle && newSections.length && typeof this._delegate.resetFilterBar === "function") {
> +            if (oldStyle[0].style.node === newSections[0].style.node)
> +                this._delegate._filterDidChange(true);
> +            else
> +                this._delegate.resetFilterBar();
> +        }

Ditto.

[Devin Rousso]

Comment on attachment 254074 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/CSSStyleDetailsSidebarPanel.js:269
>> +            return;
> 
> When is _filterDidChange called and hasFilterChanged() not true?

If the user just hits the enter key without changing the value of the filter, _filterDidChange does get called.  I added this check to avoid having to remark the matched properties again even though the filter hasn't changed.

[Timothy Hatcher]

Comment on attachment 254074 [details]
Patch

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

>>> Source/WebInspectorUI/UserInterface/Views/CSSStyleDetailsSidebarPanel.js:269
>>> +            return;
>> 
>> When is _filterDidChange called and hasFilterChanged() not true?
> 
> If the user just hits the enter key without changing the value of the filter, _filterDidChange does get called.  I added this check to avoid having to remark the matched properties again even though the filter hasn't changed.

FilterBar should do that check internally then. That way all DilterBar clients can avoid the extra work.

[Devin Rousso]

Created attachment 254536 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 254536 [details]
Patch

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

I'd like to see an updated patch with some of the comments below resolved, so I can re-read the logic and make sure it all still makes sense. Overall this looks pretty good though!

> Source/WebInspectorUI/ChangeLog:11
> +        (WebInspector.CSSStyleDeclarationTextEditor.prototype.findMatchingProperties): Searches through the properties list to find and hightlight all matching properties.

Typo: "hightlight" => "highlight"

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationSection.js:51
> +    this._propertySelectors = [];

Style: This is a poor place in the constructor to put this. Lets move it up to ~line 35 or line 65 instead of in the middle of element creation.

Hmm, the name doesn't make sense to me. This seems to be just a list of each selector element, so why does the name have "property"? How about "this._selectorElements"?

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationSection.js:199
> +            this._propertySelectors.push(selectorElement);

Seeing as we add the new selector elements on refresh, we should probably be clearing the list at the top of refresh:

    this._selectorElements = [];

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationSection.js:278
> +    findMatchingProperties: function(needle)

This is not finding matching properties, it is finding matching selectors and properties. This should probably be renamed for clarity.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationSection.js:280
> +        this._element.classList.remove("filter-section-non-matching", "filter-section-has-label");

Given that these class names are used in multiple places, it would make sense to put these out into a constant and use the constant.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationSection.js:284
> +        for (var selector of this._propertySelectors) {

It would be nice to easily see in the variable name that this is an element:

    for (var selectorElement of this._selectorElements)

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationSection.js:298
> +        if (!this._propertiesTextEditor.findMatchingProperties(needle)) {
> +            if (!needle)
> +                return false;
> +
> +            if (hasMatchingSelector)
> +                this._propertiesTextEditor.clearMatchingProperties();

This seems to be doing duplicate work.

    1. Loop through all the selector elements and find matches.
    2. Loop through all properties and find matches.
    3. If there were no property matches but there was a matching selector loop through and clear matching properties?!

Step 3 is the seemingly confusing bit. I'd like to see the pattern match the text editor more. With separate find and clear methods. Something like:

    updateFilter: function(needle)
    {
        if (!needle) {
            this._clearMatchingSelectors();
            this._propertiesTextEditor.clearMatchingProperties();
            return false;
        }

        var hasSelectorMatch = false;
        for (var selectorElement of this._selectorElements) {
            ...
        }

        var hasPropertiesMatch = this._propertiesTextEditor.findMatchingProperties(needle);
        if (!hasPropertiesMatch && hasSelectorMatch)
            this._propertiesTextEditor.clearMatchingProperties(); // FIXME: This name makes it look like we are clearing the matches, but really we are clearly the non-matches!

        if (!hasSelectorMatch && !hasPropertiesMatch) {
            this._element.classList.add("filter-section-non-matching");
            return false;
        }

        return true;
    }

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:241
> +                if (!needle)
> +                    property.__filterResultClassName = null;

Instead of doing this check for each property marker, I think it might be clearer to just hoist this up to the top:

    if (!needle) {
        this.clearMatchingProperties();
        return false;
    }

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:290
> +            var indexesOfNeedle = [property.text.indexOf(needle)];
> +            if (indexesOfNeedle[0] >= 0) {
> +                matchingPropertyNames.push(property.name);
> +
> +                property.__filterResultClassName = "filter-matching";
> +
> +                var nextIndexOfNeedle = property.text.indexOf(needle, indexesOfNeedle[indexesOfNeedle.length - 1] + 1);
> +                while (nextIndexOfNeedle > 0) {
> +                    indexesOfNeedle.push(nextIndexOfNeedle);
> +                    nextIndexOfNeedle = property.text.indexOf(needle, indexesOfNeedle[indexesOfNeedle.length - 1] + 1);
> +                }

This sounds like a great idea for a Utility function. And it would greatly simplify this particular function to focus on the higher level logic.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:296
> +        this._filterResultPropertyNames = (matchingPropertyNames.length) ? matchingPropertyNames.keySet() : {};

Style: Unnecessary parenthesis.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDetailsSidebarPanel.css:81
> +    height: 29px;

Should be covered by .filter-bar in general. But fine to keep.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDetailsSidebarPanel.css:96
> +    display: block;

.filter-bar by default has display:flex and input[type="search"] flexes. Do these new styles display the filter bar differently?

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDetailsSidebarPanel.js:92
> +        this._filterBar.placeholder = WebInspector.UIString("Filter Property List");

Maybe "Filter Styles" would be clearer. Since this filters not just properties but selectors and maybe more.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDetailsSidebarPanel.js:95
> +        this._filterBar._noResultsFoundLabel = document.createElement("div");

This element does not need to be on the filterBar, especially since this sidebar panel manages the element. It could just be:

    this._emptyFilterResultsElement

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDetailsSidebarPanel.js:97
> +        this._filterBar._noResultsFoundLabel.style.display = "none";

I prefer just doing: element.hidden = true;

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDetailsSidebarPanel.js:231
> +            this.contentElement.classList.toggle("filter-in-progress", hasFilter);

Should "filter-in-progress" instead be the value of this._filterBar.hasActiveFilters()? Otherwise we will have the class name just switching panels without filters.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDetailsSidebarPanel.js:262
> +        if (this._filterBar) {

Unnecessary check.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDetailsSidebarPanel.js:268
> +            if (this._filterBar._noResultsFoundLabel)
> +                this._filterBar._noResultsFoundLabel.style.display = "none";
> +
> +            this._selectedPanel.filterDidChange(this._filterBar);

This should just use the return value of filterDidChange.

    var hasMatch = this._selectedPanel.filterDidChange(this._filterBar);
    this._emptyFilterResultsElement.hidden = hasMatch;

> Source/WebInspectorUI/UserInterface/Views/FilterBar.js:106
> +        return this._lastFilterValue.text !== this._inputField.value || this._lastFilterValue.functions.length !== this._filterFunctionsMap.size;

To be more accurate, you would need to compare the filter functions themselves.

    // Compare sizes, values and order.
    var lastFunctions = lastFilterValue.functions;
    var currentFunctions = [...this._filterFunctionsMap.values()];
    if (lastFunctions.length !== currentFunctions.length) return false;
    for (var i = 0; i < lastFunctions.length; ++i) { ... }

> Source/WebInspectorUI/UserInterface/Views/RulesStyleDetailsPanel.js:305
> +        if (filterBar._noResultsFoundLabel)
> +            filterBar._noResultsFoundLabel.style.display = !matchFound ? "block" : "none";

This seems like a layering violation. Instead of this panel do this, the owner who called us should do this based on our return value (return matchFound). This way this inner panel doesn't need to know about toggling UI it doesn't own.

[Joseph Pecoraro]

Comment on attachment 254536 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDetailsSidebarPanel.js:98
> +        this._filterBar._noResultsFoundLabel.textContent = WebInspector.UIString("No Results Found");

Also this style doesn't match the other Section's empty text (which is gray). Look for what WebInspector.DetailsSectionRow.EmptyStyleClassName does, and the cases of "showEmptyMessage".

[Devin Rousso]

Created attachment 254845 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 254845 [details]
Patch

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

This patch looks much better to me! r=me

> Source/WebInspectorUI/UserInterface/Base/Utilities.js:760
> +            index = this.indexOf(needle, indexesOfNeedle.lastValue + 1);

Instead of "indexedOfNeedle.lastValue" it can just be "index".

    index = this.indexOf(needle, index + 1);

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:247
> +        if (!matchingProperties.includes(true))
> +            return false;

This early return worries me. What if a filter matches something, then changes to something non-empty that doesn't match something. It seems we would bail here without updating anything in the UI. We should probably be reseting.

    if (!matchingProperties.includes(true)) {
        this.resetFilteredProperties();
        return false;
    }

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDetailsSidebarPanel.css:100
> +
> +
> +

Style: Too many newlines.

[Devin Rousso]

Created attachment 254892 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 254892 [details]
Patch

Rejecting attachment 254892 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.appspot.com', '--bot-id=webkit-cq-02', 'apply-attachment', '--no-update', '--non-interactive', 254892, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
ctorUI/UserInterface/Views/RulesStyleDetailsPanel.js
Hunk #1 FAILED at 25.
Hunk #6 FAILED at 262.
Hunk #7 succeeded at 286 (offset -3 lines).
2 out of 7 hunks FAILED -- saving rejects to file Source/WebInspectorUI/UserInterface/Views/RulesStyleDetailsPanel.js.rej
patching file Source/WebInspectorUI/UserInterface/Views/StyleDetailsPanel.js

Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force', '--reviewer', u'Timothy Hatcher']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Full output: http://webkit-queues.appspot.com/results/4709083202453504

[Devin Rousso]

Created attachment 255134 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 255134 [details]
Patch

Clearing flags on attachment: 255134

Committed r185723: <http://trac.webkit.org/changeset/185723>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.