The default bug view has changed. See this FAQ.

Don't close empty groups automatically

VERIFIED FIXED in Firefox 7

Status

Firefox Graveyard
Panorama
VERIFIED FIXED
6 years ago
a year ago

People

(Reporter: ttaubert, Assigned: ttaubert)

Tracking

Trunk
Firefox 7
Dependency tree / graph
Bug Flags:
in-testsuite +

Details

(Whiteboard: [inbound])

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

6 years ago
We're closing empty groups without a title automatically on showing/hiding Panorama. This is unexpected behavior and we want this to be removed.

https://wiki.mozilla.org/Simplify_Panorama_UI
(Assignee)

Updated

6 years ago
Assignee: nobody → tim.taubert
Status: NEW → ASSIGNED
(Assignee)

Comment 1

6 years ago
Created attachment 538866 [details] [diff] [review]
patch v1
Attachment #538866 - Flags: feedback?(raymond)
Comment on attachment 538866 [details] [diff] [review]
patch v1

Looks good to me!
Attachment #538866 - Flags: feedback?(raymond) → feedback+
(Assignee)

Comment 3

6 years ago
Comment on attachment 538866 [details] [diff] [review]
patch v1

Passed try:

http://tbpl.mozilla.org/?tree=Try&pusher=tim.taubert@gmx.de&rev=bad34ef30825
(Assignee)

Updated

6 years ago
Attachment #538866 - Flags: review?(dietrich)
Comment on attachment 538866 [details] [diff] [review]
patch v1

Review of attachment 538866 [details] [diff] [review]:
-----------------------------------------------------------------
Attachment #538866 - Flags: review?(dietrich) → review+
(Assignee)

Comment 5

6 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/646f3f6f3cd5
Whiteboard: [inbound]
(Assignee)

Updated

6 years ago
Blocks: 664669

Updated

6 years ago
No longer blocks: 664669
(Assignee)

Updated

6 years ago
Blocks: 664669
Pushed:
http://hg.mozilla.org/mozilla-central/rev/646f3f6f3cd5
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [inbound]
Target Milestone: --- → Firefox 7

Comment 7

6 years ago
Mozilla/5.0 (X11; Linux i686; rv:7.0a1) Gecko/20110620 Firefox/7.0a1

Verified issue on Ubuntu 11.04 x86, WinXP, Win7 x86, Mac OS X 10.6 using the following steps:
 1. Enter Panorama
 2. Create an empty group
 3. Exit Panorama
 4. Enter Panorama
 5. Verify whether group is still present

Issue no longer present - setting status to Verified.
Status: RESOLVED → VERIFIED
(Assignee)

Comment 8

6 years ago
Created attachment 543707 [details] [diff] [review]
patch v2

Old patch backed out - turned out we still need this behavior:

http://hg.mozilla.org/integration/mozilla-inbound/rev/a8553a17f750

New patch prepared with the following behavior - close empty groups when:

1) closing the last tab of a group
2) drag out the last tab of a group

Don't close the group if those actions occur while Panorama is hidden. Don't close empty groups when toggling Panorama.
Attachment #538866 - Attachment is obsolete: true
Attachment #543707 - Flags: review?(dietrich)
(Assignee)

Updated

6 years ago
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 9

6 years ago
Created attachment 543726 [details] [diff] [review]
patch v3

Fixed some nits.

Dão, could you please review this? This bug blocks some patches I'm hoping to land for Fx 7 and all the other reviewers are out for holiday... Thanks!
Attachment #543707 - Attachment is obsolete: true
Attachment #543707 - Flags: review?(dietrich)
Attachment #543726 - Flags: review?(dao)
merged backout http://hg.mozilla.org/mozilla-central/rev/a8553a17f750 (Comment 8)
Comment on attachment 543726 [details] [diff] [review]
patch v3

>--- a/browser/base/content/browser-tabview.js
>+++ b/browser/base/content/browser-tabview.js
>@@ -350,20 +350,19 @@ let TabView = {
>         event.preventDefault();
> 
>         self._initFrame(function() {
>           let groupItems = self._window.GroupItems;
>           let tabItem = groupItems.getNextGroupItemTab(event.shiftKey);
>           if (!tabItem)
>             return;
> 
>-          // Switch to the new tab, and close the old group if it's now empty.
>+          // Switch to the new tab
>           let oldGroupItem = groupItems.getActiveGroupItem();
>           window.gBrowser.selectedTab = tabItem.tab;
>-          oldGroupItem.closeIfEmpty();
>         });

This makes oldGroupItem unused.
(Assignee)

Comment 12

6 years ago
Created attachment 543784 [details] [diff] [review]
patch v4

(In reply to comment #11)
> >+          // Switch to the new tab
> >           let oldGroupItem = groupItems.getActiveGroupItem();
> >           window.gBrowser.selectedTab = tabItem.tab;
> >-          oldGroupItem.closeIfEmpty();
> >         });
> 
> This makes oldGroupItem unused.

Oh, yeah, good catch.
Attachment #543726 - Attachment is obsolete: true
Attachment #543726 - Flags: review?(dao)
Attachment #543784 - Flags: review?(dao)
Comment on attachment 543784 [details] [diff] [review]
patch v4

>+  let assertNumberOfGroupItems = function (num) {
>+    is(cw.GroupItems.groupItems.length, num, "there are " + num + " groupItems");
>+  };

"assert" in has a conventional meaning that you're not fulfilling. Call it "test" or "check".

>+  let next = function () {
>+    if (tests.length)
>+      tests.shift()();
>+    else
>+      finish();
>+  };

"function next() {" and drop the semicolon. The same applies to various other functions in this test.

>+  newWindowWithTabView(function (tvwin) {
>+    registerCleanupFunction(function () tvwin.close());
>+
>+    win = tvwin;
>+    cw = win.TabView.getContentWindow();
>+    groupItem = createEmptyGroupItem(cw, 200, 200, 20);
>+
>+    assertNumberOfGroupItems(2);
>+    next();
>+  });

"aWin" instead of "tvwin"
Attachment #543784 - Flags: review?(dao) → review+
(Assignee)

Comment 14

6 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/c4fd1aeff6e4
Whiteboard: [inbound]
http://hg.mozilla.org/mozilla-central/rev/c4fd1aeff6e4
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago6 years ago
Resolution: --- → FIXED
Mozilla/5.0 (X11; Linux x86_64; rv:7.0) Gecko/20100101 Firefox/7.0
Mozilla/5.0 (Windows NT 5.1; rv:7.0) Gecko/20100101 Firefox/7.0
Mozilla/5.0 (Windows NT 6.1; rv:7.0) Gecko/20100101 Firefox/7.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:7.0) Gecko/20100101 Firefox/7.0

Verified fixed on Firefox7 beta1, on Mac X OS 10.6, Ubuntu 11.04, Windows XP and Windows 7.

No problems occurred when following the test cases (created based on comment 8):

1. Enter Panorama.
2. Close the tabs of any group one at a time.
3. After closing the last tab of one group, the now empty group should be automatically removed.

1. Enter Panorama.
2. Drag or close tabs from a group until one tab is left for that group.
3. Drag the last tab from that group.
4. The respective group should be removed when dragging the last tab from it.

1. Have more than two tabs in a group in Panorama.
2. Exit panorama.
3. Close or drag the tabs from that group one by one until one tab is still remaining.
4. Close or drag the last tab.
5. Enter Panorama.
6. An empty group should still be displayed instead of the old one.

1. Enter Panorama.
2. Create an empty group.
3. Exit Panorama.
4. Enter Panorama.
5. Verify whether group is still present.
Status: RESOLVED → VERIFIED
Blocks: 637840

Updated

3 years ago
Depends on: 980292

Updated

3 years ago
Depends on: 980298
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.