Bug 147576

Created attachment 258091 [details] Patch

Created attachment 258202 [details] Patch

Created attachment 258330 [details] Patch

Comment on attachment 258330 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=258330&action=review Overall looks pretty good, with a few amusing spots. One major deficiency is that there are no aria roles set, so the new widgets are not accessible. I did not get through the whole patch, will look at the rest later. > Source/WebInspectorUI/ChangeLog:6 > + Added parent class for property editors in the Visual style Please add a better description of what this new widget does and how it works. > Source/WebInspectorUI/UserInterface/Views/VisualStylePropertyCombiner.js:38 > + editor.addEventListener(WebInspector.VisualStylePropertyEditor.Event.ValueDidChange, this._handlePropertyEditorValueChanged.bind(this)); no need for bind. > Source/WebInspectorUI/UserInterface/Views/VisualStylePropertyCombiner.js:44 > + this._valueRegExp = /([^\s]+\(.+\)|[^\s]+)(?:;?)/g; new RegExp? > Source/WebInspectorUI/UserInterface/Views/VisualStylePropertyCombiner.js:54 > + var value = ""; let all the things, or even const them. > Source/WebInspectorUI/UserInterface/Views/VisualStylePropertyEditor.js:35 > + function parseValues(values) I would call this "canonicalizeValues" or something, since it's not really parsing. > Source/WebInspectorUI/UserInterface/Views/VisualStylePropertyEditor.js:50 > + if (Array.isArray(possibleValues)) cool, we should use this more. > Source/WebInspectorUI/UserInterface/Views/VisualStylePropertyEditor.js:63 > + this._possibleUnits = possibleUnits trailing ; > Source/WebInspectorUI/UserInterface/Views/VisualStylePropertyEditor.js:89 > + this._specialPropertyPlaceholder = document.createElement("span"); House style tries to keep the Element suffix for things that store elements, e.g.: this._specialPropertyPlaceholderElement > Source/WebInspectorUI/UserInterface/Views/VisualStylePropertyEditor.js:91 > + this._specialPropertyPlaceholder.hidden = true; What does this do? > Source/WebInspectorUI/UserInterface/Views/VisualStylePropertyEditor.js:98 > + this._valueRegExp = null; Is there a way to make this a getter or a function, to avoid having to null it out? > Source/WebInspectorUI/UserInterface/Views/VisualStylePropertyEditor.js:131 > + // Implemented by subclass Nit: WK style requires trailing period for comments. > Source/WebInspectorUI/UserInterface/Views/VisualStylePropertyEditor.js:164 > + set dontUpdateStyleText(flag) Please rename this to avoid a contraction. Maybe suppressUpdateStyleText? > Source/WebInspectorUI/UserInterface/Views/VisualStylePropertyEditor.js:215 > + // this._element.tabIndex = flag ? -1 : null; ? > Source/WebInspectorUI/UserInterface/Views/VisualStylePropertyEditor.js:220 > + return this._element.classList.contains("disabled"); I am generally not a fan of keeping view state in the DOM, isn't there a member variable? this.disabled? > Source/WebInspectorUI/UserInterface/Views/VisualStylePropertyEditor.js:269 > + conflictingValues = true; rename: propertyValuesConflict? > Source/WebInspectorUI/UserInterface/Views/VisualStylePropertyEditor.js:280 > + if (this._element.classList.contains("multiple")) I would prefer not checking classList here. > Source/WebInspectorUI/UserInterface/Views/VisualStylePropertyEditor.js:318 > + var value = !propertyMissing ? placeholder : (this.valueIsKeyword(text) ? text : null); ~_~ please use if/else > Source/WebInspectorUI/UserInterface/Views/VisualStylePropertyEditor.js:331 > + return value && (this._valueRegExp && this._valueRegExp.test(value) || this.valueIsKeyword(value)); I would put the keyword check before the regexp.

Comment on attachment 258330 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=258330&action=review >> Source/WebInspectorUI/UserInterface/Views/VisualStylePropertyCombiner.js:44 >> + this._valueRegExp = /([^\s]+\(.+\)|[^\s]+)(?:;?)/g; > > new RegExp? I thought that it was better to not use new RegExp if there was no calculation (like adding strings). Is this incorrect? >> Source/WebInspectorUI/UserInterface/Views/VisualStylePropertyEditor.js:91 >> + this._specialPropertyPlaceholder.hidden = true; > > What does this do? It's used as an overlay for if there are multiple values that are conflicting or, if the current value of the property editor is "Unchanged", to display the current computed value of that property for that node. >> Source/WebInspectorUI/UserInterface/Views/VisualStylePropertyEditor.js:98 >> + this._valueRegExp = null; > > Is there a way to make this a getter or a function, to avoid having to null it out? Yeah I should do that. Every subclass of this resets this value to something else, so that makes sense. >> Source/WebInspectorUI/UserInterface/Views/VisualStylePropertyEditor.js:215 >> + // this._element.tabIndex = flag ? -1 : null; > > ? Ah right I had meant to ask Tim about that.

Created attachment 258461 [details] Patch

Created attachment 258626 [details] Patch

Comment on attachment 258626 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=258626&action=review > Source/WebInspectorUI/UserInterface/Views/VisualStylePropertyCombiner.js:36 > + this._propertyEditors = propertyEditors || []; > + for (let editor of this._propertyEditors) { Line return between. > Source/WebInspectorUI/UserInterface/Views/VisualStylePropertyEditor.css:42 > + -webkit-filter: opacity(0.7) No prefix. > Source/WebInspectorUI/UserInterface/Views/VisualStylePropertyEditor.css:83 > + background-color: hsl(0, 100%, 100%); white > Source/WebInspectorUI/UserInterface/Views/VisualStylePropertyEditor.js:275 > + this._specialPropertyPlaceholderElement.textContent = "(" + WebInspector.UIString("multiple") + ")"; Include the () in the UIString. > Source/WebInspectorUI/UserInterface/Views/VisualStyleTabbedPropertiesController.css:44 > + color: hsl(0, 100%, 100%); white > Source/WebInspectorUI/UserInterface/Views/VisualStyleTabbedPropertiesController.css:49 > + color: hsl(0, 100%, 100%); white > Source/WebInspectorUI/UserInterface/Views/VisualStyleTabbedPropertiesController.js:26 > +WebInspector.VisualStyleTabbedPropertiesController = class VisualStyleTabbedPropertiesController extends WebInspector.DetailsSectionRow It is odd that his inherits from DetailsSectionRow but ends up being a "Controller". Class names should end in the same suffix.

<rdar://problem/22281720>

Comment on attachment 258626 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=258626&action=review > Source/WebInspectorUI/UserInterface/Views/VisualStyleTabbedPropertiesController.js:28 > + constructor(tabMap) Tab?

> > Source/WebInspectorUI/UserInterface/Views/VisualStyleTabbedPropertiesController.js:28 > > + constructor(tabMap) > > Tab? So I consider each of the different groups for Border ("All", "Top", "Right", "Bottom", "Left") a tab that contains a bunch of editors, hence tabMap. Is this not a good name?

Created attachment 258993 [details] Patch

Comment on attachment 258993 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=258993&action=review > Source/WebInspectorUI/UserInterface/Views/VisualStylePropertyEditor.css:42 > + filter: opacity(0.7) Use opacity: 0.7; > Source/WebInspectorUI/UserInterface/Views/VisualStylePropertyEditor.css:52 > + opacity: 0.4; You did here.

Created attachment 259017 [details] Patch

Created attachment 259018 [details] Patch

Comment on attachment 259018 [details] Patch Clearing flags on attachment: 259018 Committed r188484: <http://trac.webkit.org/changeset/188484>

All reviewed patches have been landed. Closing bug.