Since CSS variables are displayed as part of the computed style, it can get annoying sometimes (especially when inspecting WebInspector) to see computed properties for the selected element. I think the checkbox should be changed to allow more flexibility with what is displayed.
<rdar://problem/28083228>
Created attachment 287440 [details] Patch
Created attachment 287441 [details] [Image] After Patch is applied
Comment on attachment 287440 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=287440&action=review > Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:130 > + get alwaysShowPropertyNames() { return Object.keys(this._alwaysShowPropertyNames); } Nit: I think the soft rule is that we only make trivial getters one line. Since this calls a method it probably shouldn't be changed. > Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:144 > + if (!Object.values(WebInspector.CSSStyleDeclarationTextEditor.VariableVisibility).includes(variableVisibility)) Remove. Sanity checking for enum values isn't normally done. > Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:1687 > + Hidden: "css-style-declaration-text-editor-variable-visibility-hidden", We've started using Symbol for enums, with debug descriptions that are usually the "leaf" part of the old-style string: Hidden: Symbol("variable-visibility-hidden").
Comment on attachment 287440 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=287440&action=review I'd place the Variables section under the Properties section. I think the Properties section is way more commonly used. > Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:1690 > +}; Why do we need 3 states? I'm particularly confused by Only.
Created attachment 287449 [details] [Image] patch applied, double border 2x border above Variables section. Also we should either remove the Variables section when empty, or style it like the other empty sections (see Layers > Layer Info for a node with no layers).
(In reply to comment #5) > Comment on attachment 287440 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=287440&action=review > > I'd place the Variables section under the Properties section. I think the > Properties section is way more commonly used. > > > Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:1690 > > +}; > > Why do we need 3 states? I'm particularly confused by Only. Now I see. WebInspector.CSSStyleDeclarationTextEditor.VariableVisibility.Only means that the Variable section is expanded and Properties section is collapsed. The WebInspector.CSSStyleDeclarationTextEditor.VariableVisibility is only used by _iterateOverProperties to filter properties.
Created attachment 287456 [details] [Image] Empty section (In reply to comment #6) > Also we should either remove the Variables section when empty, or style it > like the other empty sections (see Layers > Layer Info for a node with no > layers). Agreed with Matt. There is too much spacing under "No properties" and not enough below it.
(In reply to comment #6) > Created attachment 287449 [details] > [Image] patch applied, double border > > 2x border above Variables section. > > Also we should either remove the Variables section when empty, or style it > like the other empty sections (see Layers > Layer Info for a node with no > layers). I just tested this on my computer and didn't see the a double border. Any steps to reproduce? See below for comment on empty sections. (In reply to comment #7) > (In reply to comment #5) > > Comment on attachment 287440 [details] > > > Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:1690 > > > +}; > > > > Why do we need 3 states? I'm particularly confused by Only. > > Now I see. > WebInspector.CSSStyleDeclarationTextEditor.VariableVisibility.Only means > that the Variable section is expanded and Properties section is collapsed. > > The WebInspector.CSSStyleDeclarationTextEditor.VariableVisibility is only > used by _iterateOverProperties to filter properties. As you said, the three states are used by _iterateOverProperties. There need to be three states because there are three different ways of displaying CSS variables: 1) Shown: included with regular properties (like in Rules panel) 2) Hidden: don't display variables (like Properties section in Computed panel) 3) Only: don't display CSS properties that aren't CSS variables (like Variables section in Computed panel) (In reply to comment #8) > (In reply to comment #6) > > Also we should either remove the Variables section when empty, or style it > > like the other empty sections (see Layers > Layer Info for a node with no > > layers). I think it is better to just hide the section if there are no CSS variables. > Agreed with Matt. There is too much spacing under "No properties" and not > enough below it. The spacing issue likely an CSS problem with CSSStyleDeclarationTextEditor and the Computed panel.
(In reply to comment #9) > The spacing issue likely an CSS problem with CSSStyleDeclarationTextEditor > and the Computed panel. It is actually an issue with WebInspector.DetailsSection, specifically the set height (and padding) in DetailsSection.css: .details-section > .header { height: 23px; padding: 4px 5px 4px 0; } Not sure how to tackle this, other than adding extra padding below the content of the section, which I don't think is a great idea because that is a lot of wasted space.
Created attachment 287468 [details] Patch
(In reply to comment #10) > (In reply to comment #9) > > The spacing issue likely an CSS problem with CSSStyleDeclarationTextEditor > > and the Computed panel. > > It is actually an issue with WebInspector.DetailsSection, specifically the > set height (and padding) in DetailsSection.css: > > .details-section > .header { > height: 23px; > padding: 4px 5px 4px 0; > } > > Not sure how to tackle this, other than adding extra padding below the > content of the section, which I don't think is a great idea because that is > a lot of wasted space. DetailsSectionRow has an `emptyMessage` property for just this situation. There are a bunch of examples of how to use this in the UI. If we're going to always show the Variables section (even if it's empty) it should have a look consistent with the other details sections.
(In reply to comment #12) > (In reply to comment #10) > > (In reply to comment #9) > > > The spacing issue likely an CSS problem with CSSStyleDeclarationTextEditor > > > and the Computed panel. > > > > It is actually an issue with WebInspector.DetailsSection, specifically the > > set height (and padding) in DetailsSection.css: > > > > .details-section > .header { > > height: 23px; > > padding: 4px 5px 4px 0; > > } > > > > Not sure how to tackle this, other than adding extra padding below the > > content of the section, which I don't think is a great idea because that is > > a lot of wasted space. > > DetailsSectionRow has an `emptyMessage` property for just this situation. > There are a bunch of examples of how to use this in the UI. If we're going > to always show the Variables section (even if it's empty) it should have a > look consistent with the other details sections. I actually just made it so that the Variables section is hidden when no properties are shown in the editor (via the _shownProperties member variable on CSSStyleDeclarationTextEditor).
Comment on attachment 287468 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=287468&action=review Looking pretty good, but I want to apply it before setting r+. > Source/WebInspectorUI/ChangeLog:32 > + - Only (don't show other properties) Let's rename this as it's causing confusion. It doesn't just control variable visibility: WebInspector.CSSStyleDeclarationTextEditor.PropertyVisibilityMode = { ShowAll HideVariables HideNonVariables } > Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:1197 > + if (property.variable && this._variableVisibility === WebInspector.CSSStyleDeclarationTextEditor.VariableVisibility.Hidden) This entire function is pretty scary and has no test coverage at all. We should think about how to improve this situation. > Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:1211 > + if (!property.variable && this._variableVisibility === WebInspector.CSSStyleDeclarationTextEditor.VariableVisibility.Only) Please make this into a switch on the editor's propertyVisibilityMode (as renamed above). It will be easier to follow. > Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:1221 > + if (filterFunction) { We could drop this guard if you initialize filterFunction to an identity function. > Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:1691 > +WebInspector.CSSStyleDeclarationTextEditor.VariableVisibility = { See renaming suggestion above.
Created attachment 287842 [details] Patch
Comment on attachment 287842 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=287842&action=review r=me. I found an existing UI bug while testing the change. (https://bugs.webkit.org/show_bug.cgi?id=161652) > Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:1222 > + } There's a missing case. Make it break; so the switch is complete, and add a default case that throws an exception. > Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:1230 > + properties.sort((a, b) => a.name.localeCompare(b.name)); Nit: extra indent
Comment on attachment 287842 [details] Patch Clearing flags on attachment: 287842 Committed r205518: <http://trac.webkit.org/changeset/205518>
All reviewed patches have been landed. Closing bug.