It seems very unnecessary to use CodeMirror for the Computed panel, especially since none of it is editable. This would massively simplify (and lighten the load) of the Computed panel, and remove some bugs in the process.
Created attachment 335796 [details] Patch This is a reviewable patch, but it is missing tabbing support. Not sure whether it'll be better to address that in this bug or a followup, as I think it'll be pretty hefty.
Created attachment 348275 [details] Patch Rebase
In general I think this looks and feels good. A few things that are different (which may just because that is how it is implemented in the SpreadSheet editor). 1. Less syntax highlighting (numbers, keywords, etc) 2. Filtering now matches the entire property + value instead of just the search term 3. Sometimes when switching between nodes the pseudo class checkboxes are visible where they weren't before 4. Text selection of properties where `display`, `width`, or `height` had an implicit value displays poorly 5. PRO: Color values have color swatches I also saw multiple existing bugs, which exhibited before and after the patch. I certainly think this is a step in the right direction to be more consistent.
Comment on attachment 348275 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=348275&action=review I think this is very close to landing. I did notice one difference which seems like a bug. Filtering did not behave as expected in the Variables section: Steps to Reproduce: 1. Inspect the inspector 2. Select an element in inspector² 3. Show Computed styles 4. Filter for "size" => Should show `box-size` in properties, and some variables. Actually shows lots of variables... Perhaps we should be setting: this._variablesTextEditor.hideFilterNonMatchingProperties = true; > Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:141 > + this._variablesTextEditor = new WI.SpreadsheetCSSStyleDeclarationEditor(this); Should we enable `hideFilterNonMatchingProperties`? > Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:55 > - if (!this.style.editable) > + if (!this._style || !this._style.editable) When might this._style be null? > Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:202 > + if (this._style) { Style: Early return to avoid nesting the entire function. > Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:208 > + if (this._style.type === WI.CSSStyleDeclaration.Type.Computed) { It is weird that we only apply the `propertyVisibilityMode` and `showsImplicitProperties` filters when the type is Computed. Nothing about those editor properties seems specific to Computed. I know they aren't used outside of Computed right now though. > Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:315 > + if (this._hideFilterNonMatchingProperties) > + this.element.append(propertyView.element); Maybe this should avoid calling `append` if `this.element.parentNode` is non-null? Otherwise it might try to remove and re-append at the end?
Comment on attachment 348275 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=348275&action=review >> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:141 >> + this._variablesTextEditor = new WI.SpreadsheetCSSStyleDeclarationEditor(this); > > Should we enable `hideFilterNonMatchingProperties`? Yup! Oops :P >> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:55 >> + if (!this._style || !this._style.editable) > > When might this._style be null? When created by the computed tab, they are initialized inside `initialLayout`, meaning that the `refresh` call won't happen until later, which is where the `style` is set. >> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:208 >> + if (this._style.type === WI.CSSStyleDeclaration.Type.Computed) { > > It is weird that we only apply the `propertyVisibilityMode` and `showsImplicitProperties` filters when the type is Computed. Nothing about those editor properties seems specific to Computed. I know they aren't used outside of Computed right now though. Agreed. Will change. >> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:315 >> + this.element.append(propertyView.element); > > Maybe this should avoid calling `append` if `this.element.parentNode` is non-null? Otherwise it might try to remove and re-append at the end? We don't want to do that actually, because otherwise previously removed `propertyView` won't get inserted at the right spot. By re-adding to the bottom, we effectively keep the same order as before because they ALL get re-added to the end.
Created attachment 349916 [details] Patch
Comment on attachment 349916 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=349916&action=review r=me > Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:234 > + > + Style: Extra empty line.
Created attachment 350282 [details] Patch
Comment on attachment 350282 [details] Patch Clearing flags on attachment: 350282 Committed r236297: <https://trac.webkit.org/changeset/236297>
All reviewed patches have been landed. Closing bug.
<rdar://problem/44664917>