663613 - remove "new tab" button from groups

Status: VERIFIED FIXED

(Firefox Graveyard :: Panorama, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

https://wiki.mozilla.org/Simplify_Panorama_UI

Comment 1

[Raymond Lee [:raymondlee]]

Attachment: v1

Please apply patch for bug 663612 before this one

Comment 2

[Raymond Lee [:raymondlee]]

(In reply to comment #1)
> Created attachment 539465 [details] [diff] [review] [review]
> v1
> 
> Please apply patch for bug 663612 before this one

Passed Try
http://tbpl.mozilla.org/?tree=Try&rev=b3c639db61ed

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 539465 [details] [diff] [review]
v1

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

Does that patch also remove the file "new-tab.png"?

::: browser/base/content/test/tabview/browser_tabview_bug588265.js
@@ +11,5 @@
> +    while (gBrowser.tabs[1])
> +      gBrowser.removeTab(gBrowser.tabs[1]);
> +    hideTabView(function() {});
> +  });
> +  gBrowser.loadOneTab("about:blank", { inBackground: false });

Please change inBackground to "true" so that we don't trigger a transition here just in case the tabview is already shown.

@@ +60,5 @@
>        is(groupItemOne.getChildren().length, 2, 
>           "The num of childen in group one is 2");
>  
>        // clean up and finish
>        groupItemTwo.addSubscriber(groupItemTwo, "close", function() {

You could use closeGroupItem(groupItemTwo, callback) here.

::: browser/base/content/test/tabview/browser_tabview_bug590606.js
@@ +36,5 @@
>         "The currently selected tab should be the first tab in the groupItemOne");
>  
>      // create another group with a tab.
> +    let groupItemTwo = createGroupItemWithBlankTabs(window, 300, 300, 40, 1);
> +    groupItemTwoId = groupItemTwoId;

groupItemTwoId = groupItemTwo.id;

::: browser/base/content/test/tabview/browser_tabview_bug595560.js
@@ +35,5 @@
>  }
>  
>  function testThree() {
> +  // create another group with a tab.
> +  let groupItem = createGroupItemWithBlankTabs(win, 300, 300, 40, 1);

Is there a reason why you changed the groupItem's padding?

@@ +37,5 @@
>  function testThree() {
> +  // create another group with a tab.
> +  let groupItem = createGroupItemWithBlankTabs(win, 300, 300, 40, 1);
> +  hideTabView(function() {
> +    showTabView(function() {

Is hiding and showing the tabview here part of the test? That seems superfluous.

@@ +67,3 @@
>  }
>  
>  function whenSearchEnabledAndDisabled(callback) {

Nit: That function name does not correctly describe what the function is doing.

::: browser/base/content/test/tabview/browser_tabview_group.js
@@ +14,3 @@
>    ok(TabView.isVisible(), "Tab View is visible");
>  
>    let contentWindow = document.getElementById("tab-view").contentWindow;

Please make that TabView.getContentWindow().

::: browser/base/content/test/tabview/browser_tabview_orphaned_tabs.js
@@ +18,3 @@
>    ok(newWin.TabView.isVisible(), "Tab View is visible");
>  
>    let contentWindow = newWin.document.getElementById("tab-view").contentWindow;

newWin.TabView.getContentWindow(), please :)

@@ +23,5 @@
>    ok(tabOne._tabViewTabItem.parent, "Tab one belongs to a group");
>    is(contentWindow.GroupItems.getOrphanedTabs().length, 0, "No orphaned tabs");
>  
> +  // 2) create a group, add a blank tab
> +  let groupItem = createGroupItemWithBlankTabs(newWin, 250, 250, 40, 1);

Is there a reason why you changed the groupItem's size and padding?

::: browser/base/content/test/tabview/browser_tabview_undo_group.js
@@ +10,5 @@
> +    while (gBrowser.tabs[1])
> +      gBrowser.removeTab(gBrowser.tabs[1]);
> +    hideTabView(function() {});
> +  });
> +  showTabView(onTabViewWindowLoaded)

Nit: missing semicolon

::: browser/base/content/test/tabview/head.js
@@ +33,5 @@
>      ++t;
>    });
> +  // to set one of tabItem to be active since we load tabs into a group 
> +  // in a non-standard flow.
> +  contentWindow.UI.setActive(groupItem);

Shouldn't that be done by https://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabview/groupitems.js#1023 - if not we should rather fix it there? The workflow doesn't seem so non-standard.

Comment 4

[Raymond Lee [:raymondlee]]

Attachment: v2

(In reply to comment #3)
> Comment on attachment 539465 [details] [diff] [review] [review]
> v1

Fixed everything except the following

> 
> ::: browser/base/content/test/tabview/head.js
> @@ +33,5 @@
> >      ++t;
> >    });
> > +  // to set one of tabItem to be active since we load tabs into a group 
> > +  // in a non-standard flow.
> > +  contentWindow.UI.setActive(groupItem);
> 
> Shouldn't that be done by
> https://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabview/
> groupitems.js#1023 - if not we should rather fix it there? The workflow
> doesn't seem so non-standard.

The workflow is a non-standard one.  User wouldn't be able to create an empty group item with a tab item in it, and get the tab item selected (with blue border) in Panorama without going through:
1) Drag and drop to create a group
2) Double click to create a tab item
3) Zoom into animation happens automatically and UI.setActive() would be called in TabItem_zoomIn()
4) UI.setActive() is also get called in UI_showTabView();

The createGroupItemWithTabs in head.js doesn't do step 3 or 4 so the tab item doesn't get selected.  Since that is a test helper function and not doing a standard workflow, IMO we shouldn't do anything in the Panorama code.

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

(In reply to comment #4)
> The workflow is a non-standard one.  User wouldn't be able to create an
> empty group item with a tab item in it, and get the tab item selected (with
> blue border) in Panorama without going through:
> 1) Drag and drop to create a group
> 2) Double click to create a tab item
> 3) Zoom into animation happens automatically and UI.setActive() would be
> called in TabItem_zoomIn()
> 4) UI.setActive() is also get called in UI_showTabView();
> 
> The createGroupItemWithTabs in head.js doesn't do step 3 or 4 so the tab
> item doesn't get selected.  Since that is a test helper function and not
> doing a standard workflow, IMO we shouldn't do anything in the Panorama code.

Convinced :)

Comment 6

[Ian Gilman [:iangilman]]

Comment on attachment 540968 [details] [diff] [review]
v2

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

Everything else looks good.

::: browser/base/content/test/tabview/browser_tabview_bug588265.js
@@ +11,5 @@
> +    while (gBrowser.tabs[1])
> +      gBrowser.removeTab(gBrowser.tabs[1]);
> +    hideTabView(function() {});
> +  });
> +  gBrowser.loadOneTab("about:blank", { inBackground: true });

Why are we loading a tab here? The rest of the changes to this test seem natural enough, but this looks like an addition (unless I'm missing something).

Comment 7

[Raymond Lee [:raymondlee]]

(In reply to comment #6)
> Comment on attachment 540968 [details] [diff] [review] [review]
> v2
> 
> Review of attachment 540968 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> Everything else looks good.
> 
> ::: browser/base/content/test/tabview/browser_tabview_bug588265.js
> @@ +11,5 @@
> > +    while (gBrowser.tabs[1])
> > +      gBrowser.removeTab(gBrowser.tabs[1]);
> > +    hideTabView(function() {});
> > +  });
> > +  gBrowser.loadOneTab("about:blank", { inBackground: true });
> 
> Why are we loading a tab here? The rest of the changes to this test seem
> natural enough, but this looks like an addition (unless I'm missing
> something).

Since the test needs to have two tab items in a group, the original code shows tabView, adds a tab item, hides tabView, etc. I think it would be much simple to add a new tab so when tabView is shown, two tab items would be in a group item.

Comment 8

[Raymond Lee [:raymondlee]]

Attachment: Patch for checkin

Please apply patch for bug 663612 before this.

Passed try.  There are two intermittents which aren't related to this.
http://tbpl.mozilla.org/?tree=Try&rev=fbfba4260605

Comment 9

[Ian Gilman [:iangilman]]

(In reply to comment #7)
> Since the test needs to have two tab items in a group, the original code
> shows tabView, adds a tab item, hides tabView, etc. I think it would be much
> simple to add a new tab so when tabView is shown, two tab items would be in
> a group item.

Thank you for the explanation!

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 540968 [details] [diff] [review]
v2

This patch landed on ux-branch yesterday.

Comment 11

[Alex Limi (:limi) — Firefox UX Team]

Comment on attachment 540968 [details] [diff] [review]
v2

Works as intended!

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

http://hg.mozilla.org/integration/fx-team/rev/54ed6181b38c

Comment 13

[Siddhartha Dugar [:sdrocking]]

The button has been removed (in latest UX builds) but still tab thumbnails are not allowed in the bottom area of the tab groups. Please verify and fix this.

Comment 14

[Raymond Lee [:raymondlee]]

(In reply to comment #13)
> The button has been removed (in latest UX builds) but still tab thumbnails
> are not allowed in the bottom area of the tab groups. Please verify and fix
> this.

That is related to bug 595224

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

http://hg.mozilla.org/mozilla-central/rev/54ed6181b38c

Comment 16

[Andreas Jung]

(In reply to comment #14)
> (In reply to comment #13)
> > The button has been removed (in latest UX builds) but still tab thumbnails
> > are not allowed in the bottom area of the tab groups. Please verify and fix
> > this.
> 
> That is related to bug 595224

Well, the bottom 33px seem to be never used because of groupitems.js:
509     box.height -= 33; // For new tab button

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

Now that the new tab button is removed this line should probably go away as well.

Comment 17

[Tim Taubert [:ttaubert] (inactive)]

(In reply to comment #16)
> Now that the new tab button is removed this line should probably go away as
> well.

Thanks, filed follow-up bug 673825.

Comment 18

[Virgil Dicu [:virgil] [QA]]

Mozilla/5.0 (X11; Linux x86_64; rv:9.0a1) Gecko/20110907 Firefox/9.0a1

The "new tab" button has been removed. Setting to Verified Fixed.