| 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: | P2 | Keywords: | InRadar | ||||||||||
| Version: | WebKit Nightly Build | ||||||||||||
| Hardware: | All | ||||||||||||
| OS: | All | ||||||||||||
| See Also: | https://bugs.webkit.org/show_bug.cgi?id=195580 | ||||||||||||
| Attachments: |
|
||||||||||||
|
Description
Nikita Vasilyev
2019-02-27 14:19:30 PST
Would also be useful when inspecting https://www.zappos.com Created attachment 364221 [details]
Patch
This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features. 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.
Created attachment 364222 [details]
Patch
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 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. Created attachment 364313 [details]
Patch
I'll clean up the test a bit in <https://webkit.org/b/195580>. 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 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 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 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 Created attachment 364641 [details]
Patch
Comment on attachment 364641 [details] Patch Clearing flags on attachment: 364641 Committed r242939: <https://trac.webkit.org/changeset/242939> All reviewed patches have been landed. Closing bug. |