Bug 148269 – Web Inspector: Visual styles sidebar should do something sane for SVG elements

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Timothy Hatcher]

We likely need to have SVG specific sections we swap in and hide the more HTML specific sections.

[Radar WebKit Bug Importer]

<rdar://problem/22371972>

[Devin Rousso]

(In reply to comment #0)
> We likely need to have SVG specific sections we swap in and hide the more
> HTML specific sections.

I like this idea.  What specific sections do you have in mind?

[Timothy Hatcher]

Anything that does not have an effect in SVG. :) I don't know off the top of my head.

[Devin Rousso]

Created attachment 286129 [details]
WIP-Patch

[WebKit Commit Bot]

Attachment 286129 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Joseph Pecoraro]

Comment on attachment 286129 [details]
WIP-Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=286129&action=review

Seems good, but I'm weary of the protocol change needed. r- while we work that part out. I suggested a frontend change, does that miss something?

> Source/JavaScriptCore/inspector/protocol/DOM.json:57
> +                { "name": "ownerSVGElement", "$ref": "Node", "optional": true, "description": "SVG element that contains this node." },

Hmm, I think we should avoid adding this to the protocol unless we (1) really need to (2) would get a good performance benefit from it.

I think we should be able to do this in the frontend. For example:

    ownerSVGElement()
    {
        if (this._nodeName === "svg")
            return this;
        if (!this.parentNode)
            return null;
        return this.parentNode.ownerSVGElement;
    }

    isSVGElement()
    {
        return !!this.ownerSVGElement;
    }

Is there something this might miss?

> Source/WebInspectorUI/UserInterface/Views/VisualStyleDetailsPanel.js:226
> +        this._groups.position.properties.cx.element.classList.toggle("inactive", !isSVGCircle && !isSVGEllipse && !isSVGRadialGradient);
> +        this._groups.position.properties.cy.element.classList.toggle("inactive", !isSVGCircle && !isSVGEllipse && !isSVGRadialGradient);

My eyes glazed over reading the states here. I trust it, but a few screenshots would help =)

[Joseph Pecoraro]

>     ownerSVGElement()
>     {
>         if (this._nodeName === "svg")
>             return this;
>         if (!this.parentNode)
>             return null;
>         return this.parentNode.ownerSVGElement;

Err, `return this.parentNode.ownerSVGElement()`


>     }
> 
>     isSVGElement()
>     {
>         return !!this.ownerSVGElement;

Same here =)

[Devin Rousso]

Created attachment 286167 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 286167 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=286167&action=review

r=me

> Source/WebInspectorUI/UserInterface/Views/VisualStyleDetailsPanel.js:212
> +        // Only show the dimensions section if the current element is not <svg> or is <ellipse>, <rect>, <circle>, or <radialGradient>.
> +        this._groups.dimensions.section.element.classList.toggle("inactive", isSVGElement && !isSVGEllipse && !isSVGRect && !isSVGCircle && !isSVGRadialGradient);

Does "is not <svg>" really mean the SVGSVGElement? Because "isSVGElement" is just checking if the element is any kind of SVG element. Perhaps the comment should say "is not an SVG element".

The comments are great but they are the opposite of the conditions. Maybe the conditions should be written to match the comment?

    !( !isSVGElement || isSVGEllipse || isSVGRect || isSVGCircle || isSVGRadialGradient )

[Devin Rousso]

Created attachment 286235 [details]
Patch

[Devin Rousso]

Created attachment 286459 [details]
Patch

[Nikita Vasilyev]

I applied this patch, opened WebInspectorUI/UserInterface/Images/NavigationItemTypes.svg, opened "Styles — Visual" and immediately got this uncaught exception:

undefined is not an object (evaluating 'this._groups.content.section')​ (at VisualStyleDetailsPanel.js:​197:​29)​
    _updateSections @ VisualStyleDetailsPanel.js:​197:​29
    dispatch @ Object.js:​162:​30
    dispatchEventToListeners @ Object.js:​169:​17
    _selectorChanged @ VisualStyleSelectorSection.js:​257:​38
    dispatch @ Object.js:​162:​30
    dispatchEventToListeners @ Object.js:​169:​17
    select @ TreeElement.js:​507:​49
    createSelectorItem @ VisualStyleSelectorSection.js:​113:​32
    update @ VisualStyleSelectorSection.js:​163:​36
    refresh @ VisualStyleDetailsPanel.js:​64:​41
    _refreshPreservingScrollPosition @ StyleDetailsPanel.js:​143:​21
    nodeStylesRefreshed @ StyleDetailsPanel.js:​114:​50
    dispatch @ Object.js:​162:​30
    dispatchEventToListeners @ Object.js:​169:​17
    fetchedComputedStyle @ DOMNodeStyles.js:​228:​42
    fetchedComputedStyle @ [native code]​
    _dispatchResponseToCallback @ InspectorBackend.js:​311:​27
    _dispatchResponse @ InspectorBackend.js:​281:​45
    dispatch @ InspectorBackend.js:​157:​35
    dispatchNextQueuedMessageFromBackend @ MessageDispatcher.js:​42:​34

Additional Details:​
    cause --> An uncaught exception was thrown while dispatching response callback for command CSS.getComputedStyleForNode.

[Devin Rousso]

(In reply to comment #12)
> I applied this patch, opened
> WebInspectorUI/UserInterface/Images/NavigationItemTypes.svg, opened "Styles
> — Visual" and immediately got this uncaught exception:
> 
> undefined is not an object (evaluating 'this._groups.content.section')​ (at
> VisualStyleDetailsPanel.js:​197:​29)​
>     _updateSections @ VisualStyleDetailsPanel.js:​197:​29
>     dispatch @ Object.js:​162:​30
>     dispatchEventToListeners @ Object.js:​169:​17
>     _selectorChanged @ VisualStyleSelectorSection.js:​257:​38
>     dispatch @ Object.js:​162:​30
>     dispatchEventToListeners @ Object.js:​169:​17
>     select @ TreeElement.js:​507:​49
>     createSelectorItem @ VisualStyleSelectorSection.js:​113:​32
>     update @ VisualStyleSelectorSection.js:​163:​36
>     refresh @ VisualStyleDetailsPanel.js:​64:​41
>     _refreshPreservingScrollPosition @ StyleDetailsPanel.js:​143:​21
>     nodeStylesRefreshed @ StyleDetailsPanel.js:​114:​50
>     dispatch @ Object.js:​162:​30
>     dispatchEventToListeners @ Object.js:​169:​17
>     fetchedComputedStyle @ DOMNodeStyles.js:​228:​42
>     fetchedComputedStyle @ [native code]​
>     _dispatchResponseToCallback @ InspectorBackend.js:​311:​27
>     _dispatchResponse @ InspectorBackend.js:​281:​45
>     dispatch @ InspectorBackend.js:​157:​35
>     dispatchNextQueuedMessageFromBackend @ MessageDispatcher.js:​42:​34
> 
> Additional Details:​
>     cause --> An uncaught exception was thrown while dispatching response
> callback for command CSS.getComputedStyleForNode.

This is because it depends on <https://webkit.org/b/158272>, which redefines the Content section in the Visual Styles sidebar.

[Nikita Vasilyev]

I still see all existing HTML sections for SVG? Is this intended?

[Devin Rousso]

(In reply to comment #14)
> I still see all existing HTML sections for SVG? Is this intended?

The hiding of sections/editors is also derived from <https://webkit.org/b/158272> via the CSS rule:

.sidebar > .panel.details.css-style .visual > .details-section :matches(.details-section, .group).inactive {
    display: none;
}

To answer your question, some of the HTML sections should disappear (float, content, border, list style, etc.) when selecting an SVG element.

[Joseph Pecoraro]

Comment on attachment 286459 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=286459&action=review

r=me

> Source/WebInspectorUI/UserInterface/Views/VisualStyleDetailsPanel.js:199
> +        this._groups.fill.section.element.classList.toggle("inactive", !isSVGElement);
> +        this._groups.stroke.section.element.classList.toggle("inactive", !isSVGElement);

Can we separate these two lines from the others, so it is obvious that they are different?

> Source/WebInspectorUI/UserInterface/Views/VisualStyleUnitSlider.css:29
> +    background-image: linear-gradient(90deg, hsla(0, 0%, 0%, 0), #000), url(../Images/Checkers.svg);

Nit: We normally just say `black` instead of #000?

[Devin Rousso]

Created attachment 286677 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 286677 [details]
Patch

Clearing flags on attachment: 286677

Committed r204758: <http://trac.webkit.org/changeset/204758>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.