Bug 160893 – Web Inspector: Add indicator to matched selector being a pseudo-element

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

[Devin Rousso]

I often find myself editing styles that belong to pseudo-elements.  A visual indicator somewhere would be very useful in preventing this problem :)

[Radar WebKit Bug Importer]

<rdar://problem/27861479>

[Devin Rousso]

Created attachment 286155 [details]
Patch

[Devin Rousso]

Created attachment 286156 [details]
[Image] After Patch is applied

[Timothy Hatcher]

Comment on attachment 286155 [details]
Patch

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

I'm surprised we still show :before and :after in the sidebar when Joe made them them appear in the DOM tree.

> Source/WebInspectorUI/UserInterface/Models/CSSSelector.js:73
> +        return this._text.endsWith(":before") || this._text.endsWith(":after");

Why just :before and :after? What about the many others?

[Joseph Pecoraro]

Comment on attachment 286155 [details]
Patch

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

This change feels too arbitrary to me. A rule can affect both regular elements and pseudo elements. If it affects any pseudo element it gets promoted to the [P]?

If a rule _only_ affects pseudo elements, then Tim's hint makes some sense: we could hide that rule on the "host element" and only show the rule when you've selected the pseudo element itself (::before / ::after). Still, I could see users expecting both behaviors.

>> Source/WebInspectorUI/UserInterface/Models/CSSSelector.js:73
>> +        return this._text.endsWith(":before") || this._text.endsWith(":after");
> 
> Why just :before and :after? What about the many others?

I think the intent here is a PsuedoElement selector. Like Tim says, there are many:
https://developer.mozilla.org/en-US/docs/Web/CSS/Pseudo-elements

Only before/after tend to have elements in the DOM Tree that you can interact with though.

[Devin Rousso]

(In reply to comment #4)
> I'm surprised we still show :before and :after in the sidebar when Joe made
> them them appear in the DOM tree.

This is how Chrome does it, and I do agree with leaving the :before/:after on the parent node.

> > Source/WebInspectorUI/UserInterface/Models/CSSSelector.js:73
> > +        return this._text.endsWith(":before") || this._text.endsWith(":after");
> 
> Why just :before and :after? What about the many others?

I actually did not know that some of these other ones existed (⚆ _ ⚆)


(In reply to comment #5)
> This change feels too arbitrary to me. A rule can affect both regular
> elements and pseudo elements. If it affects any pseudo element it gets
> promoted to the [P]?

It will actually only show the [P] if one of the currently applying selectors contains a pseudo-element.  This is why I check for whether the CSSSelector that contains the pseudo-element is inside the list of matchedSelectorIndices.
 
> If a rule _only_ affects pseudo elements, then Tim's hint makes some sense:
> we could hide that rule on the "host element" and only show the rule when
> you've selected the pseudo element itself (::before / ::after). Still, I
> could see users expecting both behaviors.

I would like to keep the pseudo-elements on the host.  I think it makes editing pseudo-elements much easier.

> >> Source/WebInspectorUI/UserInterface/Models/CSSSelector.js:73
> >> +        return this._text.endsWith(":before") || this._text.endsWith(":after");
> > 
> > Why just :before and :after? What about the many others?
> 
> I think the intent here is a PsuedoElement selector. Like Tim says, there
> are many:
> https://developer.mozilla.org/en-US/docs/Web/CSS/Pseudo-elements
> 
> Only before/after tend to have elements in the DOM Tree that you can
> interact with though.

Well :first-letter is pretty awesome, so I will add that too.

[Devin Rousso]

Created attachment 286239 [details]
Patch

I have thought about this a bit more, and I think that it is actually fine with just having :before/:after show up as [P].

1) :before/:after are really the only two pseudo-elements that have any substance (meaning they show up separately in the DOM tree and have an outline when hovered).  AFAICT, it looks like the rest of the pseudo-element selectors (with the exception of :first-letter) don't allow any dimensions/positioning styles to be applied, meaning that the developer will always know where they are if they see one.  This is not true for :before/:after (and :first-letter) as they can have any CSS applied, thus making the need for a distinction for those rules all the more useful.

2) In the context menu for the Styles sidebar, we only have entries for "Create/Focus ::before" and "Create/Focus ::after", which come from `WebInspector.CSSStyleManager.PseudoElementNames`, meaning that if we modify that list then more items will appear in the Styles sidebar context menus.

3) It looks like many of the pseudo-elements are still experimental and I am not really sure how well they are supported by WebKit.

4) To really do this correctly, we would need direct support from the backend (as Joe said on https://webkit.org/b/158272), whereas support for :before/:after is much easier to deal with (not including weird :matches() selectors).

[Joseph Pecoraro]

Comment on attachment 286239 [details]
Patch

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

r=me, but you may want to follow-up with a mass reduction of the SVGs.

> Source/WebInspectorUI/UserInterface/Base/Utilities.js:480
> +         if (exists)

Style: Indentation is off.

> Source/WebInspectorUI/UserInterface/Images/StyleRuleAuthorPseudo.svg:3
> +<svg xmlns="http://www.w3.org/2000/svg" id="root" version="1.1" viewBox="0 0 16 16">

These are all the same with a different style? It should be possible to use a single SVG and just style with CSS. Take a look at Source/WebInspectorUI/UserInterface/Images/GoToArrow.svg for example with its usages: 

SVG defines the path and styles. Then ids for using the path with a particular style:

>     <svg ... viewBox="0 0 10 10">
>         <style>
>             path { fill: inherit; }
>             #normal { fill: hsla(0, 0%, 0%, 0.5); }
>             #active { fill: hsla(0, 0%, 0%, 0.7); }
>         </style>
>         <defs>
>             <path id="go-to-arrow-path" d="..."/>
>         </defs>
>         <svg id="normal"><use xlink:href="#go-to-arrow-path"/></svg>
>         <svg id="active"><use xlink:href="#go-to-arrow-path"/></svg>
>     </svg>

Use points:

>    background-image: url(../Images/GoToArrow.svg#normal);
>    background-image: url(../Images/GoToArrow.svg#active);

It would be great to reduce all 4 to one image with a few tiny styles.

Maybe you can do this to the pre-existing non-pseudo StyleRule svgs as well?

> Source/WebInspectorUI/UserInterface/Models/CSSRule.js:163
> +         return this.matchedSelectors.some(selector => selector.isPseudoElementSelector());

Style: Indentation is off.
Style: We always put parenthesis around arrow function parameters. "(selector)"

> Source/WebInspectorUI/UserInterface/Models/CSSSelector.js:73
> +        return WebInspector.CSSStyleManager.PseudoElementNames.some(name => this._text.includes(`:${name}`));

Style: We always put parenthesis around arrow function parameters. "(name)"

> Source/WebInspectorUI/UserInterface/Views/VisualStyleSelectorTreeItem.js:280
> +        let isMatchedPseudoElementSelector = this.representedObject.ownerRule && this.representedObject.ownerRule.hasMatchedPseudoElementSelector();

Nit: Variable name should be hasMatchedPseudoElementSelector not is*.

[Devin Rousso]

(In reply to comment #8)
> r=me, but you may want to follow-up with a mass reduction of the SVGs.

https://bugs.webkit.org/show_bug.cgi?id=161071

[Devin Rousso]

Created attachment 286666 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 286666 [details]
Patch

Clearing flags on attachment: 286666

Committed r204754: <http://trac.webkit.org/changeset/204754>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.