Bug 166993 – Web Inspector: checkboxes in Settings screen use inappropriate layout

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

[Blaze Burg]

See Safari Preferences > Advanced, or Mail Preferences, for an example of how to phrase and align toggles like this.

[Radar WebKit Bug Importer]

<rdar://problem/30002272>

[Devin Rousso]

(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)?

[Blaze Burg]

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.

[Matt Baker]

Created attachment 305127 [details]
Patch

[Matt Baker]

Created attachment 305128 [details]
[Image] New UI

[Blaze Burg]

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.

[Blaze Burg]

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.

[Matt Baker]

Created attachment 305399 [details]
Patch

[Matt Baker]

Updated patch changes implementation, not UI.

[Build Bot]

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

[Build Bot]

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

[Matt Baker]

Created attachment 305421 [details]
Patch

[Matt Baker]

Created attachment 305422 [details]
Patch

"Poke"

[Build Bot]

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

[Build Bot]

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

[Matt Baker]

Created attachment 305521 [details]
Patch

Poke

[Devin Rousso]

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.

[Matt Baker]

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.

[Matt Baker]

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.

[Matt Baker]

Created attachment 306454 [details]
Patch

[Timothy Hatcher]

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

[Matt Baker]

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

[Matt Baker]

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

[Devin Rousso]

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

[Matt Baker]

Created attachment 306879 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 306879 [details]
Patch

Clearing flags on attachment: 306879

Committed r215256: <http://trac.webkit.org/changeset/215256>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.