Last Comment Bug 733115 - After creating a new tab group, 2nd RETURN should open that group.
: After creating a new tab group, 2nd RETURN should open that group.
Status: RESOLVED FIXED
:
Product: Firefox Graveyard
Classification: Graveyard
Component: Panorama (show other bugs)
: Trunk
: All All
: -- normal
: Firefox 14
Assigned To: Raymond Lee [:raymondlee]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-05 13:06 PST by Johan C
Modified: 2016-04-12 14:00 PDT (History)
3 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
v1 (5.46 KB, patch)
2012-03-21 02:48 PDT, Raymond Lee [:raymondlee]
ttaubert: review+
Details | Diff | Splinter Review
Patch for checkin (5.55 KB, patch)
2012-03-23 10:28 PDT, Raymond Lee [:raymondlee]
no flags Details | Diff | Splinter Review
Patch for checkin (5.44 KB, patch)
2012-03-26 06:01 PDT, Raymond Lee [:raymondlee]
no flags Details | Diff | Splinter Review

Description Johan C 2012-03-05 13:06:56 PST
Steps to Reproduce:
1. Create a new tab group.
(The name field is focused)
2. Enter a name and hit return / or hit return without setting a name.
3. Hit return again (2nd return).

Actual result:
The previously opened tab group and tab is opened.

Expected result:
The newly created tab group should open with a new tab.
Comment 1 Raymond Lee [:raymondlee] 2012-03-12 19:57:24 PDT
At the moment, when user presses RETURN, the currently selected (tab with blue borders) would be zoomed in and opened.

@tim: what's your thought about this bug's STR?
Comment 2 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2012-03-19 06:36:51 PDT
I think that this STR is a valid expectation. We could maybe let groups be active even without having any children. This might affect other parts of the code and other behavior as well. I guess we'll find out running the tests :)
Comment 3 Raymond Lee [:raymondlee] 2012-03-21 02:48:38 PDT
Created attachment 607882 [details] [diff] [review]
v1

When an empty group is active and doesn't contain any tabitem, press Enter would create a new tab in that group and exit Panorama.
Comment 4 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2012-03-23 04:54:04 PDT
Comment on attachment 607882 [details] [diff] [review]
v1

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

Looks good!

r=me with the comments fixed and a little re-structuring:

::: browser/components/tabview/ui.js
> +  activeTab = self.getActiveTab();
> +
> +  if (activeTab && activeTab.parent == activeGroupItem) {
> +    activeTab.zoomIn();
> +  } else {
> +    activeTab = activeGroupItem.getActiveTab();
> +    if (activeTab)
> +      activeTab.zoomIn();
> +    else
> +      activeGroupItem.newTab();
> +  }

We can write it like this:

>   activeTab = self.getActiveTab();
>   if (!activeTab || activeTab.parent != activeGroupItem)
>     activeTab = activeGroupItem.getActiveTab();
>
>   if (activeTab)
>     activeTab.zoomIn();
>   else
>     activeGroupItem.newTab();

::: browser/components/tabview/test/browser_tabview_bug733115.js
@@ +14,5 @@
> +    let contentWindow = TabView.getContentWindow();
> +    let groupItem = createEmptyGroupItem(contentWindow, 200, 200, 20);
> +    contentWindow.GroupItems.setActiveGroupItem(groupItem);
> +
> +    // new tab should be created and exit tabview for empty group.

Nit: // A new tab should be added to the active group and tabview should zoom into it.

@@ +20,5 @@
> +    whenTabViewIsHidden(function() {
> +      is(groupItem.getChildren().length, 1, "This group has one tabitem");
> +
> +      showTabView(function() {
> +        // ensure that no more new tab is added to this non-empty group.

Nit: // Ensure that no new tab is added to this non-empty tab group.
Comment 5 Raymond Lee [:raymondlee] 2012-03-23 10:28:05 PDT
Created attachment 608751 [details] [diff] [review]
Patch for checkin

Fixed the comments in comment 4.

Pushed to try and waiting for results
https://tbpl.mozilla.org/?tree=Try&rev=c838fea3a7d2
Comment 6 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2012-03-24 01:22:30 PDT
Comment on attachment 608751 [details] [diff] [review]
Patch for checkin

>diff --git a/browser/components/tabview/ui.js b/browser/components/tabview/ui.js
>+            activeGroupItem = GroupItems.getActiveGroupItem();
>+            if (activeGroupItem) {
>+              activeTab = self.getActiveTab();
>+
>+              if (!activeTab || activeTab.parent != activeGroupItem) {
>+                activeTab = activeGroupItem.getActiveTab();
>+
>+                if (activeTab)
>+                  activeTab.zoomIn();
>+                else
>+                  activeGroupItem.newTab();
>+              } else {
>+                activeTab.zoomIn();
>+              }
>+            }

That's not what I meant. I didn't want those duplicate "activeTab.zoomIn()" lines:

>+            activeGroupItem = GroupItems.getActiveGroupItem();
>+            if (activeGroupItem) {
>+              activeTab = self.getActiveTab();
>+
>+              if (!activeTab || activeTab.parent != activeGroupItem)
>+                activeTab = activeGroupItem.getActiveTab();
>+
>+              if (activeTab)
>+                activeTab.zoomIn();
>+              else
>+                activeGroupItem.newTab();
>+            }
Comment 7 Raymond Lee [:raymondlee] 2012-03-25 06:19:59 PDT
(In reply to Tim Taubert [:ttaubert] from comment #6)
> Comment on attachment 608751 [details] [diff] [review]
> Patch for checkin
> 
> >diff --git a/browser/components/tabview/ui.js b/browser/components/tabview/ui.js
> >+            activeGroupItem = GroupItems.getActiveGroupItem();
> >+            if (activeGroupItem) {
> >+              activeTab = self.getActiveTab();
> >+
> >+              if (!activeTab || activeTab.parent != activeGroupItem) {
> >+                activeTab = activeGroupItem.getActiveTab();
> >+
> >+                if (activeTab)
> >+                  activeTab.zoomIn();
> >+                else
> >+                  activeGroupItem.newTab();
> >+              } else {
> >+                activeTab.zoomIn();
> >+              }
> >+            }
> 
> That's not what I meant. I didn't want those duplicate "activeTab.zoomIn()"
> lines:
> 
> >+            activeGroupItem = GroupItems.getActiveGroupItem();
> >+            if (activeGroupItem) {
> >+              activeTab = self.getActiveTab();
> >+
> >+              if (!activeTab || activeTab.parent != activeGroupItem)
> >+                activeTab = activeGroupItem.getActiveTab();
> >+
> >+              if (activeTab)
> >+                activeTab.zoomIn();
> >+              else
> >+                activeGroupItem.newTab();
> >+            }

If we removed the duplicate "activeTab.zoomIn()", we wouldn't zoom into the activeTab when user remains the active group item.  I don't think that's behaviour we want.
Comment 8 Raymond Lee [:raymondlee] 2012-03-25 06:24:02 PDT
(In reply to Raymond Lee [:raymondlee] from comment #7)
> 
> If we removed the duplicate "activeTab.zoomIn()", we wouldn't zoom into the
> activeTab when user remains the active group item.  I don't think that's
> behaviour we want.

I mean reamins in the same active group item.
Comment 9 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2012-03-26 04:30:08 PDT
The code I wrote above does zoom into the active tab when (activeTab.parent == activeGroupItem). The indentation and the block structure is a bit different from how the code was before. Am I missing something?
Comment 10 Raymond Lee [:raymondlee] 2012-03-26 06:01:29 PDT
Created attachment 609295 [details] [diff] [review]
Patch for checkin

Sorry, I thought there are curly brackets for the if condition.  I've updated the patch based on your comment.

https://tbpl.mozilla.org/?tree=Try&rev=9c83cd7a0d8c
Comment 11 Raymond Lee [:raymondlee] 2012-03-29 03:03:16 PDT
Submitted to try and and passed.
https://tbpl.mozilla.org/?tree=Try&rev=b23f6ed3b396
Comment 12 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2012-03-29 04:25:40 PDT
https://hg.mozilla.org/integration/fx-team/rev/b1455bac2454
Comment 13 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2012-03-30 09:33:47 PDT
https://hg.mozilla.org/mozilla-central/rev/b1455bac2454

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