Add checkboxes to toggle (comment/uncomment) CSS properties. <rdar://problem/33525307>
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.