If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

[Mac] Add Cmd-Opt-L shortcut for Downloads, to match Safari and Adium

NEW
Assigned to

Status

()

Firefox
Menus
11 years ago
5 months ago

People

(Reporter: Håkan Waara, Assigned: nyee, Mentored)

Tracking

2.0 Branch
All
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug] [exterminationweek])

Attachments

(3 attachments, 8 obsolete attachments)

(Reporter)

Description

11 years ago
Something that has been bothering me is how every mac app has their own keyword combo för "Show transfers" / "Show downloads" or whatever. This is the kind of thing that would benefit from being a convention...

Here are some of the major apps' shortcuts:

Adium: Cmd-alt-L
Safari: Cmd-alt-L
Camino: Cmd-alt-D
Firefox: Cmd-J

I propose that Firefox change its shortcut to the same as Adium and Safari. That way I won't have to remember more than one shortcut. :)
"alt" = "option" for the record.
(Reporter)

Comment 2

11 years ago
Beltzner, Mconnor, do you guys agree?
I could see it as an additional shortcut (like we have for various IE compat shortcuts like Accel-E).  As a replacement, I think it would be churn without a good reason.  I think Cmd-J is a better shortcut anyway, so I don't feel inclined to throw away a one-handed shortcut after three major releases...
Summary: Change "Show Download Manager" shortcut to the same as Safari and Adium → [Mac] Add Cmd-Opt-L shortcut for Downloads, to match Safari and Adium
Whiteboard: [good first bug][mentor=gavin]

Comment 4

4 years ago
I would like to work on this. Could this be assigned to me?
Assignee: nobody → jonnyscholes

Comment 5

4 years ago
Created attachment 815320 [details] [diff] [review]
0001-changed-show-downloads-command-on-osx-bug-362587.patch

Does this look ok? Feels too simple to be correct... :p
Attachment #815320 - Flags: review?(gavin.sharp)
Comment on attachment 815320 [details] [diff] [review]
0001-changed-show-downloads-command-on-osx-bug-362587.patch

Cmd+J shouldn't be removed at this point, and I think that's going to be exposed in the menus even if Cmd+L is added.
Attachment #815320 - Flags: review?(gavin.sharp) → review-

Comment 7

4 years ago
Yep fair enough, I guess a lot of users are used to cmd+J. What did you mean by 'that's going to be exposed in the menus even if Cmd+L is added' though? The above patch changes the command in the menus if that's what you meant. I'm new to the FF code base ;)

Updated

4 years ago
Whiteboard: [good first bug][mentor=gavin] → [good first bug][mentor=gavin] [exterminationweek]
Mentor: gavin.sharp@gmail.com
Whiteboard: [good first bug][mentor=gavin] [exterminationweek] → [good first bug] [exterminationweek]
(Assignee)

Comment 8

3 years ago
Created attachment 8463057 [details] [diff] [review]
bug-362587-fix

I have tested and made minor formatting corrections to Jonny's changes, and the Tools -> Downloads menu entry did indeed change from cmd-j to opt-cmd-L. However, cmd-J continues to work, and I don't see any code breakages. I will argue that it is preferable to have the menu entry be opt-cmd-L instead of cmd-J, since it is more idiomatic in OS X and its users IME tend to be more sensitive to non-standard UI elements. If a consensus is made to support cmd-J for more homogeneity throughout different OS versions of Firefox, then cmd-J can be put into the menu entry and both shortcuts should continue to work.
Attachment #8463057 - Flags: review?(gavin.sharp)
Comment on attachment 8463057 [details] [diff] [review]
bug-362587-fix

Thanks for the patch Nathan!

This works great, but while testing this, I wondered whether we should make this shortcut open the panel if the downloads button is in the toolbar, rather than opening the library. That matches Safari's behavior a bit more closely (though they don't have a separate download library, as I understand it).

Let's flag this for ui-review and see what our excellent UX team has to say.
Attachment #8463057 - Flags: ui-review?(ux-review)
Attachment #8463057 - Flags: review?(gavin.sharp)
Attachment #8463057 - Flags: review+
Attachment #815320 - Attachment is obsolete: true
(Assignee)

Comment 10

3 years ago
I agree with opt-cmd-L opening the downloads panel if available. Perhaps our UX team should also look into determining if this behavior should apply for cmd-J as well.
Changing the long-standing behavior of Cmd+J might be more problematic for existing users. With this new shortcut we have more flexibility - though perhaps having two shortcuts that do slightly different things would also be confusing?
(Assignee)

Comment 12

3 years ago
I suggest that opt-cmd-L should have the new behavior (falling back to old behavior if necessary) and cmd-J keeps its old behavior. Since we are adding new functionality, would it be reasonable for all other platforms to get ctrl-alt-L shortcuts for the new behavior?

Comment 13

3 years ago
The above solution seems most logical to me - opening the (recent) downloads toolbar is quite different to opening the library of downloads. Whats more, Chrome on mac uses shift+cmd+j to open the full library of downloads (although they don't have the equivalent of the downloads toolbar) so keeping cmd+j would mean we are matching both Safari and Chromes functionality the closest.
Comment on attachment 8463057 [details] [diff] [review]
bug-362587-fix

This already seems to be the consensus here and it is also the most logical solution:
Cmd+J opens the library
Cmd+Alt+L opens the downloads menu item (and fall back to open the library if that item is not in the toolbar)

That seams to be the approach that produces the least amount of friction.
Attachment #8463057 - Flags: ui-review?(ux-review) → ui-review-
Sounds great. To do that, we'll need a slightly more complicated patch.

The function for opening the downloads panel if the downloads button is in the toolbar is DownloadsIndicatorView.onCommand():
http://hg.mozilla.org/mozilla-central/annotate/104254bd1fc8/browser/base/content/browser.xul#l899

But we'll likely need a helper function that checks whether the button is present, and either calls DownloadsIndicatorView.onCommand() if it is, or BrowserDownloadsUI() if it isn't.

I'm not sure offhand what the best way to check whether the button is in the toolbar, Jared, can you advise?
Flags: needinfo?(jaws)
If you want to check if the button is in the toolbar, you can do the following:

let placement = CustomizableUI.getPlacementOfWidget("downloads-button");
let isInToolbar = placement && CustomizableUI.getAreaType(placement.area) == "toolbar";
Flags: needinfo?(jaws)
(Assignee)

Comment 17

3 years ago
Where should I place the logic code?
The cleanest place to put this function might be just tacking it on to DownloadsIndicatorView, which is defined in browser/components/downloads/content/indicator.js and imported into all browser windows. You could call it something like DownloadsIndicatorView.openPanelOrLibrary().
(Assignee)

Comment 19

3 years ago
I've completed the logic code. Do you know what should call DownloadsIndicatorView.openPanelOrLibrary()?
The call to openPanelOrLibrary should be put in an "oncommand" attribute of the <key> element you added in the previous patch, and you should remove its "command" attribute.
(More info about <key> elements: https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/key)
(Assignee)

Comment 22

3 years ago
Created attachment 8466375 [details] [diff] [review]
bug-362587-fix [v. 2]

I have added the required functionality in this patch. However, since we have different functionality for each shortcut, we have some UX options:
1. Keep Opt+Cmd+L as the menu shortcut shown on OS X or change it back to Cmd+J
2. Keep Opt+Cmd+L as a hidden/non-hidden OS X shortcut as per 1. or goto 3.
3. Implement a similar Ctrl+Alt+L for all other platforms and/or add a menu entry to all platforms called: Recent Downloads [Opt+Cmd+L/Ctrl+Alt+L]

Also, please note that I got this error whenever a Downloads button panel operation via Opt+Cmd+L is successful:

JavaScript error: chrome://browser/content/downloads/indicator.js, line 522: aEvent is undefined

It appears that aEvent.stopPropagation() causes the issue. Should we concern ourselves with this?
Attachment #8463057 - Attachment is obsolete: true
Attachment #8466375 - Flags: ui-review?(ux-review)
Attachment #8466375 - Flags: review?(gavin.sharp)
(In reply to Nathan Yee [:nyee] from comment #22)
> 3. Implement a similar Ctrl+Alt+L for all other platforms

Ctrl+Alt is for OS-wide shortcuts.
Comment on attachment 8466375 [details] [diff] [review]
bug-362587-fix [v. 2]

>+      DownloadsPanel.isPanelShowing ? DownloadsPanel.hidePanel()
>+        : DownloadsPanel.showPanel();

>+    isInToolbar ? DownloadsIndicatorView.onCommand() : BrowserDownloadsUI();

Please use proper if/else.
(Assignee)

Comment 25

3 years ago
Created attachment 8466402 [details] [diff] [review]
bug-362587-fix [v. 3]
Attachment #8466375 - Attachment is obsolete: true
Attachment #8466375 - Flags: ui-review?(ux-review)
Attachment #8466375 - Flags: review?(gavin.sharp)
Attachment #8466402 - Flags: ui-review?(ux-review)
Attachment #8466402 - Flags: review?(gavin.sharp)
(In reply to Nathan Yee [:nyee] from comment #22)
> Also, please note that I got this error whenever a Downloads button panel
> operation via Opt+Cmd+L is successful:
> 
> JavaScript error: chrome://browser/content/downloads/indicator.js, line 522:
> aEvent is undefined
> 
> It appears that aEvent.stopPropagation() causes the issue. Should we concern
> ourselves with this?

You can fix this by changing:
oncommand="DownloadsIndicatorView.openPanelOrLibrary();"
to:
oncommand="DownloadsIndicatorView.openPanelOrLibrary(event);"

("event" is magically defined in attribute event handlers).

and then adjusting openPanelOrLibrary to pass that in to its call to DownloadsIndicatorView.onCommand.
Comment on attachment 8466402 [details] [diff] [review]
bug-362587-fix [v. 3]

>diff --git a/browser/components/downloads/content/indicator.js b/browser/components/downloads/content/indicator.js

>   onCommand: function DIV_onCommand(aEvent)

>-      DownloadsPanel.showPanel();
>+      if (DownloadsPanel.isPanelShowing) {
>+        DownloadsPanel.hidePanel();
>+      } else {
>+        DownloadsPanel.showPanel();
>+      }

Does this change interfere with the behavior of the downloads button (when in the toolbar)? It seems to currently open/close on click correctly in that case, just want to make sure that you tested that this this didn't break that.

With this done and the change from comment 26, this should be good to go!

No need for ui-review again, since Philipp's already signed off on the desired behavior.
Attachment #8466402 - Flags: ui-review?(ux-review)
Attachment #8466402 - Flags: review?(gavin.sharp)
(Assignee)

Comment 28

3 years ago
I don't know how to test for breakages, although a search for "DownloadsIndicatorView.onCommand(" on DXR only found it used once in [browser/base/content/browser.xul]. I have also found a bug where if you enter Customize, then press Opt+Cmd+L and exit Customize, it takes two button presses or Opt+Cmd+L shortcuts in any combination to open the downloads panel. It appears that Opt+Cmd+L still registers when the Customize mode is active. Is there any way to detect if the user is in Customize mode or if opening the Downloads panel failed?
(In reply to Nathan Yee [:nyee] from comment #28)
> I don't know how to test for breakages

I meant testing this case:
1) With the downloads button in the toolbar, click it once to open
2) Click it again to close
3) Click once more to open
(and so on)

My concern was that the isPanelShowing attribute check could conflict with the click consuming for open panels, or something like that. It should work fine, but worth testing it explicitly.

> I have also found a bug where if you
> enter Customize, then press Opt+Cmd+L and exit Customize, it takes two
> button presses or Opt+Cmd+L shortcuts in any combination to open the
> downloads panel. It appears that Opt+Cmd+L still registers when the
> Customize mode is active. Is there any way to detect if the user is in
> Customize mode or if opening the Downloads panel failed?

It looks like you can use CustomizationHandler.isCustomizing to determine whether we're customizing, but it seems odd that a widget action would need to do that (I don't see others that do), so it's probably worth digging deeper here. Why does the panel failing to open in customization mode mean that two open commands are required for it to open after exiting customize mode?
(Assignee)

Comment 30

3 years ago
Created attachment 8470242 [details] [diff] [review]
bug-362587-fix [v. 4] (Partial)

I've added some minor changes to the patch. I have tested for breakages following your method, and everything is fine except for the previously mentioned bug in Customize mode. However, I've discovered another bug where if Cmd+J is pressed when the download button menu panel is open, the Library window will open but focus quickly returns to the main window and then the menu panel closes. Perhaps there is a conflict between the two shortcuts?

Additionally, Tools->Downloads still behaves like Cmd+J although Opt+Cmd+L now replaces it in the menu bar listing. We could either change the functionality of the menu bar listing to match the behavior of Opt+Cmd+L or change the listing back to Cmd+J. I prefer the former since it behaves most like Safari when the download button is present and falls back to previous behavior if it is not present (Safari avoids the fallback problem because the downloads button can never be removed from the toolbar).
Attachment #8466402 - Attachment is obsolete: true
Attachment #8470242 - Flags: review?(gavin.sharp)
Thanks Nathan!

(In reply to Nathan Yee [:nyee] from comment #30)
> However, I've discovered another bug where
> if Cmd+J is pressed when the download button menu panel is open, the Library
> window will open but focus quickly returns to the main window and then the
> menu panel closes. Perhaps there is a conflict between the two shortcuts?

This sounds odd - your patch doesn't change anything about how Cmd+J works, and the new code to handle the new shortcut shouldn't run at all in that case, right?

> Additionally, Tools->Downloads still behaves like Cmd+J although Opt+Cmd+L
> now replaces it in the menu bar listing. We could either change the
> functionality of the menu bar listing to match the behavior of Opt+Cmd+L or
> change the listing back to Cmd+J. I prefer the former since it behaves most
> like Safari when the download button is present and falls back to previous
> behavior if it is not present (Safari avoids the fallback problem because
> the downloads button can never be removed from the toolbar).

I don't think we want to change the shortcut that is displayed in the menu, or the functionality of the menu item. We're not trying to match Safari's behavior exactly here, just cater to people who might be used to it with an additional (hidden) shortcut.
Comment on attachment 8470242 [details] [diff] [review]
bug-362587-fix [v. 4] (Partial)

Oh, I see the issue - you're reusing the key_openDownloads ID for the new key, which causes them to get confused. Just change the newly added one to a different ID (key_openDownloads2) and things should work fine!
Attachment #8470242 - Flags: review?(gavin.sharp) → review-
(Assignee)

Comment 33

3 years ago
Created attachment 8471001 [details] [diff] [review]
bug-362587-fix [v. 5] (Partial)

This patch preserves the original Cmd+J menu bar entry behavior and shortcut label while also fixing the Opt+Cmd+L bug while in Customize mode. The only problem remaining is the Library window/download button panel conflict.
Attachment #8470242 - Attachment is obsolete: true
Attachment #8471001 - Flags: review?(gavin.sharp)
(In reply to Nathan Yee [:nyee] from comment #33)
> The only problem remaining is the Library window/download button panel conflict.

You mean the first issue from comment 30? I don't see how that could possibly be caused by this patch, which doesn't affect any code related to Cmd+J. Are you sure you tested correctly?
(Assignee)

Comment 35

3 years ago
What I mean by conflict is that when the downloads button panel is open and Cmd+J is pressed, in a split second the Library window opens, then focus gets put on the browser window and the downloads button panel closes. Remember that all of this happens in a split second. I am sure I'm testing correctly and I've tried this patch after pulling the latest updates.
Does the same problem happen without your patch? As I mentioned, I don't see how it's possible for your patch to affect Cmd+J behavior.
Nathan: hoping for an answer to comment 36.
Assignee: jonnyscholes → ny.nathan.yee
Flags: needinfo?(ny.nathan.yee)
Hardware: x86 → All
Comment on attachment 8471001 [details] [diff] [review]
bug-362587-fix [v. 5] (Partial)

I shouldn't have suggested key_openDownloads2 - let's use "key_openDownloadsMac" instead.

I think this might need to handle a few other cases:
- does openPanelOrLibrary currently handle no browser windows being open?
- if we are customizing, openPanelOrLibrary should probably just cause the downloads library to appear, to match Cmd+J (as-is it looks like it does nothing)

Very close, thanks for being persistent on this as we sort out the details, Nathan!
Attachment #8471001 - Flags: review?(gavin.sharp) → review-
(Assignee)

Comment 39

3 years ago
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #36)
> Does the same problem happen without your patch? As I mentioned, I don't see
> how it's possible for your patch to affect Cmd+J behavior.

Without the patch, the downloads button panel closes and the Library window opens. I'm not sure what is causing the patch to not follow this behavior.
Flags: needinfo?(ny.nathan.yee)
(Assignee)

Comment 40

3 years ago
Created attachment 8473811 [details] [diff] [review]
bug-362587-fix [v. 6] (Partial)

This patch adds Opt+Cmd+L functionality to open the Library downloads window when in Customize mode. Opt+Cmd+L is non-functional when no windows are open (the OS X system bell sound effect plays when pressed). We still need to resolve the Download menu panel conflict.
Attachment #8471001 - Attachment is obsolete: true
Attachment #8473811 - Flags: review?(gavin.sharp)
(In reply to Nathan Yee [:nyee] from comment #35)
> What I mean by conflict is that when the downloads button panel is open and
> Cmd+J is pressed, in a split second the Library window opens, then focus
> gets put on the browser window and the downloads button panel closes.
> Remember that all of this happens in a split second. I am sure I'm testing
> correctly and I've tried this patch after pulling the latest updates.

OK, I can reproduce that problem in some cases (surprisingly, it seems to only happen on first run after creating a new profile). I can also reproduce it without your patch applied, though, so it sounds like a separate problem worth investigating independently.
Comment on attachment 8473811 [details] [diff] [review]
bug-362587-fix [v. 6] (Partial)

>diff --git a/browser/components/downloads/content/indicator.js b/browser/components/downloads/content/indicator.js

>+  openPanelOrLibrary: function() {

>+    if (isInToolbar) {
>+      if (!CustomizationHandler.isCustomizing()) {

You can just combine these two into a single condition, and remove the duplicated inner "else" that calls BrowserDownloadsUI();.

To handle the "no window case", you can also add the "window.location.href == getBrowserURL()" condition (to only run this panel branch if a browser window is open and focused).

r=me with that change.
Attachment #8473811 - Flags: review?(gavin.sharp) → review+
(Assignee)

Comment 43

3 years ago
Created attachment 8474202 [details] [diff] [review]
bug-362587-fix [v. 7]

Are these changes what you were expecting? Opt+Cmd+L still doesn't do anything and causes the OS X system beep when no browser windows are open. However, Cmd+J still opens the Library downloads window when no browser windows are open.
Attachment #8473811 - Attachment is obsolete: true
Attachment #8474202 - Flags: review?(gavin.sharp)
(Assignee)

Comment 44

3 years ago
(In reply to Nathan Yee [:nyee] from comment #43)
> Created attachment 8474202 [details] [diff] [review]
> bug-362587-fix [v. 7]
> 
> Are these changes what you were expecting? Opt+Cmd+L still doesn't do
> anything and causes the OS X system beep when no browser windows are open.
> However, Cmd+J still opens the Library downloads window when no browser
> windows are open.

A reminder for someone to look into this.
(In reply to Nathan Yee [:nyee] from comment #43)
> Created attachment 8474202 [details] [diff] [review]
> bug-362587-fix [v. 7]
> 
> Are these changes what you were expecting? Opt+Cmd+L still doesn't do
> anything and causes the OS X system beep when no browser windows are open.
> However, Cmd+J still opens the Library downloads window when no browser
> windows are open.

Hm, in Safari, Cmd+Opt+L opens a new browser window with the downloads panel when no window is open.
Since we have the library, it would make sense to open that in that case.
Sorry for the response lag Nathan :(

I agree with Philipp, we want the shortcut to do something when no window is open. I've been meaning to suggest how to do that exactly, but haven't had time to look into it.
(Assignee)

Comment 47

3 years ago
(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #46)
> Sorry for the response lag Nathan :(
> 
> I agree with Philipp, we want the shortcut to do something when no window is
> open. I've been meaning to suggest how to do that exactly, but haven't had
> time to look into it.

Do you have any advice on where we should look?
indicator.js is loaded in global-scripts.inc, so DownloadsIndicatorView.openPanelOrLibrary() should be getting called when using Cmd+Opt+L in the no-window-open case, with your patch. window.location.href won't be equal to getBrowserURL() in that case, so we should end up calling BrowserDownloadsUI(), and that should also work fine even if no windows are opened. Probably the best way to figure out what's going wrong is to validate which of these things aren't happening (either with the debugger or with dump()/console.log()-debugging).
Attachment #8474202 - Flags: review?(gavin.sharp)
(Assignee)

Comment 49

3 years ago
We should also prevent Cmd+Opt+L from opening the downloads panel when the customize panel is open.
(Assignee)

Comment 50

3 years ago
I get no debug output when doing Cmd+Opt+L in the no-window-open case; the only response is the OS beep. Any ideas on what we should do next?
Hmm, I think the issue is that with no associated menu item, the <xul:key> doesn't work in hiddenWindow.xul. Neil/Jared, do you know if there's a way around that?
Flags: needinfo?(jaws)
Flags: needinfo?(enndeakin)
Yeah, we need a menuitem to get the <xul:key> to work in the hidden window. If the menuitem has hidden="true" then the command won't get executed. I tried removing the hidden="true" and adding class="show-only-for-keyboard" but the key-combo didn't open the Downloads panel from the hidden window with that. I'll keep trying to think of other work-arounds.
If there is no easy way to make a no-menuitem shortcut key work on Mac, we can just split that off to a new bug. We don't need to block all of this work on that particular issue.

Comment 54

3 years ago
I suspect that performKeyEquivalent is returning false as there isn't a real cocoa window/view/responder to send it to, but Masayuki might know more.
Flags: needinfo?(enndeakin) → needinfo?(masayuki)
I don't know more the detail of Cocoa key event model.

Don't we receive keydown event for that?
http://mxr.mozilla.org/mozilla-central/source/widget/cocoa/nsChildView.mm?rev=71b13c6e679a#5237

Anyway, Steven Michaud might know something more.

And also I agree with comment 53.
Flags: needinfo?(masayuki) → needinfo?(smichaud)
I don't whether or not it's reasonable to try to make a no-menuitem shortcut key work on the Mac.  But I agree with Masayuki that all such (native) events probably get at least as far as the following line:

https://hg.mozilla.org/mozilla-central/annotate/206205dd8bd1/widget/cocoa/nsChildView.mm#l5245

So whoever wants to explore this possibility needs to add an NSLog() statement there, and also in (at least) the following locations:

https://hg.mozilla.org/mozilla-central/annotate/206205dd8bd1/widget/cocoa/nsMenuBarX.mm#l861
https://hg.mozilla.org/mozilla-central/annotate/206205dd8bd1/widget/cocoa/nsMenuItemX.mm#l152
Flags: needinfo?(smichaud)
OK, so let's split off handling of the "no window" case to a new bug.

Nathan: if you want to file that new bug, that would be great. Your patch should also be adjusted to remove the " window.location.href == getBrowserURL()" check, since we can add that in in the new bug.
Flags: needinfo?(jaws)
(Assignee)

Comment 58

3 years ago
Created attachment 8546321 [details] [diff] [review]
bug-362587-fix.patch
Attachment #8474202 - Attachment is obsolete: true
Attachment #8546321 - Flags: review?(gavin.sharp)
(Assignee)

Comment 59

3 years ago
The above patch removes the "window.location.href == getBrowserURL()" check. On another note, what should I describe when I create the new bug report?
Comment on attachment 8546321 [details] [diff] [review]
bug-362587-fix.patch

Thanks Nathan!
Attachment #8546321 - Flags: review?(gavin.sharp) → review+
(In reply to Nathan Yee [:nyee] from comment #59)
> On another note, what should I describe when I create the new bug report?

The summary should be "Make Cmd+Opt+L shortcut work even when no Firefox windows are open", and then you can just have the description refer to this bug for context.
(Assignee)

Comment 62

3 years ago
Here is the link to the new bug report: https://bugzilla.mozilla.org/show_bug.cgi?id=1120035. Gavin can you confirm this bug?

Comment 63

5 months ago
Created attachment 8858160 [details] [diff] [review]
Unbitrotted Patch

The patch had a couple of conflicts, I have resolved them.
Comment on attachment 8858160 [details] [diff] [review]
Unbitrotted Patch

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

LGTM.

Once you upload a new patch with the nit fixed, I'll mark this checkin-needed.

Also, please update the commit message to indicate the reviewers:

Bug 362587 - [Mac] Add Cmd-Opt-L shortcut for Downloads, to match Safari and Adium. r=gavin,nhnt11

::: browser/base/content/browser-sets.inc
@@ +271,4 @@
>  #else
>      <key id="goBackKb" keycode="VK_LEFT" command="Browser:Back" modifiers="accel" />
>      <key id="goForwardKb" keycode="VK_RIGHT" command="Browser:Forward" modifiers="accel" />
> +#endif`

Nit: There's a stray backquote at the end of this line.
Attachment #8858160 - Flags: review+

Comment 65

5 months ago
Created attachment 8858653 [details] [diff] [review]
unbitrotted patch v2

Fixed the nit
(In reply to Sanath Shetty from comment #65)
> Created attachment 8858653 [details] [diff] [review]
> unbitrotted patch v2
> 
> Fixed the nit

Thanks! This probably deserves an automated test, so I'm going to hold off on checkin-needed for now.
You need to log in before you can comment on or make changes to this bug.