Bug 84708 – Web Inspector: Shortcuts screen UI polish

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector (Deprecated)

1) HUD have a different light source than the rest of the UI.
2) The border radius is too big.
3) The “X” button is not vertically centered in the header.
4) Some of the shortcuts are too small, so you can’t see them.

Created attachment 138981 [details]
Screenshot: before patch

Created attachment 138982 [details]
Screenshot: after patch

Created attachment 138989 [details]
Patch

Created attachment 138992 [details]
Patch

[WebKit Review Bot]

Attachment 138992 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/ChangeLog:15:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Pavel Feldman]

Comment on attachment 138989 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        No new tests. (OOPS!)

You need to explain what has happened and why here (enumerate changes you've done briefly).

> Source/WebCore/inspector/front-end/KeyboardShortcut.js:64
> +    Left: { code: 37, name: "<Left>" },           // also NUM_WEST

Make sure these are getting localized (get into English.lproj/localizedStrings.js)

> Source/WebCore/inspector/front-end/ShortcutsScreen.js:82
> +        var minMax = totalHeight;

It is not clear what minMax stands for.

> Source/WebCore/inspector/front-end/ShortcutsScreen.js:83
> +        var minMaxIdx = -1;

Please do not use abbreviations (should be Index)

> Source/WebCore/inspector/front-end/ShortcutsScreen.js:86
> +        for (idx = 0; idx < orderedSections.length; ++idx) {

for (var i = 0; i < ... ++i) {
}

> Source/WebCore/inspector/front-end/ShortcutsScreen.js:-80
> -        // This manual layout ugliness should be gone once WebKit implements

The underlying issue it not fixed though - what if user resizes the screen?

> Source/WebCore/inspector/front-end/ShortcutsScreen.js:106
> +            for (idx = stopIndices[columnIndex - 1]; idx < stopIndices[columnIndex]; ++idx) {

No need for { } around single line blocks.

Created attachment 138995 [details]
Patch

Created attachment 139019 [details]
Patch

Comment on attachment 138989 [details]
Patch

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

>> Source/WebCore/ChangeLog:8
>> +        No new tests. (OOPS!)
> 
> You need to explain what has happened and why here (enumerate changes you've done briefly).

Done.

>> Source/WebCore/inspector/front-end/KeyboardShortcut.js:64
>> +    Left: { code: 37, name: "<Left>" },           // also NUM_WEST
> 
> Make sure these are getting localized (get into English.lproj/localizedStrings.js)

These strings are not localized.

>> Source/WebCore/inspector/front-end/ShortcutsScreen.js:82
>> +        var minMax = totalHeight;
> 
> It is not clear what minMax stands for.

Clarifying comment added.

>> Source/WebCore/inspector/front-end/ShortcutsScreen.js:83
>> +        var minMaxIdx = -1;
> 
> Please do not use abbreviations (should be Index)

Done

>> Source/WebCore/inspector/front-end/ShortcutsScreen.js:86
>> +        for (idx = 0; idx < orderedSections.length; ++idx) {
> 
> for (var i = 0; i < ... ++i) {
> }

Done.

>> Source/WebCore/inspector/front-end/ShortcutsScreen.js:-80
>> -        // This manual layout ugliness should be gone once WebKit implements
> 
> The underlying issue it not fixed though - what if user resizes the screen?

I'm planning to turn 2 column table to 2 inlined divs in farther patches. So it will be 1 or 2 columned, depending container on width.

>> Source/WebCore/inspector/front-end/ShortcutsScreen.js:106
>> +            for (idx = stopIndices[columnIndex - 1]; idx < stopIndices[columnIndex]; ++idx) {
> 
> No need for { } around single line blocks.

Done.

[Pavel Feldman]

Comment on attachment 139019 [details]
Patch

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

> Source/WebCore/inspector/front-end/KeyboardShortcut.js:64
> +    Left: { code: 37, name: "<Left>" },           // also NUM_WEST

Even though they are not currently localized, they should be (at least Left, Right, Up and Down)

> Source/WebCore/inspector/front-end/ShortcutsScreen.js:58
> +     * Renders sections to and appends them to the given node.

We don't comment on code in WebKit unless necessary.

> Source/WebCore/inspector/front-end/ShortcutsScreen.js:110
> +};

No ; after }

> Source/WebCore/inspector/front-end/helpScreen.css:132
> +    color: #DD0;

We try to use rgb() notation for colors.

Created attachment 139353 [details]
Patch

Comment on attachment 139019 [details]
Patch

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

>> Source/WebCore/inspector/front-end/KeyboardShortcut.js:64
>> +    Left: { code: 37, name: "<Left>" },           // also NUM_WEST
> 
> Even though they are not currently localized, they should be (at least Left, Right, Up and Down)

Done

>> Source/WebCore/inspector/front-end/ShortcutsScreen.js:58
>> +     * Renders sections to and appends them to the given node.
> 
> We don't comment on code in WebKit unless necessary.

Removed

>> Source/WebCore/inspector/front-end/ShortcutsScreen.js:110
>> +};
> 
> No ; after }

Rolled back here and anywhere it was added.

>> Source/WebCore/inspector/front-end/helpScreen.css:132
>> +    color: #DD0;
> 
> We try to use rgb() notation for colors.

fixed

[WebKit Review Bot]

Comment on attachment 139353 [details]
Patch

Clearing flags on attachment: 139353

Committed r115568: <http://trac.webkit.org/changeset/115568>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.