Closed Bug 357235 Opened 13 years ago Closed 12 years ago

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

Categories

(Firefox :: Tabbed Browser, defect)

2.0 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 3 beta1

People

(Reporter: dietrich, Assigned: ventnor.bugzilla)

Details

(Whiteboard: PRD:TAB-003d)

Attachments

(1 file, 3 obsolete files)

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).
Attached patch Patch (obsolete) — Splinter Review
This patch does exactly that.
Attachment #242744 - Flags: review?(dietrich)
(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 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.
Attachment #242744 - Flags: review?(dietrich) → review?(zeniko)
Attached patch Patch w/o tabs (obsolete) — Splinter Review
Crap, I hate it how Notepad++ fills in tabs for you. Anyone know how to change it?
Attachment #242744 - Attachment is obsolete: true
Attachment #242805 - Flags: review?(zeniko)
Attachment #242744 - Flags: review?(zeniko)
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).
Attachment #242805 - Flags: review?(zeniko)
Attachment #242805 - Flags: review?(mano)
Attachment #242805 - Flags: review+
Status: NEW → ASSIGNED
Assignee: nobody → ventnor.bugzilla
Status: ASSIGNED → NEW
Status: NEW → ASSIGNED
Attached patch Patch v2 (obsolete) — Splinter Review
There have been changes to the section which affects this patch (read: I broke my own patch with bug 344140). Updated the patch.
Attachment #242805 - Attachment is obsolete: true
Attachment #243303 - Flags: review?(mano)
Attachment #242805 - Flags: review?(mano)
Comment on attachment 243303 [details] [diff] [review]
Patch v2

r=mano code-wise.
Attachment #243303 - Flags: ui-review?(beltzner)
Attachment #243303 - Flags: review?(mano)
Attachment #243303 - Flags: review+
Whiteboard: [needs ui-review beltzner]
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 :)
Attachment #243303 - Flags: ui-review?(beltzner) → ui-review+
Attached patch For checkinSplinter Review
Wow, its been almost a year since the last patch.
Attachment #243303 - Attachment is obsolete: true
Keywords: checkin-needed
Whiteboard: [needs ui-review beltzner]
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.
Attachment #281569 - Flags: approval1.9?
Please re-request checkin-needed once approval1.9 has been granted.
Keywords: checkin-needed
Attachment #281569 - Flags: approval1.9? → approval1.9+
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
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
OS: Mac OS X → All
Hardware: PC → All
Whiteboard: PRD:TAB-003d
Target Milestone: --- → Firefox 3 M9
You need to log in before you can comment on or make changes to this bug.