Bug 158272 – Web Inspector: Visual Styles: "Text -> Content" section should only be visible for pseudo-element

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Nikita Vasilyev]

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.

[Radar WebKit Bug Importer]

<rdar://problem/26585010>

[Devin Rousso]

Created attachment 286106 [details]
Patch

[Joseph Pecoraro]

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?

[Devin Rousso]

Created attachment 286157 [details]
Patch

[Joseph Pecoraro]

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.

[Devin Rousso]

Created attachment 286240 [details]
Patch

[Devin Rousso]

Created attachment 286461 [details]
Patch

[Joseph Pecoraro]

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"?

[Devin Rousso]

Created attachment 286672 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 286672 [details]
Patch

Clearing flags on attachment: 286672

Committed r204757: <http://trac.webkit.org/changeset/204757>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.