Last Comment Bug 663612 - clicking a group should zoom into the group's active tab
: clicking a group should zoom into the group's active tab
Status: RESOLVED FIXED
:
Product: Firefox Graveyard
Classification: Graveyard
Component: Panorama (show other bugs)
: Trunk
: All All
: -- normal
: Firefox 8
Assigned To: Raymond Lee [:raymondlee]
:
Mentors:
: 627743 (view as bug list)
Depends on: 673729
Blocks: 660175 663611 663613 663614
  Show dependency treegraph
 
Reported: 2011-06-11 03:08 PDT by Tim Taubert [:ttaubert]
Modified: 2016-04-12 14:00 PDT (History)
8 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
v1 (7.83 KB, patch)
2011-06-14 01:04 PDT, Raymond Lee [:raymondlee]
ttaubert: feedback-
Details | Diff | Splinter Review
v2 (6.86 KB, patch)
2011-06-14 14:02 PDT, Raymond Lee [:raymondlee]
ttaubert: feedback+
Details | Diff | Splinter Review
v3 (8.83 KB, patch)
2011-06-14 21:31 PDT, Raymond Lee [:raymondlee]
dietrich: review+
limi: ui‑review+
Details | Diff | Splinter Review
Patch for checkin (9.08 KB, patch)
2011-06-16 10:49 PDT, Raymond Lee [:raymondlee]
no flags Details | Diff | Splinter Review
v5 (8.83 KB, patch)
2011-07-03 21:51 PDT, Raymond Lee [:raymondlee]
ttaubert: feedback+
Details | Diff | Splinter Review
v6 (8.99 KB, patch)
2011-07-04 04:03 PDT, Raymond Lee [:raymondlee]
dao+bmo: review+
Details | Diff | Splinter Review
Patch for checkin (9.29 KB, patch)
2011-07-05 00:37 PDT, Raymond Lee [:raymondlee]
no flags Details | Diff | Splinter Review
v8 (9.14 KB, patch)
2011-07-06 12:54 PDT, Raymond Lee [:raymondlee]
sdwilsh: review+
limi: ui‑review+
ttaubert: feedback+
Details | Diff | Splinter Review

Description Tim Taubert [:ttaubert] 2011-06-11 03:08:09 PDT
* drag/drop of groups is still supported (on the whole container)
* when clicking an empty group create a new tab and zoom into it

https://wiki.mozilla.org/Simplify_Panorama_UI
Comment 1 Raymond Lee [:raymondlee] 2011-06-12 23:01:31 PDT
(In reply to comment #0)
> * drag/drop of groups is still supported (on the whole container)

Is that user can drag and drop a group using the group titlebar area?

> * when clicking an empty group create a new tab and zoom into it

When clicking on a non-empty group, do we create a new tab and zoom into in as well?

> 
> https://wiki.mozilla.org/Simplify_Panorama_UI
Comment 2 Tim Taubert [:ttaubert] 2011-06-13 06:10:33 PDT
(In reply to comment #1)
> (In reply to comment #0)
> > * drag/drop of groups is still supported (on the whole container)
> 
> Is that user can drag and drop a group using the group titlebar area?

I mean: we want to keep the current drag/drop behavior so that the user can drag groups around by clicking the container or the titlebar. If we detect a single click and dragging hasn't started yet then we'll zoom in.

> > * when clicking an empty group create a new tab and zoom into it
> 
> When clicking on a non-empty group, do we create a new tab and zoom into in
> as well?

No, when clicking a non-empty group we just zoom into the group's activeTab (or just the first if non is active somehow).
Comment 3 Raymond Lee [:raymondlee] 2011-06-14 01:04:34 PDT
Created attachment 539152 [details] [diff] [review]
v1

Please apply the patch for Bug 663614 before this one.
Comment 4 Tim Taubert [:ttaubert] 2011-06-14 01:28:06 PDT
Comment on attachment 539152 [details] [diff] [review]
v1

Review of attachment 539152 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/tabview/groupitems.js
@@ +1645,5 @@
> +      if (!self.isDragging) {
> +        if (time > 0)
> +          setTimeout(function() { 
> +            shouldHandleAsClick(time - self._CHECK_HANDLE_AS_CLICK_INTERVAL, callback)
> +          }, self._CHECK_HANDLE_AS_CLICK_INTERVAL);

I don't think we should use setTimeout() to determine if this is a single click. We should use the same approach as with the groupItem title shields (just check the last mousedown target and .isDragging).

https://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabview/groupitems.js#211

::: browser/base/content/test/tabview/browser_tabview_bug663612.js
@@ +17,5 @@
> +  showTabView(function() {
> +    contentWindow = TabView.getContentWindow();
> +    groupItem = createEmptyGroupItem(contentWindow, 300, 300, 200);
> +
> +    testMouseClickOnEmptyGroupItem();

Please use waitForFocus() before starting with all those tests that use EventUtils.

@@ +62,5 @@
> +      groupItem.removeSubscriber(groupItem, "close");
> +      groupItem = null;
> +      finish();
> +    });
> +    groupItem.destroy({ immediately: true });

"closeGroupItem(groupItem, finish)" is much simpler here.
Comment 5 Dão Gottwald [:dao] 2011-06-14 02:13:24 PDT
> > +  showTabView(function() {
> > +    contentWindow = TabView.getContentWindow();
> > +    groupItem = createEmptyGroupItem(contentWindow, 300, 300, 200);
> > +
> > +    testMouseClickOnEmptyGroupItem();
> 
> Please use waitForFocus() before starting with all those tests that use
> EventUtils.

Why doesn't showTabView call waitForFocus (via whenTabViewIsShown)?
Comment 6 Tim Taubert [:ttaubert] 2011-06-14 02:16:08 PDT
(In reply to comment #5)
> > Please use waitForFocus() before starting with all those tests that use
> > EventUtils.
> 
> Why doesn't showTabView call waitForFocus (via whenTabViewIsShown)?

That would be a useful addition so, Raymond, feel free to add this to head.js :)
Comment 7 Raymond Lee [:raymondlee] 2011-06-14 14:02:18 PDT
Created attachment 539317 [details] [diff] [review]
v2

(In reply to comment #4)
> Comment on attachment 539152 [details] [diff] [review] [review]
> v1
> 
> Review of attachment 539152 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/base/content/tabview/groupitems.js
> @@ +1645,5 @@
> > +      if (!self.isDragging) {
> > +        if (time > 0)
> > +          setTimeout(function() { 
> > +            shouldHandleAsClick(time - self._CHECK_HANDLE_AS_CLICK_INTERVAL, callback)
> > +          }, self._CHECK_HANDLE_AS_CLICK_INTERVAL);
> 
> I don't think we should use setTimeout() to determine if this is a single
> click. We should use the same approach as with the groupItem title shields
> (just check the last mousedown target and .isDragging).
> 
> https://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabview/
> groupitems.js#211

Thanks, I missed that part.

> 
> ::: browser/base/content/test/tabview/browser_tabview_bug663612.js
> @@ +17,5 @@
> > +  showTabView(function() {
> > +    contentWindow = TabView.getContentWindow();
> > +    groupItem = createEmptyGroupItem(contentWindow, 300, 300, 200);
> > +
> > +    testMouseClickOnEmptyGroupItem();
> 
> Please use waitForFocus() before starting with all those tests that use
> EventUtils.

Added it to the head.js

> 
> @@ +62,5 @@
> > +      groupItem.removeSubscriber(groupItem, "close");
> > +      groupItem = null;
> > +      finish();
> > +    });
> > +    groupItem.destroy({ immediately: true });
> 
> "closeGroupItem(groupItem, finish)" is much simpler here.

Done
Comment 8 Tim Taubert [:ttaubert] 2011-06-14 15:29:54 PDT
Comment on attachment 539317 [details] [diff] [review]
v2

Review of attachment 539317 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/tabview/groupitems.js
@@ +1642,5 @@
> +      let target = e.target;
> +      if (Utils.isLeftClick(e) && self.$closeButton[0] != target &&
> +          self.$ntb[0] != target && self.$titlebar[0] != target &&
> +          !self.$titlebar.contains(target))
> +        lastMouseDownTarget = target;

Please set lastMouseDownTarget to null here if the condition is not met. We could receive two mousedowns here without a mouseup.

::: browser/base/content/test/tabview/browser_tabview_bug663612.js
@@ +56,5 @@
> +  whenTabViewIsHidden(function() {
> +    is(groupItem.getChildren().length, 1, "The group item still contains one tab item");
> +
> +    closeGroupItem(groupItem, function() {
> +      delete groupItem;

We can't delete a variable declared with let - and we don't need to.

::: browser/base/content/test/tabview/head.js
@@ +131,5 @@
>  function showTabView(callback, win) {
>    win = win || window;
>  
>    if (win.TabView.isVisible()) {
>      callback();

Please add waitForFocus() here, too.
Comment 9 Raymond Lee [:raymondlee] 2011-06-14 21:31:04 PDT
Created attachment 539422 [details] [diff] [review]
v3

(In reply to comment #8)
> Comment on attachment 539317 [details] [diff] [review] [review]
> v2
> 
> Review of attachment 539317 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/base/content/tabview/groupitems.js
> @@ +1642,5 @@
> > +      let target = e.target;
> > +      if (Utils.isLeftClick(e) && self.$closeButton[0] != target &&
> > +          self.$ntb[0] != target && self.$titlebar[0] != target &&
> > +          !self.$titlebar.contains(target))
> > +        lastMouseDownTarget = target;
> 
> Please set lastMouseDownTarget to null here if the condition is not met. We
> could receive two mousedowns here without a mouseup.

Added

> 
> ::: browser/base/content/test/tabview/browser_tabview_bug663612.js
> @@ +56,5 @@
> > +  whenTabViewIsHidden(function() {
> > +    is(groupItem.getChildren().length, 1, "The group item still contains one tab item");
> > +
> > +    closeGroupItem(groupItem, function() {
> > +      delete groupItem;
> 
> We can't delete a variable declared with let - and we don't need to.

Removed and updated registerCleanupFunction since we need to determine whether the groupItem still exists or not at that point.

> 
> ::: browser/base/content/test/tabview/head.js
> @@ +131,5 @@
> >  function showTabView(callback, win) {
> >    win = win || window;
> >  
> >    if (win.TabView.isVisible()) {
> >      callback();
> 
> Please add waitForFocus() here, too.

Added
Comment 10 Dão Gottwald [:dao] 2011-06-14 22:32:28 PDT
Comment on attachment 539422 [details] [diff] [review]
v3

>+    if (createdGroupItem)
>+      closeGroupItem(createdGroupItem, function() {});
>+    hideTabView(function() {});

Apparently closeGroupItem's and hideTabView's callbacks should be optional.
Comment 11 Tim Taubert [:ttaubert] 2011-06-15 01:40:08 PDT
(In reply to comment #10)
> Apparently closeGroupItem's and hideTabView's callbacks should be optional.

Yep, I thought the same when reading this. Filed bug 664379.
Comment 12 Raymond Lee [:raymondlee] 2011-06-15 09:53:39 PDT
Passed Try
http://tbpl.mozilla.org/?tree=Try&rev=8543cfe8ca3a

There is an intermittent orange but not related to this patch.
Comment 13 Ian Gilman [:iangilman] 2011-06-15 13:27:39 PDT
Sorry I'm late to this party, but I just want to point out that we originally had this feature (you could click anywhere in a group to go into its active tab), and we removed it because it was too easy to hit accidentally, and it's a pretty extreme action to have happen when you're not expecting it.
Comment 14 Dietrich Ayala (:dietrich) 2011-06-16 05:34:54 PDT
Comment on attachment 539422 [details] [diff] [review]
v3

Review of attachment 539422 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/tabview/groupitems.js
@@ +1641,5 @@
> +    container.mousedown(function(e) {
> +      let target = e.target;
> +      if (Utils.isLeftClick(e) && self.$closeButton[0] != target &&
> +          self.$ntb[0] != target && self.$titlebar[0] != target &&
> +          !self.$titlebar.contains(target))

that's a bunch of conditions. i'd like to see a comment here summarizing.
Comment 15 Raymond Lee [:raymondlee] 2011-06-16 10:49:12 PDT
Created attachment 539825 [details] [diff] [review]
Patch for checkin

(In reply to comment #14)
> Comment on attachment 539422 [details] [diff] [review] [review]
> v3
> 
> Review of attachment 539422 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/base/content/tabview/groupitems.js
> @@ +1641,5 @@
> > +    container.mousedown(function(e) {
> > +      let target = e.target;
> > +      if (Utils.isLeftClick(e) && self.$closeButton[0] != target &&
> > +          self.$ntb[0] != target && self.$titlebar[0] != target &&
> > +          !self.$titlebar.contains(target))
> 
> that's a bunch of conditions. i'd like to see a comment here summarizing.

Added a comment.


However, I am not sure whether we should land this or not based on comment 13
Comment 16 Alex Faaborg [:faaborg] (Firefox UX) 2011-06-16 16:15:02 PDT
It seems like this makes panorama more complex as opposed to simplifying it.  Two navigation steps before getting to the site also might slow users down.  The one exception is that this might be a more natural way to deal with piles in panorama, instead of the overlay box that we currently have.
Comment 17 Alex Limi (:limi) — Firefox UX Team 2011-06-17 17:55:48 PDT
(In reply to comment #16)
> It seems like this makes panorama more complex as opposed to simplifying it.
> Two navigation steps before getting to the site also might slow users down. 
> The one exception is that this might be a more natural way to deal with
> piles in panorama, instead of the overlay box that we currently have.

It still supports clicking on the specific page (or at least that's the intention, I haven't tried the patch). This just makes it possible to click anywhere in the group to switch to it.
Comment 18 Alex Faaborg [:faaborg] (Firefox UX) 2011-06-17 17:58:49 PDT
ok, I slightly misunderstood the change (thought this was zooming the group to fill the panorama area before letting you select one of the pages).

I would want to play around with what this feels like for awhile, it seems like group switching would feel a lot faster, which is good (you don't have the cognitive load of picking a target inside of the group first), but is also bad (if you were trying to move the group around, you select it by mistake).  Perhaps land first on the UX branch?
Comment 19 Raymond Lee [:raymondlee] 2011-06-17 21:41:19 PDT
(In reply to comment #18)
> ok, I slightly misunderstood the change (thought this was zooming the group
> to fill the panorama area before letting you select one of the pages).
> 
> I would want to play around with what this feels like for awhile, it seems
> like group switching would feel a lot faster, which is good (you don't have
> the cognitive load of picking a target inside of the group first), but is
> also bad (if you were trying to move the group around, you select it by
> mistake).  Perhaps land first on the UX branch?

Yes, it would be good to land on the UX branch first and then decide what we want to do with this patch.
Comment 20 Tim Taubert [:ttaubert] 2011-06-17 23:58:48 PDT
I tried to push this patch to ux-branch but seems I don't have the permissions to do so:

remote: abort: could not lock repository /repo/hg/mozilla/projects/ux: Permission denied

Who else could do this for us?
Comment 21 Tim Taubert [:ttaubert] 2011-06-24 00:14:33 PDT
Comment on attachment 539422 [details] [diff] [review]
v3

This patch landed on ux-branch yesterday.
Comment 22 Alex Limi (:limi) — Firefox UX Team 2011-07-01 11:59:42 PDT
Thank you! This works great, and feels right.

The one bug I found is if you have an app tab as the active tab, it seems to pick the first tab that is not an app tab instead — i.e. the app tab doesn't seem to count as "active".

Especially once we get global app tabs (which means we don't have to show app tabs in every group), this is going to be confusing if it doesn't keep the app tab active if that was the one you just came from. :)
Comment 23 Alex Limi (:limi) — Firefox UX Team 2011-07-01 12:01:38 PDT
Also, even if you explicitly click on the app tab icon in that group, it doesn't activate it.
Comment 24 Alex Limi (:limi) — Firefox UX Team 2011-07-01 12:17:17 PDT
Comment on attachment 539422 [details] [diff] [review]
v3

Approved as long as the app tab bug is addressed.
Comment 25 Raymond Lee [:raymondlee] 2011-07-01 20:03:26 PDT
(In reply to comment #22)
> Thank you! This works great, and feels right.
> 
> The one bug I found is if you have an app tab as the active tab, it seems to
> pick the first tab that is not an app tab instead — i.e. the app tab doesn't
> seem to count as "active".
> 
> Especially once we get global app tabs (which means we don't have to show
> app tabs in every group), this is going to be confusing if it doesn't keep
> the app tab active if that was the one you just came from. :)

I believe the issue would be addressed by bug 600665
Comment 26 Tim Taubert [:ttaubert] 2011-07-03 12:12:22 PDT
(In reply to comment #25)
> > The one bug I found is if you have an app tab as the active tab, it seems to
> > pick the first tab that is not an app tab instead — i.e. the app tab doesn't
> > seem to count as "active".
> > 
> > Especially once we get global app tabs (which means we don't have to show
> > app tabs in every group), this is going to be confusing if it doesn't keep
> > the app tab active if that was the one you just came from. :)
> 
> I believe the issue would be addressed by bug 600665

No, the one bug left is that clicking an app tab is handled by the container's new click handler. We need to add the app tab tray to the exclusion list here:

>+      if (Utils.isLeftClick(e) && self.$closeButton[0] != target &&
>+          self.$ntb[0] != target && self.$titlebar[0] != target &&
>+          !self.$titlebar.contains(target))
>+        lastMouseDownTarget = target;
Comment 27 Raymond Lee [:raymondlee] 2011-07-03 21:51:03 PDT
Created attachment 543717 [details] [diff] [review]
v5

(In reply to comment #26)
> No, the one bug left is that clicking an app tab is handled by the
> container's new click handler. We need to add the app tab tray to the
> exclusion list here:
> 
> >+      if (Utils.isLeftClick(e) && self.$closeButton[0] != target &&
> >+          self.$ntb[0] != target && self.$titlebar[0] != target &&
> >+          !self.$titlebar.contains(target))
> >+        lastMouseDownTarget = target;

Done
Comment 28 Tim Taubert [:ttaubert] 2011-07-04 00:45:46 PDT
Comment on attachment 543717 [details] [diff] [review]
v5

Review of attachment 543717 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, that looks and works good! Sorry, you were actually right about bug 600665 because we still need to keep the app tab focused when leaving Panorama with a click on a group (as per comment #22). We can fix this for now pretty easily (see below).

F+ with that additional fix.

::: browser/base/content/tabview/groupitems.js
@@ +1644,5 @@
> +    container.mouseup(function(e) {
> +      let same = (e.target == lastMouseDownTarget);
> +      lastMouseDownTarget = null;
> +
> +      if (same && !self.isDragging) {

if (gBrowser.selectedTab.pinned && UI.getActiveTab() != self.getActiveTab()) {
  UI.goToTab(gBrowser.selectedTab);
} else {
  // do zoom into group's active tab...
}
Comment 29 Raymond Lee [:raymondlee] 2011-07-04 04:03:01 PDT
Created attachment 543741 [details] [diff] [review]
v6

(In reply to comment #28)
> Comment on attachment 543717 [details] [diff] [review] [review]
> v5
> 
> Review of attachment 543717 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> Thanks, that looks and works good! Sorry, you were actually right about bug
> 600665 because we still need to keep the app tab focused when leaving
> Panorama with a click on a group (as per comment #22). We can fix this for
> now pretty easily (see below).
> 
> F+ with that additional fix.
> 
> ::: browser/base/content/tabview/groupitems.js
> @@ +1644,5 @@
> > +    container.mouseup(function(e) {
> > +      let same = (e.target == lastMouseDownTarget);
> > +      lastMouseDownTarget = null;
> > +
> > +      if (same && !self.isDragging) {
> 
> if (gBrowser.selectedTab.pinned && UI.getActiveTab() != self.getActiveTab())
> {
>   UI.goToTab(gBrowser.selectedTab);
> } else {
>   // do zoom into group's active tab...
> }

Added
Comment 30 Tim Taubert [:ttaubert] 2011-07-04 16:22:47 PDT
We'd really like to get this into Fx 7 together with bug 663613 and bug 663611. That patch is just a little addition to the already reviewed one... Dão, if you could review this in time we and the UX team would be truly grateful :)
Comment 31 Dão Gottwald [:dao] 2011-07-04 23:13:12 PDT
Comment on attachment 543741 [details] [diff] [review]
v6

>+      // only set the last mouse down target if it is a left click, not on the
>+      // close button, not on the new tab button, not on the title bar and its
>+      // element
>+      if (Utils.isLeftClick(e) && self.$closeButton[0] != target &&
>+          self.$ntb[0] != target && self.$titlebar[0] != target &&
>+          !self.$titlebar.contains(target) &&
>+          !self.$appTabTray.contains(target))

break the line after &&

>+        if (gBrowser.selectedTab.pinned && UI.getActiveTab() != self.getActiveTab()) {

ditto

I only reviewed the v3->v6 interdiff.
Comment 32 Raymond Lee [:raymondlee] 2011-07-05 00:37:37 PDT
Created attachment 543888 [details] [diff] [review]
Patch for checkin

(In reply to comment #31)
> Comment on attachment 543741 [details] [diff] [review] [review]
> v6
> 
> >+      // only set the last mouse down target if it is a left click, not on the
> >+      // close button, not on the new tab button, not on the title bar and its
> >+      // element
> >+      if (Utils.isLeftClick(e) && self.$closeButton[0] != target &&
> >+          self.$ntb[0] != target && self.$titlebar[0] != target &&
> >+          !self.$titlebar.contains(target) &&
> >+          !self.$appTabTray.contains(target))
> 
> break the line after &&
> 

Updated

> >+        if (gBrowser.selectedTab.pinned && UI.getActiveTab() != self.getActiveTab()) {
> 
> ditto
>

Updated 
> I only reviewed the v3->v6 interdiff.
Comment 33 Tim Taubert [:ttaubert] 2011-07-05 01:31:10 PDT
Thanks for your quick review, Dão! Alas, this doesn't seem ready to land, yet. I noticed two remaining problems:

Have some app tabs and one of them is the selectedTab. Create an empty group. Click this empty group.

1) If you viewed the app tab in another group before switching to Panorama this group will be shown again. So we'd need to additionally switch the group when an app tab is active, like here:

https://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabview/groupitems.js#1169

2) What is the expected workflow when there is an active app tab and we click an empty group? Should a new tab be created? Should no new tab be created and we'll just show the app tab? (That's the current behavior.) Seems like a question for the UX team.
Comment 34 Raymond Lee [:raymondlee] 2011-07-06 12:54:43 PDT
Created attachment 544316 [details] [diff] [review]
v8

(In reply to comment #33)
> Thanks for your quick review, Dão! Alas, this doesn't seem ready to land,
> yet. I noticed two remaining problems:
> 
> Have some app tabs and one of them is the selectedTab. Create an empty
> group. Click this empty group.
> 
> 1) If you viewed the app tab in another group before switching to Panorama
> this group will be shown again. So we'd need to additionally switch the
> group when an app tab is active, like here:
> 
> https://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabview/
> groupitems.js#1169

Fixed

> 
> 2) What is the expected workflow when there is an active app tab and we
> click an empty group? Should a new tab be created? Should no new tab be
> created and we'll just show the app tab? (That's the current behavior.)
> Seems like a question for the UX team.

I've added the code to create new tab if the group is empty.  Need feedback from UX team.
Comment 35 Tim Taubert [:ttaubert] 2011-07-06 13:23:25 PDT
Comment on attachment 544316 [details] [diff] [review]
v8

Review of attachment 544316 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!
Comment 36 Alex Limi (:limi) — Firefox UX Team 2011-07-12 17:25:32 PDT
Comment on attachment 544316 [details] [diff] [review]
v8

Works great!
Comment 37 Tim Taubert [:ttaubert] 2011-07-12 17:28:07 PDT
Comment on attachment 544316 [details] [diff] [review]
v8

Review of attachment 544316 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!
Comment 38 Shawn Wilsher :sdwilsh 2011-07-17 13:40:44 PDT
Comment on attachment 544316 [details] [diff] [review]
v8

Review of attachment 544316 [details] [diff] [review]:
-----------------------------------------------------------------

r=sdwilsh

::: browser/base/content/tabview/groupitems.js
@@ +1666,5 @@
> +          !self.$titlebar.contains(target) &&
> +          !self.$appTabTray.contains(target))
> +        lastMouseDownTarget = target;
> +      else
> +        lastMouseDownTarget = null;

Please brace your ifs if they are this complicated.

::: browser/base/content/test/tabview/browser_tabview_bug663612.js
@@ +1,2 @@
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */

Please use a descriptive name for this filename instead of a bug number.
Comment 39 Tim Taubert [:ttaubert] 2011-07-19 06:50:53 PDT
http://hg.mozilla.org/integration/fx-team/rev/936e084e74b8
Comment 40 Tim Taubert [:ttaubert] 2011-07-21 03:58:13 PDT
http://hg.mozilla.org/mozilla-central/rev/936e084e74b8
Comment 41 Virgil Dicu [:virgil] [QA] 2011-09-08 05:05:36 PDT
Mozilla/5.0 (X11; Linux x86_64; rv:9.0a1) Gecko/20110907 Firefox/9.0a1

Focusing a group works fine. When selecting any group, the active tab from the last group is selected. 
2 questions, however:

When app tabs are available and one of them is active in any group, the respective app tab will be displayed every time another group is focused in panorama. Shouldn't other groups focus on the previously active tab or app tab in that group?

Regarding the second question in Comment 33, when an empty group with global app tabs is focused, a new tab is created. Is this the approved behavior?
Comment 42 eyal gruss (eyaler) 2011-09-29 20:03:31 PDT
*** Bug 627743 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.