[webkit-reviews] review granted: [Bug 199946] Web Inspector: Elements: Styles: add icons for various CSS rule types : [Attachment 374505] Patch

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has granted Devin Rousso
<drousso at apple.com>'s request for review:
Bug 199946: Web Inspector: Elements: Styles: add icons for various CSS rule
types
https://bugs.webkit.org/show_bug.cgi?id=199946

Attachment 374505: Patch

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




--- Comment #17 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 374505
  --> https://bugs.webkit.org/attachment.cgi?id=374505
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
> + at 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; }
	}
    }