1903378 - 'Recently Closed Tabs' menu item command in 'History' menu is disabled when shown for the first time despite tabs closed before - cannot reopen tabs

Status: VERIFIED FIXED

(Firefox :: Menus, defect)

Description

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Firefox 129.0a1 on Windows 10

The 'Recently Closed Tabs' menu item command in the 'History' menu is disabled when shown for the first time despite tabs closed before.

Steps to reproduce:

1. Launch Firefox.
2. Visit https://www.mozilla.org
3. Open the links in the header.
4. Close 2 tabs, e.g. by clicking on them with the middle mouse button/mouse wheel.
5. Press the Alt key to show the menu bar.
6. Open the 'History' menu.

Expected result:
'Recently Closed Tabs' menu enabled, provides a submenu to reopen closed tabs

Actual result:
'Recently Closed Tabs' menu disabled

Comment 1

[Tom Schuster]

So the problem is that the "disabled" state of the "Recently Closed Tabs" menu is set by HistoryMenu.toggleRecentlyClosedTabs, which is called by the HistoryMenu._onPopupShowing event listener, however that listener doesn't fire at all, because we just created the HistoryMenu object inside our own popupshowing event listener. Opening the menu bar a second time makes it work correctly, because the listener is already attached.

Is it really expected that this works with inline event handlers? i.e. do they always behave like addEventListener with {capture: true}?

Comment 2

[Najjar]

This issue is also present on 127.0.1 in Linux

Comment 3

[:Gijs (away/busy until Mar 10)]

(In reply to Tom Schuster (MoCo) from comment #1)

So the problem is that the "disabled" state of the "Recently Closed Tabs" menu is set by HistoryMenu.toggleRecentlyClosedTabs, which is called by the HistoryMenu._onPopupShowing event listener, however that listener doesn't fire at all, because we just created the HistoryMenu object inside our own popupshowing event listener. Opening the menu bar a second time makes it work correctly, because the listener is already attached.

I tried this out because it didn't make sense to me. A naive test suggests the order of events is normally capture->inline->bubble, but adding a capturing listener from inline doesn't fire - https://jsbin.com/xetacevexe/edit?html,js,output .

So I don't think this is accurate. Then I built beta (before the regressing patch landed) and set a breakpoint in popupshowing in the browser debugger. That showed me that the constructor in PlacesMenu calls _onPopupShowing manually - https://searchfox.org/mozilla-central/rev/74518d4f6979b088e28405ba7e6238f4707639cf/browser/components/places/content/browserPlacesViews.js#1991 .

We're still creating the HistoryMenu as we were before, and passing the event along - so why does that not work anymore?

Comment 4

[:Gijs (away/busy until Mar 10)]

(In reply to Najjar from comment #2)

This issue is also present on 127.0.1 in Linux

This doesn't make any sense because this bug is marked as a regression in 129. Please file a separate bug with more details about what you're seeing.

Comment 5

[Tom Schuster]

I hadn't even considered that we could be manually calling the popupshowing event handler from the super constructor. The reason that is doesn't work anymore is this check:

    // Don't handle events for submenus.
    if (aEvent.target != aEvent.currentTarget) {
      return;
    }

currentTarget is always the <menubar id="main-menubar">.

Comment 6

[Tom Schuster]

Attachment: Bug 1903378 - Handle the popupshowing event during the HistoryMenu construction. r?Gijs

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/df53f5182d1d
Handle the popupshowing event during the HistoryMenu construction. r=Gijs,places-reviewers,mak

Comment 8

[Aron Cseh]

https://hg.mozilla.org/mozilla-central/rev/df53f5182d1d

Comment 9

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1893068

Comment 10

[Simona Badau, Desktop QA]

Reproduced the issue on Nightly 129.0a1 Build ID: 20240618094855 on Windows 10.

Verified as fixed using Firefox 129.0b2 and Nightly 130.0a1 on Windows 10, Ubuntu 22.04 and macOS 13.