I often find myself editing styles that belong to pseudo-elements. A visual indicator somewhere would be very useful in preventing this problem :)
<rdar://problem/27861479>
Created attachment 286155 [details] Patch
Created attachment 286156 [details] [Image] After Patch is applied
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?
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.
(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.
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).
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*.
(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
Created attachment 286666 [details] Patch
Comment on attachment 286666 [details] Patch Clearing flags on attachment: 286666 Committed r204754: <http://trac.webkit.org/changeset/204754>
All reviewed patches have been landed. Closing bug.