Bug 199946 – Web Inspector: Elements: Styles: add icons for various CSS rule types

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

[Devin Rousso]

[{}] for "regular" rules
[P] for pseudo-element rules
[E] for inline styles/attributes

[Nikita Vasilyev]

These icons were removed based on the feedback from web developers in 2017. People didn’t understand the icons meaning and they took precious horizontal space (Elements tab is especially common to be used in docked to right Web Inspector).

[Nikita Vasilyev]

(In reply to Nikita Vasilyev from comment #1)
> These icons were removed based on the feedback from web developers in 2017.

I think it was in 2016, actually.

[Devin Rousso]

Created attachment 374481 [details]
Patch

[Devin Rousso]

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

[Devin Rousso]

Created attachment 374483 [details]
[Image] After Patch is applied (context menu)

[Devin Rousso]

Comment on attachment 374481 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.css:51
> +    margin-top: 1px;

This prevents the selector from shifting vertically when editing.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.css:109
> +    margin-top: -1px;

This makes the vertical space above the icon match the leading space before the icon.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.css:110
> +    -webkit-margin-start: -2px;

This makes the icon centered with the property checkboxes.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.css:111
> +    vertical-align: -3.5px;

This vertically centers the icon with the selector text.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.css:116
> +    -webkit-padding-start: 2px;

This is done this way so that if a user tries to select the icon, it is uniform in how it appears.

[Nikita Vasilyev]

(In reply to Devin Rousso from comment #4)
> Created attachment 374482 [details]
> [Image] After Patch is applied

I don't think this is a good idea. It looks particularly bad when a CSS selector takes more than one line.

When we had these icons, people we surveyed didn't even know they could click on them to toggle rules. They certanly didn't know about context menus. If you really want more context menu items, you can add a context menu for the entire style rule.

[Devin Rousso]

(In reply to Nikita Vasilyev from comment #7)
The main reason I want to do this is because it's VERY hard to identify the "category" of rule I may be looking for just from a quick glance.  If I'm trying to find a pseudo-selector rule, I either have to look for the "header" that lists the name, or try to find a matched selector with ":{blah}".  That's _hard_, and could instead be an instant process just by looking for a [P].

> I don't think this is a good idea. It looks particularly bad when a CSS selector takes more than one line.
How?  What's bad about it?  The text wraps around the icon quite nicely.  How is this any "worse" than the selector overlapping the source code location?

> When we had these icons, people we surveyed didn't even know they could click on them to toggle rules.
I'm not suggesting we add that back, although it was super useful.  If anything, I'd want to add this to the context menu.

> They certanly didn't know about context menus.
Just because someone doesn't know about something doesn't mean that it won't be useful.  Furthermore, there's things we can do to educate people about this functionality, which is what I'm trying to do via the WebKit blog.

> If you really want more context menu items, you can add a context menu for the entire style rule.
That doesn't work as we'd potentially clash with the default context menu  when any text is selected.  Try context menu (right) clicking on any text anywhere in the Styles sidebar and see how it will _always_ highlight some text.  We don't want to clash with the text selection actions, so we need something somewhere that isn't text that we can click.  Furthermore, this "version" only requires a "mousedown" to be useful, which is _far_ more discoverable than a "contextmenu".

[Nikita Vasilyev]

Created attachment 374499 [details]
[Image] Devin vs Tim's design

[Devin Rousso]

Created attachment 374502 [details]
Patch

[Nikita Vasilyev]

(In reply to Devin Rousso from comment #8)
> (In reply to Nikita Vasilyev from comment #7)
> The main reason I want to do this is because it's VERY hard to identify the
> "category" of rule I may be looking for just from a quick glance.  If I'm
> trying to find a pseudo-selector rule, I either have to look for the
> "header" that lists the name, or try to find a matched selector with
> ":{blah}".  That's _hard_, and could instead be an instant process just by
> looking for a [P].
> 
> > I don't think this is a good idea. It looks particularly bad when a CSS selector takes more than one line.
> How?  What's bad about it?  The text wraps around the icon quite nicely. 
> How is this any "worse" than the selector overlapping the source code
> location?

I actually don't particularly like selector overlapping the source code location. It was a trade-off to make it easier for an eye to scan for resources.

It is worse because the icons you're adding are taller than text letters.

[Devin Rousso]

Created attachment 374505 [details]
Patch

[Devin Rousso]

(In reply to Nikita Vasilyev from comment #11)
> I actually don't particularly like selector overlapping the source code location. It was a trade-off to make it easier for an eye to scan for resources.
I agree.  In the same vein, this is a trade off worth making to help developers more easily find the "category" of rule they're looking for, as well as proving even clearer demarcations between rules and an actionable "icon"/button that provides additional useful functionality.

> It is worse because the icons you're adding are taller than text letters.
Yes, but I made sure that no matter what you do there's no vertical shifting (e.g. editing).

[Nikita Vasilyev]

(In reply to Devin Rousso from comment #8)
> (In reply to Nikita Vasilyev from comment #7)
> The main reason I want to do this is because it's VERY hard to identify the
> "category" of rule I may be looking for just from a quick glance.  If I'm
> trying to find a pseudo-selector rule, I either have to look for the
> "header" that lists the name, or try to find a matched selector with
> ":{blah}".  That's _hard_, and could instead be an instant process just by
> looking for a [P].

I don't belive finding pseudo-selectors is VERY hard. Okay, let say we want to make it easier. I don't belive adding icons is the right solution.

There are 3 types of icons:
[{}] for "regular" rules
[P] for pseudo-element rules
[E] for inline styles/attributes

Style attribute rules always come first. They are very easy to find.

Pseudo-element rules have start with "::". That's how people find them in their code editors - just by looking at the selectors.

The vast majority of CSS rules would have the same icon — [{}]. Having the same icon for every rule is just noise.

If the main goal is to make pseudo-element rules easier to find, I don't belive adding these icons to every rule is the best solution. For example, uneditable rules have a slight gray background and a small lock icon at the top right corner.

[Devin Rousso]

(In reply to Nikita Vasilyev from comment #14)
> Pseudo-element rules have start with "::". That's how people find them in their code editors - just by looking at the selectors.
This is not as easy as you make it sound. At a glance, is it immediately obvious that the following selector contains a pseudo-selector?  Furthermore, is it obvious whether this particular element is matching the pseudo-selector, or the non-pseudo-selector?

```css
    body[dir=ltr] .details-section > .header::before, body[dir=rtl] .details-section > .header > .options
```

> The vast majority of CSS rules would have the same icon — [{}]. Having the same icon for every rule is just noise.
The colors also mean something very different, and can easily distinguish the type of rule (e.g. attribute styles vs a "regular" CSS rule).

> If the main goal is to make pseudo-element rules easier to find, I don't belive adding these icons to every rule is the best solution. For example, uneditable rules have a slight gray background and a small lock icon at the top right corner.
It's less about "finding" them, and more about identifying them.  An icon can convey SO much more information at a MUCH faster pace than having to grok text.

Now that you mention it, I think that the grey background is WAY too subtle of a thing to notice, and the lock icon is far enough out of the way to have the same issue.  I would like to put the icon in a much more immediately visible place (such as right next to the [ ] icon).

[Joseph Pecoraro]

I think adding icons provides the following benefits with no obvious drawbacks:

    (1) Distinguishing CSS "Rule" Types and providing visual anchors for rule sections
        Types:
          • Style Attribute
          • Author Rule
          • Inherited Rule
          • User Agent Rule
          • Pseudo Element Rule

    (2) Provides a convenient Hit Target for Discovery and Activation of Useful Actions associated with the "Rule"
        Actions:
          • Copy (selector + properties)
          • Enable/Disable
          • Duplicate (starting point to create a new rule like this one much easier then typing it all out from scratch)

None of these have corollaries in the current sidebar.

    • Finding a pseudo element selector in the current design means reading the bold and only bold text of each selector in a list which may be difficult in a list of complex selectors.
    • Finding the next selector in a list of complex selectors is not easy as it could be, you have to find the thin gray line and the typically obfuscated link
    • Copying an entire CSS Rule (selector + properties) is not possible today without a seemingly broken selection rect
    • Enabling / Disabling all properties in a rule takes many clicks or deleting the properties
    • Duplicating a Rule is not possible today today without copying and pasting a few times or re-typing the full selector

So I see a lot of benefits to this without any drawbacks.

----

> It looks particularly bad when a CSS selector takes more than one line.

I'm not sure it is much worse than today. The large wrapping text on complex selectors is hard to read.

> It is worse because the icons you're adding are taller than text letters.

This can be experimented with, but I think consistency with the size of icons elsewhere will want us to keep the icon this size. Maybe adjusting the baseline a tad we can find something that feels more suitable?

> If you really want more context menu items, you can add a context menu for the entire style rule.

Where would it go? The icon is a natural good hit target and one we use for this purpose elsewhere (Object Views).

> The vast majority of CSS rules would have the same icon — [{}]. Having the same icon for every rule is just noise.

Due to the added benefit of the context menu / actions this wouldn't be just noise. I do see the concern here, but I don't recall it feeling like noise in the old sidebar, so I'm not too concerned now.

> If the main goal is to make pseudo-element rules easier to find, I don't believe adding these icons to every rule is the best solution.

I'd be willing to brainstorm other ideas. Do you have any? Given the existing benefits to icons that seems an apt starting point at least.

> Pseudo-element rules have start with "::". That's how people find them in their code editors - just by looking at the selectors.

Text editors have a different workflow than Web Inspector's Styles sidebar that make it easier to find what you're looking for:

  - the order of rules is as the author has written them, so they may already know where to look for them
    - the Styles sidebar shows a potentially mixed order collection of author rules which require careful re-reading by the developer to find the one they want
  - search capabilities in a text editor move around the entire file
    - the Styles sidebar doesn't (and I'd argue shouldn't) support the same rich search features as it is contextual data

Note that filtering is available but serves a different purpose. Filtering to pseudo elements may be useful at times, but it is still different from "finding" the pseudo element styles to then edit it while still seeing the other styles alongside it.

----

> as proving even clearer demarcations between rules and an actionable "icon"/button that provides additional useful functionality.

Yeah, this aligns with my thoughts above.

----

Perhaps we start this as an experimental feature, and aim to enable by default / remove it after living on it for a month. I haven't played with it yet, but it looks like something I'd like to use!

[Joseph Pecoraro]

Comment on attachment 374505 [details]
Patch

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

r=me

It still may be worth making this an experimental feature (it just needs to guard iconElement) for a period of time so we can experiment with it on and off quickly.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:338
> +            this._iconElement.addEventListener("contextmenu", this._handleIconElementContextMenu.bind(this));

Can we use `WI.appendContextMenuItemsForSourceCode` for this element and eliminate most of the code in the current _handleMouseDown?

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:366
> +            this._selectorElement.append(" ", WI.UIString("Style Attribute", "CSS properties defined via HTML style attribute"));

In the screenshot this space seems a different horizontal width than the space between an icon and a selector.

It looks subtly like this:

    [E]  Style Attribute
    [#] svg

I think we should make these align a bit better.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:445
> +        if (this._iconElement.contains(event.target)) {

I'm hoping we can eliminate this by using `WI.appendContextMenuItemsForSourceCode` on the iconElement itself. I suspect that came after this patch was first written.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:498
> +    _handleIconElementContextMenu(event)

This could be eliminated as well.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:519
> +        if (this._style.inherited)
> +            return;

These early returns (especially this one) seem likely to cause mistakes.

For example, down below there is this exact condition again, which must be true.

This kind of populate function would read better with no returns and if blocks for adding additional functionality:

    _populateContextMenu(contextMenu)
    {
        // Always...

        if (editable) {
            ...
        }

        if (!inline) {
            ...
        }

        if (!inherited) {
            ...
        }

        if (!pseudo) {
            ...
        }
    }

Additional Context Menu ideas:

• "Reveal in Sources Tab" if there is a sourceCodeLocation which would do what the location link on the side does Would be nice if that could be named "Reveal in Stylesheet" if it takes you to a CSS (Less/Sass) resource and not a <style> or style attribute definition.

> LayoutTests/inspector/css/generateCSSRuleString-expected.txt:12
> +@media only screen and (min-width: 0px) {

Does this handle multiple-nested blocks correctly?

    @media (prefers-color-scheme: dark) {
        @media (min-width: 0px) {
            body { color: blue; }
        }
    }

I doubt it, but do we / should we handle @supports?

    @media (prefers-color-scheme: dark) {
        @supports (position: sticky) {
            body { position: sticky; }
        }
    }

[Devin Rousso]

Comment on attachment 374505 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:366
>> +            this._selectorElement.append(" ", WI.UIString("Style Attribute", "CSS properties defined via HTML style attribute"));
> 
> In the screenshot this space seems a different horizontal width than the space between an icon and a selector.
> 
> It looks subtly like this:
> 
>     [E]  Style Attribute
>     [#] svg
> 
> I think we should make these align a bit better.

They're slightly off because "Style Attribute" is sans-serif, whereas the selectors are monospaced.  I'll adjust it as best I can.

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:445
>> +        if (this._iconElement.contains(event.target)) {
> 
> I'm hoping we can eliminate this by using `WI.appendContextMenuItemsForSourceCode` on the iconElement itself. I suspect that came after this patch was first written.

Yup! :P

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:519
>> +            return;
> 
> These early returns (especially this one) seem likely to cause mistakes.
> 
> For example, down below there is this exact condition again, which must be true.
> 
> This kind of populate function would read better with no returns and if blocks for adding additional functionality:
> 
>     _populateContextMenu(contextMenu)
>     {
>         // Always...
> 
>         if (editable) {
>             ...
>         }
> 
>         if (!inline) {
>             ...
>         }
> 
>         if (!inherited) {
>             ...
>         }
> 
>         if (!pseudo) {
>             ...
>         }
>     }
> 
> Additional Context Menu ideas:
> 
> • "Reveal in Sources Tab" if there is a sourceCodeLocation which would do what the location link on the side does Would be nice if that could be named "Reveal in Stylesheet" if it takes you to a CSS (Less/Sass) resource and not a <style> or style attribute definition.

Good point.  I like "Reveal in Stylesheet" :)

>> LayoutTests/inspector/css/generateCSSRuleString-expected.txt:12
>> +@media only screen and (min-width: 0px) {
> 
> Does this handle multiple-nested blocks correctly?
> 
>     @media (prefers-color-scheme: dark) {
>         @media (min-width: 0px) {
>             body { color: blue; }
>         }
>     }
> 
> I doubt it, but do we / should we handle @supports?
> 
>     @media (prefers-color-scheme: dark) {
>         @supports (position: sticky) {
>             body { position: sticky; }
>         }
>     }

It does work with nested `@media`.  I'll add another test.

I also created this: <https://webkit.org/b/200419> Web Inspector: Styles: show @supports media queries

[Devin Rousso]

Created attachment 375488 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 375488 [details]
Patch

Clearing flags on attachment: 375488

Committed r248202: <https://trac.webkit.org/changeset/248202>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/53904309>