Bug 147576 – Web Inspector: Add visual editors for CSS properties

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Devin Rousso]

Used in the new Visual style details panel in the CSS sidebar panel.

[Devin Rousso]

Created attachment 258091 [details]
Patch

[Devin Rousso]

Created attachment 258202 [details]
Patch

[Devin Rousso]

Created attachment 258330 [details]
Patch

[Blaze Burg]

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.

[Devin Rousso]

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.

[Devin Rousso]

Created attachment 258461 [details]
Patch

[Devin Rousso]

Created attachment 258626 [details]
Patch

[Timothy Hatcher]

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.

[Radar WebKit Bug Importer]

<rdar://problem/22281720>

[Timothy Hatcher]

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?

[Devin Rousso]

> > 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?

[Devin Rousso]

Created attachment 258993 [details]
Patch

[Timothy Hatcher]

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.

[Devin Rousso]

Created attachment 259017 [details]
Patch

[Devin Rousso]

Created attachment 259018 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 259018 [details]
Patch

Clearing flags on attachment: 259018

Committed r188484: <http://trac.webkit.org/changeset/188484>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.