Currently, "Content" is the first item in the "Text" section, yet it doesn't do anything for regular (non pseudo) elements. We should only show it for pseudo elements.
<rdar://problem/26585010>
Created attachment 286106 [details] Patch
Comment on attachment 286106 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=286106&action=review r=me, but maybe file a follow-up on better detection. > Source/WebInspectorUI/UserInterface/Views/VisualStyleDetailsPanel.css:-100 > -.visual-style-property-container.transition, > -.visual-style-property-container.animation { > - margin-left: 11px; > -} Seems unrelated, but fine! > Source/WebInspectorUI/UserInterface/Views/VisualStyleDetailsPanel.js:193 > + let isPseudoElement = currentSelector.includes(":before") || currentSelector.includes(":after") || this._nodeStyles.node.isPseudoElement(); This is tricky. Selector text of: "foo x::before bar" wouldn't want this. But "foo, x::before, bar" would. Perhaps this could be made smarter to check each of the selectors to see if one ends with :before/:after?
Created attachment 286157 [details] Patch
Comment on attachment 286157 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=286157&action=review > Source/WebInspectorUI/UserInterface/Models/CSSRule.js:164 > + hasMatchedPseudoSelector() > + { > + if (this.nodeStyles && this.nodeStyles.node && this.nodeStyles.node.isPseudoElement()) > + return true; > + > + return this.selectors.some((selector, i) => selector.isPseudoSelector() && this.matchedSelectors.includes(i)); > + } The intent here is PsuedoElement selectors. But there are plenty of other pseudo element selectors and the CSS content property only affects ::before/::after, so this should be hasBeforeAfterPseudoElementSelector, with a CSSSelector method isBeforeAfterPsuedoElementSelector. In reality I don't think we can do this perfectly without re-implementing a selector engine or backend support, since things like ":matches(div::before, foo)" which expands to "div::before, foo" would be missed. But what you have catches the 90% case, so I think its worth doing. > Source/WebInspectorUI/UserInterface/Views/VisualStyleDetailsPanel.js:194 > + let isMatchedPseudoSelector = this._currentStyle.ownerRule && this._currentStyle.ownerRule.hasMatchedPseudoSelector(); > + this._groups.content.section.element.classList.toggle("inactive", !isMatchedPseudoSelector); > + this._groups.listStyle.section.element.classList.toggle("inactive", isMatchedPseudoSelector); Variable name suggestion: let ruleMatchesBeforeAfterPseudoElement. > Source/WebInspectorUI/UserInterface/Views/VisualStyleSelectorTreeItem.js:58 > + let iconClasses = [iconClassName]; > + if (style.ownerRule && style.ownerRule.hasMatchedPseudoSelector()) > + iconClasses.push(WebInspector.CSSStyleDeclarationSection.PseudoSelectorStyleClassName); I see this is doing the [P] things from the other patch. Which is still being discussed. I think the top parts above regarding `content` look good to me. But these still need some discussion.
Created attachment 286240 [details] Patch
Created attachment 286461 [details] Patch
Comment on attachment 286461 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=286461&action=review r=me > Source/WebInspectorUI/UserInterface/Views/VisualStyleDetailsPanel.js:668 > + properties.content = new WebInspector.VisualStyleBasicInput("content", null, WebInspector.UIString("Enter a value")); Something feels off about this placeholder string. Safari's URL bar says "Search or enter website name" not "Search or enter a website name". Maybe drop the "a" here making this "Enter value"?
Created attachment 286672 [details] Patch
Comment on attachment 286672 [details] Patch Clearing flags on attachment: 286672 Committed r204757: <http://trac.webkit.org/changeset/204757>
All reviewed patches have been landed. Closing bug.