Bug 147712

Created attachment 258465 [details] Patch

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

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.

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

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.

Created attachment 258628 [details] Patch

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.

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

Created attachment 258662 [details] Patch

Comment on attachment 258662 [details] Patch Whoops. Forgot to include your review Brian.

Created attachment 258663 [details] Patch

Comment on attachment 258663 [details] Patch Clearing flags on attachment: 258663 Committed r188238: <http://trac.webkit.org/changeset/188238>

All reviewed patches have been landed. Closing bug.

<rdar://problem/22292082>

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?

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

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