Bug 195123 - Web Inspector: Styles: `::-webkit-scrollbar*` rules aren't shown
Summary: Web Inspector: Styles: `::-webkit-scrollbar*` rules aren't shown
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-02-27 14:19 PST by Nikita Vasilyev
Modified: 2019-03-14 02:33 PDT (History)
11 users (show)

See Also:


Attachments
Patch (16.73 KB, patch)
2019-03-10 19:32 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (17.12 KB, patch)
2019-03-10 19:40 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (46.46 KB, patch)
2019-03-11 17:04 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (47.89 KB, patch)
2019-03-14 00:56 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nikita Vasilyev 2019-02-27 14:19:30 PST
Steps:
1. Open https://css-tricks.com/examples/WebKitScrollbars/
2. Inspect <html>

Expected:
The following rules should be at the bottom of the styles editor:
::-webkit-scrollbar
::-webkit-scrollbar-track
::-webkit-scrollbar-thumb
::-webkit-scrollbar-thumb:window-inactive

Actual:
None of the ::-webkit-scrollbar* rules are visible.
Comment 1 Radar WebKit Bug Importer 2019-02-27 14:19:53 PST
<rdar://problem/48450148>
Comment 2 Simon Fraser (smfr) 2019-02-27 15:03:51 PST
Would also be useful when inspecting https://www.zappos.com
Comment 3 Devin Rousso 2019-03-10 19:32:15 PDT
Created attachment 364221 [details]
Patch
Comment 4 Build Bot 2019-03-10 19:34:57 PDT Comment hidden (obsolete)
Comment 5 Build Bot 2019-03-10 19:35:07 PDT Comment hidden (obsolete)
Comment 6 Devin Rousso 2019-03-10 19:40:49 PDT
Created attachment 364222 [details]
Patch
Comment 7 Joseph Pecoraro 2019-03-11 11:27:13 PDT
Comment on attachment 364222 [details]
Patch

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

We should be able to have a test for this.

Code changes look good to me.

> Source/WebInspectorUI/UserInterface/Controllers/CSSManager.js:107
> +                return WI.unlocalizedString("::first-line");

I don't think unlocalizedString is needed for any of these, they clearly look like code.
Comment 8 Devin Rousso 2019-03-11 11:32:56 PDT
Comment on attachment 364222 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Controllers/CSSManager.js:107
>> +                return WI.unlocalizedString("::first-line");
> 
> I don't think unlocalizedString is needed for any of these, they clearly look like code.

I personally prefer using `unlocalizedString` as it makes it immediately clear that this value is supposed/expected to be shown in the UI somewhere, as opposed to something like an enum, or event/css name.
Comment 9 Devin Rousso 2019-03-11 17:04:00 PDT
Created attachment 364313 [details]
Patch
Comment 10 Devin Rousso 2019-03-11 17:04:34 PDT
I'll clean up the test a bit in <https://webkit.org/b/195580>.
Comment 11 Nikita Vasilyev 2019-03-12 19:57:46 PDT
Comment on attachment 364313 [details]
Patch

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

Looks great! Some nits below.

> Source/JavaScriptCore/ChangeLog:11
> +        which pseudo type the rule corresponds to (e.g. a string is more descriptive than a number).

I think pseudo-element and pseudo-type should be hyphenated. That's how it is usually in W3C specs and MDN documentation.

> Source/WebInspectorUI/ChangeLog:7
> +        Reviewed by NOBODY (OOPS!).

Where can these new pseudo-element rules be found? Are they always at the bottom of the Styles panel? You should mention this; things like this help with STP changelogs. I would even list all the new rules we show now.

> Source/WebInspectorUI/ChangeLog:14
> +        * UserInterface/Models/DOMNodeStyles.js:
> +        (WI.DOMNodeStyles.static uniqueOrderedStyles): Added.
> +        (WI.DOMNodeStyles.prototype.get uniqueOrderedStyles):

Nice refactoring.

> Source/WebInspectorUI/ChangeLog:24
> +        Rather than iterate over the `WI.DOMNode`'s list of pseudo elements (which is only ::before
> +        and ::after), we iterate over the `WI.DOMNodeStyle`'s list of pseudo element rules. This is
> +        an object where the key is a `CSS.PseudoId` and the value is an object containing all the
> +        matched rules and ordered styles for that pseudo type. We can preserve the current
> +        functionality by using the ::before/::after `WI.DOMNode` when we encounter one of those
> +        pseudo ids.

Hyphenate pseudo-*.

> Source/JavaScriptCore/inspector/protocol/CSS.json:52
> +            "description": "Pseudo style identifier (see <code>enum PseudoId</code> in <code>RenderStyleConstants.h</code>)."

Hyphenate pseudo-style.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:289
> +            createHeader(WI.UIString("Pseudo Element"), nodeOrPseudoId);

Pseudo-element

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:295
> +                let section = createSection(style);
> +
> +                if (nodeOrPseudoId === pseudoId)
> +                    section.__pseudoId = pseudoId;

`__publicProperty` looks like a dirty hack. Can we just modify WI.SpreadsheetCSSStyleDeclarationSection? Either by adding a parameter to the constructor or introducing a getter&setter.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:323
> +        let header = this._headerMap.get(event.target.__pseudoId || event.target.style.node);

Ditto.
Comment 12 Devin Rousso 2019-03-12 20:57:03 PDT
Comment on attachment 364313 [details]
Patch

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

>> Source/WebInspectorUI/ChangeLog:7
>> +        Reviewed by NOBODY (OOPS!).
> 
> Where can these new pseudo-element rules be found? Are they always at the bottom of the Styles panel? You should mention this; things like this help with STP changelogs. I would even list all the new rules we show now.

Good point.  They'd appear in the same area as the existing `::before`/`::after` styles.

One fun "side effect"/bonus of this change (which I definitely should've mentioned) is that `::before`/`::after` styles will still appear in the sidebar even if they don't have a `content` property set (e.g. when the `::before`/`::after` pseudo-element doesn't exist).  This is because the styles are no longer fetched from those pseudo-element nodes directly, but rather as a matched style for the parent node.  As such, editing a `content` property in a `::before`/`::after` rule will now no longer appear if you comment it out (or make it unapply/invalid in any other way).

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:295
>> +                    section.__pseudoId = pseudoId;
> 
> `__publicProperty` looks like a dirty hack. Can we just modify WI.SpreadsheetCSSStyleDeclarationSection? Either by adding a parameter to the constructor or introducing a getter&setter.

We do this pretty often, for "bookkeeping" that the underlying object/class doesn't really need to know about.  The only reason why this is needed is so that we can relate a header to a style when filtering.  Unless there's an active filter, the property won't do anything or even be used for anything.  `WI.SpreadsheetCSSStyleDeclarationSection` doesn't need to know about what `pseudoId` it correlates to at all, just like how it doesn't need to know what header it's under.  Alternatively, we could maintain another `Map` that holds `section` => `header`, but that requires a lot more work to keep in sync, and is really overkill for something this small.
Comment 13 Joseph Pecoraro 2019-03-13 23:21:59 PDT
Comment on attachment 364313 [details]
Patch

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

r=me

>>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:295
>>> +                    section.__pseudoId = pseudoId;
>> 
>> `__publicProperty` looks like a dirty hack. Can we just modify WI.SpreadsheetCSSStyleDeclarationSection? Either by adding a parameter to the constructor or introducing a getter&setter.
> 
> We do this pretty often, for "bookkeeping" that the underlying object/class doesn't really need to know about.  The only reason why this is needed is so that we can relate a header to a style when filtering.  Unless there's an active filter, the property won't do anything or even be used for anything.  `WI.SpreadsheetCSSStyleDeclarationSection` doesn't need to know about what `pseudoId` it correlates to at all, just like how it doesn't need to know what header it's under.  Alternatively, we could maintain another `Map` that holds `section` => `header`, but that requires a lot more work to keep in sync, and is really overkill for something this small.

Yeah this is a situation where we would normally do the __property trick.

> LayoutTests/inspector/css/getMatchedStylesForNode.html:45
> +        function replacer(key, value) {
> +            if (key === "ruleId" || key === "styleId" || key === "range" || key === "sourceLine" || key === "sourceURL")
> +                return "<filtered>";
> +            return value;
> +        }

You could completely skip keys you don't care about. For example some these you could probably just hide to simplify the output, including:

    if (key === "matchingSelectors" || key === "shorthandEntries")
        return undefined;

> LayoutTests/inspector/css/getMatchedStylesForNode.html:50
> +        ProtocolTest.log(JSON.stringify(matchedCSSRules, replacer, 2));

We have `*Test.json(...)` which should allow you to simplify this:

    ProtocolTest.log(JSON.stringify(matchedCSSRules, replacer, 2));

to just:

    ProtocolTest.json(matchedCSSRules, replacer);
Comment 14 Devin Rousso 2019-03-14 00:40:18 PDT
Comment on attachment 364313 [details]
Patch

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

>> LayoutTests/inspector/css/getMatchedStylesForNode.html:45
>> +        }
> 
> You could completely skip keys you don't care about. For example some these you could probably just hide to simplify the output, including:
> 
>     if (key === "matchingSelectors" || key === "shorthandEntries")
>         return undefined;

I'd prefer to keep these there, as this is the first test for the bare functionality/payload of CSS.getMatchedStylesForNode.

>> LayoutTests/inspector/css/getMatchedStylesForNode.html:50
>> +        ProtocolTest.log(JSON.stringify(matchedCSSRules, replacer, 2));
> 
> We have `*Test.json(...)` which should allow you to simplify this:
> 
>     ProtocolTest.log(JSON.stringify(matchedCSSRules, replacer, 2));
> 
> to just:
> 
>     ProtocolTest.json(matchedCSSRules, replacer);

0.0
Comment 15 Devin Rousso 2019-03-14 00:56:56 PDT
Created attachment 364641 [details]
Patch
Comment 16 WebKit Commit Bot 2019-03-14 02:33:28 PDT
Comment on attachment 364641 [details]
Patch

Clearing flags on attachment: 364641

Committed r242939: <https://trac.webkit.org/changeset/242939>
Comment 17 WebKit Commit Bot 2019-03-14 02:33:30 PDT
All reviewed patches have been landed.  Closing bug.