Closed Bug 1300458 Opened 3 years ago Closed 3 years ago

Switching browser tabs with Cmd+Shift+[ and Cmd+Shift+] on Mac OS X also switches devtools toolbox tabs

Categories

(DevTools :: Framework, defect, P1)

49 Branch
Unspecified
macOS
defect

Tracking

(firefox48 unaffected, firefox49+ wontfix, firefox50+ fixed, firefox51+ fixed)

RESOLVED FIXED
Firefox 51
Tracking Status
firefox48 --- unaffected
firefox49 + wontfix
firefox50 + fixed
firefox51 + fixed

People

(Reporter: cpeterson, Assigned: jdescottes)

References

Details

(Keywords: regression)

Attachments

(3 files)

[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
Flags: needinfo?(poirot.alex)
Summary: Switching browser tabs with Cmd+Shift+[ and Cmd+Shift+] also switches devtools toolbox tabs → Switching browser tabs with Cmd+Shift+[ and Cmd+Shift+] on Mac OS X also switches devtools toolbox tabs
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.
Attachment #8788261 - Flags: review?(poirot.alex) → review+
btw, it would be great to add a dedicated testcase to devtools/client/shared/test/browser_key_shortcuts.js
Flags: needinfo?(poirot.alex)
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.
Comment on attachment 8788431 [details]
Bug 1300458 - fix linting errors in browser_key_shortcuts.js;

https://reviewboard.mozilla.org/r/76936/#review75038
Attachment #8788431 - Flags: review?(poirot.alex) → review+
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
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
Component: Developer Tools → Developer Tools: Framework
Priority: -- → P1
https://hg.mozilla.org/mozilla-central/rev/139dd60aa0e7
https://hg.mozilla.org/mozilla-central/rev/dc3fed21da74
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
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
Attachment #8789258 - Flags: review+
Attachment #8789258 - Flags: approval-mozilla-aurora?
Comment on attachment 8789258 [details] [diff] [review]
bug1300458.aurora.patch

Fixes a recent regression and a common scenario, Aurora50+
Attachment #8789258 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Version: unspecified → 49 Branch
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.