954219 - Close tab option doesn't work properly when using the tab drop down button

Status: RESOLVED FIXED

(Instantbird Graveyard :: Conversation, defect)

Description

[Instantbot]

*** Original post on bio 785 by Wayne (:waynenguyen) <duy.nghoang AT gmail.com> at 2011-05-16 12:03:00 UTC ***

*** Due to BzAPI limitations, the initial description is in comment 1 ***

Comment 1

[Instantbot]

Attachment: As for this, clicking Close tab will close the Instantbird tab instead of ChanServ.

*** Original post on bio 785 as attmnt 621 by duy.nghoang AT gmail.com at 2011-05-16 12:03:00 UTC ***

When there're multiple tabs, clicking on the right most *drop down* button to see the tab list, then right clicking on a random tab and choose Close tab doesn't close that tab but instead the current tab.

Comment 2

[Benedikt Pfeifer]

*** Original post on bio 785 at 2011-05-16 12:07:37 UTC ***

I'm seeing this on Windows XP and a current nightly (20110516) as well.

Comment 3

[aleth [:aleth]]

Attachment: Patch

*** Original post on bio 785 as attmnt 1727 at 2012-07-01 15:43:00 UTC ***

Another papercut.

Patch in its current form depends on bug 954292 (bio 859) (specifically, anonid -> id).

Comment 4

[Florian Quèze [:florian]]

*** Original post on bio 785 at 2012-07-11 14:00:53 UTC ***

Why is it even possible to open a context menu above the all-tabs menu? Can't we just prevent this? (btw, it's not possible to reproduce on Mac)

Comment 5

[aleth [:aleth]]

*** Original post on bio 785 at 2012-07-11 14:07:17 UTC ***

(In reply to comment #3)
> Why is it even possible to open a context menu above the all-tabs menu? Can't
> we just prevent this? (btw, it's not possible to reproduce on Mac)

Sure, one could just as easily prevent it. I also think that would be better. I assumed since no one had responded in that way to the bug that it was wanted there.

Comment 6

[aleth [:aleth]]

Attachment: Patch

*** Original post on bio 785 as attmnt 1733 at 2012-07-11 14:42:00 UTC ***

Removes the context menu instead. Also removes it from the tab bar scroll arrows etc., which I noticed also had them.

Comment 7

[aleth [:aleth]]

Comment on attachment 8353486 [details] [diff] [review]
Patch

*** Original change on bio 785 attmnt 1727 at 2012-07-11 14:42:53 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 8

[Florian Quèze [:florian]]

Comment on attachment 8353492 [details] [diff] [review]
Patch

*** Original change on bio 785 attmnt 1733 at 2012-07-11 15:01:24 UTC ***

>-            this.mContextTab = document.popupNode.localName == "tab" ?
>-                               document.popupNode : this.selectedTab;
>+            switch(document.popupNode.localName) {
>+              case "tab":
>+                this.mContextTab = document.popupNode;
>+                break;
>+              case "tabs":
>+                return false;
>+              default:
>+                this.mContextTab = this.selectedTab;

I dislike this switch statement, it's not really simplifying the code.
I think I would prefer something like:

let tagName = document.popupNode.localName;
if (tagName == "tabs")
  return false;
this.mContextTab = tagName == "tab" ?
                   document.popupNode : this.selectedTab;

Looks good otherwise.

Comment 9

[Florian Quèze [:florian]]

Comment on attachment 8353492 [details] [diff] [review]
Patch

*** Original change on bio 785 attmnt 1733 at 2012-07-11 15:03:51 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 10

[aleth [:aleth]]

Attachment: Patch

*** Original post on bio 785 as attmnt 1734 at 2012-07-11 16:10:00 UTC ***

I preferred the switch, but not enough to argue about it ;)

Comment 11

[aleth [:aleth]]

Comment on attachment 8353492 [details] [diff] [review]
Patch

*** Original change on bio 785 attmnt 1733 at 2012-07-11 16:10:38 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 12

[Patrick Cloke [:clokep]]

Comment on attachment 8353493 [details] [diff] [review]
Patch

*** Original change on bio 785 attmnt 1734 at 2012-07-17 13:24:24 UTC ***

As far as I can tell, this change will have the desired effect and seems fairly straight forward. I'd definitely like flo to look this over before checking in, however.

Comment 13

[Florian Quèze [:florian]]

Comment on attachment 8353493 [details] [diff] [review]
Patch

*** Original change on bio 785 attmnt 1734 at 2012-07-17 13:31:10 UTC ***

I already looked (comment 6). Still looks good.

Comment 14

[Florian Quèze [:florian]]

*** Original post on bio 785 at 2012-07-20 14:13:40 UTC ***

Hmm, this also removes the context menu on the empty area of the tab bar (when you don't have enough tabs to fill it), I'm not sure this is wanted.

Comment 15

[aleth [:aleth]]

*** Original post on bio 785 at 2012-07-20 19:50:47 UTC ***

(In reply to comment #10)
> Hmm, this also removes the context menu on the empty area of the tab bar (when
> you don't have enough tabs to fill it), I'm not sure this is wanted.

This was intentional. I've always found the context menu there confusing. It is not at all clear which tab it is going to refer to. That area is not part of any tab.

Comment 16

[Florian Quèze [:florian]]

*** Original post on bio 785 at 2012-07-21 22:28:01 UTC ***

I was going to argue back that Firefox does it like the behavior we have had for a while (ie open the context menu for the selected tab when the right click was on the empty area of the tab bar), but I've just tested it and it actually doesn't do that any more. I'm sure it did at the time I forked tabbrowser.xml though :).

Comment 17

[Patrick Cloke [:clokep]]

*** Original post on bio 785 at 2012-07-22 04:22:00 UTC ***

Checked in as http://hg.instantbird.org/instantbird/rev/df45f47c0590

Thanks for fixing this annoyance. :)