342169 - Disable "Undo Close Tab" if no tabs to restore

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Dietrich Ayala (:dietrich)]

If there are no tabs to restore, disable this option in the tab context menu. See discussion at 254021 for more discussion.

Comment 1

[Dietrich Ayala (:dietrich)]

Attachment: hides menu

This hides the menu, per mconnor's request. However, Simon's argument for discoverability makes sense IMO. I don't think having a disabled context menu item on tabs would be terribly discordant with our context-menu practices elsewhere, as the content window context menu has disabled items.

Comment 2

[Simon Bünzli]

Comment on attachment 226563 [details] [diff] [review]
hides menu

>Index: base/content/browser.js
>+      AugmentTabs.undoCloseTabMenu.hidden = (ss.getClosedTabCount(window) > 0) ? false : true;

Why convert a boolean to a boolean? Why not:
....disabled = ss.getClosedTabCount(window) <= 0; 

And as I already mentioned (and you acknowledged), items should really only be hidden depending on the direct context, otherwise they should just be disabled.

>Index: components/sessionstore/src/nsSessionStore.js
>+    if (tabState && tabState.Entry[0].url != "about:blank") {

Are you sure that there's no possibility that about:blank is just the first entry in the tab's history? If not, please also check for tabState.Entry.length == 1.

Comment 3

[Mike Connor [:mconnor]]

Comment on attachment 226563 [details] [diff] [review]
hides menu


>+  onTabContextMenuLoad: function at_onTabContextMenuLoad() {
>+    if (AugmentTabs.undoCloseTabMenu) {
>+      // only add the menu of there are tabs to restore
>+      var ss = Cc["@mozilla.org/browser/sessionstore;1"].
>+               getService(Ci.nsISessionStore);
>+      AugmentTabs.undoCloseTabMenu.hidden = (ss.getClosedTabCount(window) > 0) ? false : true;
>+    }

just do:

AugmentTabs.undoCloseTabMenu.hidden = !(ss.getClosedTabCount(window) > 0);

otherwise, r=me

Comment 4

[Dietrich Ayala (:dietrich)]

Checked in on trunk.

Comment 5

[Bryan]

> And as I already mentioned (and you acknowledged), items should really only be
> hidden depending on the direct context, otherwise they should just be disabled.

Isn't this inconsistent with the current behavior of FF where actions are "disabled" instead of removed/hidden if the action cannot be performed?  Why introduce another inconsistency?  With FF 2.0 there have been a number of "new" inconsistencies introduced throughout the UI that just don't make sense...

~B