Bug 147711 – Web Inspector: Add different types of non-numerical Visual editors for CSS properties

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

[Devin Rousso]

Adds the following types of Visual editors:

 - Color picker
 - Checkbox for a single keyword value
 - Selectable buttons in a list for multiple keyword values
 - Dropdown menu for multiple keyword values
 - Input (with autocomplete) for all the CSS property names
 - Slider with a range of [0, 1]
 - Input for URLs

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

[Devin Rousso]

Created attachment 258333 [details]
Patch

[Blaze Burg]

Comment on attachment 258333 [details]
Patch

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

Looks good!

A few overall issues to address, in addition to lower level changes:

 - `let`/`const` all the things
 - Needs ARIA roles and tabIndex

> Source/WebInspectorUI/UserInterface/Views/VisualStyleColorPicker.css:44
> +    background-color: white;

(Multiple places) Nit: don't use color keywords.

> Source/WebInspectorUI/UserInterface/Views/VisualStyleColorPicker.css:45
> +    background-size: calc(50%);

Is this different from just '50%'?

> Source/WebInspectorUI/UserInterface/Views/VisualStyleColorPicker.js:42
> +        this._textInput = document.createElement("input");

Rename: this._textInputElement

> Source/WebInspectorUI/UserInterface/Views/VisualStyleColorPicker.js:163
> +        if (keyCode === 9 || keyCode === 13) {

(Multiple places) It would be less obscure to do something like:

var tabKeyCode = WebInspector.KeyboardShortcut.Key.Tab.keyCode;
var enterKeyCode = WebInspector.KeyboardShortcut.Key.Enter.keyCode;
if (keyCode === tabKeyCode || keyCode === enterKeyCode) { ...

> Source/WebInspectorUI/UserInterface/Views/VisualStyleKeywordCheckbox.js:37
> +        this._contentElement.appendChild(this._checkbox);

(Multiple places) If contentElement is created by the base class, then it should not be prefixed with an underscore. Add a getter and put it in the // Protected section.

> Source/WebInspectorUI/UserInterface/Views/VisualStyleKeywordCheckbox.js:45
> +        this._valueRegExp = /^([^;]+)\s*;?$/;

(Multiple places) If valueRegExp is used by the base class, then it should be an overridden getter. Even better, don't require that the derived class provide a RegExp instance; just have a parseValue() function that everyone overrides, possibly returning null or something if there's no current regex.

> Source/WebInspectorUI/UserInterface/Views/VisualStyleKeywordIconList.js:39
> +        var prettyPropertyReferenceName = this._propertyReferenceName.capitalize().replace(/(-.)/g,function(x){ return x[1].toUpperCase() });

Um, that's quite the line. :) Please break the regex into a separate line. Don't forget the semicolon in the callback.

> Source/WebInspectorUI/UserInterface/Views/VisualStyleKeywordIconList.js:42
> +        {

Nit: Opening brace doesn't need its own line for function declaration statements.

> Source/WebInspectorUI/UserInterface/Views/VisualStyleKeywordIconList.js:43
> +            var iconButton = document.createElement("button");

Rename: iconButtonElement

> Source/WebInspectorUI/UserInterface/Views/VisualStyleKeywordIconList.js:58
> +            var icon = createListItem.call(this, key, this._possibleValues.basic[key]);

Rename: iconElement

> Source/WebInspectorUI/UserInterface/Views/VisualStyleKeywordIconList.js:94
> +        var selected = this._selectedIcon && this._selectedIcon.classList.toggle("selected");

Rename: iconIsSelected

I didn't know classList.toggle returned a value. Can we avoid using this for both the side-effect and return value?

> Source/WebInspectorUI/UserInterface/Views/VisualStyleKeywordPicker.js:32
> +        this._keywordSelect = document.createElement("select");

Rename: this._keywordSelectElement

> Source/WebInspectorUI/UserInterface/Views/VisualStyleKeywordPicker.js:44
> +        this._unchangedOption = document.createElement("option");

Rename: this._unchangedOptionElement

> Source/WebInspectorUI/UserInterface/Views/VisualStylePropertyNameInput.js:89
> +        if (keyCode === 9 || keyCode === 13) {

See note from above.

> Source/WebInspectorUI/UserInterface/Views/VisualStyleURLInput.js:34
> +        this._urlInput.placeholder = WebInspector.UIString("Enter a url");

"Enter a URL"

[Devin Rousso]

Created attachment 258464 [details]
Patch

[Blaze Burg]

Comment on attachment 258464 [details]
Patch

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

r=me

Make sure you land things in the correct order, if there are dependencies.

> Source/WebInspectorUI/UserInterface/Views/Slider.js:132
>      get _maxX()

I don't think we have any other getters in the codebase that are private. I would make this public (why not?) and drop the leading underscore for the getter. Then use just one underscore for the member.

> Source/WebInspectorUI/UserInterface/Views/VisualStyleColorPicker.js:34
> +        this._swatchElement.title = WebInspector.UIString("Click to open a colorpicker. Shift-click to change color format.");

This message does not conform to the verbiage recommended by the Apple Style Guide. Something like "Click to select a color. Shift-click to switch color formats."

https://help.apple.com/asg/mac/2013/#apsg1f284f4c

> Source/WebInspectorUI/UserInterface/Views/VisualStyleColorPicker.js:41
> +

We should be setting aria roles here. My opinion on this has changed since Friday, I think we should add accessibility support in a follow-up patch. It is really hard to determine the proper role and make sure we set the right attributes, without a live example to play with.

> Source/WebInspectorUI/UserInterface/Views/VisualStyleColorPicker.js:109
> +        this._swatchInnerElement.style.backgroundColor = this._color ? value : null;

What does setting background color to "null" do? Does it ignore this inline style property?

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

Rename: this._checkboxElement

> Source/WebInspectorUI/UserInterface/Views/VisualStyleKeywordCheckbox.js:37
> +        this.contentElement.appendChild(this._checkbox);

Because this uses a real checkbox, the accessibility role should be taken care of automatically, I think. It might need a custom aria-label per instance, though.

> Source/WebInspectorUI/UserInterface/Views/VisualStyleKeywordIconList.js:37
> +        this._selectedIcon = null;

Not sure what to do about accessibility here. Let's defer until it's all connected.

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

Alt-click

> Source/WebInspectorUI/UserInterface/Views/VisualStyleKeywordPicker.js:39
> +        this._unchangedOptionElement.text = WebInspector.UIString("Unchanged");

"Default"? Don't exactly remember where this goes in the UI.

[Devin Rousso]

Comment on attachment 258464 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/VisualStyleColorPicker.js:34
>> +        this._swatchElement.title = WebInspector.UIString("Click to open a colorpicker. Shift-click to change color format.");
> 
> This message does not conform to the verbiage recommended by the Apple Style Guide. Something like "Click to select a color. Shift-click to switch color formats."
> 
> https://help.apple.com/asg/mac/2013/#apsg1f284f4c

I was just copying what the current color picker uses for a title.  Should I change that too?

>> Source/WebInspectorUI/UserInterface/Views/VisualStyleColorPicker.js:109
>> +        this._swatchInnerElement.style.backgroundColor = this._color ? value : null;
> 
> What does setting background color to "null" do? Does it ignore this inline style property?

It allows the "transparent" background to be visible (its a checkerboard pattern set in CSS).

>> Source/WebInspectorUI/UserInterface/Views/VisualStyleKeywordPicker.js:39
>> +        this._unchangedOptionElement.text = WebInspector.UIString("Unchanged");
> 
> "Default"? Don't exactly remember where this goes in the UI.

Default isn't exactly what we are going for here as that implies what is set originally on the item (like the default for Display is Block).  Unchanged is meant to state that this property has not been changed at all from the computed value.

[Blaze Burg]

(In reply to comment #5)
> Comment on attachment 258464 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=258464&action=review
> 
> >> Source/WebInspectorUI/UserInterface/Views/VisualStyleColorPicker.js:34
> >> +        this._swatchElement.title = WebInspector.UIString("Click to open a colorpicker. Shift-click to change color format.");
> > 
> > This message does not conform to the verbiage recommended by the Apple Style Guide. Something like "Click to select a color. Shift-click to switch color formats."
> > 
> > https://help.apple.com/asg/mac/2013/#apsg1f284f4c
> 
> I was just copying what the current color picker uses for a title.  Should I
> change that too?

Yes.

[Devin Rousso]

Created attachment 258640 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 258640 [details]
Patch

Clearing flags on attachment: 258640

Committed r188229: <http://trac.webkit.org/changeset/188229>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/22290849>

[Andres Gomez Garcia]

Comment on attachment 258640 [details]
Patch

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

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

> Source/WebInspectorUI/UserInterface/Images/FontVariantSmallCaps.svg:4
> +    <rect class="filled" x="-2" width="20" height="20" fill="white" stroke="none" />

Is this white background on purpose?

[Andres Gomez Garcia]

Comment on attachment 258640 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Images/FontStyleItalic.svg:3
> +<svg version="1.1" xmlns="http://www.w3.org/2000/svg" viewBox="0 0 16 16" font-size="9" font-family="-apple-system, sans-serif" font-style="italic">

I'm sure you know better, but it doesn't seems wise to use a font and not convert the text to curves.

> Source/WebInspectorUI/UserInterface/Images/FontStyleNormal.svg:3
> +<svg version="1.1" xmlns="http://www.w3.org/2000/svg" viewBox="0 0 16 16" font-size="9" font-family="-apple-system, sans-serif">

Ditto.

> Source/WebInspectorUI/UserInterface/Images/FontVariantSmallCaps.svg:3
> +<svg version="1.1" xmlns="http://www.w3.org/2000/svg" viewBox="0 0 16 16" font-size="7" font-family="-apple-system, sans-serif">

Ditto.

> Source/WebInspectorUI/UserInterface/Images/TextDecorationLineThrough.svg:3
> +<svg version="1.1" xmlns="http://www.w3.org/2000/svg" viewBox="0 0 16 16" font-size="9" font-family="-apple-system, sans-serif">

Ditto.

> Source/WebInspectorUI/UserInterface/Images/TextDecorationOverline.svg:3
> +<svg version="1.1" xmlns="http://www.w3.org/2000/svg" viewBox="0 0 16 16" font-size="9" font-family="-apple-system, sans-serif">

Ditto.

> Source/WebInspectorUI/UserInterface/Images/TextDecorationUnderline.svg:3
> +<svg version="1.1" xmlns="http://www.w3.org/2000/svg" viewBox="0 0 16 16" font-size="9" font-family="-apple-system, sans-serif">

Ditto.

> Source/WebInspectorUI/UserInterface/Images/TextTransformCapitalize.svg:3
> +<svg version="1.1" xmlns="http://www.w3.org/2000/svg" viewBox="0 0 16 16" font-size="9" font-family="-apple-system, sans-serif">

Ditto.

> Source/WebInspectorUI/UserInterface/Images/TextTransformLowercase.svg:3
> +<svg version="1.1" xmlns="http://www.w3.org/2000/svg" viewBox="0 0 16 16" font-size="9" font-family="-apple-system, sans-serif">

Ditto.

> Source/WebInspectorUI/UserInterface/Images/TextTransformUppercase.svg:3
> +<svg version="1.1" xmlns="http://www.w3.org/2000/svg" viewBox="0 0 16 16" font-size="9" font-family="-apple-system, sans-serif">

Ditto.