[webkit-reviews] review denied: [Bug 191984] Web Inspector: Computed panel: allow to expand properties to show list of overridden values : [Attachment 355686] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Nov 26 17:11:57 PST 2018
Devin Rousso <drousso at apple.com> has denied Nikita Vasilyev
<nvasilyev at apple.com>'s request for review:
Bug 191984: Web Inspector: Computed panel: allow to expand properties to show
list of overridden values
https://bugs.webkit.org/show_bug.cgi?id=191984
Attachment 355686: Patch
https://bugs.webkit.org/attachment.cgi?id=355686&action=review
--- Comment #3 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 355686
--> https://bugs.webkit.org/attachment.cgi?id=355686
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=355686&action=review
r-, as this breaks tabbing
When filtering, I also noticed that the "Show All" checkbox remains visible
even if no matches were found. Filtering for values found in the override
chain but not in any computed value is also somewhat "broken", in that the
properties are hidden (e.g. if `display: flex;` gets overridden by `display:
block;` and I filter for "flex", the property is not visible even if I've
expanded `display` and am able to see `display: flex;`). Not sure how we want
that to work 🤔 Frankly, all of the changes like this should be in a separate
patch.
I get the following error when tabbing from the pseudo-class checkboxes:
> TypeError:​ this._computedStyleSection.focus is not a function. (In
'this._computedStyleSection.focus()​', 'this._computedStyleSection.focus' is
undefined)​ (at ComputedStyleDetailsPanel.js:​87:​41)​
> focusFirstSection @ ComputedStyleDetailsPanel.js:​87:​41
> _handleForcedPseudoClassCheckboxKeydown @
GeneralStyleDetailsSidebarPanel.js:​263:​46
> _handleForcedPseudoClassCheckboxKeydown @ [native code]​
The majority of my comments are related to design. I think this looks really
nice, but the way in which it's built could use some reworking.
Also, is there a reason you're not using this for the Variables section as
well?
> Source/WebInspectorUI/ChangeLog:8
> + Introduce the new experimental Computed panel with traces.
I understand what you're getting at here, but in my mind "trace" has a very
specific meaning (backtrace in JS), so maybe use a different word ("override
chain")?
> Source/WebInspectorUI/ChangeLog:17
> + Remove "Properties" section header and move "Show All" checkbox to
the left so
> + it isn't covered by the scrollbar.
Why was this done? It is weird to have everything else in the sidebar be part
of a collapsable section, but not have the computed style be in a collapsable
section?
> Source/WebInspectorUI/UserInterface/Base/Setting.js:133
> + experimentalEnableComputedStylesWithTraces: new
WI.Setting("experimental-enable-computed-styles-with-traces", true),
All experimental settings should start as off (`false`)
> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.css:56
> + padding-left: var(--disclosure-button-size);
-webkit-padding-start, unless this shouldn't be RTL-compatible?
> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:50
> + let propertyTraces =
this._computePropertyTraces(this.nodeStyles.uniqueOrderedStyles);
> + this._computedStyleSection.styleTraces = propertyTraces;
NIT: you could just directly assign the value instead of saving it to a local
variable
> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:108
> +
computedStyleShowAllLabel.append(this._computedStyleShowAllCheckbox,
WI.UIString("Show All"));
See ChangeLog:16
It seems weird to have a standalone label-checkbox be at the top of a section
with no title. I'd expect this to be the last item.
> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:124
> + this._computedStyleSection.propertyVisibilityMode =
WI.SpreadsheetCSSStyleDeclarationEditor.PropertyVisibilityMode.HideVariables;
If/When we remove this code, we can also remove all support for
`WI.SpreadsheetCSSStyleDeclarationEditor.PropertyVisibilityMode`, as this was
the only user =D
> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:138
> + this.element.appendChild(this._propertiesSection.element);
This shouldn't be necessary because of the `addSubview` on line 136
> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:157
> - this.element.appendChild(this._propertiesSection.element);
> + if (!WI.settings.experimentalEnableComputedStylesWithTraces.value)
> + this.element.appendChild(this._propertiesSection.element);
Ditto (138)
Not sure why this was added/needed to begin with 🤔
> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:186
> + if (!result.has(property.name))
> + result.set(property.name, []);
> +
> + result.get(property.name).push(property);
Rather than do a `has` and a `get`, you can use the result of `get` to infer
`has`:
let properties = result.get(property.name);
if (!Array.isArray(properties)) {
properties = [];
result.set(property.name, properties);
}
properties.push(property);
> Source/WebInspectorUI/UserInterface/Views/ComputedStyleSection.css:27
> + font: 12px -webkit-system-font, sans-serif;
NIT: I have a personal ordering that I use for CSS:
display/visibility/etc
position/float/etc
sizing
margin
padding
content
background
border
outline
miscellaneous
> Source/WebInspectorUI/UserInterface/Views/ComputedStyleSection.css:28
> + padding-bottom: 3px;
This causes there to be a small area after the last property if it is expanded
that has the non-expanded background color
> Source/WebInspectorUI/UserInterface/Views/ComputedStyleSection.css:37
> + padding-left: calc(var(--disclosure-button-size));
> + padding-right: var(--css-declaration-horizontal-padding);
Ditto ComputedStyleDetailsPanel.css:56
> Source/WebInspectorUI/UserInterface/Views/ComputedStyleSection.css:38
> +
Style: don't add extra newlines
> Source/WebInspectorUI/UserInterface/Views/ComputedStyleSection.css:43
> +
Ditto (38)
> Source/WebInspectorUI/UserInterface/Views/ComputedStyleSection.css:49
> + background-color: var(--background-color);
Why are we changing the background color when we expand the property?
Shouldn't we be always be using this background color, just like the Variables
section and all of the sections in the Rules sidebar panel?
> Source/WebInspectorUI/UserInterface/Views/ComputedStyleSection.css:61
> + margin-left: calc(-1 * var(--disclosure-button-size));
Ditto ComputedStyleDetailsPanel.css:56
> Source/WebInspectorUI/UserInterface/Views/ComputedStyleSection.css:99
> + text-align: right;
Ditto ComputedStyleDetailsPanel.css:56 (`text-align: end;`)
> Source/WebInspectorUI/UserInterface/Views/ComputedStyleSection.css:105
> + padding-right: 0.6em;
Ditto ComputedStyleDetailsPanel.css:56
> Source/WebInspectorUI/UserInterface/Views/ComputedStyleSection.js:42
> + this._propertyVisibilityMode =
WI.ComputedStyleSection.PropertyVisibilityMode.ShowAll;
> + this._hideFilterNonMatchingProperties = false;
Considering this class is only used once, you could remove these values and
associated logic for simplicity.
> Source/WebInspectorUI/UserInterface/Views/ComputedStyleSection.js:48
> + layout()
Please add a `super.layout()` call at the top of this function
> Source/WebInspectorUI/UserInterface/Views/ComputedStyleSection.js:73
> + let expandableItem = new WI.ExpandableItem(property.name,
propertyView.element, traceElement);
`property.name` is not really a specific enough key for a `WI.Setting`, unless
you want it to be that every `display` gets auto-expanded once you expand any
`display` once? I'd imagine it to be based on the path of the DOM node for
that property (similar to the collapsed state of the "Properties" section for
the selected node in the "Node" sidebar panel).
> Source/WebInspectorUI/UserInterface/Views/ComputedStyleSection.js:79
> + detached()
Please add a `super.detached()` call at the top of this function
> Source/WebInspectorUI/UserInterface/Views/ComputedStyleSection.js:91
> + get style()
Style: we prefer putting getters/setters above "regular" functions
> Source/WebInspectorUI/UserInterface/Views/ComputedStyleSection.js:102
> +
this._style.removeEventListener(WI.CSSStyleDeclaration.Event.PropertiesChanged,
this._propertiesChanged, this);
NIT: I like to prefix all event handler callbacks with `_handle*`
> Source/WebInspectorUI/UserInterface/Views/ComputedStyleSection.js:119
> + if (this._styleTraces === styleTraces)
Considering that `styleTraces` will be an array, we should also use
`Array.shallowEqual` or just drop it completely
> Source/WebInspectorUI/UserInterface/Views/ComputedStyleSection.js:138
> + this._alwaysShowPropertyNames = new Set(propertyNames);
Instead of constructing a `Set` inside this function, I think it's better to
expect that a `Set` be passed in as an argument
> Source/WebInspectorUI/UserInterface/Views/ComputedStyleSection.js:187
> + highlightProperty(property)
This doesn't appear to be used outside of the Rules sidebar panel
> Source/WebInspectorUI/UserInterface/Views/ComputedStyleSection.js:254
> + return traceElement;
It seems weird to create an element that has nothing in it and isn't actually
displayed
> Source/WebInspectorUI/UserInterface/Views/ComputedStyleSection.js:279
> + let selectorElement = document.createElement("span");
> + selectorElement.textContent = selectorText.truncateMiddle(24);
> + selectorElement.className = "selector";
This has VERY weird wrapping at any semi-small width. I get what you're trying
to go for here (have every value be aligned to the same horizontal start
"point"), but I don't think we have enough room to make this feasible, unless
we do far more aggressive truncation (e.g. via CSS).
> Source/WebInspectorUI/UserInterface/Views/ExpandableItem.js:26
> +WI.ExpandableItem = class ExpandableItem
NIT: I'd rather you call this something like `WI.ExpandableElement`, as "item"
implies a list to me
> Source/WebInspectorUI/UserInterface/Views/ExpandableItem.js:41
> + let settingKey = "expanded-" + key;
NIT: you can inline this
> Source/WebInspectorUI/UserInterface/Views/ExpandableItem.js:61
> + const shouldExpand = !this._element.classList.contains("expanded");
You should use the value of the `WI.Setting`, not the DOM for state
let shouldExpand = !this._expandedSetting.value;
this._update(shouldExpand);
> Source/WebInspectorUI/UserInterface/Views/SettingsTabContentView.js:279
> +
listenForChange(WI.settings.experimentalEnableComputedStylesWithTraces);
Does this actually require reloading WebInspector? Wouldn't it use the new UI
the next time the user shows the Elements tab?
> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:28
> + constructor(delegate, property, options = {})
Nice!
> Source/WebInspectorUI/UserInterface/Views/StyleOrigin.js:26
> +WI.StyleOrigin = class StyleOrigin
This name makes me think it's a model object. It should have `View` or
something to that effect as a suffix.
> Source/WebInspectorUI/UserInterface/Views/StyleOrigin.js:28
> + constructor(style)
Rather than having this class effectively be a factory for producing origin DOM
structure, I think you'd be better having the DOM creation be inside a `layout`
so that `WI.SpreadsheetCSSStyleDeclarationSection` doesn't have to do a
`replaceElement` and can instead just call `this._originView..needsLayout()`
(the same is true for Computed)
More information about the webkit-reviews
mailing list