Add a keyboard shortcut for Undo Close Tab

RESOLVED FIXED in Firefox 2 beta2

Status

()

enhancement
RESOLVED FIXED
13 years ago
13 years ago

People

(Reporter: zeniko, Assigned: ventnor.bugzilla)

Tracking

({fixed1.8.1})

unspecified
Firefox 2 beta2
Points:
---
Bug Flags:
blocking-firefox2 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 9 obsolete attachments)

Reporter

Description

13 years ago
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).
Assignee

Comment 1

13 years ago
Posted patch Adds keyboard shortcut (obsolete) — Splinter Review
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.
Assignee

Comment 2

13 years ago
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.
Assignee

Comment 3

13 years ago
(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.
Assignee

Updated

13 years ago
Attachment #229332 - Flags: review?(myk)
Assignee

Updated

13 years ago
Attachment #229332 - Flags: review?(myk) → review?(mconnor)
Assignee

Updated

13 years ago
Attachment #229332 - Flags: review?(mconnor)
Assignee

Comment 4

13 years ago
Posted patch Adds shortcuts v2 (obsolete) — Splinter Review
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
Attachment #229332 - Attachment is obsolete: true
Attachment #229340 - Flags: review?(mconnor)
Assignee

Updated

13 years ago
Attachment #229340 - Flags: review?(mconnor)
Assignee

Comment 5

13 years ago
Posted patch Adds shortcuts v2 re-release (obsolete) — Splinter Review
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. :)
Attachment #229340 - Attachment is obsolete: true
Attachment #229341 - Flags: review?(mconnor)
Reporter

Comment 6

13 years ago
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)?
Keywords: uiwanted
(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 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).
Attachment #229341 - Flags: review?(mconnor) → review-
Assignee

Comment 9

13 years ago
Posted patch Adds shortcuts v3 (obsolete) — Splinter Review
Addresses everyone's feedback. The key is now under historyUndoCmd.key in browser.dtd and is set to "t" by default.
Attachment #229341 - Attachment is obsolete: true
Assignee

Updated

13 years ago
Attachment #229346 - Flags: review?
Assignee

Updated

13 years ago
Attachment #229346 - Flags: review? → review?(bugs.mano)
Assignee

Updated

13 years ago
Attachment #229346 - Flags: review?(bugs.mano) → review?(mconnor)
Assignee

Updated

13 years ago
Attachment #229346 - Flags: review?(mconnor) → review?
Michael, why did you change the review request? You need to set a requestee if you want the patch to be reviewed.
Assignee

Updated

13 years ago
Attachment #229346 - Flags: review? → review?(gavin.sharp)
Assignee

Comment 11

13 years ago
(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

13 years ago
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"
Assignee

Comment 13

13 years ago
(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 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.
Attachment #229346 - Flags: review?(gavin.sharp) → review-
Assignee

Comment 15

13 years ago
Posted patch Adds shortcuts v4 (obsolete) — Splinter Review
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.
Attachment #229346 - Attachment is obsolete: true
Assignee

Comment 16

13 years ago
Posted patch Adds shortcuts v4.1 (obsolete) — Splinter Review
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.
Attachment #229471 - Attachment is obsolete: true
Assignee

Updated

13 years ago
Attachment #229475 - Flags: review?(mconnor)

Comment 17

13 years ago
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).
Assignee

Comment 18

13 years ago
> 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
Assignee

Comment 19

13 years ago
Posted patch Adds shortcuts v4.2 (obsolete) — Splinter Review
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?
Attachment #229475 - Attachment is obsolete: true
Attachment #229791 - Flags: review?(mconnor)
Attachment #229475 - Flags: review?(mconnor)
Reporter

Comment 20

13 years ago
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 dietrich@mozilla.com who is the author of the original code.
Target Milestone: --- → Firefox 2 beta2
Assignee

Comment 21

13 years ago
Posted patch Adds shortcuts v4.3 (obsolete) — Splinter Review
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.
Attachment #229791 - Attachment is obsolete: true
Attachment #229793 - Flags: review?(dietrich)
Attachment #229791 - Flags: review?(mconnor)
Reporter

Comment 22

13 years ago
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.
Assignee

Comment 23

13 years ago
Posted patch Adds shortcuts v4.4 (obsolete) — Splinter Review
*Sigh*
Attachment #229793 - Attachment is obsolete: true
Attachment #229798 - Flags: review?(dietrich)
Attachment #229793 - Flags: review?(dietrich)
Assignee

Comment 24

13 years ago
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?
Reporter

Comment 25

13 years ago
(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

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

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

Comment 27

13 years ago
You're kidding me. Oh well, once again I make a fix.
Attachment #229798 - Attachment is obsolete: true
Attachment #229919 - Flags: review?(dietrich)
Attachment #229798 - Flags: review?(dietrich)
Reporter

Updated

13 years ago
Assignee: nobody → ventnors_dogs234
Flags: blocking-firefox2?
Whiteboard: [has patch][needs review dietrich]
Flags: blocking-firefox2? → blocking-firefox2+
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
Attachment #229919 - Flags: review?(dietrich) → review+
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Keywords: uiwanted
Resolution: --- → FIXED
Whiteboard: [has patch][needs review 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.
Attachment #229919 - Flags: approval1.8.1?
Comment on attachment 229919 [details] [diff] [review]
Adds shortcuts v4.5

a=drivers. Please land on the 1.8.1 branch.
Attachment #229919 - Flags: approval1.8.1? → approval1.8.1+
fix confirmed in tinderbox build
Keywords: fixed1.8.1
You need to log in before you can comment on or make changes to this bug.