[Webkit-unassigned] [Bug 38667] Web Inspector: add help on keyboard shortcuts

Fri May 7 13:52:00 PDT 2010

https://bugs.webkit.org/show_bug.cgi?id=38667

--- Comment #9 from Andrey Kosyakov <caseq at chromium.org>  2010-05-07 13:51:58 PST ---
(In reply to comment #7)
> (From update of attachment 55285 [details])
> ChangeLog is missing
> 
> You should modify visual studio project and WebCore.qrc to make Windows & Qt
> happy.

I just skipped it for the draft to save time (Qt was fine, though). Fixed it
now.

> WebCore/inspector/front-end/ConsoleView.js:107
>  +      section.addAlternateKeys(keys, "Clear Console");
> I don't think we need such a strict API. You could just have an addRow method
> receiving two strings: first is non-localizable shortcut name name and second
> is localizable description. That way you would have:
> section.addRow(keys.join("/"), "Clear Console") here.
> Rationale: sometimes you need to do things like 'Ctrl + ("A" - "Z")' and such.
> Why limiting ourselves artificially.

As agreed offline, lower level now exposes generic API that accepts arbitrary
HTML as "keys", higher level provides specific utility methods to decorate
shortcuts and their combinations. Also, this will complicate localization and
won't allow us to render special strings (like '+' here) distinguishably.

> WebCore/inspector/front-end/ConsoleView.js:110
>  +          shortcut.shortcutToString(shortcut.Keys.Tab),
> I think this is overcomplication. I'd replace code below with:
> section.addRow("Tab / Shift + Tab", "Next / previous suggestion")

This won't allow for platform-specific key names (e.g. Shift vs. ⇑)

> section.addRow("Up / Down", "Next / previous line")

This complicates localization (if we ever want one for key names).

> section.addRow("Alt + N / Alt + P", ...

And this does not account for shortcuts varying by platform (e.g. "Ctrl + ["
vs. "Cmd + [")

> Rationale: there is no need to deal with shortcut objects if they are not used
> in the code explicitly.

This also allows us to keep consistent key names. And then, it would be good
idea to migrate all shortcuts to be configured with the help of
WebInspector.KeyboardShortcut anyway.

> 
> WebCore/inspector/front-end/ShortcutKeysPopover.js:3
>  +      this._element = document.getElementById("shortcuts-window");
> You should create these elements dynamically in the code and add to body
> element. No need to touch html.

Fixed.

> 
> WebCore/inspector/front-end/ShortcutKeysPopover.js:4
>  +      this._table = document.getElementById("shortcuts-table");
> All element objects should have Element suffix. this._tableElement in this
> case.

Fixed for class members, not done for local vars for sake of brevity and
readability.

> WebCore/inspector/front-end/ShortcutKeysPopover.js:21
>  +          this._element.style.visibility = "visible";
> You'll be simply adding it here, not unhiding.

I'd rather render table only once, as it's expensive.

> WebCore/inspector/front-end/ShortcutKeysPopover.js:18
>  +              this._buildShortcutsTable(this._sections, 2);
> If this method was creating the table object itself, you would not need the
> ugly needBuild flag.

Fixed.

> 
> WebCore/inspector/front-end/ShortcutKeysPopover.js:31
>  +      get shown()
> No one should know your status. Why is it here? Cancel key events?

Exactly. Status may change as a result of click on close button, which the
upper level won't know.

> WebCore/inspector/front-end/ShortcutKeysPopover.js:132
>  +  WebInspector.KeyboardShortcutsTable = function()
> The size of this class suggests that it is a map, not a class.

It almost is, except for section() helper.

> WebCore/inspector/front-end/ShortcutKeysPopover.js:151
>  +      this.order = ++WebInspector.KeyboardShortcutsSection._seqNo;
> _sequenceNumber

Done.

> WebCore/inspector/front-end/ShortcutKeysPopover.js:156
>  +  WebInspector.KeyboardShortcutsSection.prototype = {
> As mentioned above, please keep it simple and not distinguish between related /
> alternate / general keys. It simplifies both client and panel code + adds
> flexibility.

As discussed -- added generic method + specific helpers.

> WebCore/inspector/front-end/inspector.html:146
>  +      <div id="shortcuts-window">
> As stated above, no need to add this.

Fixed.

> 
> WebCore/inspector/front-end/inspector.js:538
>  +      this._shortcutsPopover = new
> WebInspector.ShortcutKeysPopover(WebInspector.keyboardShortcuts.sections);
> This guy could be created lazily. No need to pay for its creation at startup.

Fixed.

> WebCore/inspector/front-end/inspector.js:707
>  +          if (event.keyIdentifier === "U+001B") {
> Please add comment explaining the character
> 
> WebCore/inspector/front-end/inspector.js:713
>  +      var helpKey = WebInspector.isMac() ? "U+003F" : "U+00BF";
> ditto

Fixed.

> WebCore/inspector/front-end/shortcutKeysPopover.css:1
>  +  #shortcuts-window {
> No need
> 
> WebCore/inspector/front-end/shortcutKeysPopover.css:11
>  +      -webkit-border-radius: 8px;
> border-radius?

Fixed.

> 
> WebCore/inspector/front-end/shortcutKeysPopover.css:15
>  +      border-bottom-left-radius: 0;
> This is strange - you don't set radius to non-0, but you zero it for bottom
> parts.

This is to override more generic style (help-window now)

> WebCore/inspector/front-end/shortcutKeysPopover.css:48
>  +      width: 100%;
> right: 0

Wouldn't work -- we have size: 90% in another style we use.

> WebCore/inspector/front-end/shortcutKeysPopover.css:53
>  +      font-family: Helvetica, Arial, sans-serif;
> You should not define this. It'll pick default sans serif defined for body.

Fixed.

> WebCore/inspector/front-end/shortcutKeysPopover.css:1
>  +  #shortcuts-window {
> This component and a style should be generalized as a help screen, not
> necessarily related to popover. I think we should have similar one for console
> API and such. It is great you have a dedicated file for it, not that great
> there are so many styles here. Let us at least load this stylesheet on demand
> not to harm the startup time!

Done.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.