Bug 165761 – Web Inspector: RTL: keyboard shortcuts with directionality need to be flipped (forward/back, etc)

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Blaze Burg]

See also https://bugs.webkit.org/show_bug.cgi?id=158823

[Blaze Burg]

<rdar://problem/29621308>

[Devin Rousso]

Created attachment 303744 [details]
Patch

[Blaze Burg]

Comment on attachment 303744 [details]
Patch

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

AFAIK, switching Left and Right arrow keys is not appropriate. Switching the action performed for Cmd-{ to go forwards instead of backwards is appropriate.

> Source/WebInspectorUI/ChangeLog:8
> +        * UserInterface/Controllers/CodeMirrorCompletionController.js:

Please fill out the change log so I know what each diff is supposed to accomplish.

> Source/WebInspectorUI/UserInterface/Views/ContentBrowser.js:51
> +            let forwardButtonTitle = isRTL ? WebInspector.UIString("Back (%s)") : WebInspector.UIString("Forward (%s)");

Please rename these to leftButton / rightButton. Otherwise you could have backButtonTitle be "Forward" which makes no sense.

> Source/WebInspectorUI/UserInterface/Views/ContentBrowser.js:53
> +            this._backButtonNavigationItem = new WebInspector.ButtonNavigationItem("back", backButtonTitle.format(this._backKeyboardShortcut.displayName), "Images/BackForwardArrows.svg#left-arrow-mask", 8, 13);

Again, I think it will be less confusing if we refer to the buttons as "leftButton" and "rightButton" and bind different actions / tooltips to them. It is less confusing than to flip the order in which they are inserted.

> Source/WebInspectorUI/UserInterface/Views/DataGrid.js:-1393
> -        } else if (event.keyIdentifier === "Right") {

What does this do?

> Source/WebInspectorUI/UserInterface/Views/LogContentView.js:-741
> -        else if (event.keyIdentifier === "Right")

What does this do?

> Source/WebInspectorUI/UserInterface/Views/NavigationBar.js:349
> +        } else if ((!isRTL && event.keyIdentifier === "Right") || (isRTL && event.keyIdentifier === "Left")) {

What does this do?

> Source/WebInspectorUI/UserInterface/Views/TabBrowser.js:65
> +        let nextModifier2 = isRTL ? WebInspector.KeyboardShortcut.Modifier.Shift : 0;

Keeping logical (next/previous) naming is fine for the actual keyboard shortcut object itself.

> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:499
> +        } else if ((!isRTL && event.keyIdentifier === "Left") || (isRTL && event.keyIdentifier === "Right")) {

Ditto.

[Devin Rousso]

Created attachment 303886 [details]
Patch

[Blaze Burg]

Comment on attachment 303886 [details]
Patch

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

r- because the back/forward buttons still seem mixed up.

> Source/WebInspectorUI/UserInterface/Views/ContentBrowser.js:51
> +            let rightButtonTitle = isRTL ? WebInspector.UIString("Forward (%s)") : WebInspector.UIString("Back (%s)");

Isn't this backwards? If in LTR, the rightmost button should go forward. If in RTL, the rightmost button should go backward.

> Source/WebInspectorUI/UserInterface/Views/DataGrid.js:1380
> +        } else if ((!isRTL && event.keyIdentifier === "Left") || (isRTL && event.keyIdentifier === "Right")) {

Good.

> Source/WebInspectorUI/UserInterface/Views/TabBrowser.js:60
> +        let previousKey1 = isRTL ? WebInspector.KeyboardShortcut.Key.RightCurlyBrace : WebInspector.KeyboardShortcut.Key.LeftCurlyBrace;

This is correct.

[Devin Rousso]

Created attachment 304572 [details]
Patch

[Blaze Burg]

Created attachment 304959 [details]
Revised Patch

[Matt Baker]

Comment on attachment 304959 [details]
Revised Patch

r=me

[WebKit Commit Bot]

Comment on attachment 304959 [details]
Revised Patch

Clearing flags on attachment: 304959

Committed r214514: <http://trac.webkit.org/changeset/214514>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.