587040 - After navigating with arrow keys, active tab highlighting will move but clicking on the group will bring up the last active tab

Status: VERIFIED FIXED

(Firefox Graveyard :: Panorama, defect, P3)

Description

[Martijn Wargers (dead)]

When I click in the empty area of a tab group box, then the last viewed tab gets opened.
I would expect nothing to happen instead.

Comment 1

[Ed Lee :Mardak]

I believe something related to bug 566643 added the functionality of click-anywhere-in-group-to-activate-group.

Comment 2

[Ed Lee :Mardak]

Actually, there does seem to be a bug where if you click the empty spot of any group, it'll go to the last selected tab -- *not* the last selected tab of that group.

Comment 3

[Raymond Lee [:raymondlee]]

Aza/Ian: what is the expected behaviour for this?

Comment 4

[Ian Gilman [:iangilman]]

(In reply to comment #2)
> Actually, there does seem to be a bug where if you click the empty spot of any
> group, it'll go to the last selected tab -- *not* the last selected tab of that
> group.

That's definitely the wrong behavior. 

The correct behavior, according to Aza, is that clicking in the empty space in a group should just go to the last selected tab from that group.

I, on the other hand, agree with the original poster, that clicking in the empty space in a group shouldn't do anything.

Comment 5

[Aza Raskin [:aza]]

I am constantly torn in this discussion. I'm generally not a fan of side-effect behaviors. I.e., clicking on a group should interact only with the group and not open a tab.

That said, it is consistently one of the most requested features and once learned as a behavior can help reduce the Hick's-law penalty of deciding which tab to zoom to when you just want to go back to a group's context.

Thus, for now, let's keep the behavior. Clicking on a group should bring you back to the last active tab from that group. If there was no active tab it should go to the first tab in the group.

Comment 6

[Ian Gilman [:iangilman]]

Raymond, can you fix the bug Mardak noted (that it doesn't always open into the correct group)?

Comment 7

[Raymond Lee [:raymondlee]]

Attachment: v1

Comment 8

[Christophe Calvez]

It might become a different bug, but when the last selected tab of a group "A" has been moved to group "B", and you click in the blank of the group "A", the tab is opened from group "B".

Mozilla/5.0 (Windows NT 5.0; rv:2.0b5pre) Gecko/20100829 Firefox/4.0b5pre ID:20100829040614

Comment 9

[Aza Raskin [:aza]]

Ian, can you review Raymond's patch?

Comment 10

[Ian Gilman [:iangilman]]

Comment on attachment 468209 [details] [diff] [review]
v1

Looks good.

Comment 11

[Dietrich Ayala (:dietrich)]

Comment on attachment 468209 [details] [diff] [review]
v1

r=me.

please update the summary of this bug to reflect the actual change it's handling. (better still would be to put this patch on a different bug entirely, since it has nothing at all to do with the original reason for this bug!)

also, fwiw, the original report i made in bug 566643 was for *stacked* groups only. but it looks like the implemented behavior made the empty space in any and all groups clickable.

Comment 12

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: Patch NOT for checkin

Comment 13

[Reed Loden [:reed]]

mitcho, not sure if you're aware, but you're stealing people's bugs and patches, which isn't cool. Please stop. :)

Comment 14

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: Patch for checkin, for real this time

Comment 15

[Josh Matthews [:jdm]]

http://hg.mozilla.org/mozilla-central/rev/1f94199af190

Comment 16

[Josh Matthews [:jdm]]

And backed out due to Moth test failures.

Comment 17

[Michael Yoshitaka Erlewine [:mitcho]]

Comment on attachment 472054 [details] [diff] [review]
Patch for checkin, for real this time

># HG changeset patch
># User Raymond Lee <[e-mail]>
># Date 1283561638 14400
># Node ID 6113be45cfb81ffeb674900c5b47b043a2eb1fce
># Parent  00528ec7d79ce97e58bb51d7027e36c6f25be165
>Bug 587040 - After navigating with arrow keys, active tab highlighting will move but clicking on the group will bring up the last active tab [r+a=dietrich]
>
>diff --git a/browser/base/content/tabview/tabitems.js b/browser/base/content/tabview/tabitems.js
>--- a/browser/base/content/tabview/tabitems.js
>+++ b/browser/base/content/tabview/tabitems.js
>@@ -492,27 +492,29 @@ window.TabItem.prototype = Utils.extend(
>       this.resizable(false);
>     }
>   },
> 
>   // ----------
>   // Function: makeActive
>   // Updates this item to visually indicate that it's active.
>   makeActive: function() {
>-   iQ(this.container).find("canvas").addClass("focus");
>-   iQ(this.container).find("img.cached-thumb").addClass("focus");
>+    iQ(this.container).find("canvas").addClass("focus");
>+    iQ(this.container).find("img.cached-thumb").addClass("focus");
> 
>+    if (this.parent)
>+      this.parent.setActiveTab(this);
>   },
> 
>   // ----------
>   // Function: makeDeactive
>   // Updates this item to visually indicate that it's not active.
>   makeDeactive: function() {
>-   iQ(this.container).find("canvas").removeClass("focus");
>-   iQ(this.container).find("img.cached-thumb").removeClass("focus");
>+    iQ(this.container).find("canvas").removeClass("focus");
>+    iQ(this.container).find("img.cached-thumb").removeClass("focus");
>   },
> 
>   // ----------
>   // Function: zoomIn
>   // Allows you to select the tab and zoom in on it, thereby bringing you
>   // to the tab in Firefox to interact with.
>   // Parameters:
>   //   isNewBlankTab - boolean indicates whether it is a newly opened blank tab.
>@@ -531,17 +533,17 @@ window.TabItem.prototype = Utils.extend(
> 
>       function onZoomDone() {
>         TabItems.resumePainting();
> 
>         $tabEl
>           .css(orig.css())
>           .removeClass("front");
> 
>-        // If it's not focused, the onFocus lsitener would handle it.
>+        // If it's not focused, the onFocus listener would handle it.
>         if (gBrowser.selectedTab == tab)
>           UI.onTabSelect(tab);
>         else
>           gBrowser.selectedTab = tab;
> 
>         if (isNewBlankTab)
>           gWindow.gURLBar.focus();
>

Comment 18

[Michael Yoshitaka Erlewine [:mitcho]]

Note that bug 591705 was the culprit for failing mochitests on jdm's checking.

However, we need a mochitest for this before we can land it.

Comment 19

[Raymond Lee [:raymondlee]]

Attachment: v2

Pushed it to the try server and waiting for the results.

Comment 20

[Raymond Lee [:raymondlee]]

Passed try

Comment 21

[Ian Gilman [:iangilman]]

Comment on attachment 473420 [details] [diff] [review]
v2

Looks good to me.

Comment 22

[Raymond Lee [:raymondlee]]

Attachment: Patch for check-in

Comment 23

[Daniel Holbert [:dholbert]]

http://hg.mozilla.org/mozilla-central/rev/181294166500

Comment 24

[Tracy Walker [:tracy]]

verified with recent builds of minefield