472104 - Everything in the tabstrip has a Close Tab context menuitem, which only works right-clicking tabs

Status: RESOLVED FIXED

(Thunderbird :: Toolbars and Tabs, defect)

Description

[Stefan Sitter [:ssitter]]

Lightning 1.0pre (2009010404) with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3pre) Gecko/20090104 Shredder/3.0b2pre

Steps to Reproduce:
===================
1. Start Shredder with Lightning extension installed
2. Open context menu for Calendar or Tasks shortcut button in tabbar
3. Select "Close Tab"

Actual Results:
===============
Nothing happens. Error Console shows:

    Error: tab is undefined
    Source File: chrome://messenger/content/tabmail.xml
    Line: 366

Expected Results:
=================
No error. The corresponding Calendar or Tasks tab should be closed if opened.
Or: "Close Tab" command should not be available for shortcut buttons in tabbar.

I'm not sure if this is a bug in Thunderbird tab implementation or a bug in Lightning tab shortcut button implementation. CCing some Thunderbird developers from Bug 402365	that added the tab shortcut buttons.

Comment 1

[Andrew Sutherland [:asuth] (he/him)]

It never occurred to me that the shortcut buttons would end up with context menus effectively attached to them.  I don't think it makes a lot of sense for the shortcut buttons to serve as proxies for the actual tabs, but I'll let clarkbw weigh in on that.  Speculatively moving to Thunderbird Front-end.

Comment 2

[Bryan Clark (DevTools PM) [:clarkbw]]

These context menus show up even if the the tab isn't opened.  I'd say we just drop the context menu from these buttons altogether.

Comment 3

[Phil Ringnalda (:philor)]

Yeah, I was mildly worried that this was exposing an underlying tabmail bug when I first glanced at it, but that was my main thought: "omgwtf, make the bad context menu go away!"

Comment 5

[Bryan Clark (DevTools PM) [:clarkbw]]

Philipp, do you think you can take a look at this?

Comment 6

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: Fix - v1

Yes, this was quite easy to fix. I removed the context menu for the tab toolbarbuttons.

Comment 7

[Phil Ringnalda (:philor)]

Crap. Sorry, I should have actually looked sooner - this is totally a tabmail bug, that needs to be fixed higher and wider than just on your buttons.

Firefox, which we copied, has context menu items that are sane for clicking the blank part of the tabstrip, like new tab and reload all tabs, and because of that has made the less-sane decision that everything on the tabstrip has a single context menu. If you right-click a tab, the this-tab things like close tab or reload tab apply to the one you right-clicked, selected or not, but if you right-click the tabstrip, or the new tab button, or the alltabs button, or a scroll arrow, they apply to the selected tab.

Camino and Safari, both generally good things to copy, do not have a tabstrip context menu which applies to the selected tab on which you did not right-click, instead having just new tab, and reload all for Cm, and do not have context menus for scroll arrows or the alltabs button.

Imitating Firefox is easy: you just have a mContextTab field that you set from the context menu's onpopupshowing, to either the clicked-on tab or the selected tab, and then pass that to removeTabByNode (and hoist Fallen's |context=""| up to the tabmail-buttons box, since it's a bad bet that anything overlaying in there will actually want the generic context menu). Restricting our one-item context menu to just tabs should be easy, too: just move the context attribute down onto the xul:tabs element. Probably imitating Cm/Safari is fairly easy, too, though we'll have to scatter a lot of |context=""| around, on the scrollbuttons and whatnot, but we need to decide pretty quickly whether or not we're going to have more context menuitems, and where they'd be sane (thus the reassign to clarkbw).

Comment 8

[Phil Ringnalda (:philor)]

Comment on attachment 359122 [details] [diff] [review]
Fix - v1

Hold that thought for a bit :)

Comment 9

[Bryan Clark (DevTools PM) [:clarkbw]]

I'm going to give this over to Phil as he's the one most interested in this.  On the drivers call today we talked about how we don't think we need to block this release for this bug, so I'm moving this out.

Comment 10

[Phil Ringnalda (:philor)]

clarkbw: It was only assigned to you for an answer to the question that I neatly hid at the tail end of the final paragraph of comment 7. Though I guess since we're not blocking on being able to open new blank tabs, I can treat that as an answer of "we aren't going to have another menuitem, and only having the one item "Close Tab" menu appear on tabs thus makes sense."

Comment 11

[Phil Ringnalda (:philor)]

Inconveniently, "just moving it down to the xul:tabs element" isn't actually an option, since context either takes an id or the magic _child value, and the context menupopup's not going to be a child of xul:tabs. Conveniently, the alternative is to return false from onpopupshowing if the target wasn't a tab, and hendrik is already busy making an onpopupshowing for it in bug 395388 :)

Comment 12

[Phil Ringnalda (:philor)]

Fixed by bug 395388. (Yay!)