344736 - Add a keyboard shortcut for Undo Close Tab

Status: RESOLVED FIXED

(Firefox :: General, enhancement)

Description

[Simon Bünzli]

The most recently closed tab should be reopenable through a shortcut such as Ctrl+Shift+Z, Ctrl+Shift+T or whatever would fit. The shortcut key should be displayed next to the first item in the popup menu.

Currently the keyboard way is "Alt+I -> R -> Enter" (where BTW "R" is only an implicit accesskey which should be made explicit).

Comment 1

[Michael Ventnor]

Attachment: Adds keyboard shortcut

Ctrl+Shift+Z except on *nix where its Ctrl+Shift+T, since the former is taken by Redo. This particular trunk patch isn't tested, but I have made very similar modifications to my Fx2b1 and had it working.

Comment 2

[Michael Ventnor]

As for displaying the shortcut key next to the first item, the shortcut keys for each menu item appear to be set in stone on startup. I can't change them with setAttribute. I'll probably file a bug for this later.

Comment 3

[Michael Ventnor]

(In reply to comment #2)
> As for displaying the shortcut key next to the first item, the shortcut keys
> for each menu item appear to be set in stone on startup. I can't change them
> with setAttribute. I'll probably file a bug for this later.
> 
By that I mean what is displayed beside each menu item, i.e. the "key" attribute for <menuitem> nodes.

Comment 4

[Michael Ventnor]

Attachment: Adds shortcuts v2

After testing I found out that Ctrl+Shift+Z doesn't work when a text field is selected, so now its Ctrl+Shift+T for all platforms. Also fixed a bug I hadn't noticed, all unassigned Ctrl+Shift+* became the undo tab close key, all because I didn't notice the difference between "key" and "keycode". :P

Comment 5

[Michael Ventnor]

Attachment: Adds shortcuts v2 re-release

Whoopsie, typo in cvs command caused a lot more lines to be printed in the patch than anticipated. Yeah yeah, I know I'm hopeless. Don't worry, I'll improve soon. :)

Comment 6

[Simon Bünzli]

Looks good. A few details:
* Make sure not to include tabs in your patches. Mozilla's devs instist on pure spaces.
* I'm not sure it's such a good idea to not allow localizers to use a different key for Undo Close Tab than just Shift+New Tab.
* If you introduce History:UndoCloseTab, please make sure that the tab context menu also uses that command (instead of the command event listener currently used).

Mike: Any suggestion on what a good shortcut for this would be (or who could tell)?

Comment 7

[Asaf Romano (gone)]

(In reply to comment #0)

> Currently the keyboard way is "Alt+I -> R -> Enter" (where BTW "R" is only an
> implicit accesskey which should be made explicit).
> 

It shouldn't, as that would block letter-access to pages starting with "R" in the History menu (see bug 308812)

Comment 8

[Asaf Romano (gone)]

Comment on attachment 229341 [details] [diff] [review]
Adds shortcuts v2 re-release

Accel+Q won't work on OS X. 

Also
 1) set the "key" attribute outside of the loop.
 2) No hard-tabs (use spaces instead).

Comment 9

[Michael Ventnor]

Attachment: Adds shortcuts v3

Addresses everyone's feedback. The key is now under historyUndoCmd.key in browser.dtd and is set to "t" by default.

Comment 10

[:Gavin Sharp [email: [e-mail]]]

Michael, why did you change the review request? You need to set a requestee if you want the patch to be reviewed.

Comment 11

[Michael Ventnor]

(In reply to comment #10)
> Michael, why did you change the review request? You need to set a requestee if
> you want the patch to be reviewed.
> 

Whoah how did that happen? Sorry about that, reviewee -> you.

Comment 12

[Johannes Buchner]

Hmm, why did you put it on Ctrl-Shift-T? 
Ctrl-Shift-F4 would make more sense if you close the tab with Ctrl-F4!? 
Would that be a problem on Macs?

Or you could define it as "always the same as new tab, but with shift"

Comment 13

[Michael Ventnor]

(In reply to comment #12)
> Hmm, why did you put it on Ctrl-Shift-T? 
> Ctrl-Shift-F4 would make more sense if you close the tab with Ctrl-F4!? 
> Would that be a problem on Macs?
> 
> Or you could define it as "always the same as new tab, but with shift"
> 

Ctrl+Shift+T is easier to remember because the T is synonymous with tab shortcuts. It was also one of the reporter's suggestions. I haven't heard of that F4 shortcut until now.

Originally I did map it as Shift+New tab, but comment #6 recommended otherwise.

Comment 14

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 229346 [details] [diff] [review]
Adds shortcuts v3

>Index: browser/base/content/browser-sets.inc

>+    <command id="History:UndoCloseTab" oncommand="undoCloseTab(0);"/>

If you're adding a command, you should probably ensure that it's only enabled at the correct time - nsISessionStore::undoCloseTab never throws, but that's only because of bug 287107.

>+    <key id="key_undoCloseTab" command="History:UndoCloseTab" key="&historyUndoCmd.key;" modifiers="accel,shift"/>

This should use tabCmd.commandkey, it shouldn't be different per-locale. I'm not sure why comment 6 led you to believe otherwise.

>Index: browser/base/content/browser.js

>+  if(undoPopup.hasChildNodes())
>+    undoPopup.childNodes[0].setAttribute("key", "key_undoCloseTab");

Why do you need to set the key attribute on the first child node? The key element is in the global scope, and it's already tied to undoCloseTab(0). Seems to me like you can just remove this.

All that being said, I'm not a browser peer - see http://www.mozilla.org/projects/firefox/review.html for the list of valid reviewers.

Comment 15

[Michael Ventnor]

Attachment: Adds shortcuts v4

Thanks for the comments. I implemented the "key" attribute because the reporter requested the shortcut be displayed next to the first item in the undo menu, but it doesn't seem to work anyway.

Comment 16

[Michael Ventnor]

Attachment: Adds shortcuts v4.1

Just a small change. This should be the last patch for this bug. After all, this has all been over a keyboard shortcut. Nevertheless, I've learned a lot about Mozilla source code these past few days.

Comment 17

[TimoT]

My tree isn't clean but as far as I can tell this patch breaks the history -> recently closed tabs menu and the tabbar context menu item (they'll always stay disabled).

Comment 18

[Michael Ventnor]

> My tree isn't clean but as far as I can tell this patch breaks the history ->
> recently closed tabs menu and the tabbar context menu item (they'll always stay
> disabled).
> 

WFM. In order to address Gavin's first recommendation I separated the closed tabs count check into its own function so that the key can be easily disabled without unnecessarily getting into disabled attributes. Maybe the function didn't get put into your browser.js? Anything in Error Console?

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060714 Minefield/3.0a1

Comment 19

[Michael Ventnor]

Attachment: Adds shortcuts v4.2

Ah I figured out the problem. The updated browser.js conflicted with the patch. This one should work now. I also added improved exception handling. Somebody please get Mr Connor in here ASAP?

Comment 20

[Simon Bünzli]

One more nit:
* The argument to getClosedTabCount should read aWindow (or aWin) instead of win. OTOH, there's actually no need for that argument at all - simply use window as undoCloseTab above.

And why not make sure that we can actually reopen a closed tab in undoCloseTab itself where you've already got the SessionStore service? This would slightly less pollute the global namespace and allow other callers the same benefit.

(In reply to comment #19)
> Somebody please get Mr Connor in here ASAP?

He'll come soon enough. Alternatively, request review from [e-mail] who is the author of the original code.

Comment 21

[Michael Ventnor]

Attachment: Adds shortcuts v4.3

Thanks for the comments, I have removed the closed tab counting function and instead moved the count to within undoCloseTab. This has been too much fuss over adding a keyboard shortcut and I just want to get it checked in.

Comment 22

[Simon Bünzli]

Thanks for your efforts on this bug. Now, with the risk of annoying you, there are two more nits:

> +  if(ss.getClosedTabCount(window) == 0)
> +    return false;

* There should be a space after the if.
* Don't return anything (i.e. use "return;" instead of "return false;") as otherwise you'll trigger the strict warning "function undoCloseTab does not always return a value".

Looks good otherwise.

Comment 23

[Michael Ventnor]

Attachment: Adds shortcuts v4.4

*Sigh*

Comment 24

[Michael Ventnor]

I just found out something, Opera's undo close page shortcut is Ctrl+Alt+Z. Would this make a better keyboard shortcut or should we stick with Ctrl+Shift+T?

Comment 25

[Simon Bünzli]

(In reply to comment #24)
> Opera's undo close page shortcut is Ctrl+Alt+Z.

Ctrl+Alt+<key> is used for OS wide shortcuts under Windows and are thus no option (BTW: copying UI from Opera is usually a bad idea because it violates dozens of platform guidelines).

Comment 26

[TimoT]

+    <key id="key_undoCloseTab" command="History:UndoCloseTab" key="&tabCmd.commandkey;" modifiers=accel,shift"/>

should be modifiers="accel,shift"/> ;)

Comment 27

[Michael Ventnor]

Attachment: Adds shortcuts v4.5

You're kidding me. Oh well, once again I make a fix.

Comment 28

[Dietrich Ayala (:dietrich)]

Comment on attachment 229919 [details] [diff] [review]
Adds shortcuts v4.5

This looks good and works as advertised. Thanks for your contribution and your patience Michael. r=me

Comment 29

[Dietrich Ayala (:dietrich)]

Comment on attachment 229919 [details] [diff] [review]
Adds shortcuts v4.5

Problem: No mouse-less way to close the most recently closed tab.

Solution: Add a command and key for calling undoCloseTab().

Risk: Low. Baked on trunk for a couple of days with no problems. The only issue I've noticed is that the shortcut doesn't work with some XUL windows (eg: chrome://global/content/console.xul) when they are the currently selected tab, and focus is in the content window. But this is not a common use-case, so I don't think this is a scenario we need to handle at this point.

Comment 30

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 229919 [details] [diff] [review]
Adds shortcuts v4.5

a=drivers. Please land on the 1.8.1 branch.

Comment 31

[Dietrich Ayala (:dietrich)]

fix confirmed in tinderbox build