Add secondary keyboard shortcut of cmd-shift-K for the web console on Mac

RESOLVED WONTFIX

Status

defect
RESOLVED WONTFIX
8 years ago
11 months ago

People

(Reporter: rcampbell, Assigned: rcampbell)

Tracking

unspecified
x86
macOS

Firefox Tracking Flags

(firefox10-)

Details

(Whiteboard: [has-patch][reviewed][needs-update])

Attachments

(1 attachment, 1 obsolete attachment)

Assignee

Description

8 years ago
with the landing of bug 689924, we've introduced another keyboard shortcut for the web console. Unfortunately, this breaks existing muscle memory and has the potential to exist with other keyboard shortcuts on the mac. I'd like to re-add the previous shortcut as an alternate.
Duplicate of this bug: 706978
Assignee

Comment 2

8 years ago
Posted patch alternate hud key (obsolete) — Splinter Review
Assignee: nobody → rcampbell
Status: NEW → ASSIGNED
Attachment #578456 - Flags: review?(paul)
Assignee

Updated

8 years ago
Whiteboard: [good-first-bugs][mentor=robcee][lang=xul]
Comment on attachment 578456 [details] [diff] [review]
alternate hud key

Review of attachment 578456 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/browser-sets.inc
@@ +241,5 @@
>  #endif
>      <key id="key_openAddons" key="&addons.commandkey;" command="Tools:Addons" modifiers="accel,shift"/>
>      <key id="key_errorConsole" key="&errorConsoleCmd.commandkey;" oncommand="toJavaScriptConsole();" modifiers="accel,shift" disabled="true"/>
>      <key id="key_webConsole" key="&webConsoleCmd.commandkey;" oncommand="HUDConsoleUI.toggleHUD();"
> +        modifiers="accel,shift"/>

So this is going to be the primary one that shows up in the menu, but we want that to be Cmd+Opt right? As is, it'll become Cmd+Shift

We need to keep the ifdef and then add another below it for osx to define the alt key. Or if there's a better way to make that look neat, but that's the gist of it.

The alternative would be to add and ifdef in the menuitem to select the primary shortcut, but that spreads the confusion around so I think we should keep it all here.

@@ +246,3 @@
>  #ifdef XP_MACOSX
> +    <key id="key_webConsoleAlt" key="&webConsoleCmd.commandkey;" oncommand="HUDConsoleUI.toggleHUD();"
> +        modifiers="accel,alt"/>

Let's make a new <command> and then have both of these use it. Tools:WebConsole seems pretty consistent, but use your discretion. That way we keep it a bit more DRY and in line with how most of these work.
Attachment #578456 - Flags: review?(paul) → review-
Assignee

Comment 4

8 years ago
how's this?
Attachment #578456 - Attachment is obsolete: true
Attachment #578681 - Flags: review?(paul)
Assignee

Comment 5

8 years ago
Comment on attachment 578681 [details] [diff] [review]
alternate hud key

canceling review until I verify what our disabling code is doing, if anything.
Attachment #578681 - Flags: review?(paul)
Assignee

Comment 6

8 years ago
Comment on attachment 578681 [details] [diff] [review]
alternate hud key

ok, looks like this landed before we had that requirement. No references in delayedStartup() in browser.js.

Not sure it's worth disabling this particular feature, but I'll ask here if it's a requirement. There's no new JS in play.
Attachment #578681 - Flags: review?(paul)
Comment on attachment 578681 [details] [diff] [review]
alternate hud key

Review of attachment 578681 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Rob Campbell [:rc] (robcee) from comment #6)
> ok, looks like this landed before we had that requirement. No references in
> delayedStartup() in browser.js.
> 
> Not sure it's worth disabling this particular feature, but I'll ask here if
> it's a requirement. There's no new JS in play.

Not a requirement to add disabling in this patch, no. I think it would be good to do in a followup though for consistency. I filed bug 707296 since there's more than just this menuitem being different.

::: browser/base/content/browser-sets.inc
@@ +243,5 @@
>      <key id="key_openAddons" key="&addons.commandkey;" command="Tools:Addons" modifiers="accel,shift"/>
>      <key id="key_errorConsole" key="&errorConsoleCmd.commandkey;" oncommand="toJavaScriptConsole();" modifiers="accel,shift" disabled="true"/>
> +<!-- WebConsole Command Key changed to Cmd-Opt-K on OS X in bug 689924.
> +     To help with muscle memory, we want to keep the original key shortcut of
> +     Cmd-Shift-K as well. Bug 706204. -->

I think # line comments is going to be better here - consistent with other comments in this file & they'll get preprocessed out.
Attachment #578681 - Flags: review?(paul) → review+
Umm, is this bug really worth it, given the young age of this feature? How big a disruption is this outside the core devtools community?
Assignee

Comment 9

8 years ago
I don't have a good answer for that. Hard to believe but the Web Console's been in the browser for a year now and that's certainly enough time for people to have become accustomed to learning that keystroke.

We've had at least one dupe, if that counts for anything.
Assignee

Updated

8 years ago
Whiteboard: [has-patch][reviewed][needs-update]
(In reply to Rob Campbell [:rc] (robcee) from comment #9)
> I don't have a good answer for that. Hard to believe but the Web Console's
> been in the browser for a year now and that's certainly enough time for
> people to have become accustomed to learning that keystroke.
>
> We've had at least one dupe, if that counts for anything.

stechz is a mozilla employee. Maybe that makes a difference, maybe it doesn't. I'm not saying the change wasn't disruptive at all, but we often make changes with a seemingly larger impact on users. This one certainly affects a limited set of advanced users and is easily solved by looking at the web developer menu and some retraining. I'm worried that this bug sets a bad precedent for carrying around old stuff when making secondary UI changes. It also doesn't help that keys are already a mess.
(In reply to Dão Gottwald [:dao] from comment #10)
> stechz is a mozilla employee.

Not anymore, fwiw :)

There's no real downside to supporting both keys on Mac, AFAICT. There is a benefit to not changing a shortcut unnecessarily, and to maintaining cross-platform parity. If bug 689924 was just about adding an additional shortcut key rather than changing it, we wouldn't be having this discussion :)
Wouldn't it be simpler to have:
    <key id="key_webConsole" key="&webConsoleCmd.commandkey;" command="Tools:WebConsole" modifiers="accel,shift"/>
#ifdef XP_MACOSX
    <key id="key_webConsoleMac" key="&webConsoleCmd.commandkey;" command="Tools:WebConsole" modifiers="accel,alt"/>
#endif

and then just ifdef the key="" attribute for the menuitem?
Do we generally pursue cross-platform parity for similar shortcuts? My understanding is that Cmd and Opt on Mac have different meanings than Ctrl and Alt elsewhere, so cross-platform parity might be another misguided goal here and a bad precedent... We wouldn't support Ctrl+Alt+I on Windows and Linux either.
Cmd+Shift+K is the cross platform shortcut. Cmd+Opt+K is a Mac-specific additional shortcut, to match the Safari convention of using Cmd+Opt for developer tools.
Cmd+Shift+K isn't the shortcut we display in the menu on OS X, so for anyone switching between OS X and Windows/Linux, it seems that it would make just as much sense to expect Cmd+Opt+K (Ctrl+Alt+K) to work on Windows/Linux... Parity works both ways (or it doesn't).
(In reply to Dão Gottwald [:dao] from comment #15)
> Cmd+Shift+K isn't the shortcut we display in the menu on OS X

Note that I'm intentionally implying that we could change this. I'm not saying we should.

By the way, key_inspect lacks cross-platform parity too...
It sounds like you're actually arguing against bug 689924's patch, rather than this patch. I would have been fine with WONTFIXing bug 689924, personally, but I don't really have any basis for evaluating the arguments in favor of compatibility with Safari shortcuts (they seem somewhat tenuous). But given that we did fix bug 689924, and that not breaking existing users of the shortcut is basically free (this patch), I believe we should do that.
I don't have any strong feelings about which shortcut should be shown in the menu.
(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #17)
> It sounds like you're actually arguing against bug 689924's patch, rather
> than this patch

In terms of cross-platform parity, bug 689924 was a regression and this patch is an insufficient attempt to restore it. I don't consider cross-platform parity important here, though; I'm playing the devil's advocate.
Assignee

Comment 20

8 years ago
I've been in favor of changing the devtools shortcuts to Cmd+opt+key for awhile. It makes sense on a Mac with the extra Cmd key for accel, but makes things a little harder to map to other platforms. I.e., we lose parity and end up with a bunch of #ifdefs in browser.xul.

I do agree with Dão that having two keys for a secondary feature feels wrong, so maybe we should just get people used to it on the new binding now.

As for wontfixing bug 689924, we've had similar/dupes of that wontfixed at least 2 times before. This was a patch from a community member, so I felt it was time we took it. Also, it finally moves us away from DOM Inspector's shortcut on Mac.

So yeah, let's WONTFIX this. Reopen if anyone feels strongly about it.
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → WONTFIX
I won't reopen, but one argument to keep Cmd+Shift is that it makes the Web Console and Error Console shortcuts a nice pair.  (I also find Cmd+Option harder to type, but that might just be me.)
Sorry, I had no problem with the keyboard shortcut changing, I'm just bad at reading Mac shortcut key descriptions after all these years. :) As long as there's a shortcut I'm happy.

Updated

11 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.