Bug 166993 - Web Inspector: checkboxes in Settings screen use inappropriate layout
Summary: Web Inspector: checkboxes in Settings screen use inappropriate layout
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Matt Baker
URL:
Keywords: InRadar
Depends on:
Blocks: 169511
  Show dependency treegraph
 
Reported: 2017-01-12 15:22 PST by BJ Burg
Modified: 2017-04-11 20:05 PDT (History)
8 users (show)

See Also:


Attachments
Patch (38.89 KB, patch)
2017-03-22 14:25 PDT, Matt Baker
no flags Details | Formatted Diff | Diff
[Image] New UI (230.24 KB, image/png)
2017-03-22 14:28 PDT, Matt Baker
no flags Details
Patch (42.45 KB, patch)
2017-03-25 14:23 PDT, Matt Baker
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews124 for ios-simulator-wk2 (994.02 KB, application/zip)
2017-03-26 03:09 PDT, Build Bot
no flags Details
Patch (42.45 KB, patch)
2017-03-26 08:26 PDT, Matt Baker
no flags Details | Formatted Diff | Diff
Patch (42.45 KB, patch)
2017-03-26 08:28 PDT, Matt Baker
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews123 for ios-simulator-wk2 (6.51 MB, application/zip)
2017-03-27 02:41 PDT, Build Bot
no flags Details
Patch (42.44 KB, patch)
2017-03-27 15:44 PDT, Matt Baker
no flags Details | Formatted Diff | Diff
Patch (46.73 KB, patch)
2017-04-06 18:43 PDT, Matt Baker
no flags Details | Formatted Diff | Diff
[Image] Horiz. alignment of controls (172.45 KB, image/png)
2017-04-10 14:37 PDT, Matt Baker
no flags Details
Patch (46.75 KB, patch)
2017-04-11 17:36 PDT, Matt Baker
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description BJ Burg 2017-01-12 15:22:13 PST
See Safari Preferences > Advanced, or Mail Preferences, for an example of how to phrase and align toggles like this.
Comment 1 Radar WebKit Bug Importer 2017-01-12 15:23:02 PST
<rdar://problem/30002272>
Comment 2 Devin Rousso 2017-01-13 15:25:20 PST
(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)?
Comment 3 BJ Burg 2017-01-13 15:50:56 PST
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.
Comment 4 Matt Baker 2017-03-22 14:25:17 PDT
Created attachment 305127 [details]
Patch
Comment 5 Matt Baker 2017-03-22 14:28:51 PDT
Created attachment 305128 [details]
[Image] New UI
Comment 6 BJ Burg 2017-03-22 15:45:16 PDT
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 7 BJ Burg 2017-03-22 15:48:36 PDT
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.
Comment 8 Matt Baker 2017-03-25 14:23:27 PDT
Created attachment 305399 [details]
Patch
Comment 9 Matt Baker 2017-03-25 14:25:34 PDT
Updated patch changes implementation, not UI.
Comment 10 Build Bot 2017-03-26 03:09:29 PDT
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
Comment 11 Build Bot 2017-03-26 03:09:32 PDT
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
Comment 12 Matt Baker 2017-03-26 08:26:00 PDT
Created attachment 305421 [details]
Patch
Comment 13 Matt Baker 2017-03-26 08:28:58 PDT
Created attachment 305422 [details]
Patch

"Poke"
Comment 14 Build Bot 2017-03-27 02:41:07 PDT
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
Comment 15 Build Bot 2017-03-27 02:41:09 PDT
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
Comment 16 Matt Baker 2017-03-27 15:44:36 PDT
Created attachment 305521 [details]
Patch

Poke
Comment 17 Devin Rousso 2017-03-28 10:33:42 PDT
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 18 Matt Baker 2017-03-28 11:18:44 PDT
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 19 Matt Baker 2017-03-28 12:11:47 PDT
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.
Comment 20 Matt Baker 2017-04-06 18:43:22 PDT
Created attachment 306454 [details]
Patch
Comment 21 Timothy Hatcher 2017-04-10 14:16:28 PDT
This looks good in the screenshot. The whole thing seems off-center to the right though.
Comment 22 Matt Baker 2017-04-10 14:37:28 PDT
Created attachment 306742 [details]
[Image] Horiz. alignment of controls
Comment 23 Matt Baker 2017-04-10 14:38:14 PDT
(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 24 Devin Rousso 2017-04-11 16:17:58 PDT
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
Comment 25 Matt Baker 2017-04-11 17:36:51 PDT
Created attachment 306879 [details]
Patch
Comment 26 WebKit Commit Bot 2017-04-11 20:05:37 PDT
Comment on attachment 306879 [details]
Patch

Clearing flags on attachment: 306879

Committed r215256: <http://trac.webkit.org/changeset/215256>
Comment 27 WebKit Commit Bot 2017-04-11 20:05:39 PDT
All reviewed patches have been landed.  Closing bug.