[Tracking Requested - why for this release]: Alexandre, this bug is a Mac-only regression from bug 1268450, similar to bug 1297288. STR: 1. On a Mac, open two browser tabs. 2. In the first tab, open the devtools to the "Console" toolbox tab. 3. Use the Cmd+Shift+] keyboard shortcut to switch to the second browser tab. 4. Click back to the first tab and look at the devtools RESULT: The devtools have switched to the "Debugger" toolbox tab. Cmd+Shift+[ and Cmd+Shift+] are the Mac keyboard shortcuts to switch browser tabs. The devtools are capturing those keyboard shortcuts and treating them as Cmd+[ and Cmd+] keyboard shortcuts to switch toolbox tabs. I bisected this regression to the pushlog for bug 1268450: https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=fef64183cb939b13fbfdd5b1b44bd3c63d64d88f&tochange=df627da479f868c9704d7ac0b1c6aa76fb298150

That's surprising, I don't have a Mac next to me to verify, but doesn't "Cmd+Shift+[" ends up generating another character than "[" ? http://store.storeimages.cdn-apple.com/4662/as-images.apple.com/is/image/AppleInc/aos/published/images/M/B1/MB110B/MB110B?wid=1000&hei=1000&fmt=jpeg&qlt=95&op_sharpen=0&resMode=bicub&op_usm=0.5,0.5,0,0&iccEmbed=0&layer=comp&.v=tn4kb3 Looking at this keyboard, [ doesn't need any modifier to be run. Shift+[ should end up doing a "{". Then I imagine adding Cmd modifier wouldn't change the final character? Or do you have another kind of keyboard?

I can reproduce on OSX with the US International keyboard layout, on FF49 & 50. In FF51, the behavior is slightly different due to the changes from Bug 1297288: cmd+shift+[ / ] change the selected tool in devtools instead of changing the browser tab. I logged which event.key was captured for various combinations of shortcuts: - shift + ] : } - cmd + ] : ] - ctrl + ] : ] - alt + ] : ‘ - shift + cmd + ] : ] - shift + ctrl + ] : } - shift + alt + ] : ’ So shift + cmd + ] actually still maps to ]. I quickly tested with a few other keys and it seems consistent behavior on OSX. Looking at the way the Shift modifier is handled in key-shortcuts: > if (shortcut.shift != event.shiftKey && event.key && > event.key.match(/[a-zA-Z]/)) { > return false; Maybe we should add another bail out that checks (shortcut.meta && !shortcut.alt && !shortcut.ctrl && shortcut.shift != shortcut.shiftKey).

Comment on attachment 8788261 [details] Bug 1300458 - devtools key shortcuts fix shift+cmd shortcuts on OSX; https://reviewboard.mozilla.org/r/76826/#review74918 Thanks a lot for looking into this osx specific!! ::: devtools/client/shared/key-shortcuts.js:200 (Diff revision 1) > - // Shift is a special modifier, it may implicitely be required if the > - // expected key is a special character accessible via shift. > - if (shortcut.shift != event.shiftKey && event.key && > - event.key.match(/[a-zA-Z]/)) { > + if (shortcut.shift != event.shiftKey) { > + // Shift is a special modifier, it may implicitely be required if the expected key > + // is a special character accessible via shift. > + let isAlphabetical = event.key && event.key.match(/[a-zA-Z]/); > + // Cmd + shift on OSX sends the original key (same as without shift). > + let shiftCmdShortcut = shortcut.meta && !shortcut.alt && !shortcut.ctrl; Do we have to assert ctrlKey and altKey ? You don't say what happens when pressing Cmd+Alt or Cmd+ctrl. Also note that this codepath is about having a mismatch around Shift. It doesn't mean the shortcut contains Shift. I say chat as your comment is misleading.

btw, it would be great to add a dedicated testcase to devtools/client/shared/test/browser_key_shortcuts.js

Thanks for the review, added a test case to browser_key_shortcuts, on try atm, will push to review when green. (In reply to Alexandre Poirot [:ochameau] from comment #4) > Comment on attachment 8788261 [details] > Bug 1300458 - devtools key shortcuts fix shift+cmd shortcuts on OSX; > > https://reviewboard.mozilla.org/r/76826/#review74918 > > Do we have to assert ctrlKey and altKey ? > You don't say what happens when pressing Cmd+Alt or Cmd+ctrl. - alt + ] : ‘ - ctrl + ] : ] - cmd + alt + ] : ‘ - cmd + ctrl + ] : ] But since for alt and ctrl we always bail out if there is a mismatch between the shortcut and the event, I don't think we need anything extra here? Then of course it makes this piece of code depend on its position in the method, which is a bit fragile. I can update it so that it works in isolation too if you prefer? > > Also note that this codepath is about having a mismatch around Shift. It > doesn't mean the shortcut contains Shift. I say chat as your comment is > misleading. I updated the comment with a link to this bug. A mismatch on shift should trigger a bail out if the only other modifier is cmd, because shift+cmd+key and cmd+key both create an event with the same event.key.

(In reply to Julian Descottes [:jdescottes] from comment #6) > But since for alt and ctrl we always bail out if there is a mismatch between > the shortcut and the event, I don't think we need anything extra here? Then > of course it makes this piece of code depend on its position in the method, > which is a bit fragile. I can update it so that it works in isolation too if > you prefer? No, that's fine. OSX behavior is really weird. I would not have expected this particular behavior: cmd + alt + ] : ‘

On Firefox 48 - Cmd+[ and Cmd+] were the only keyboard shortcuts that allowed switching the devtools toolbox tabs. Cmd+Shift+[ and Cmd+Shift+] were used only for switching the browser's tabs. On Nightly 51 - When the focus is in the dev toolbox, Cmd+Shift+[ and Cmd+Shift+] switches toolbox tabs when it shouldn't ( Cmd+[ and Cmd+] should be the only designated keyboard for this as on Firefox 48). Since the issue is partially reproducible also on Firefox 51, setting the status-firefox:51 to affected.

green try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=eedaef796d42

Comment on attachment 8788431 [details] Bug 1300458 - fix linting errors in browser_key_shortcuts.js; https://reviewboard.mozilla.org/r/76936/#review75038

Pushed by jdescottes@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/139dd60aa0e7 devtools key shortcuts fix shift+cmd shortcuts on OSX;r=ochameau https://hg.mozilla.org/integration/autoland/rev/dc3fed21da74 fix linting errors in browser_key_shortcuts.js;r=ochameau

https://hg.mozilla.org/mozilla-central/rev/139dd60aa0e7 https://hg.mozilla.org/mozilla-central/rev/dc3fed21da74

Too late for 49 but we could still possibly uplift to aurora (50).

Track for 49+/50+/51+ as this is a regression for Mac.

Rebased "Bug 1300458 - devtools key shortcuts fix shift+cmd shortcuts on OSX;" to apply cleanly on aurora. Carrying over r+. Approval Request Comment [Feature/regressing bug #]: Bug 1268450 [User impact if declined]: OSX only: the keyboard shortcut to select the previous/next browser tab (cmd+shift+[/]) is not working if devtools are focused. Instead, the previous/next devtools module is selected. [Describe test coverage new/current, TreeHerder]: covered by new test case in devtools/client/shared/test/browser_key_shortcuts.js, try push on top of aurora at https://treeherder.mozilla.org/#/jobs?repo=try&revision=dae64e92477b (previous try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=eedaef796d42) [Risks and why]: minor JS change, covered by tests [String/UUID change made/needed]: N/A

Comment on attachment 8789258 [details] [diff] [review] bug1300458.aurora.patch Fixes a recent regression and a common scenario, Aurora50+