Bug 85061 – Web Inspector: Add "Show keymap" button to settings screen.

• Component: Web Inspector (Deprecated)
• Status: RESOLVED WONTFIX

Created attachment 139194 [details]
Screenshot: before patch

Keyboard shortcuts screen can be opened only with a key shortcut.
So users has no chance to know it even exists.

Created attachment 139195 [details]
Screenshot: after patch

Created attachment 139199 [details]
Patch

[Alexander Pavlov (apavlov)]

Comment on attachment 139199 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=139199&action=review

I'm leaving the screenshot analysis to pfeldman, otherwise looks mostly good, with some comments to address.

> Source/WebCore/ChangeLog:6
> +        Added "Show keymap" button to settings screen + minor LnF adjustments.

I've had hard time doing the guesswork as to what "LnF" means. Please use the "UI" term instead.

> Source/WebCore/ChangeLog:12
> +        * English.lproj/localizedStrings.js: add "Miscellaneous" and "Show keyboard shortcuts reference"

The explanation style is "full sentence" ("Something like this.") - see http://trac.webkit.org/changeset/43259 as an example given in http://www.webkit.org/coding/contributing.html#changelogs

> Source/WebCore/ChangeLog:19
> +        * inspector/front-end/helpScreen.css: LnF adjustmens

typo: adjustmens -> adjustments

> Source/WebCore/ChangeLog:21
> +        (.help-button): button styles

styles -> style?

> Source/WebCore/ChangeLog:24
> +        (.help-content select): ajdust select LnF

typo: ajdust -> adjust (with 4 lines that follow)

> Source/WebCore/inspector/front-end/SettingsScreen.js:159
> +    _createButton: function(name, listener)

Please consider adding a jsdoc to the new method.

optional: Seemingly, "uiName" would be more intuitive, since "name" may refer to the "name" DOM attribute.

> Source/WebCore/inspector/front-end/SettingsScreen.js:163
> +        var button = document.createElement("button");

JFYI, we've got a respective utility method in utilities.js, so you can rewrite this as:

var button = p.createChild("button", "help-button");
thereby avoiding the next line and "p.appendChild(button);"

> Source/WebCore/inspector/front-end/SettingsScreen.js:241
> +        var div = document.createElement("div");

Why do you need a wrapper around "select"? Can the "help-shadowbox" class be applied directly to the "select" element?

[Andrey Kosyakov]

Comment on attachment 139199 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=139199&action=review

I'm not fan of the UI approach and "Miscellaneous" section in particular. How about moving it to the title bar, perhaps as a link or some iconic button?

> Source/WebCore/inspector/front-end/SettingsScreen.js:109
> +    function showKeyMap()
> +    {
> +        WebInspector.shortcutsScreen.show();
> +    }

I'd rarther pass WebInspector.shortcutsScreen.bind(WebInspector.shortcutsScreen) to createButton.

> Source/WebCore/inspector/front-end/SettingsScreen.js:165
> +        button.appendChild(document.createTextNode(WebInspector.UIString(name)));

Drop WebInspector.UIString(), you're already calling it on the call site (which is the preferred approach, btw)

>> Source/WebCore/inspector/front-end/SettingsScreen.js:241
>> +        var div = document.createElement("div");
> 
> Why do you need a wrapper around "select"? Can the "help-shadowbox" class be applied directly to the "select" element?

Why did this change? Is this "minor LnF" change? Please split it into a separate patch.

Created attachment 139357 [details]
Patch

Comment on attachment 139199 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=139199&action=review

>> Source/WebCore/ChangeLog:6
>> +        Added "Show keymap" button to settings screen + minor LnF adjustments.
> 
> I've had hard time doing the guesswork as to what "LnF" means. Please use the "UI" term instead.

Done.

>> Source/WebCore/ChangeLog:12
>> +        * English.lproj/localizedStrings.js: add "Miscellaneous" and "Show keyboard shortcuts reference"
> 
> The explanation style is "full sentence" ("Something like this.") - see http://trac.webkit.org/changeset/43259 as an example given in http://www.webkit.org/coding/contributing.html#changelogs

OK, thanks.

>> Source/WebCore/ChangeLog:19
>> +        * inspector/front-end/helpScreen.css: LnF adjustmens
> 
> typo: adjustmens -> adjustments

fixed

>> Source/WebCore/ChangeLog:21
>> +        (.help-button): button styles
> 
> styles -> style?

rephrased

>> Source/WebCore/ChangeLog:24
>> +        (.help-content select): ajdust select LnF
> 
> typo: ajdust -> adjust (with 4 lines that follow)

fixed

>> Source/WebCore/inspector/front-end/SettingsScreen.js:109
>> +    }
> 
> I'd rarther pass WebInspector.shortcutsScreen.bind(WebInspector.shortcutsScreen) to createButton.

Done. But still afraid of the race condition.

>> Source/WebCore/inspector/front-end/SettingsScreen.js:159
>> +    _createButton: function(name, listener)
> 
> Please consider adding a jsdoc to the new method.
> 
> optional: Seemingly, "uiName" would be more intuitive, since "name" may refer to the "name" DOM attribute.

Done

>> Source/WebCore/inspector/front-end/SettingsScreen.js:163
>> +        var button = document.createElement("button");
> 
> JFYI, we've got a respective utility method in utilities.js, so you can rewrite this as:
> 
> var button = p.createChild("button", "help-button");
> thereby avoiding the next line and "p.appendChild(button);"

Great. Thanks.

>> Source/WebCore/inspector/front-end/SettingsScreen.js:165
>> +        button.appendChild(document.createTextNode(WebInspector.UIString(name)));
> 
> Drop WebInspector.UIString(), you're already calling it on the call site (which is the preferred approach, btw)

Surely. Fixed.

>>> Source/WebCore/inspector/front-end/SettingsScreen.js:241
>>> +        var div = document.createElement("div");
>> 
>> Why do you need a wrapper around "select"? Can the "help-shadowbox" class be applied directly to the "select" element?
> 
> Why did this change? Is this "minor LnF" change? Please split it into a separate patch.

@Alexander: applying shadow to "select" doesn't work because of "select" element nature.

@Andrey: Splitting to a separate patch will make UI look inconsistent in between these patches.

[Pavel Feldman]

I'd rather convert this pane into a tabbed pane.

[Yury Semikhatsky]

Comment on attachment 139357 [details]
Patch

r- per Pavel's comments.

Keyboard shortcuts are accessible through tab in settings screen.