Bug 147712 – Web Inspector: Add numerical input and slider based Visual editors for CSS properties

• Component: Web Inspector
• Status: RESOLVED FIXED

[Devin Rousso]

Adds the following types of Visual editors:

 - Combined input and select for CSS properties with both keywords and number values (can have units)
 - Toggle-able link to sync multiple editors when the value of one changes
 - Relative number slider with the range of either [-50, 50] for properties that allow negative values or [0, 100] otherwise

Will be use in the new Visual style details panel in the CSS sidebar.

[Devin Rousso]

Created attachment 258465 [details]
Patch

[Blaze Burg]

Comment on attachment 258465 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=258465&action=review

Looking very good overall. Little renamings are signs that I have nothing big to nitpick. Please add proper aria roles, then I will r+ this patch.

> Source/WebInspectorUI/UserInterface/Views/VisualStyleNumberInputBox.js:30
> +        super(propertyNames, text, possibleValues, possibleUnits || ["No Units"], "number-input-box", layoutReversed);

Is "No Units" a UIString?

> Source/WebInspectorUI/UserInterface/Views/VisualStyleNumberInputBox.js:33
> +        this._allowNegativeValues = allowNegativeValues || false;

Nit: !!allowNegativeValues

> Source/WebInspectorUI/UserInterface/Views/VisualStyleNumberInputBox.js:38
> +        let focusRing = document.createElement("div");

Rename: focusRingElement

> Source/WebInspectorUI/UserInterface/Views/VisualStyleNumberInputBox.js:90
> +        let editable = this.numberInputEditable;

A bool property like this often has 'is' in it, i.e., this.numberInputIsEditable, or this.numberInputSupportsEditing

> Source/WebInspectorUI/UserInterface/Views/VisualStyleNumberInputBox.js:91
> +        if (editable && !this._valueNumberInputElement.value || !this._keywordSelectElement.value)

Add parens to make grouping unambiguous.

> Source/WebInspectorUI/UserInterface/Views/VisualStyleNumberInputBox.js:102
> +        if (!isNaN(value)) {

How do keywords not go into this branch?

> Source/WebInspectorUI/UserInterface/Views/VisualStyleNumberInputBox.js:113
> +        if (this.valueIsKeyword(value)) {

is this a static method? If not, I would pick a name that makes it clearer that the function results depend on the current unit or other state.

> Source/WebInspectorUI/UserInterface/Views/VisualStyleNumberInputBox.js:124
> +        if (!this.valueIsUnit(keyword))

See above note about valueIsKeyword.

> Source/WebInspectorUI/UserInterface/Views/VisualStyleNumberInputBox.js:158
> +        this._valueNumberInputElement.setAttribute("placeholder", (!isNaN(text) && text) || 0);

This reads like an interview puzzle. Can you simplify?

> Source/WebInspectorUI/UserInterface/Views/VisualStyleNumberInputBox.js:176
> +    syncValue(text, value)

I would name this 'updateValue' as syncValue is not as clear as a verb.

> Source/WebInspectorUI/UserInterface/Views/VisualStyleNumberInputBox.js:196
> +        let switchedUnits = this.valueIsUnit(this._keywordSelectElement.value);

This name could be confusing. Is it a boolean, or a list of the units that switched? WK style is to prefer *Changed or *DidChange for booleans like this.

Rename: unitsChanged

> Source/WebInspectorUI/UserInterface/Views/VisualStyleNumberInputBox.js:197
> +        if (!this.numberInputEditable && switchedUnits)

For example, 

if (!this.numberInputIsEditable && unitsChanged)
   ...

> Source/WebInspectorUI/UserInterface/Views/VisualStyleNumberInputBox.js:203
> +        this.contentElement.classList.toggle("number-input-editable", switchedUnits);

it's weird that this is looking at switchedUnits instead of this.numberInputIsEditable. What if we switched between two non-editable units?

> Source/WebInspectorUI/UserInterface/Views/VisualStyleNumberInputBox.js:213
> +        function shiftValue(direction)

Rename: adjustValue(delta)? You aren't doing a bit shift or exponent shift...

> Source/WebInspectorUI/UserInterface/Views/VisualStyleNumberInputBox.js:237
> +        if (key.startsWith("Page"))

(Multiple places) Please use WI.KeyboardShortcut.Key.*, as we discussed in the other patch.

> Source/WebInspectorUI/UserInterface/Views/VisualStyleNumberInputBox.js:271
> +    _addValues(values)

(Multiple places) Rename: _createValueOptions. You aren't 'adding' values :)

> Source/WebInspectorUI/UserInterface/Views/VisualStyleNumberInputBox.js:300
> +        if (this._advancedUnitsElements)

Should you check this._isAdvancedUnits instead?

> Source/WebInspectorUI/UserInterface/Views/VisualStylePropertyEditorLink.js:48
> +        this._iconElement.title = WebInspector.UIString("Click to link.");

Hmm: "Click to link property values"? It might be unclear what is being linked.

> Source/WebInspectorUI/UserInterface/Views/VisualStylePropertyEditorLink.js:89
> +    _handleLinkedPropertyValueChanged(event)

We don't like prefixing handlers with 'handle'.

> Source/WebInspectorUI/UserInterface/Views/VisualStylePropertyEditorLink.js:105
> +    _syncValue(property)

See rename note above.

> Source/WebInspectorUI/UserInterface/Views/VisualStylePropertyEditorLink.js:136
> +        this._iconElement.title = this._linked ? WebInspector.UIString("Click to remove link.") : WebInspector.UIString("Click to link.");

See above.

> Source/WebInspectorUI/UserInterface/Views/VisualStyleRelativeNumberSlider.js:34
> +        this._slider = document.createElement("input");

Rename: SliderElement

[Devin Rousso]

Comment on attachment 258465 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=258465&action=review

>> Source/WebInspectorUI/UserInterface/Views/VisualStyleNumberInputBox.js:102
>> +        if (!isNaN(value)) {
> 
> How do keywords not go into this branch?

This only occurs if the value is a number.  isNaN() will return true if the value is not a number, so adding a ! in front will ensure that the entire statement will return only if the value is a number.

>> Source/WebInspectorUI/UserInterface/Views/VisualStyleNumberInputBox.js:203
>> +        this.contentElement.classList.toggle("number-input-editable", switchedUnits);
> 
> it's weird that this is looking at switchedUnits instead of this.numberInputIsEditable. What if we switched between two non-editable units?

That's fine.  "number-input-editable" is only supposed to be added to the class when the value inside the keyword select is a unit.  If the value inside the keyword select is a unit, add the class.  Otherwise (like "auto", "initial", or "inherit"), remove it.

>> Source/WebInspectorUI/UserInterface/Views/VisualStyleNumberInputBox.js:237
>> +        if (key.startsWith("Page"))
> 
> (Multiple places) Please use WI.KeyboardShortcut.Key.*, as we discussed in the other patch.

I thought that it was better to use KeyIdentifier where able.  KeyboardShortcut doesn't have any information about KeyIdentifiers.

>> Source/WebInspectorUI/UserInterface/Views/VisualStyleNumberInputBox.js:300
>> +        if (this._advancedUnitsElements)
> 
> Should you check this._isAdvancedUnits instead?

Im trying not to add the same advanced elements twice, so I check to see if the array "this._advancedUnitsElements" has been set, and if so, that must mean that I have already added the advanced options.

[Blaze Burg]

Comment on attachment 258465 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=258465&action=review

> Source/WebInspectorUI/UserInterface/Views/VisualStyleNumberInputBox.js:45
> +            this._keywordSelectElement.title = WebInspector.UIString("Alt Click to show all units");

Alt-click

>>> Source/WebInspectorUI/UserInterface/Views/VisualStyleNumberInputBox.js:102
>>> +        if (!isNaN(value)) {
>> 
>> How do keywords not go into this branch?
> 
> This only occurs if the value is a number.  isNaN() will return true if the value is not a number, so adding a ! in front will ensure that the entire statement will return only if the value is a number.

Oh, for some reason I thought that isNaN only returns true for NaN (and thus isNaN("string") would be false).

>> Source/WebInspectorUI/UserInterface/Views/VisualStyleNumberInputBox.js:196
>> +        let switchedUnits = this.valueIsUnit(this._keywordSelectElement.value);
> 
> This name could be confusing. Is it a boolean, or a list of the units that switched? WK style is to prefer *Changed or *DidChange for booleans like this.
> 
> Rename: unitsChanged

Reading this function again, I'm having trouble with the names and how the logic flows. Maybe this should be called keywordAllowsUnits?

>>> Source/WebInspectorUI/UserInterface/Views/VisualStyleNumberInputBox.js:237
>>> +        if (key.startsWith("Page"))
>> 
>> (Multiple places) Please use WI.KeyboardShortcut.Key.*, as we discussed in the other patch.
> 
> I thought that it was better to use KeyIdentifier where able.  KeyboardShortcut doesn't have any information about KeyIdentifiers.

Oh, right. I thought it had key names as well.

>> Source/WebInspectorUI/UserInterface/Views/VisualStyleNumberInputBox.js:271
>> +    _addValues(values)
> 
> (Multiple places) Rename: _createValueOptions. You aren't 'adding' values :)

Or, _addValuesToKeywordSelect

[Devin Rousso]

Comment on attachment 258465 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=258465&action=review

>>> Source/WebInspectorUI/UserInterface/Views/VisualStyleNumberInputBox.js:196
>>> +        let switchedUnits = this.valueIsUnit(this._keywordSelectElement.value);
>> 
>> This name could be confusing. Is it a boolean, or a list of the units that switched? WK style is to prefer *Changed or *DidChange for booleans like this.
>> 
>> Rename: unitsChanged
> 
> Reading this function again, I'm having trouble with the names and how the logic flows. Maybe this should be called keywordAllowsUnits?

Basically, both units and keywords are in the same select element.  Since units need number values associated with them (while keywords don't), every time the user selects a unit option, have to ensure that this class is applied to the container element (it makes the input for the number value visible and editable).  So, if the currently selected value inside the select element (which contains both keywords and units) is a unit, add the class.  Otherwise, remove the class.

[Devin Rousso]

Created attachment 258628 [details]
Patch

[Timothy Hatcher]

Comment on attachment 258628 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=258628&action=review

> Source/WebInspectorUI/UserInterface/Views/VisualStyleNumberInputBox.js:45
> +            this._keywordSelectElement.title = WebInspector.UIString("Alt-click to show all units");

It is Option not Alt on Mac.

[Blaze Burg]

Comment on attachment 258628 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=258628&action=review

r=me, this patch is looking much improved. Please address the one small naming issue, then we can land.

>> Source/WebInspectorUI/UserInterface/Views/VisualStyleNumberInputBox.js:45
>> +            this._keywordSelectElement.title = WebInspector.UIString("Alt-click to show all units");
> 
> It is Option not Alt on Mac.

Maybe we should add and use WebInspector.KeyboardShortcut.Modifier.OptionOrAlt in the Web Inspector. I'm sure there are other strings that make no sense on other ports.

> Source/WebInspectorUI/UserInterface/Views/VisualStyleRelativeNumberSlider.js:77
> +    _handleSliderChanged()

(Multiple places)

Inspector style is to not use "handle" as an event listener callback prefix. I think you should just remove it, or add "Did" to cases that lack a verb. (i.e., didMouseoutIcon).

[Devin Rousso]

Created attachment 258662 [details]
Patch

[Devin Rousso]

Comment on attachment 258662 [details]
Patch

Whoops.  Forgot to include your review Brian.

[Devin Rousso]

Created attachment 258663 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 258663 [details]
Patch

Clearing flags on attachment: 258663

Committed r188238: <http://trac.webkit.org/changeset/188238>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/22292082>

[Andres Gomez Garcia]

Comment on attachment 258663 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=258663&action=review

I know this patch already landed, but wanted to provide some comment. I hope they are helpful.

> Source/WebInspectorUI/UserInterface/Images/VisualStylePropertyLinked.svg:4
> +    <circle class="filled" cx="8" cy="8" r="4" fill="black" stroke="none" />

This is fill="black" ...

> Source/WebInspectorUI/UserInterface/Images/VisualStylePropertyUnlinked.svg:5
> +    <path class="filled" d="M 6 4 C 0 4 0 12 6 12" stroke="none" />

... but these are not.

Is this correct?

[Timothy Hatcher]

(In reply to comment #15)
> Comment on attachment 258663 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=258663&action=review
> 
> I know this patch already landed, but wanted to provide some comment. I hope
> they are helpful.
> 
> > Source/WebInspectorUI/UserInterface/Images/VisualStylePropertyLinked.svg:4
> > +    <circle class="filled" cx="8" cy="8" r="4" fill="black" stroke="none" />
> 
> This is fill="black" ...
> 
> > Source/WebInspectorUI/UserInterface/Images/VisualStylePropertyUnlinked.svg:5
> > +    <path class="filled" d="M 6 4 C 0 4 0 12 6 12" stroke="none" />
> 
> ... but these are not.
> 
> Is this correct?

fill="black" is the default, so it is fine to leave out. We should be consistent though.

[Devin Rousso]

(In reply to comment #16)
> fill="black" is the default, so it is fine to leave out. We should be
> consistent though.

I will change this with the changes for bug https://bugs.webkit.org/show_bug.cgi?id=147711.  I have noticed that some of the text icons for the buttons are slightly clipped, so I have wanted to redo them.  I'll add this fix to that as well.