Closed Bug 954219 Opened 7 years ago Closed 7 years ago

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

Categories

(Instantbird :: Conversation, defect)

x86
All
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: bugzilla, Assigned: aleth)

References

Details

Attachments

(2 files, 2 obsolete files)

*** 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 ***
*** 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.
*** 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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows Vista → All
Version: 0.3a2 → trunk
Attached patch Patch (obsolete) — Splinter Review
*** 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).
Attachment #8353486 - Flags: review?(clokep)
Assignee: nobody → aleth
Status: NEW → ASSIGNED
*** 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)
*** 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.
Attached patch Patch (obsolete) — Splinter Review
*** 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.
Attachment #8353492 - Flags: review?(clokep)
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 ***
Attachment #8353486 - Attachment is obsolete: true
Attachment #8353486 - Flags: review?(clokep)
Depends on: 954292
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.
Attachment #8353492 - Flags: review?(clokep) → review+
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 ***
Attachment #8353492 - Flags: review+ → review-
Attached patch PatchSplinter Review
*** 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 ;)
Attachment #8353493 - Flags: review?(clokep)
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 ***
Attachment #8353492 - Attachment is obsolete: true
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.
Attachment #8353493 - Flags: review?(clokep) → review+
Attachment #8353493 - Flags: review?(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.
Attachment #8353493 - Flags: review?(florian) → review+
Whiteboard: [checkin-needed]
*** 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.
*** 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.
*** 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 :).
*** 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. :)
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [checkin-needed]
Target Milestone: --- → 1.2
You need to log in before you can comment on or make changes to this bug.