1712840 - "No container" menuitem in new tab button context menu is broken when menu is opened by right-click, but works correctly when opened by click & hold

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Shane Hughes [:aminomancer]]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:90.0) Gecko/20100101 Firefox/90.0

Steps to reproduce:

• I noticed this on Windows 10, Nightly 90.0a1, build 20210525093431, with proton settings enabled. I went ahead and made a brand new, empty profile with the profile manager to test it. The bug is always present, seems to just be an oversight

STR:

• Click & hold on the new tab button
• Opens a context menu, .new-tab-popup
• Click "No container", it opens a new tab with context ID 0
• Now right click the new tab button
• Opens a similar context menu, #new-tab-button-popup
• Click "No container"

Actual results:

• It does nothing
• Looking at the DOM, the menus look identical. In both cases the menuitems for the actual containers have command attributes that invoke openNewUserContextTab, which gets the context ID from an attribute on the click event target, the menuitem.
• But in both popups, the "No container" menuitem has no command attribute, nor does it have any other event listeners.

Expected results:

So why the different behavior — I think this is because the click & hold popup is a child of the new tab button, whereas the right-click popup is off on its own in #mainPopupSet.

So when you click the "No container" item in the click & hold popup, the event is bubbling up to the new tab button, which has event listeners like command="cmd_newNavigatorTab" and oncommand="BrowserOpenTab(event);".

When you click the same item in the right-click popup, nothing opens because there isn't any command event listener targeting it or its parent.

You can see that adding command="cmd_newNavigatorTab" or command="Browser:NewUserContextTab" to the "No container" item in the right-click menu fixes the issue. The command attribute can also just be added to the right-click menupopup itself.

So I'm pretty sure the issue is purely that there aren't any event listeners for the "No container" items, and I guess this was just overlooked because it's accidentally working correctly in the click & hold menu, due to the event bubbling up to an element that just so happens to have an almost identical purpose/behavior.

So I think my suggestion is to add the command attribute to the menuitem when it's being created.

Comment 1

[Shane Hughes [:aminomancer]]

I think I figured out the source of the confusion. It's probably because the same code is used to generate the menu that shows in the content area context menu when right-clicking a URL. In that condition, command attributes aren't needed for the individual menuitems because the menupopup element has oncommand="gContextMenu.openLinkInTab(event);":

    // We don't set an oncommand/command attribute because if we have
    // to exclude a userContextId we are generating the contextMenu and
    // isContextMenu will be true.

But that comment is mistaken because that code isn't only executed if excludeUserContextId, it's excludeUserContextId || showDefaultTab. Although containers are only excluded when isContextMenu is true, it's also checking for showDefaultTab, which can be true when isContextMenu is false.

In other words it's skipping the command attribute for the "No container" menu in every circumstance, regardless of the parameters. Then the item in the click & hold popup is only working because, like the items in the content area context menu, its parent already handles command events. But showDefaultTab is true when both popups are generated, the right-click menu too.

That method could have another parameter, or it could check if the event target has a valid command event handler. But really none of that is necessary — a command attribute can just be added to the "No container" menuitem under any circumstances where isContextMenu is false. So just like is already done for the container menuitems, the command attribute should only be added if isContextMenu is false

Comment 2

[Shane Hughes [:aminomancer]]

This should work (@line 779)

  // If we are excluding a userContextId, we want to add a 'no-container' item.
  if (excludeUserContextId || showDefaultTab) {
    let menuitem = document.createXULElement("menuitem");
    menuitem.setAttribute("data-usercontextid", "0");
    menuitem.setAttribute(
      "label",
      bundle.GetStringFromName("userContextNone.label")
    );
    menuitem.setAttribute(
      "accesskey",
      bundle.GetStringFromName("userContextNone.accesskey")
    );
    if (!isContextMenu) {
      menuitem.setAttribute("command", "Browser:NewUserContextTab");
    }

    docfrag.appendChild(menuitem);

    let menuseparator = document.createXULElement("menuseparator");
    docfrag.appendChild(menuseparator);
  }

Comment 3

[Timea Cernea [:tbabos][inactive]]

Hi aminomancer,
This appears to be a known issue that has been reported in Bug 1699198 as well.
Thanks for taking the time into investigating this and I encourage you to paste all the debugging you did in this issue to the other bug report as well. It might help developers to solve this.
I could also reproduce this on MacOS on the latest Nightly as well as other releases if I install the multi-account container addon. Could reproduce back to 76 as well, didn't look further tho.

Comment 4

[Shane Hughes [:aminomancer]]

Doesn't seem to require the multi-account containers addon for me. I tested on an empty profile with no addons, it just requires enabling container tabs in about:preferences

Comment 6

[:Gijs (he/him)]

(In reply to Frederik Braun [:freddy] from bug 1699198 comment #2)

A bit of a shame that bug 1712840 was closed as a dupe of this instead of the other way around. It contains much more useful information.

Freddy was right in the other bug, so I've inverted the dupe. I'll see if I can get a patch together based on all this info.

Comment 7

[:Gijs (he/him)]

Before bug 1606265 the context menu didn't exist; after that it appears to have always been broken like this.

(In reply to aminomancer from comment #1)

That method could have another parameter, or it could check if the event target has a valid command event handler. But really none of that is necessary — a command attribute can just be added to the "No container" menuitem under any circumstances where isContextMenu is false. So just like is already done for the container menuitems, the command attribute should only be added if isContextMenu is false

Indeed. The name of this property is kind of confusing though - the new tab popup is also a context menu when opened via right-click...

Comment 8

[:Gijs (he/him)]

Also, the "no container" entry doesn't work correctly for "Open in new container tab" in the tab context menu, where isContextMenu is true...

Comment 9

[:Gijs (he/him)]

(In reply to :Gijs (he/him) from comment #8)

Also, the "no container" entry doesn't work correctly for "Open in new container tab" in the tab context menu, where isContextMenu is true...

Oh, seems this is just because I had "always open in X container" set for the site in question, so it automatically replaces the no-container tab with one that has the same container the site was already open in. D'oh.

Comment 10

[:Gijs (he/him)]

Attachment: Bug 1712840 - fix new tab context menu 'no container' entry so it actually works, and add tests, r?nhnt11

Comment 11

[(inactive) Nihanth Subramanya [:nhnt11]]

Wow, do we know if this is a regression? I feel like I use the "No Container" option fairly often and I never noticed this...

edit: I guess I never used the context menu on the newtab button though, I usually go through the web content context menu

Comment 12

[:Gijs (he/him)]

(In reply to Nihanth Subramanya [:nhnt11] from comment #11)

Wow, do we know if this is a regression? I feel like I use the "No Container" option fairly often and I never noticed this...

edit: I guess I never used the context menu on the newtab button though, I usually go through the web content context menu

Yeah, that one works, as does using "long press" on the button. It was only the new tab context menu that was broken.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d8e147502d3b
fix new tab context menu 'no container' entry so it actually works, and add tests, r=nhnt11

Comment 14

[Andreea Pavel [:apavel]]

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

Comment 15

[Shane Hughes [:aminomancer]]

(In reply to :Gijs (he/him) from comment #7)

Indeed. The name of this property is kind of confusing though - the new tab popup is also a context menu when opened via right-click...

yeah maybe changing the key to isContentContextMenu would prevent future confusion