remove "new tab" button from groups

VERIFIED FIXED in Firefox 8

Status

Firefox Graveyard
Panorama
VERIFIED FIXED
6 years ago
a year ago

People

(Reporter: ttaubert, Assigned: raymondlee)

Tracking

Trunk
Firefox 8
Dependency tree / graph

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

6 years ago
https://wiki.mozilla.org/Simplify_Panorama_UI
(Assignee)

Updated

6 years ago
Assignee: nobody → raymond
(Assignee)

Comment 1

6 years ago
Created attachment 539465 [details] [diff] [review]
v1

Please apply patch for bug 663612 before this one
Attachment #539465 - Flags: feedback?(tim.taubert)
(Assignee)

Comment 2

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

Updated

6 years ago
Status: NEW → ASSIGNED
(Reporter)

Comment 3

6 years ago
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.
Attachment #539465 - Flags: feedback?(tim.taubert) → feedback+
(Assignee)

Comment 4

6 years ago
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.
Attachment #539465 - Attachment is obsolete: true
Attachment #540968 - Flags: review?(ian)
(Reporter)

Comment 5

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

Comment 7

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

Comment 8

6 years ago
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
Attachment #540968 - Attachment is obsolete: true
(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!
(Reporter)

Comment 10

6 years ago
Comment on attachment 540968 [details] [diff] [review]
v2

This patch landed on ux-branch yesterday.
Attachment #540968 - Flags: ui-review?(faaborg)
Comment on attachment 540968 [details] [diff] [review]
v2

Works as intended!
Attachment #540968 - Flags: ui-review?(faaborg) → ui-review+
(Reporter)

Updated

6 years ago
Blocks: 595402
(Assignee)

Updated

6 years ago
Blocks: 597767
(Reporter)

Updated

6 years ago
Blocks: 671347
(Reporter)

Comment 12

6 years ago
http://hg.mozilla.org/integration/fx-team/rev/54ed6181b38c
Whiteboard: [fixed-in-fx-team]
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.
(Assignee)

Comment 14

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

Comment 15

6 years ago
http://hg.mozilla.org/mozilla-central/rev/54ed6181b38c
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 8

Comment 16

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

Comment 17

6 years ago
(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.
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.
Status: RESOLVED → VERIFIED
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.