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.
Created attachment 258333 [details] Patch
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"
Created attachment 258464 [details] Patch
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.
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.
(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.
Created attachment 258640 [details] Patch
Comment on attachment 258640 [details] Patch Clearing flags on attachment: 258640 Committed r188229: <http://trac.webkit.org/changeset/188229>
All reviewed patches have been landed. Closing bug.
<rdar://problem/22290849>
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?
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.