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.
rdar://problem/13658649
Created attachment 254039 [details] Patch
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 ("
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.
Created attachment 254074 [details] Patch
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.
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.
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.
Created attachment 254536 [details] Patch
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.
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".
Created attachment 254845 [details] Patch
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.
Created attachment 254892 [details] Patch
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
Created attachment 255134 [details] Patch
Comment on attachment 255134 [details] Patch Clearing flags on attachment: 255134 Committed r185723: <http://trac.webkit.org/changeset/185723>
All reviewed patches have been landed. Closing bug.