Bug 176643 – Web Inspector: Styles Redesign: support toggling properties

• Component: Web Inspector
• Status: RESOLVED FIXED

[Nikita Vasilyev]

Add checkboxes to toggle (comment/uncomment) CSS properties.

<rdar://problem/33525307>

[Nikita Vasilyev]

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

[Nikita Vasilyev]

Created attachment 320849 [details]
Patch

[Nikita Vasilyev]

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

[Matt Baker]

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.

[Matt Baker]

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

[Nikita Vasilyev]

(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).

[Nikita Vasilyev]

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.

[Nikita Vasilyev]

Created attachment 320985 [details]
Patch

[Matt Baker]

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

[Nikita Vasilyev]

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.

[Nikita Vasilyev]

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

?

[Matt Baker]

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.

[Nikita Vasilyev]

Created attachment 321027 [details]
Patch

[Matt Baker]

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.

[Matt Baker]

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,

[Nikita Vasilyev]

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.

[Nikita Vasilyev]

Created attachment 321049 [details]
Patch

[Matt Baker]

Comment on attachment 321049 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 321049 [details]
Patch

Clearing flags on attachment: 321049

Committed r222137: <http://trac.webkit.org/changeset/222137>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.