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

RESOLVED FIXED in Firefox 14

Status

Firefox Graveyard
Panorama
RESOLVED FIXED
6 years ago
a year ago

People

(Reporter: Johan C, Assigned: raymondlee)

Tracking

Trunk
Firefox 14

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

6 years ago
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.
(Assignee)

Comment 1

6 years ago
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?
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 :)
(Assignee)

Updated

6 years ago
Assignee: nobody → raymond
Status: NEW → ASSIGNED
(Assignee)

Comment 3

6 years ago
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.
Attachment #607882 - Flags: review?(ttaubert)
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.
Attachment #607882 - Flags: review?(ttaubert) → review+
(Assignee)

Comment 5

6 years ago
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
Attachment #607882 - Attachment is obsolete: true
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();
>+            }
(Assignee)

Comment 7

6 years ago
(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.
(Assignee)

Comment 8

6 years ago
(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.
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?
(Assignee)

Comment 10

6 years ago
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
Attachment #608751 - Attachment is obsolete: true
(Assignee)

Comment 11

6 years ago
Submitted to try and and passed.
https://tbpl.mozilla.org/?tree=Try&rev=b23f6ed3b396
https://hg.mozilla.org/integration/fx-team/rev/b1455bac2454
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 14
Version: unspecified → Trunk
https://hg.mozilla.org/mozilla-central/rev/b1455bac2454
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.