580956 - 'Undo Close Tab' should be disabled not hidden when there are none to undo

Status: VERIFIED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Frank Yan (:fryn)]

Attachment: patch v1

The 'Undo Close Tab' tab context menu item is currently hidden when there are no tabs to restore. This is inconsistent with the rest of the tab context menu items, which are disabled when unavailable due to state.

This is not comparable to the hiding of menu items in the content area context menu, since that involving the filtering of menu items based on the TYPE of the node, which is an implementation detail that should not interfere with the conceptual model presented to the user.

Comment 1

[Dão Gottwald [:dao]]

You need to request approval2.0 on the patch before this can land.

Comment 2

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/276f149e29e0

Comment 3

[[not reading bugmail]]

I see the option and its always in an active state., but its not in a disabled state and I tried to undo all my tabs, still not seeing it switch to inactive though.  

Also tested also a clean profile, the homepage opens and the option shows its active, not disabled like the other menu options.

Comment 4

[Frank Yan (:fryn)]

Attachment: take two (do what patch v1 was supposed to do)

Patch v1 caused a regression, because I briefly forgot that <menuitem command/>'s disabled state was actually controlled by its <command/>.

This fixes the problem.

Comment 5

[Dão Gottwald [:dao]]

I backed out the first patch: http://hg.mozilla.org/mozilla-central/shortlog

Comment 6

[Dão Gottwald [:dao]]

(In reply to comment #4)
> Patch v1 caused a regression, because I briefly forgot that <menuitem
> command/>'s disabled state was actually controlled by its <command/>.

Why would setting 'disabled' on an observing node fail when the broadcasting node doesn't have and never had that attribute?

Comment 7

[Frank Yan (:fryn)]

(In reply to comment #6)
> (In reply to comment #4)
> > Patch v1 caused a regression, because I briefly forgot that <menuitem
> > command/>'s disabled state was actually controlled by its <command/>.
> 
> Why would setting 'disabled' on an observing node fail when the broadcasting
> node doesn't have and never had that attribute?

Apparently, that's how XUL commands work:
https://developer.mozilla.org/en/XUL_Tutorial/Broadcasters_and_Observers#Command_Attribute_Forwarding

Comment 8

[Frank Yan (:fryn)]

Attachment: take 2 (updated to tip of tree)

updated, since patch v1 was backed out.

Comment 9

[Dão Gottwald [:dao]]

(In reply to comment #7)
> Apparently, that's how XUL commands work:

<command> and command="" are just aliases for <broadcaster> and observes="", I think.

> https://developer.mozilla.org/en/XUL_Tutorial/Broadcasters_and_Observers#Command_Attribute_Forwarding

There it says: "In addition, if you place the disabled  attribute on the command  element, any elements hooked up to it will also become disabled automatically."

The 'disabled' attribute doesn't appear to be set on the History:UndoCloseTab command element.

Comment 10

[Frank Yan (:fryn)]

(In reply to comment #9)

I know what it says, and the documentation doesn't explicitly cover the case of not setting a disabled attribute on a command. Patch v1 still didn't work, so it seems that the lack of the disabled attribute on a command will still override the disabled state of corresponding menu items. You can experiment with it too.

Comment 11

[Dão Gottwald [:dao]]

(In reply to comment #10)
> Patch v1 still didn't work, so
> it seems that the lack of the disabled attribute on a command will still
> override the disabled state of corresponding menu items.

At which point exactly would it override it?

Comment 12

[Frank Yan (:fryn)]

At this point (during onpopupshowing):
http://mxr.mozilla.org/mozilla-central/source/layout/xul/base/src/nsXULPopupManager.cpp#1547

(For future reference, if the line numbers have shifted, look for the method UpdateMenuItems.)

Comment 13

[Dão Gottwald [:dao]]

As a workaround, would "observes" instead of "command" work?

Btw, since the first attempt failed, drivers will probably want to see an automated test for this.

Comment 14

[Frank Yan (:fryn)]

Attachment: take 3 (uses observes attribute and includes test)

"observes" works. Thanks.

Comment 15

[Dão Gottwald [:dao]]

Comment on attachment 461160 [details] [diff] [review]
take 3 (uses observes attribute and includes test)

>+++ b/browser/base/content/test/browser_bug580956.js

>+function isUndoCloseEnabled() {
>+    document.popupNode = gBrowser.tabs[0];
>+    TabContextMenu.updateContextMenu(document.getElementById("tabContextMenu"));
>+    return !document.getElementById("context_undoCloseTab").disabled;
>+}

indentation is off

>+function undoCloseAndRecheck(tries) {
>+  if (numClosedTabs()) {
>+    if (tries > 99) {
>+      ok(false, "Undo Close Tab failed to complete.");
>+      finish();
>+    }
>+    undoCloseTab();
>+    setTimeout(arguments.callee, 100, ++tries);
>+    return;
>+  }

Can you explain this? The closed tab count should increment immediately when closing a tab.

Comment 16

[Frank Yan (:fryn)]

(In reply to comment #15)
> >+function undoCloseAndRecheck(tries) {
> >+  if (numClosedTabs()) {
> >+    if (tries > 99) {
> >+      ok(false, "Undo Close Tab failed to complete.");
> >+      finish();
> >+    }
> >+    undoCloseTab();
> >+    setTimeout(arguments.callee, 100, ++tries);
> >+    return;
> >+  }
> 
> Can you explain this? The closed tab count should increment immediately when
> closing a tab.

But it doesn't decrement immediately upon undoCloseTab().

Comment 17

[Dão Gottwald [:dao]]

Why not? That sounds like a bug. Also, it looks like you're going to call undoCloseTab multiple times.

Comment 18

[Frank Yan (:fryn)]

Attachment: take 4 (uses observes attribute and includes basic test)

(In reply to comment #17)
> Why not? That sounds like a bug. Also, it looks like you're going to call
> undoCloseTab multiple times.

Actually, I was wrong; it does decrement immediately. Also, I found the cause of needing to call undoCloseTabs multiple times, and I filed a bug for it:
https://bugzilla.mozilla.org/show_bug.cgi?id=582917

I changed my test to be a more basic test. Once bug 582917 is fixed, I can make the test more robust if wanted.

Comment 19

[Dão Gottwald [:dao]]

Comment on attachment 461177 [details] [diff] [review]
take 4 (uses observes attribute and includes basic test)

>+  ok(!numClosedTabs(), "This test should start with no closed tabs.");

is(numClosedTabs(), 0, ...)

>+  is(isUndoCloseEnabled(), false, "Undo Close Tab should be disabled.");
>+
>+  var tab = gBrowser.addTab("about:blank", {skipAnimation: true});
>+  var browser = gBrowser.getBrowserForTab(tab);
>+  browser.addEventListener("load", function() {
>+    gBrowser.removeTab(tab);
>+    is(isUndoCloseEnabled(), true, "Undo Close Tab should be enabled.");

Blank tabs shouldn't actually be added to the undo stack. This probably regressed in recently.

Also, why {skipAnimation: true}? Doesn't seem relevant for this test.

Comment 20

[Frank Yan (:fryn)]

Attachment: take 5

I replaced 'about:blank' with 'http://mochi.test:8888/' and added a comment to bug 582917 about not adding blank tabs to the undo stack.

Comment 21

[Dão Gottwald [:dao]]

Comment on attachment 461181 [details] [diff] [review]
take 5

>+  is(numClosedTabs(), 0, "This test should start with 0 closed tabs.");
>+  is(isUndoCloseEnabled(), false, "Undo Close Tab should be disabled.");

Are these sane assumptions when other tests close tabs before this test runs? Does this actually pass?

Comment 22

[Frank Yan (:fryn)]

Attachment: take 6

No, those weren't sane assumptions.

Comment 23

[Dão Gottwald [:dao]]

Comment on attachment 461187 [details] [diff] [review]
take 6

>+  is(isUndoCloseEnabled(), false, "Undo Close Tab should be disabled.");

ok(!isUndoCloseEnabled(), ...

>+    is(isUndoCloseEnabled(), true, "Undo Close Tab should be enabled.");

ok(isUndoCloseEnabled(), ...

Comment 24

[Frank Yan (:fryn)]

Attachment: take 7 (seventh time's the charm)

Thanks for all your help!

Comment 25

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

http://hg.mozilla.org/mozilla-central/rev/a68fa21f86b2

Comment 26

[Nochum Sossonko [:Natch]]

Comment on attachment 461190 [details] [diff] [review]
take 7 (seventh time's the charm)

>diff --git a/browser/base/content/test/browser_bug580956.js b/browser/base/content/test/browser_bug580956.js
[...snip...]
>+  browser.addEventListener("load", function() {
This event listener is never removed. Just saying.

Comment 27

[Dão Gottwald [:dao]]

I doesn't need to be removed. The browser is removed from the document as the tab is closing.

Comment 28

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified fixed with builds on Windows and OS X like Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b3pre) Gecko/20100802 Minefield/4.0b3pre

Updated the following Litmus test:
https://litmus.mozilla.org/show_test.cgi?id=10010

How well is the context menu covered by automated tests? I wonder if we need the Litmus test at all.