1441658 - Firefox's main menu should expose the exit shortcut on Windows, too

Status: RESOLVED FIXED

(Firefox :: Menus, enhancement, P5)

Description

[:Gijs (out until Jan 5th; he/him)]

We expose the shortcut for Exit on Windows (Ctrl-Shift-Q) in the hamburger menu, but not in the 'old' menu.

This can be fixed by ensuring the old menu's menuitem has a 'key' attribute. To do this, we need to remove the `ifdef XP_UNIX` and `endif` bits in this block:

https://dxr.mozilla.org/mozilla-central/rev/580d833df9c44acec686a9fb88b5f27e9d29f68d/browser/base/content/browser-menubar.inc#107-109

and just always include the `key` attribute.

Comment 1

[Souvik Mandal]

I want to work on this bug. What I understand, I only have to remove "#ifdef XP_UNIX" and "#endif" from attachment file.Is there anything else?

Comment 2

[:Gijs (out until Jan 5th; he/him)]

(In reply to Souvik Mandal from comment #1)
> I want to work on this bug.

Great!

> What I understand, I only have to remove "#ifdef
> XP_UNIX" and "#endif" from attachment file.Is there anything else?

Check that it works, and attach the patch or review request to this bug. That's all! :-)

Comment 3

[Alex]

Hey, can I work on this bug?

Comment 4

[:Gijs (out until Jan 5th; he/him)]

(In reply to Alex from comment #3)
> Hey, can I work on this bug?

Looks like Souvik is working on bug 1441892 instead, so I think that would be OK.

Comment 5

[grimlockmobile]

Testing now. Running build with changes and if everything works ill upload the patch for review. You can assign it to me for now. if not ill do some more testing.

Comment 6

[grimlockmobile]

Attachment: browser-menubar.inc

commit to fix close option in legacy menu

Comment 7

[:Gijs (out until Jan 5th; he/him)]

Comment on attachment 8957855 [details] [diff] [review]
browser-menubar.inc

This seems to just be a file, not a diff, and it has no commit message. Can you attach an exported commit (ie patch, with commit message) or push to mozreview? See e.g. https://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview/commits.html and https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch#Creating_a_patch

Comment 8

[Jake Nixon]

Is anyone still working on this bug?

If not then I'd be interested in taking it on.

Comment 9

[:Gijs (out until Jan 5th; he/him)]

(In reply to Jake Nixon [:wightsnowolf] from comment #8)
> Is anyone still working on this bug?

It's been 3 weeks, so I don't think so...

> If not then I'd be interested in taking it on.

Feel free to upload a patch, let me know if you need more details than what is in the bug right now. :-)

Comment 10

[Jake Nixon]

Attachment: Bug 1441658 Change 'old' menu to show exit shortcut

Comment 11

[:Gijs (out until Jan 5th; he/him)]

Comment on attachment 8964729 [details] [diff] [review]
Bug 1441658 Change 'old' menu to show exit shortcut

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

This looks good, can you commit the change and then export it so it includes a commit message?

Something like:

Bug 1441658 - also show shortcut on the 'Exit' entry on Windows, r=gijs

would work well as a commit message (it generally needs to include bug number, a description and the reviewer(s))

Comment 12

[Jake Nixon]

Attachment: Patch to also show shortcut on the 'Exit' entry on Windows, r=gijs

Comment 13

[:Gijs (out until Jan 5th; he/him)]

Comment on attachment 8964808 [details] [diff] [review]
Patch to also show shortcut on the 'Exit' entry on Windows, r=gijs

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

Carrying over r+ :-)

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd1c3d575c53
also show shortcut on the 'Exit' entry on Windows, r=gijs

Comment 15

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/bd1c3d575c53