Bug 85061 - Web Inspector: Add "Show keymap" button to settings screen.
Summary: Web Inspector: Add "Show keymap" button to settings screen.
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: eustas.bug
URL: http://jtaby.com/2012/04/23/modern-we...
Keywords:
Depends on:
Blocks:
 
Reported: 2012-04-27 07:01 PDT by eustas.bug
Modified: 2012-10-22 05:49 PDT (History)
10 users (show)

See Also:


Attachments
Screenshot: before patch (68.12 KB, image/png)
2012-04-27 07:01 PDT, eustas.bug
no flags Details
Screenshot: after patch (54.11 KB, image/png)
2012-04-27 07:01 PDT, eustas.bug
no flags Details
Patch (7.64 KB, patch)
2012-04-27 07:46 PDT, eustas.bug
no flags Details | Formatted Diff | Diff
Patch (6.68 KB, patch)
2012-04-28 04:57 PDT, eustas.bug
yurys: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description eustas.bug 2012-04-27 07:01:14 PDT
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.
Comment 1 eustas.bug 2012-04-27 07:01:49 PDT
Created attachment 139195 [details]
Screenshot: after patch
Comment 2 eustas.bug 2012-04-27 07:46:10 PDT
Created attachment 139199 [details]
Patch
Comment 3 Alexander Pavlov (apavlov) 2012-04-27 08:22:57 PDT
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?
Comment 4 Andrey Kosyakov 2012-04-28 03:40:57 PDT
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.
Comment 5 eustas.bug 2012-04-28 04:57:24 PDT
Created attachment 139357 [details]
Patch
Comment 6 eustas.bug 2012-04-28 05:00:40 PDT
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.
Comment 7 Pavel Feldman 2012-04-28 07:06:10 PDT
I'd rather convert this pane into a tabbed pane.
Comment 8 Yury Semikhatsky 2012-05-03 05:47:19 PDT
Comment on attachment 139357 [details]
Patch

r- per Pavel's comments.
Comment 9 eustas.bug 2012-07-22 22:14:26 PDT
Keyboard shortcuts are accessible through tab in settings screen.