Closed Bug 595893 Opened 9 years ago Closed 9 years ago

New tabs should never be orphans (tabs in lower right on first visit to Panorama during a session)

Categories

(Firefox Graveyard :: Panorama, defect, P1)

defect

Tracking

(blocking2.0 beta7+)

VERIFIED FIXED
Tracking Status
blocking2.0 --- beta7+

People

(Reporter: iangilman, Assigned: raymondlee)

References

Details

(Whiteboard: [on-panorama-central][b8])

Attachments

(2 files, 7 obsolete files)

You should be able to create an orphan by dragging a tab out of a group, but a newly created tab should never be an orphan. If there is no currently selected group, pick the one with any tabs showing. If no groups at all, create a new one.
Priority: -- → P2
Duplicate of this bug: 585529
Assignee: nobody → raymond
Priority: P2 → P1
Blocks: 597043
The problem for this bug seems to be that blank tabs are not assigned to any group so they become orphans after restarting Fx.
Attached patch v1 (obsolete) — Splinter Review
Sent to try.
Attachment #477085 - Flags: feedback?(ian)
Comment on attachment 477085 [details] [diff] [review]
v1

Good point. 

In addition, please take care of the else clause in GroupItems.newTab and get rid of GroupItems.positionNewTabAtBottom. 

I guess theoretically we'll never get to that else clause, but as a stop-gap, change it to select the first group, or if there are no groups, use the first orphan tab. 

A couple minor points: 

+  is(contentWindow.GroupItems.getOrphanedTabs(), 0, "No orphaned tabs");

You need .length in there. You've got this kind of check in two places; please update both. 

+  // 2) create a group, add a blank tab and 

This comment ends abruptly; did you mean to say more? If the "and" is meant to lead to the next comment a number of lines down, I think it's just confusing.
Attachment #477085 - Flags: feedback?(ian) → feedback-
Attached patch v1 (obsolete) — Splinter Review
Attachment #477085 - Attachment is obsolete: true
Attachment #477392 - Flags: feedback?(ian)
Blocks: 586558
Status: NEW → ASSIGNED
Comment on attachment 477392 [details] [diff] [review]
v1

+        // browser must have at least one tab so in this case at least one 
+        // orphaned tab would exist.

This is actually no longer true, because of app tabs. It's possible for there to be no GroupItems and no TabItems but the browser doesn't close because there's still an app tab open. In that circumstance let's create a fresh group nicely positioned in the upper-left (similar to what we do on "first-run") and stick the new tab in it. 

+          let newGroupItemBounds = orphanedTabs[0].getBoundsWithTitle();
+          newGroupItemBounds.inset(-40,-40);
+          let newGroupItem = 
+            new GroupItem([orphanedTabs[0], tabItem], 
+                          { bounds: newGroupItemBounds });
+          newGroupItem.snap();
+          this.setActiveGroupItem(newGroupItem);

This chunk is almost identical to the orphan bit above it; combine somehow.
Attachment #477392 - Flags: feedback?(ian) → feedback-
Blocks: 598600
Attached patch v1 (obsolete) — Splinter Review
Sent to try
Attachment #477392 - Attachment is obsolete: true
Attachment #478188 - Flags: feedback?(ian)
Attached patch v1.1 (obsolete) — Splinter Review
Improved the patch.
Attachment #478188 - Attachment is obsolete: true
Attachment #478209 - Flags: feedback?(ian)
Attachment #478188 - Flags: feedback?(ian)
Comment on attachment 478209 [details] [diff] [review]
v1.1

We're getting there! Sorry to keep going around with you on this. 

I think the sequence should be tweaked a little, so it's: 

1. Active group
2. Active orphan
3. First visible non-app tab (that's not the tab in question), whether it's an orphan or not (make a new group if it's an orphan, add it to the group if it's not)
4. First group
5. First orphan that's not the tab in question
6. At this point there should be no groups or tabs (except for app tabs and the tab in question): make a new group

You're mostly there already, with the exception of step 3, I think, which currently only works on orphans.

Also, since it's kind of a complicated sequence, might be good to comment it more explicitly. 

Btw, checking to see if a TabItem is an orphan is as simple as checking to see if it has a parent; no need to loop through the orphan list. 

+            orphanTab = orphanTabItem;

It's getting a little confusing what's a xul:tab and what's a TabItem. The line above is a good example. I think I'd prefer to avoid the word tab by itself and use either tabItem or xulTab. If not that, let's at least be consistent within a routine.
Attachment #478209 - Flags: feedback?(ian) → feedback-
Attached patch v1.2 (obsolete) — Splinter Review
Attachment #478209 - Attachment is obsolete: true
Attachment #478332 - Flags: feedback?(ian)
Attachment #478332 - Attachment description: v1,2 → v1.2
Comment on attachment 478332 [details] [diff] [review]
v1.2

Cool, I think we have the logic we want. A couple of nits:

+      let orphanedTabs = this.getOrphanedTabs();

Move this down to where orphanedTabs is first used. 

+        orphanTab = otherTab.tabItem;

This is what I'm talking about in terms of confusing naming; you can't tell just by looking at their names that orphanTab is a TabItem and otherTab is a xul:tab, in fact just by their names you'd think they were the same type of object. How about keeping otherTab, but change orphanTab to orphanTabItem?

f+ with that
Attachment #478332 - Flags: feedback?(ian) → feedback+
Attached patch v1.2 (obsolete) — Splinter Review
Fixed Ian's comments.
Attachment #478332 - Attachment is obsolete: true
Attachment #478501 - Flags: review?(dolske)
Blocks: 599822
Comment on attachment 478501 [details] [diff] [review]
v1.2

Oh! Thought of another thing. If there are no other groups or tabs and you're therefore creating a fresh group for a fresh tab, please make sure that group and tab are positioned nicely in the upper-left-hand corner. The code in this patch just assumes the new tab is already in a good position, but I don't know that it is.
Duplicate of this bug: 599822
Blocks: 587990
Attached patch v1.3 (obsolete) — Splinter Review
Attachment #478501 - Attachment is obsolete: true
Attachment #478981 - Flags: review?(dolske)
Attachment #478501 - Flags: review?(dolske)
blocking2.0: --- → beta8+
Comment on attachment 478981 [details] [diff] [review]
v1.3

>+    // 1. Active group
>+    // 2. Active orphan
>+    // 3. First visible non-app tab (that's not the tab in question), whether it's an
>+    // orphan or not (make a new group if it's an orphan, add it to the group if it's
>+    // not)
>+    // 4. First group
>+    // 5. First orphan that's not the tab in question
>+    // 6. At this point there should be no groups or tabs (except for app tabs and the
>+    // tab in question): make a new group

This seems a bit like overkill, but meh.
Attachment #478981 - Flags: review?(dolske) → review+
Whiteboard: [on-panorama-central][b8]
Landed on mozilla-central:

http://hg.mozilla.org/mozilla-central/rev/03350cd18853
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Summary: New tabs should never be orphans → New tabs should never be orphans (tabs in lower right on first visit to Panorama during a session)
I'm tentatively clawing this and bug 602547 back into b7+ blockers; big wins for users, and as the shortcuts for Panorama become more discoverable, we want it to work properly if we can.

Ian, can you also land on GECKO20b7pre_20101006_RELBRANCH?
blocking2.0: beta8+ → beta7+
Landed for b7
verified in recent nightly build of minefield
Status: RESOLVED → VERIFIED
I had just checked normal browser restart. This not good coming out of Private Browsing.  

Here's what I had:

App Tabs: Yahoo.com and cnn.com
Group A with http://www.mozilla.com/en-US/about/
Group B with 3 New Tab
Group C with about:license

1) from the about Mozilla tab, Start Private Browsing, then Stop Private browsing

tested results:

If the groups were not actually given a name, the following happens:
upon return from PB, all tabs noted appear in the browser window. when you enter Group View.  all of the New Tabs and the about:license tab are orphaned. The http://www.mozilla.com/en-US/about/ Tab still in a group but the group has been shrunk to nearly stacked type

If the groups are all named:
upon return from PB, the tab with http://www.mozilla.com/en-US/about/ is moved from group A to Group C where the about:license tab is.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
appeared in group view when Fx restarted after being in and out of Private Browsing mode while investigating for my last comment.
Aren't there other bugs about bad transitions from PB mode -> normal mode?
(In reply to comment #24)
> Aren't there other bugs about bad transitions from PB mode -> normal mode?

Yes, Bug 594644 so closing this bug
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Status: RESOLVED → VERIFIED
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.