Closed Bug 587040 Opened 9 years ago Closed 9 years ago

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

Categories

(Firefox Graveyard :: Panorama, defect, P3)

x86
All
defect

Tracking

(Not tracked)

VERIFIED FIXED
Firefox 4.0b7

People

(Reporter: martijn.martijn, Assigned: raymondlee)

References

Details

(Whiteboard: [Aug-13-testday] )

Attachments

(1 file, 4 obsolete files)

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.
Depends on: 587047
Component: Tabbed Browser → TabCandy
QA Contact: tabbed.browser → tabcandy
I believe something related to bug 566643 added the functionality of click-anywhere-in-group-to-activate-group.
Depends on: 566643
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.
Aza/Ian: what is the expected behaviour for this?
(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.
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.
Raymond, can you fix the bug Mardak noted (that it doesn't always open into the correct group)?
Assignee: nobody → raymond
Status: NEW → ASSIGNED
Attached patch v1 (obsolete) — Splinter Review
Attachment #468209 - Flags: review?(dolske)
Attachment #468209 - Flags: feedback?(ian)
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
Ian, can you review Raymond's patch?
Assignee: raymond → ian
Comment on attachment 468209 [details] [diff] [review]
v1

Looks good.
Attachment #468209 - Flags: feedback?(ian) → feedback+
Attachment #468209 - Flags: review?(dolske) → review?(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.
Attachment #468209 - Flags: review?(dietrich)
Attachment #468209 - Flags: review+
Attachment #468209 - Flags: approval2.0+
Summary: Clicking in the tab group box opens the last viewed tab in that group for some reason → After navigating with arrow keys, active tab highlighting will move but clicking on the group will bring up the last active tab
Attached patch Patch NOT for checkin (obsolete) — Splinter Review
Assignee: ian → mitcho
Attachment #468209 - Attachment is obsolete: true
mitcho, not sure if you're aware, but you're stealing people's bugs and patches, which isn't cool. Please stop. :)
Assignee: mitcho → raymond
Keywords: checkin-needed
Attachment #472015 - Attachment description: Patch for checkin → Patch NOT for checkin
Attachment #472015 - Attachment is obsolete: true
Attachment #468209 - Attachment is obsolete: false
Attachment #468209 - Attachment is obsolete: true
And backed out due to Moth test failures.
Depends on: 593669
Comment on attachment 472054 [details] [diff] [review]
Patch for checkin, for real this time

># HG changeset patch
># User Raymond Lee <raymond@appcoast.com>
># 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();
>
Attachment #472054 - Attachment is obsolete: true
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.
Blocks: 587047, 593669
No longer depends on: 587047, 593669
Attached patch v2 (obsolete) — Splinter Review
Pushed it to the try server and waiting for the results.
Attachment #473420 - Flags: feedback?(ian)
Blocks: 588439
Passed try
Comment on attachment 473420 [details] [diff] [review]
v2

Looks good to me.
Attachment #473420 - Flags: review?(dietrich)
Attachment #473420 - Flags: feedback?(ian)
Attachment #473420 - Flags: feedback+
Blocks: 597043
Priority: -- → P3
Attachment #473420 - Flags: review?(dietrich)
Attachment #473420 - Flags: review+
Attachment #473420 - Flags: approval2.0+
Attachment #473420 - Attachment is obsolete: true
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/181294166500
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b7
Version: unspecified → Trunk
verified with recent builds of minefield
Status: RESOLVED → VERIFIED
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.