Last Comment Bug 663613 - remove "new tab" button from groups
: remove "new tab" button from groups
Status: VERIFIED FIXED
:
Product: Firefox Graveyard
Classification: Graveyard
Component: Panorama (show other bugs)
: Trunk
: All All
: -- normal
: Firefox 8
Assigned To: Raymond Lee [:raymondlee]
:
:
Mentors:
Depends on: 663612
Blocks: 595402 597767 660175 671347
  Show dependency treegraph
 
Reported: 2011-06-11 03:10 PDT by Tim Taubert [:ttaubert]
Modified: 2016-04-12 14:00 PDT (History)
6 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
v1 (34.88 KB, patch)
2011-06-15 02:51 PDT, Raymond Lee [:raymondlee]
ttaubert: feedback+
Details | Diff | Splinter Review
v2 (35.67 KB, patch)
2011-06-21 21:53 PDT, Raymond Lee [:raymondlee]
ian: review+
limi: ui‑review+
Details | Diff | Splinter Review
Patch for checkin (35.91 KB, patch)
2011-06-23 08:15 PDT, Raymond Lee [:raymondlee]
no flags Details | Diff | Splinter Review

Description Tim Taubert [:ttaubert] 2011-06-11 03:10:12 PDT
https://wiki.mozilla.org/Simplify_Panorama_UI
Comment 1 Raymond Lee [:raymondlee] 2011-06-15 02:51:59 PDT
Created attachment 539465 [details] [diff] [review]
v1

Please apply patch for bug 663612 before this one
Comment 2 Raymond Lee [:raymondlee] 2011-06-15 11:51:05 PDT
(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] 2011-06-17 07:10:24 PDT
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] 2011-06-21 21:53:00 PDT
Created attachment 540968 [details] [diff] [review]
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] 2011-06-22 03:24:28 PDT
(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] 2011-06-22 10:11:52 PDT
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] 2011-06-23 08:10:22 PDT
(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] 2011-06-23 08:15:20 PDT
Created attachment 541384 [details] [diff] [review]
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] 2011-06-23 09:47:38 PDT
(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] 2011-06-24 00:15:13 PDT
Comment on attachment 540968 [details] [diff] [review]
v2

This patch landed on ux-branch yesterday.
Comment 11 Alex Limi (:limi) — Firefox UX Team 2011-07-01 12:15:33 PDT
Comment on attachment 540968 [details] [diff] [review]
v2

Works as intended!
Comment 12 Tim Taubert [:ttaubert] 2011-07-19 06:49:10 PDT
http://hg.mozilla.org/integration/fx-team/rev/54ed6181b38c
Comment 13 Siddhartha Dugar [:sdrocking] 2011-07-19 09:25:30 PDT
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] 2011-07-19 23:51:53 PDT
(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] 2011-07-21 04:00:33 PDT
http://hg.mozilla.org/mozilla-central/rev/54ed6181b38c
Comment 16 Andreas Jung 2011-07-23 13:09:50 PDT
(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] 2011-07-24 17:40:27 PDT
(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] 2011-09-08 05:17:55 PDT
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.

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