Bug 166993

<rdar://problem/30002272>

(In reply to comment #0) > See Safari Preferences > Advanced, or Mail Preferences, for an example of > how to phrase and align toggles like this. Most of the text and layout matches Xcode. Are we trying to match Safari preferences or Xcode preferences (they are slightly different)?

I should have made a screenshot. Whereas Xcode says Show:__[ ] Line Numbers _______[ ] Code Folding Ribbon etc we should be doing Show:__[ ] Whitespace Characters _______[ ] Invalid Characters The other settings look fine to me.

Created attachment 305127 [details] Patch

Created attachment 305128 [details] [Image] New UI

Comment on attachment 305127 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=305127&action=review I don't like introducing "Page" as a new type of view subclass. Can we set these up like: WI.GeneralSettingsView < WI.SettingsView < WI.View > Source/WebInspectorUI/UserInterface/Views/SettingsPage.js:44 > + addSetting(title, setting, optionsOrLabel) Preferred parameter arrangement: addSetting(setting, {options}) title is not optional, throw an exception if not found in options. There's no reason to do optionsOrLabel unless you are retrofitting an existing function with many callers to support taking an options object. > Source/WebInspectorUI/UserInterface/Views/SettingsPage.js:77 > + case WebInspector.SettingsPage.EditorStyle.Checkbox: I would prefer we add a way to specify the type of a WI.Setting instance rather than treating this as a view-level concept. That way we could check for improper reads/writes for any setting that declares a type, and this checking wouldn't need to live in view code. It could help determine the input type displayed and the set of allowed characters too.

Comment on attachment 305127 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=305127&action=review >> Source/WebInspectorUI/UserInterface/Views/SettingsPage.js:77 >> + case WebInspector.SettingsPage.EditorStyle.Checkbox: > > I would prefer we add a way to specify the type of a WI.Setting instance rather than treating this as a view-level concept. That way we could check for improper reads/writes for any setting that declares a type, and this checking wouldn't need to live in view code. It could help determine the input type displayed and the set of allowed characters too. After team discussion, let's just keep this typechecking in this layer.

Created attachment 305399 [details] Patch

Updated patch changes implementation, not UI.

Comment on attachment 305399 [details] Patch Attachment 305399 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/3412261 New failing tests: fast/css/getComputedStyle/computed-style-font-family.html

Created attachment 305418 [details] Archive of layout-test-results from ews124 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews124 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6

Created attachment 305421 [details] Patch

Created attachment 305422 [details] Patch "Poke"

Comment on attachment 305422 [details] Patch Attachment 305422 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/3416387 New failing tests: fast/scrolling/ios/scroll-events-back-forward.html fast/css/getComputedStyle/computed-style-font-family.html

Created attachment 305458 [details] Archive of layout-test-results from ews123 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews123 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6

Created attachment 305521 [details] Patch Poke

Comment on attachment 305521 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=305521&action=review Awesome job! This looks really nice. Here are some minor suggestions/concerns. > Source/WebInspectorUI/UserInterface/Views/GeneralSettingsView.js:72 > + let zoomLevels = [0.6, 0.8, 1, 1.2, 1.4, 1.6, 1.8, 2, 2.2, 2.4]; NIT: I'd make this const. > Source/WebInspectorUI/UserInterface/Views/GeneralSettingsView.js:80 > + this.addSetting(WebInspector.UIString("Zoom:"), WebInspector.settings.zoomFactor, zoomOptions); I think you are missing a `null`. this.addSetting(WebInspector.UIString("Zoom:"), WebInspector.settings.zoomFactor, null, zoomOptions); > Source/WebInspectorUI/UserInterface/Views/SettingsGroup.js:26 > +WebInspector.SettingsGroup = class SettingsGroup extends WebInspector.Object Is there a reason to not make this a subclass of WebInspector.View? > Source/WebInspectorUI/UserInterface/Views/SettingsGroup.js:44 > + addSetting(setting, label, options) Since the label isn't always used, I'd put it as part of options. I personally believe that only required parameters should have their own variable, and using options will definitely help with future changes. > Source/WebInspectorUI/UserInterface/Views/SettingsGroup.js:48 > + options = options || {}; NIT: you could make this a default value for the parameter. > Source/WebInspectorUI/UserInterface/Views/SettingsGroup.js:63 > + debugger; I think this was left in accidentally. > Source/WebInspectorUI/UserInterface/Views/SettingsGroup.js:66 > + } > + if (label) { Style: add newline. > Source/WebInspectorUI/UserInterface/Views/SettingsGroup.js:73 > + if (label) { > + editorElement.id = setting.name; > + let labelElement = document.createElement("label"); > + labelElement.setAttribute("for", editorElement.id); > + labelElement.textContent = label; > + > + editorWrapperElement.append(labelElement); > + } I think a better way of doing this instead of adding more IDs to the DOM would be to wrap the editorElement with the <label>. if (label) { let labelElement = document.createElement("label"); labelElement.append(editorElement); labelElement.append(label); editorWrapperElement.append(labelElement); } else editorWrapperElement.append(editorElement); > Source/WebInspectorUI/UserInterface/Views/SettingsGroup.js:106 > + const defaultValue = setting.defaultValue || 0; I think this should be 1 to match the minimum value. > Source/WebInspectorUI/UserInterface/Views/SettingsGroup.js:113 > + let keyValuePairs = []; NIT: this should be var https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/let#Temporal_dead_zone_and_errors_with_let > Source/WebInspectorUI/UserInterface/Views/SettingsGroup.js:126 > + keyValuePairs = options.values.map((value) => [value, value]); I feel like doing this is not helpful. This doesn't really seem like it would be a case we want to support (I don't think this would work with localizations). We should expect callers of this function to follow our procedures or get an error. > Source/WebInspectorUI/UserInterface/Views/SettingsGroup.js:143 > + setting.value = !event.target.selectedIndex; I think this deserves a comment. I am assuming that the first item is the trueValue and the second value is the falseValue? > Source/WebInspectorUI/UserInterface/Views/SettingsTabContentView.css:117 > +body[dir=rtl] .content-view.settings > .settings-page > .container > .value-editor input[type="number"] { I don't think "value-editor" applies to anything. body[dir=rtl] .content-view.settings > .settings-page > .container .editor > input[type="number"] > Source/WebInspectorUI/UserInterface/Views/SettingsTabContentView.js:47 > + this._SettingsViewIdentifierMap = new Map; Style: capitalization. > Source/WebInspectorUI/UserInterface/Views/SettingsTabContentView.js:79 > + get selectedSettingsView() Style: this can be on one line. > Source/WebInspectorUI/UserInterface/Views/SettingsTabContentView.js:95 > + this._selectedSettingsView.updateLayout(); Should you give this a LayoutReason? > Source/WebInspectorUI/UserInterface/Views/SettingsTabContentView.js:100 > + addSettingsView(SettingsView) Style: capitalization.

Comment on attachment 305521 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=305521&action=review >> Source/WebInspectorUI/UserInterface/Views/GeneralSettingsView.js:80 >> + this.addSetting(WebInspector.UIString("Zoom:"), WebInspector.settings.zoomFactor, zoomOptions); > > I think you are missing a `null`. > > this.addSetting(WebInspector.UIString("Zoom:"), WebInspector.settings.zoomFactor, null, zoomOptions); Oops! >> Source/WebInspectorUI/UserInterface/Views/SettingsGroup.js:26 >> +WebInspector.SettingsGroup = class SettingsGroup extends WebInspector.Object > > Is there a reason to not make this a subclass of WebInspector.View? The initial motivation for WebInspector.View was to codify the rAF-style layout done by views with complex DOM subtrees : DataGrid, TimelineOverview, etc. I think we'll want to add a new base class for views which are backed by a DOM tree but don't require any special layout logic (beyond what can be done with CSS). I propose adding WebInspector.Control for this purpose. For example: Object <- Control <- View <- TimelineOverview Object <- Control <- DetailsSection A control would encapsulate creating/attaching a DOM element, and potentially other things: focus history, visibility, etc: class Control extends WebInspector.Object { constructor(element) { super(); this._element = element || document.createElement("div"); } get element() { return this._element; } } Until we have a clear vision for what the responsibilities of WebInspector.Control should be (or whether to add it at all), the ad-hoc approach of creating elements all over the place is fine. >> Source/WebInspectorUI/UserInterface/Views/SettingsGroup.js:48 >> + options = options || {}; > > NIT: you could make this a default value for the parameter. We haven't been making much use of defaults (I'm not sure why), but I like the idea. >> Source/WebInspectorUI/UserInterface/Views/SettingsGroup.js:63 >> + debugger; > > I think this was left in accidentally. :( >> Source/WebInspectorUI/UserInterface/Views/SettingsGroup.js:73 >> + } > > I think a better way of doing this instead of adding more IDs to the DOM would be to wrap the editorElement with the <label>. > > if (label) { > let labelElement = document.createElement("label"); > labelElement.append(editorElement); > labelElement.append(label); > > editorWrapperElement.append(labelElement); > } else > editorWrapperElement.append(editorElement); I think the markup makes more sense semantically by not nesting inputs in labels. The W3C guidance for labels also recommends explicitly using the `for` attribute for accessibility reasons: https://www.w3.org/WAI/tutorials/forms/labels/ >> Source/WebInspectorUI/UserInterface/Views/SettingsGroup.js:106 >> + const defaultValue = setting.defaultValue || 0; > > I think this should be 1 to match the minimum value. Good catch. >> Source/WebInspectorUI/UserInterface/Views/SettingsGroup.js:143 >> + setting.value = !event.target.selectedIndex; > > I think this deserves a comment. I am assuming that the first item is the trueValue and the second value is the falseValue? Correct. Yeah It could use a comment. >> Source/WebInspectorUI/UserInterface/Views/SettingsTabContentView.js:79 >> + get selectedSettingsView() > > Style: this can be on one line. We've been standardizing on having both the getter and setter be multi-line if one of them is.

Comment on attachment 305521 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=305521&action=review >> Source/WebInspectorUI/UserInterface/Views/SettingsGroup.js:113 >> + let keyValuePairs = []; > > NIT: this should be var > > https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/let#Temporal_dead_zone_and_errors_with_let I forgot about this! Is the issue that all cases share a block? Does a real TDZ risk exist with the code as written, or is this a future-proofing comment? >> Source/WebInspectorUI/UserInterface/Views/SettingsTabContentView.css:117 >> +body[dir=rtl] .content-view.settings > .settings-page > .container > .value-editor input[type="number"] { > > I don't think "value-editor" applies to anything. > > body[dir=rtl] .content-view.settings > .settings-page > .container .editor > input[type="number"] Right! I also need to change `setting-page` to `setting-view`. >> Source/WebInspectorUI/UserInterface/Views/SettingsTabContentView.js:95 >> + this._selectedSettingsView.updateLayout(); > > Should you give this a LayoutReason? Not needed. In the future if a subview implements View.prototype.layout we can add a resize reason. >> Source/WebInspectorUI/UserInterface/Views/SettingsTabContentView.js:100 >> + addSettingsView(SettingsView) > > Style: capitalization. Good grief.

Created attachment 306454 [details] Patch

This looks good in the screenshot. The whole thing seems off-center to the right though.

Created attachment 306742 [details] [Image] Horiz. alignment of controls

(In reply to Timothy Hatcher from comment #21) > This looks good in the screenshot. The whole thing seems off-center to the > right though. The controls are all centered with respect to the setting title's trailing-edge.

Comment on attachment 306454 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=306454&action=review r=me. Super awesome job! > Source/WebInspectorUI/UserInterface/Views/SettingEditor.js:130 > + let keyValuePairs = []; NIT: should be `var` not `let` https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/let#Temporal_dead_zone_and_errors_with_let

Created attachment 306879 [details] Patch

Comment on attachment 306879 [details] Patch Clearing flags on attachment: 306879 Committed r215256: <http://trac.webkit.org/changeset/215256>

All reviewed patches have been landed. Closing bug.