195123 – Web Inspector: Styles: `::-webkit-scrollbar*` rules aren't shown

RESOLVED FIXED 195123

Web Inspector: Styles: `::-webkit-scrollbar*` rules aren't shown

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

Summary Web Inspector: Styles: `::-webkit-scrollbar*` rules aren't shown

Nikita Vasilyev

Reported 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.

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
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Radar WebKit Bug Importer

Comment 1 2019-02-27 14:19:53 PST

<rdar://problem/48450148>

Simon Fraser (smfr)

Comment 2 2019-02-27 15:03:51 PST

Would also be useful when inspecting https://www.zappos.com

Devin Rousso

Comment 3 2019-03-10 19:32:15 PDT

Created attachment 364221 [details] Patch

EWS Watchlist

Comment 4 2019-03-10 19:34:57 PDT Comment hidden (obsolete)

This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features.

EWS Watchlist

Comment 5 2019-03-10 19:35:07 PDT Comment hidden (obsolete)

Attachment 364221 [details] did not pass style-queue: ERROR: Source/WebCore/inspector/agents/InspectorCSSAgent.cpp:64: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.

Devin Rousso

Comment 6 2019-03-10 19:40:49 PDT

Created attachment 364222 [details] Patch

Joseph Pecoraro

Comment 7 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.

Devin Rousso

Comment 8 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.

Devin Rousso

Comment 9 2019-03-11 17:04:00 PDT

Created attachment 364313 [details] Patch

Devin Rousso

Comment 10 2019-03-11 17:04:34 PDT

I'll clean up the test a bit in <https://webkit.org/b/195580>.

Nikita Vasilyev

Comment 11 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.

Devin Rousso

Comment 12 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.

Joseph Pecoraro

Comment 13 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);

Devin Rousso

Comment 14 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

Devin Rousso

Comment 15 2019-03-14 00:56:56 PDT

Created attachment 364641 [details] Patch

WebKit Commit Bot

Comment 16 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>

WebKit Commit Bot

Comment 17 2019-03-14 02:33:30 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version WebKit Nightly Build

Hardware All

OS All

Product WebKit

Component Web Inspector

Assignee

Devin Rousso

Reported

2019-02-27 14:19 PST

Modified

2019-03-14 02:33 PDT History

CC List

11 users Show

URL

Keywords InRadar

Depends on

Blocks