Closed Bug 344736 Opened 14 years ago Closed 14 years ago

Add a keyboard shortcut for Undo Close Tab

Categories

(Firefox :: General, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 2 beta2

People

(Reporter: zeniko, Assigned: ventnor.bugzilla)

Details

(Keywords: fixed1.8.1)

Attachments

(1 file, 9 obsolete files)

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).
Attached 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.
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.
(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.
Attachment #229332 - Flags: review?(myk)
Attachment #229332 - Flags: review?(myk) → review?(mconnor)
Attachment #229332 - Flags: review?(mconnor)
Attached 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)
Attachment #229340 - Flags: review?(mconnor)
Attached 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)
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-
Attached 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
Attachment #229346 - Flags: review?
Attachment #229346 - Flags: review? → review?(bugs.mano)
Attachment #229346 - Flags: review?(bugs.mano) → review?(mconnor)
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.
Attachment #229346 - Flags: review? → review?(gavin.sharp)
(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.
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"
(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-
Attached 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
Attached 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
Attachment #229475 - Flags: review?(mconnor)
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).
> 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
Attached 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)
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
Attached 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)
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.
Attached 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)
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?
(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).
+    <key id="key_undoCloseTab" command="History:UndoCloseTab" key="&tabCmd.commandkey;" modifiers=accel,shift"/>

should be modifiers="accel,shift"/> ;)
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)
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
Closed: 14 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.