348016 - The "Recently closed tabs" menu is no longer functional after opening the "Customize toolbars" dialogue

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Frédéric Buclin]

Mozilla/5.0 (X11; U; Linux i686; fr; rv:1.8.1b1) Gecko/2006080804 BonEcho/2.0b1

Installing the latest nightly a few hours ago, I noted that closed tabs are not always added to the "Recently closed tabs" menu. Looks like this behavior is not systematic, so I have no testcase so far. I never saw this problem using Fx2b1.

Comment 1

[Arie Paap [:wildmyron]]

I noticed this once on Windows while working through Litmus tests (testing functionality of menus) but I haven't been able to reproduce either.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1b1) Gecko/20060812 BonEcho/2.0b1 ID:2006081205

OS All perhaps?

Comment 2

[Frédéric Buclin]

(In reply to comment #1)
> OS All perhaps?

Yup! Makes sense.

I agree that this one is hard to reproduce. I wonder if this could happen only with a new profile or just after an upgrade. In my case, I saw this after upgrading from Fx2b1 to the one shown in comment 0.

Comment 3

[Simon Bünzli]

AFAICT not a regression (unless you can point me to a nightly which doesn't exhibit this behavior at all).

When you see this behavior, do you get any (SessionStore related) messages in the Error Console? Some of these would help for debugging.

Also, if you note this several times, please apply the patch from bug 348461 and see if that helps.

Comment 4

[Arie Paap [:wildmyron]]

I have found a way to (personally) reproduce this but it is frustratingly convoluted. Also tested with a fresh profile in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1b1) Gecko/20060813 BonEcho/2.0b1 ID:2006081305. It doesn't seem related to an upgrade or to having a clean profile as I've also reproduced with my daily Bon Echo profile. I don't see any errors in the messages in the Error Console.

Essence of steps required:
I have Litmus open in Fx 1.5 and use set MOZ_NO_REMOTE=1 for Bon Echo
Create clean profile and start Bon Echo
Switch to second tab (Home page)
Run through the edit menu test on Litmus (testcase id #1333)
Run through the View menu test on Litmus (testcase id #1331)
Then begin the History menu test:
Browse to another page. Go Back. Go Forward. Go Home. Close Tab.
Open History Menu and "Recently Closed Tabs" is greyed out.

This obviously involves some switching between windows and a few other random actions but this sequence reliably triggers the bug for me. I have tried leaving out the Edit menu test or the View menu test but both seem to be required. Because of the time taken I haven't been able to narrow down which parts of the test cause the problem and I haven't tested with older builds yet.

@Simon I haven't got a build system, can you point me to a build with this patch or can someone build for me? I can test on Linux or Windows XP

Comment 5

[Arie Paap [:wildmyron]]

I managed to narrow down the steps to reproduce the bug. Not sure why I thought the Edit menu test was involved (I must have missed the crucial step while testing.) Opening the Customize Toolbars dialogue is the step that prevents recently closed tabs from being updated.

Steps to reproduce:
1. Start BonEcho
2. Open a second tab
3. View -> Toolbars -> Customize...
4. Close dialogue
5. Close tab
6. History -> Recently Closed Tabs(RCT) is not updated

Notes:
Tested in clean profile and working profile.
Undo Close Tab still works from the tab bar context menu
Old entries in RCT (before opening Customize Toolbars) remain but are no longer fuctional.
Only tested as far back as 2006-07-22-03 where this bug is still present

Comment 6

[Simon Bünzli]

Seems like the event listener attached to #goPopup in HistoryMenu.initializeUndoSubmenu stripped while customizing the toolbar.

Dietrich: Why not set the submenu's disabled attribute from updateGoMenu and add onpopupshowing="HistoryMenu.populateUndoSubmenu();" to #historyUndoPopup in browser-menubar.inc directly?

Comment 7

[Frédéric Buclin]

Wow, Arie, nice catch! It took me less than 30 seconds to reproduce the bug using your testcase from comment 5. :) No new entries are added and existing ones are indeed broken.

This explains why I had this problem after upgrading: that's because I wanted to remove the "Go" button, and so I opened the "Customize Toolbars" dialogue.

Updating the bug summary accordingly and retargetting the bug to Fx2b2.

Comment 8

[Dietrich Ayala (:dietrich)]

Attachment: fix

Per Simon's suggestion, adds static onpopupshowing handler to the menu, and changes menu state when the history (or go) menu is updated.

Comment 9

[Mike Connor [:mconnor]]

Comment on attachment 233639 [details] [diff] [review]
fix

>+HistoryMenu.toggleRecentlyClosedTabs = function PHM_toggleRecentlyClosedTabs() {
>+  // enable/disable the Recently Closed Tabs sub menu
>+  var undoPopup = document.getElementById("historyUndoPopup");
>+  undoPopup.parentNode.removeAttribute("disabled");
>+  // get closed-tabs from nsSessionStore
>+  var ss = Cc["@mozilla.org/browser/sessionstore;1"].
>+           getService(Ci.nsISessionStore);
>+  // no restorable tabs, so make sure menu is disabled, and return
>+  if (ss.getClosedTabCount(window) == 0) {
>+    undoPopup.parentNode.setAttribute("disabled", true);
>+  }

so, this is weird, and unnecessary.  Check the count, and set disabled appropriate, instead of the on/off here.  Just the right approach in general.

Comment 10

[Dietrich Ayala (:dietrich)]

Attachment: fix - less weird

Comment 11

[Mike Connor [:mconnor]]

Comment on attachment 233900 [details] [diff] [review]
fix - less weird

>@@ -3216,16 +3216,20 @@ function FillHistoryMenu(aParent, aMenu,
>                                     j,
>                                     entry.title,
>                                     entry.URI ? entry.URI.spec : null,
>                                     j==index,
>                                     aInsertBefore);
>             }
>           break;
>       }
>+
>+  // enable/disable RCT sub menu
>+  HistoryMenu.toggleRecentlyClosedTabs();
>+
>     return true;
>   }

crazy indenting...uh, do something about that

r=me otherwise, thanks

Comment 12

[Mike Schroepfer]

Comment on attachment 233900 [details] [diff] [review]
fix - less weird

a=schrep for drivers.

Comment 13

[Frédéric Buclin]

Working fine again using:
Mozilla/5.0 (X11; U; Linux i686; fr; rv:1.8.1b2) Gecko/20060821 BonEcho/2.0b2

Comment 14

[Adam Guthrie]

*** Bug 345014 has been marked as a duplicate of this bug. ***