Bug 195123

Summary:

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

Product:

WebKit

Reporter:

Nikita Vasilyev <nvasilyev>

Component:

Web Inspector

Assignee:

Devin Rousso <hi>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

commit-queue, ews-watchlist, hi, inspector-bugzilla-changes, joepeck, keith_miller, mark.lam, msaboff, saam, simon.fraser, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

All

OS:

All

See Also:

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

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none

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.