Bug 165761 - Web Inspector: RTL: keyboard shortcuts with directionality need to be flipped (forward/back, etc)
Summary: Web Inspector: RTL: keyboard shortcuts with directionality need to be flipped...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
Depends on: 165759
Blocks:
  Show dependency treegraph
 
Reported: 2016-12-12 09:48 PST by BJ Burg
Modified: 2017-03-28 18:56 PDT (History)
5 users (show)

See Also:


Attachments
Patch (19.72 KB, patch)
2017-03-07 17:13 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (16.85 KB, patch)
2017-03-08 18:50 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (16.70 KB, patch)
2017-03-15 15:59 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Revised Patch (16.90 KB, patch)
2017-03-20 15:40 PDT, BJ Burg
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description BJ Burg 2016-12-12 09:48:37 PST
See also https://bugs.webkit.org/show_bug.cgi?id=158823
Comment 1 BJ Burg 2016-12-12 09:49:25 PST
<rdar://problem/29621308>
Comment 2 Devin Rousso 2017-03-07 17:13:26 PST
Created attachment 303744 [details]
Patch
Comment 3 BJ Burg 2017-03-08 16:53:43 PST
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.
Comment 4 Devin Rousso 2017-03-08 18:50:51 PST
Created attachment 303886 [details]
Patch
Comment 5 BJ Burg 2017-03-15 15:39:52 PDT
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.
Comment 6 Devin Rousso 2017-03-15 15:59:40 PDT
Created attachment 304572 [details]
Patch
Comment 7 BJ Burg 2017-03-20 15:40:28 PDT
Created attachment 304959 [details]
Revised Patch
Comment 8 Matt Baker 2017-03-28 18:21:42 PDT
Comment on attachment 304959 [details]
Revised Patch

r=me
Comment 9 WebKit Commit Bot 2017-03-28 18:56:20 PDT
Comment on attachment 304959 [details]
Revised Patch

Clearing flags on attachment: 304959

Committed r214514: <http://trac.webkit.org/changeset/214514>
Comment 10 WebKit Commit Bot 2017-03-28 18:56:25 PDT
All reviewed patches have been landed.  Closing bug.