<html>
    <head>
      <base href="https://bugs.webkit.org/">
    </head>
    <body>
      <p>
        <div>
            <b><a class="bz_bug_link 
          bz_status_NEW "
   title="NEW - Web Inspector: add contextual documentation for CSS properties"
   href="https://bugs.webkit.org/show_bug.cgi?id=226883#c23">Comment # 23</a>
              on <a class="bz_bug_link 
          bz_status_NEW "
   title="NEW - Web Inspector: add contextual documentation for CSS properties"
   href="https://bugs.webkit.org/show_bug.cgi?id=226883">bug 226883</a>
              from <span class="vcard"><a class="email" href="mailto:drousso@apple.com" title="Devin Rousso <drousso@apple.com>"> <span class="fn">Devin Rousso</span></a>
</span></b>
        <pre>Comment on <span class=""><a href="attachment.cgi?id=432655&action=diff" name="attach_432655" title="Patch v1.4 -Trimmed database of unwanted properties, console.assert() checks added, uniform icon sizing">attachment 432655</a> <a href="attachment.cgi?id=432655&action=edit" title="Patch v1.4 -Trimmed database of unwanted properties, console.assert() checks added, uniform icon sizing">[details]</a></span>
Patch v1.4 -Trimmed database of unwanted properties, console.assert() checks added, uniform icon sizing

View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=432655&action=review">https://bugs.webkit.org/attachment.cgi?id=432655&action=review</a>

Looking good!  Almost there =D

I think after these we'll be in a good place to land :)

<span class="quote">> Source/WebInspectorUI/UserInterface/Base/Setting.js:215
> +    showContextualSyntax: new WI.Setting("show-contextual-syntax", true),</span >

I thought we agreed to default disabled?

also, I think this needs a more clear name like `showCSSPropertySyntaxInDocumentationPopover`

<span class="quote">> Source/WebInspectorUI/UserInterface/Images/InfoIcon.svg:21
> +    #unavailable {</span >

i think you can remove this now

<span class="quote">> Source/WebInspectorUI/UserInterface/Images/InfoIcon.svg:29
> +    #selected {</span >

This seems unused.  Remove?

<span class="quote">> Source/WebInspectorUI/UserInterface/Images/InfoIcon.svg:33
> +    #selected-active {</span >

ditto :(29)

<span class="quote">> Source/WebInspectorUI/UserInterface/Images/InfoIcon.svg:46
> +  <svg id="selected"><use xlink:href="#info"/></svg>
> +  <svg id="selected-active"><use xlink:href="#info"/></svg></span >

ditto :(29)

<span class="quote">> Source/WebInspectorUI/UserInterface/Images/InfoIcon.svg:47
> +  <svg id="unavailable"><use xlink:href="#noinfo"/></svg></span >

ditto (:21)

<span class="quote">> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.css:53
> +.sidebar > .panel.details.css-style > .content > .computed .computed-style-variables .property .go-to-arrow {</span >

`.sidebar > .panel.details.css-style > .content > .computed > .details-section.computed-style-variables .property .go-to-arrow`

<span class="quote">> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.css:59
> +.sidebar > .panel.details.css-style > .content > .computed .computed-property-item .property .go-to-arrow {</span >

The styles containing `.computed-property-item` should be moved to `ComputedStyleSection.css` as that's where all of the other `.computed-property-item` styles are.

<span class="quote">> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.css:66
> +.sidebar > .panel.details.css-style > .content > .computed .computed-property-item:not(:hover) > .property .go-to-arrow,
>  .sidebar > .panel.details.css-style > .content > .computed .property:not(:hover) .go-to-arrow {</span >

This doesn't quite work the way I'd expect it.  It causes the go-to arrow to appear anywhere when hovering anywhere on the line in the Variables section, but not the Properties section.  I think you need something more like this
```
    .sidebar > .panel.details.css-style > .content > .computed > .details-section.computed-style-variables .property:not(:hover) .go-to-arrow
```

<span class="quote">> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.css:70
> +.computed-property-item > .property > .content > .contextual-documentation-button {</span >

This should also have `.sidebar > .panel.details.css-style > .content > .computed` to match all of the above.

<span class="quote">> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.css:76
> +.computed-property-item:not(:hover) > .property > .content > .contextual-documentation-button {</span >

ditto (:70)

<span class="quote">> Source/WebInspectorUI/UserInterface/Views/ContextualDocumentationPopover.css:26
> + .popover .documentation-popover-content {</span >

Style: unnecessary leading space

<span class="quote">> Source/WebInspectorUI/UserInterface/Views/ContextualDocumentationPopover.js:26
> +WI.ContextualDocumentationPopover = class ContextualDocumentationPopover</span >

I just realized that this doesn't actually inherit from `WI.Popover`.  That's what all the other `WI.*Popover` classes do.
```
    WI.ContextualDocumentationPopover = class ContextualDocumentationPopover extends WI.Popover
    {
        constructor(cssProperty, delegate)
        {
            console.assert(cssProperty instanceof WI.CSSProperty, cssProperty);

            super(delegate);

            this._cssProperty = cssProperty;
            this._targetElement = null;

            this.windowResizeHandler = this._presentOverTargetElement.bind(this);
    }

    // Public

    show(targetElement)
    {
        this._targetElement = targetElement;

        let documentationDetails = this._getDocumentationDetails(this._cssProperty);
        let documentationElement = this._createDocumentationElement(documentationDetails);
        this._popover.content = documentationElement;
        this._presentOverTargetElement();
    }

    // Private

    _presentOverTargetElement()
    {
        if (!this._targetElement)
            return;

        let targetFrame = WI.Rect.rectFromClientRect(this._targetElement.getBoundingClientRect());
        this.present(targetFrame.pad(2), [WI.RectEdge.MIN_X, WI.RectEdge.MIN_Y, WI.RectEdge.MAX_Y]);
    }

    ...
```
This structure will make it so that if the Web Inspector window is resized, the popover will remain in the right spot.  It's also a more familiar pattern to those who've worked with other `WI.Popover` subclasses elsewhere in Web Inspector.  (And it better matches the fact that this has the word "Popover" in its name)

You'll need to adjust the callsites as well
```
    _presentContextualDocumentation()
    {
        this._contextualDocumentationPopover ??= new WI.ContextualDocumentationPopover(this._property, this);
        this._contextualDocumentationPopover.show(this._nameElement);
        ...
```

<span class="quote">> Source/WebInspectorUI/UserInterface/Views/ContextualDocumentationPopover.js:40
> +        let documentationDetails = this._getDocumentationDetails(this._delegate.property);</span >

This is a really odd way of using a delegate.  Normally, in order to keep code more readable (e.g. "who is calling this function") we prefix any delegate function with the name of the thing calling it (e.g. `contextualDocumentationPopoverProperty`).  Delegates are really meant to be more of a "this object can be _anything_ so long as it has these specifically named functions" and not some specific kind of object.

Given my comment on :26 tho, you'll likely change this anyways.  This is more of a FYI :)

<span class="quote">> Source/WebInspectorUI/UserInterface/Views/ContextualDocumentationPopover.js:68
> +        return details;</span >

NIT: you could inline this whole object
```
    let propertyDocumentation = ContextualDocumentationDatabase[propertyName];
    console.assert(propertyDocumentation, propertyName, ContextualDocumentationDatabase);
    return {
        name: propertyName,
        description: propertyDocumentation.description,
        syntax: propertyDocumentation.syntax,
        url: propertyDocumentation.url,
    };
```

<span class="quote">> Source/WebInspectorUI/UserInterface/Views/Main.css:350
> +    vertical-align: bottom;</span >

This doesn't look good in the Styles panel.

<span class="quote">> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.css:91
> +.spreadsheet-style-declaration-editor > .property:not(:hover) > .content .contextual-documentation-button,</span >

NIT: this could/should have a `>` between `.content` and `.contextual-documentation-button`

<span class="quote">> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:108
> +    willDismissPopover()</span >

Style: this should be moved to a new `// Popover delegate` "section" (just like how there's one for `// SpreadsheetTextField delegate`) right above `// Private`

<span class="quote">> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:548
> +
> +</span >

Style: unnecessary newlines</pre>
        </div>
      </p>


      <hr>
      <span>You are receiving this mail because:</span>

      <ul>
          <li>You are the assignee for the bug.</li>
      </ul>
    </body>
</html>