Closed Bug 518203 Opened 11 years ago Closed 11 years ago

Non-working "Close tab" context menu appears when right-clicking anywhere beside tabs

Categories

(SeaMonkey :: MailNews: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.0

People

(Reporter: stefanh, Assigned: philip.chee)

References

Details

(Keywords: fixed-seamonkey2.0)

Attachments

(1 file, 1 obsolete file)

Recent 1.9.1 trunk, mac os x:

Right-click on the new tab button or on the alltabs button or anywhere beside your tabs (but not on the close tab button).

--> Context menu for "Close tab" appears.

This is even worse on mac, because when you scroll your tabs by holding down the scrollbutton in the tabmail-arrowscrollbox the context menu appears.
Flags: blocking-seamonkey2.0?
Flags: blocking-seamonkey2.0? → blocking-seamonkey2.0+
Do you get the same on a browser window with tabs?
(In reply to comment #1)
> Do you get the same on a browser window with tabs?

No... I get a context menu with the close tab menuitem disabled.

Fwiw, when trying to actually use the menuitem in mailnews (when it appears in the  wrong context) I get this in the error console:

Error: tabInfo is undefined
Source File: chrome://messenger/content/tabmail.xml
Line: 599
Have two options for tabmail:
1/ Disable Close Tab menuitem on non-tabs - fix would be around http://mxr.mozilla.org/comm-central/source/suite/mailnews/tabmail.xml#219 I think
2/ Not show a context menu on elements which are not tabs - fix would be in tabbrowser.xml I think
Seems to be different on WindowsXP. For both Minefield and SeaMonkey 2.0pre navigator the behaviour is the same. Right click on a blank spot on the tab bar. The close tab menu item is active.Clicking on this menu item closes the active tab. To be consistent smTabmail should close the active tab as well when the context menu is invoked on a blank area of the tab bar.
If you can invoke the context menu on the scrollbutton, the context menu will appear on mac when you hold down the mouse button for like 2 sec on the scrollbutton (see comment #0). That's not acceptable imo.
> If you can invoke the context menu on the scrollbutton, the context menu will
> appear on mac when you hold down the mouse button for like 2 sec on the
> scrollbutton (see comment #0). That's not acceptable imo.
What does Firefox/Minefield do if you try to invoke the context menu on the scrollbutton?
Firefox/Minefiled/Thunderbird et al doesn't have the click-hold pref set to true, so my comment #6 doesn't apply to them (the click-hold "feature" is something that seamonkey decided to keep when the other toolkit apps stopped with it back in 2005/2006).
Actually, (In reply to comment #2)
> (In reply to comment #1)
> > Do you get the same on a browser window with tabs?
> 
> No... I get a context menu with the close tab menuitem disabled.

Actually, I was wrong here. The behaviour is the same as what Philip describe in comment #5.

Assuming we want the behaviour to be consistent with tabbrowser, I guess there are 2 problems here (one general and one mac-specific)

1) The tabmail tab doesn't close when you select the menuitem
2) The context menu will appear when you hold down the tabmail scrollbutton (lets say you have 20 tabs open and want to scroll through to the first/last).
Summary: "Close tab" context menu appears when right-clicking anywhere beside tabs → Non-working "Close tab" context menu appears when right-clicking anywhere beside tabs
(In reply to comment #4)
> Removing the context="_child" off the tabbrowser-strip
> and adding a context to tabbrowser-tabs
> works if you set an id rather than anonid on the menupopup
Not any more it doesn't ;-) IIRC that "bug" got fixed on 1.9.1 recently.
(In reply to comment #10)
> Not any more it doesn't ;-) IIRC that "bug" got fixed on 1.9.1 recently.

So is this bug fixed? Do we still need to do something in time for final? I would really like to get it off the 2.0 radar ;-)
No, it's not fixed. 1) and 2) are still valid and 1) probably happens on all OS. To summarize and repeat STR:

1) Open a bunch of tabs

2) Right-click anywhere (tab-strip etc), but not on an actual tab

--> Nothing happens

As for the mac case, as long as you have the click-hold feature enabled you'll always get a menuitem if you hold down the scroll-arrow (unless you special-case it).
The non-working menuitem appears on all OS, so setting All/All.
OS: Mac OS X → All
Hardware: x86 → All
There may (or may not) be something useful in Thunderbird's bug 472104 or the bug that actually fixed it.
Attached patch Patch v1.0 Minimal fix for 2.0 (obsolete) — Splinter Review
> There may (or may not) be something useful in Thunderbird's bug 472104 or the
> bug that actually fixed it.

Thanks for the hint.

This is a minimal fix for 2.0RC2. Something more elaborate might be done for 2.1 but that's grist for another bug.
Assignee: nobody → philip.chee
Status: NEW → ASSIGNED
Attachment #405422 - Flags: superreview?(neil)
Attachment #405422 - Flags: review?(neil)
Attachment #405422 - Flags: superreview?(neil)
Attachment #405422 - Flags: superreview+
Attachment #405422 - Flags: review?(neil)
Attachment #405422 - Flags: review?(mnyromyr)
Comment on attachment 405422 [details] [diff] [review]
Patch v1.0 Minimal fix for 2.0

>+                         onpopupshowing="return document.getBindingParent(this)

Oh, nice, I (obviously ;-)) didn't know that one.

>+            // The user might right-click on a non-tab area of the tab strip.
>+            var node = document.popupNode;
>+            if (node.localName != "tab")
>+              return false;
>+            this.mContextTab = node;
>+            return true;

So, since you don't (need to) care for mContextTab in the false case, what you actually meant to say here is:
            // The user might right-click on a non-tab area of the tab strip.
            this.mContextTab = document.popupNode;
            return this.mContextTab.localName == "tab";

r/a=me with that.
Attachment #405422 - Flags: review?(mnyromyr)
Attachment #405422 - Flags: review+
Attachment #405422 - Flags: approval-seamonkey2.0+
> Oh, nice, I (obviously ;-)) didn't know that one.

?? But you use this extensively in the rest of tabmail.xml!
e.g. |let tabContainer = document.getBindingParent(this);|

> So, since you don't (need to) care for mContextTab in the false case, what you
> actually meant to say here is:

I most certainly did NOT mean to say that!!!!!!!!!

>             // The user might right-click on a non-tab area of the tab strip.
>             this.mContextTab = document.popupNode;
>             return this.mContextTab.localName == "tab";

You are trying to be too clever and this will end in tears, mark my words.

Before doing this patch I looked at all the other tabbrowser and tabmail bindings in the comm- and mozilla- trees. While our tabmail implementation /currently/ does not make use of mContextTab, we certainly might do so at some point in the future. Unless you absolutely insist on this change, I would prefer that this patch goes into the tree as it is.
(In reply to comment #17)
> You are trying to be too clever and this will end in tears, mark my words.

Heh. :)

> Before doing this patch I looked at all the other tabbrowser and tabmail
> bindings in the comm- and mozilla- trees. While our tabmail implementation
> /currently/ does not make use of mContextTab, we certainly might do so at some
> point in the future.

The problem that I see is that mContextTab keeps an old, most likely invalid (because you are already handling a different context click) value. It might be even more clean if we nulled it in the false case here.

So what is your apprehension here about exactly?
Never mind. Neil convinced me that I was barking up the wrong tree.
Attachment #405422 - Attachment is obsolete: true
Attachment #405763 - Flags: superreview+
Attachment #405763 - Flags: review+
Attachment #405763 - Flags: approval-seamonkey2.0?
Keywords: checkin-needed
Comment on attachment 405763 [details] [diff] [review]
Patch v1.1 Nits fixed. r=mnyromyr sr=neil a2.0=mnyromyr [Checkin: Comment 20]

http://hg.mozilla.org/comm-central/rev/ca1a780e6bd7
Attachment #405763 - Attachment description: Patch v1.1 Nits fixed. r=mnyromyr sr=neil a2.0=mnyromyr → Patch v1.1 Nits fixed. r=mnyromyr sr=neil a2.0=mnyromyr [Checkin: Comment 20]
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.0
Attachment #405763 - Flags: approval-seamonkey2.0? → approval-seamonkey2.0+
You need to log in before you can comment on or make changes to this bug.