Summary: | Web Inspector: RTL: keyboard shortcuts with directionality need to be flipped (forward/back, etc) | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | BJ Burg <bburg> | ||||||||||
Component: | Web Inspector | Assignee: | Devin Rousso <hi> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | commit-queue, hi, inspector-bugzilla-changes, mattbaker, webkit-bug-importer | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | WebKit Nightly Build | ||||||||||||
Hardware: | All | ||||||||||||
OS: | All | ||||||||||||
Bug Depends on: | 165759 | ||||||||||||
Bug Blocks: | |||||||||||||
Attachments: |
|
Description
BJ Burg
2016-12-12 09:48:37 PST
Created attachment 303744 [details]
Patch
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. Created attachment 303886 [details]
Patch
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. Created attachment 304572 [details]
Patch
Created attachment 304959 [details]
Revised Patch
Comment on attachment 304959 [details]
Revised Patch
r=me
Comment on attachment 304959 [details] Revised Patch Clearing flags on attachment: 304959 Committed r214514: <http://trac.webkit.org/changeset/214514> All reviewed patches have been landed. Closing bug. |