Bug 148269

<rdar://problem/22371972>

(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?

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

Created attachment 286129 [details] WIP-Patch

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.

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 =)

> 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 =)

Created attachment 286167 [details] Patch

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 )

Created attachment 286235 [details] Patch

Created attachment 286459 [details] Patch

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.

(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.

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

(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.

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?

Created attachment 286677 [details] Patch

Comment on attachment 286677 [details] Patch Clearing flags on attachment: 286677 Committed r204758: <http://trac.webkit.org/changeset/204758>

All reviewed patches have been landed. Closing bug.