Bug 176643

We used to have toggleProperty and setPropertyText protocol methods. We may want to bring them back, but likely not in the first iteration of the new styles sidebar. https://trac.webkit.org/changeset/179286/webkit

Created attachment 320849 [details] Patch

Created attachment 320851 [details] [Animated GIF] With patch applied Page for manual testing: http://nv.github.io/webkit-inspector-bugs/styles-redesign/tests/comments.html

Comment on attachment 320849 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=320849&action=review > Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:284 > + // `_styleSheetTextRange` is a position of a property within the stylesheet. Use the definite article, since we're taking about this specific property: "...is the position of the property with the stylesheet." > Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:285 > + // `range` is a position of a property within its style rule. Same: "...is the position of the property within the rule." > Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:232 > + this._allVisibleProperties = this._allProperties.filter(function(property) { This can be written as a "concise" arrow function: this._allVisibleProperties = this._allProperties.filter(property => !!property.styleDeclarationTextRange); > Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:98 > + this._checkboxElement = checkboxElement; Drop the temporary `checkboxElement`: this._checkboxElement = this.element.appendChild(document.createElement("input")); > Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:99 > + checkboxElement.classList.add("property-toggle"); I don't think a class name is necessary for the checkbox, it can be selected with `.property > input[type=checkbox]`. > Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:103 > + this.element.classList.toggle("property-disabled", !this._property.enabled); `.property-disabled` is redundant, since the element already has a `.property` class name. Let's make this just `.disabled`, and update the selectors in SpreadsheetCSSStyleDeclarationEditor.css to `.property.disabled`. > Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:132 > + this.needsLayout(WI.View.LayoutReason.Dirty); WI.View.LayoutReason.Dirty is the default, and can be omitted.

r- due to the following issue: 1. Open http://nv.github.io/webkit-inspector-bugs/styles-redesign/tests/comments.html 2. In Elements tab, select the first paragraph: body > p 3. In styles sidebar, uncheck the first property of rule p {} => /* font-size: 12px; */ 4. Click link at the top of the rule (comments.css:1) 5. Click the Elements tab to return => The unchecked property is missing from the rule

(In reply to Matt Baker from comment #5) > r- due to the following issue: > > 1. Open > http://nv.github.io/webkit-inspector-bugs/styles-redesign/tests/comments.html > 2. In Elements tab, select the first paragraph: body > p > 3. In styles sidebar, uncheck the first property of rule p {} > => /* font-size: 12px; */ > 4. Click link at the top of the rule (comments.css:1) > 5. Click the Elements tab to return > => The unchecked property is missing from the rule Discussed offline. This patch requires WebKit to be recompiled, since there was a backend change in bug 166787. Once recompiled, the unchecked property should be present in the rule (as shown in the attached animated GIF).

Comment on attachment 320849 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=320849&action=review >> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:284 >> + // `_styleSheetTextRange` is a position of a property within the stylesheet. > > Use the definite article, since we're taking about this specific property: > > "...is the position of the property with the stylesheet." "with the stylesheet"? Should it be "within"? >> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:99 >> + checkboxElement.classList.add("property-toggle"); > > I don't think a class name is necessary for the checkbox, it can be selected with `.property > input[type=checkbox]`. I like having a class here. When refactoring, it's easier to find all relevant code. >> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:103 >> + this.element.classList.toggle("property-disabled", !this._property.enabled); > > `.property-disabled` is redundant, since the element already has a `.property` class name. Let's make this just `.disabled`, and update the selectors in SpreadsheetCSSStyleDeclarationEditor.css to `.property.disabled`. I was afraid of collisions with other styles. We don't have any global `.disable`, so it should be fine to use `.spreadsheet-style-declaration-editor .property.disabled`. >> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:132 >> + this.needsLayout(WI.View.LayoutReason.Dirty); > > WI.View.LayoutReason.Dirty is the default, and can be omitted. Nice! I used this.needsLayout(WI.View.LayoutReason.Dirty) in my previous patches, I'll omit it there as well.

Created attachment 320985 [details] Patch

Comment on attachment 320985 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=320985&action=review > Source/WebInspectorUI/UserInterface/Base/Utilities.js:793 > +}); These utility functions could use tests: LayoutTests/inspector/unit-tests/string-utilities.html > Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:153 > + this._updateOwnerStyleText(oldText, newText); It seems safe to reorder these lines, since _updateOwnerStyleText doesn't use this._text. Then you can eliminate the temporary: this._updateOwnerStyleSheet(this._text, newText); this._text = newText; > Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:282 > + let styleText = this.ownerStyle.text || ""; The rest of the class accesses `this._ownerStyle` directly. We should be consistent and do that in this function too, instead of mixing it up and accessing the public property and private variable. > Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:285 > + // `range` is the position of the property within the rule. This is a helpful comment, but I don't think the backticks are needed. > Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:288 > + // startOffset and endOffset are initially NaN. You can drop this comment and the newline above too. > Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:234 > + return this._allVisibleProperties; The body can be simplified: if (!this._allVisibleProperties) this._allVisibleProperties = this._allProperties.filter((property) => !!property.styleDeclarationTextRange); return this._allVisibleProperties; > Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:242 > + this._visibleProperties = this._properties.filter((property) => !!property.styleDeclarationTextRange); Ditto. > Source/WebInspectorUI/UserInterface/Models/TextRange.js:114 > + console.assert(!isNaN(this._startLine), "TextRange needs line/column data"); Nit: end sentence with a period. > Source/WebInspectorUI/UserInterface/Models/TextRange.js:120 > + console.assert(!isNaN(this._startLine), "TextRange needs line/column data"); Ditto. > Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:80 > +WI.SpreadsheetStyleProperty = class SpreadsheetStyleProperty extends WI.View After taking another look at this patch, I don't think it's necessary to inherit from WI.View. This class isn't doing any complex rendering or DOM layouts. Is there a need to lazily create the DOM elements used by WI.SpreadsheetStyleProperty objects? If not, this can just be a WI.Object with an _update() function. > Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:100 > + this._checkboxElement.addEventListener("change", this._onChange.bind(this)); I think prefixing event handlers with _on was the old style. _handleChange or _handleCheckboxChange would be better, but since the listener is pretty simple you could make it arrow function and avoid naming it altogether. > Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:106 > + this.element.append(": "); This could be pure CSS, like you did with comment delimiters: .property .name::after { content: ": "; } > Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:112 > + this.element.append(";"); Here too. > Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:125 > + _onChange() We've mo

Comment on attachment 320985 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=320985&action=review >> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:80 >> +WI.SpreadsheetStyleProperty = class SpreadsheetStyleProperty extends WI.View > > After taking another look at this patch, I don't think it's necessary to inherit from WI.View. This class isn't doing any complex rendering or DOM layouts. Is there a need to lazily create the DOM elements used by WI.SpreadsheetStyleProperty objects? If not, this can just be a WI.Object with an _update() function. Should we only use WI.View when we need lazily create the DOM elements? >> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:106 >> + this.element.append(": "); > > This could be pure CSS, like you did with comment delimiters: > > .property .name::after { > content: ": "; > } I just realized "/* " and " */" don't get copied to the clipboard and I need to convert them to plain text.

Comment on attachment 320985 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=320985&action=review >> Source/WebInspectorUI/UserInterface/Models/TextRange.js:114 >> + console.assert(!isNaN(this._startLine), "TextRange needs line/column data"); > > Nit: end sentence with a period. I searched — we don't usually end our assert messages with a period. I can go either way. >> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:125 >> + _onChange() > > We've mo ?

Comment on attachment 320985 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=320985&action=review >>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:125 >>> + _onChange() >> >> We've mo > > ? Ignore that, typo.

Created attachment 321027 [details] Patch

Comment on attachment 321027 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=321027&action=review Mostly nits, but r- because of the issue with CSSProperty.prototype.commentOut. > LayoutTests/inspector/unit-tests/string-utilities.html:60 > + InspectorTest.expectEqual("1\n2\n3".lineCount, 3, "a string with two line breaks should have three lines"); Descriptions should all begin with a capital and end in a period. > LayoutTests/inspector/unit-tests/string-utilities.html:63 > + return true; The following cases would be good to have: - Two consequtive line breaks - Trailing line break > LayoutTests/inspector/unit-tests/string-utilities.html:72 > + InspectorTest.expectEqual("".lastLine, "", "last line of an empty string is empty string"); Same as above. > Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:123 > + this._enabled = !disabled; Early return if `this._enabled` isn't changing. It should be safe to call this repeatedly with the same `disabled` value, without thrashing `this._text`. > Source/WebInspectorUI/UserInterface/Models/TextRange.js:114 > + console.assert(!isNaN(this._startLine), "TextRange needs line/column data"); Assert description should end in a period. > Source/WebInspectorUI/UserInterface/Models/TextRange.js:120 > + console.assert(!isNaN(this._startLine), "TextRange needs line/column data"); Same. > Source/WebInspectorUI/UserInterface/Views/SyntaxHighlightingDefaultTheme.css:38 > + color: var(--syntax-highlight-comment-color); Nice.

Comment on attachment 320985 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=320985&action=review >>> Source/WebInspectorUI/UserInterface/Models/TextRange.js:114 >>> + console.assert(!isNaN(this._startLine), "TextRange needs line/column data"); >> >> Nit: end sentence with a period. > > I searched — we don't usually end our assert messages with a period. I can go either way. Period (or exclamation point): ack 'console\.assert\(.*, \".*(\.|\!)"\)' | wc -l Count: 86 No period: ack 'console\.assert\(.*, \".*[^.]"\)' | wc -l Count: 51 So it's a mixed bag, but we definitely favor ending in a period,

Comment on attachment 321027 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=321027&action=review >> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:123 >> + this._enabled = !disabled; > > Early return if `this._enabled` isn't changing. It should be safe to call this repeatedly with the same `disabled` value, without thrashing `this._text`. I'll add an assert along with an early return. Calling this repeatedly with the same `disabled` value would mean that the view is no longer in sync with the model, which would be a problem.

Created attachment 321049 [details] Patch

Comment on attachment 321049 [details] Patch r=me

Comment on attachment 321049 [details] Patch Clearing flags on attachment: 321049 Committed r222137: <http://trac.webkit.org/changeset/222137>

All reviewed patches have been landed. Closing bug.