733115 - After creating a new tab group, 2nd RETURN should open that group.

Status: RESOLVED FIXED

(Firefox Graveyard :: Panorama, defect)

Description

[Johan C]

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]]

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] (inactive)]

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]]

Attachment: 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] (inactive)]

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]]

Attachment: 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] (inactive)]

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]]

(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]]

(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] (inactive)]

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]]

Attachment: 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]]

Submitted to try and and passed.
https://tbpl.mozilla.org/?tree=Try&rev=b23f6ed3b396

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/b1455bac2454

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/b1455bac2454