357235 - The "undo close tab" keyboard shortcut should be more discoverable

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Dietrich Ayala (:dietrich)]

Adding the shortcut to the first entry in the Recently Closed Tabs menu is a possible solution (via https://bugzilla.mozilla.org/show_bug.cgi?id=345568#c1).

Comment 1

[Michael Ventnor]

Attachment: Patch

This patch does exactly that.

Comment 2

[Simon Bünzli]

(In reply to comment #1)
> Created an attachment (id=242744) [edit]

> -    var m = undoPopup.appendChild(document.createElement("menuitem"));
> +    var m = document.createElement("menuitem");

What's wrong with adding the menuitem right after creation?

> +	undoPopup.appendChild(m);

Please watch your tabs.

Does this work correctly for you? I thought that we had already tried this but it failed for whatever reason...

Comment 3

[Michael Ventnor]

Comment on attachment 242744 [details] [diff] [review]
Patch

(In reply to comment #2)
> (In reply to comment #1)
> > Created an attachment (id=242744) [edit]
> 
> > -    var m = undoPopup.appendChild(document.createElement("menuitem"));
> > +    var m = document.createElement("menuitem");
> 
> What's wrong with adding the menuitem right after creation?
> 
> > +	undoPopup.appendChild(m);
> 
> Please watch your tabs.
> 
> Does this work correctly for you? I thought that we had already tried this but
> it failed for whatever reason...
> 

Nope. My testing showed that the key attribute could not be changed after the menu item was appended. However, by separating the two functions, it still works correctly and the key attribute could still be applied.

Comment 4

[Michael Ventnor]

Attachment: Patch w/o tabs

Crap, I hate it how Notepad++ fills in tabs for you. Anyone know how to change it?

Comment 5

[Simon Bünzli]

Comment on attachment 242805 [details] [diff] [review]
Patch w/o tabs

Works as expected. It does however look slightly strange when there's hardly any space between the most recently closed tab's title and Ctrl+Shift+T -- which happens when only one tabs has been closed or when it's got the longest title from the list. Might still be worth taking though (at least for further evaluation on Trunk).

Comment 6

[Michael Ventnor]

Attachment: Patch v2

There have been changes to the section which affects this patch (read: I broke my own patch with bug 344140). Updated the patch.

Comment 7

[Asaf Romano (gone)]

Comment on attachment 243303 [details] [diff] [review]
Patch v2

r=mano code-wise.

Comment 8

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

Comment on attachment 243303 [details] [diff] [review]
Patch v2

Let's try this for now. It might look awkward, but I'm not sure the other alternative (adding an "Open last closed tab" command to the History menu beneath the "Recently Closed Tabs" submenu) is any less awkward :)

Comment 9

[Michael Ventnor]

Attachment: For checkin

Wow, its been almost a year since the last patch.

Comment 10

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 281569 [details] [diff] [review]
For checkin

This has been sitting around too long.  It's a simple, obvious fix, and we should get it in.  Requesting approval to land this for M9.  If approved, I'll check it in and then take responsibility for dealing with regressions.

Comment 11

[Reed Loden [:reed]]

Please re-request checkin-needed once approval1.9 has been granted.

Comment 12

[Myk Melez [:myk] [@mykmelez]]

Checking in browser/base/content/browser.js;
/cvsroot/mozilla/browser/base/content/browser.js,v  <--  browser.js
new revision: 1.845; previous revision: 1.844
done