Remove the "orphan tab" concept from Panorama

VERIFIED FIXED in Firefox 7

Status

Firefox Graveyard
Panorama
--
enhancement
VERIFIED FIXED
6 years ago
a year ago

People

(Reporter: ithinc, Assigned: ttaubert)

Tracking

Trunk
Firefox 7
Dependency tree / graph

Details

(Whiteboard: [inbound])

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

6 years ago
User-Agent:       Mozilla/5.0 (Windows NT 5.1; rv:6.0a1) Gecko/20110503 Firefox/6.0a1
Build Identifier: Mozilla/5.0 (Windows NT 5.1; rv:6.0a1) Gecko/20110503 Firefox/6.0a1

Is it necessary to keep an "orphan tab" concept? Why is it not a group with only one tab? If it is just for display reason, CSS could help. I'm sure many things could be simplified with the "orphan tab" concept removed.

Reproducible: Always
(Reporter)

Updated

6 years ago
Blocks: 653099
Orphan tabs already act like groups of one anyway; I agree we should update the code to make that explicit. It remains to be seen whether we would want to update the visual display as well. I'm cc'ing Aza, as he may have some thoughts about what aspects of the orphan tab concept we may or may not want to hold on to.
(Assignee)

Comment 2

6 years ago
With the new internal API for global tab group storage we actually don't allow "real orphan tabs" anymore. They're implemented as tab groups with a special attribute and only one tab. So it's a Panorama-specific view for specific tab groups. We might as well change the UI to reflect those internal changes.

There really is a lot of code that could be much cleaner without the special handling and some of that will get at least a bit cleaner with the new storage.
Keywords: uiwanted
(Assignee)

Comment 3

6 years ago
Limi, do we want this?

It's listed as "Tabs should not be able to exist without a group" in https://wiki.mozilla.org/Simplify_Panorama_UI.

This would be a great thing to have before we address the global storage because we wouldn't need to re-implement orphan tabs for the new storage.
If we remove the "orphan tab" concept, we would also fix bugs like bug 623440 - Orphaned tabs don't show app tab icons: can cause situation where app tabs are completely hidden.

Shall we do that?
Blocks: 623440
(Assignee)

Comment 5

6 years ago
Created attachment 535012 [details] [diff] [review]
patch v1 (WIP)

I already took a stab - here's the current progress.
(Assignee)

Comment 6

6 years ago
bugspam
No longer blocks: 653099
(Assignee)

Comment 7

6 years ago
bugspam
Blocks: 660175

Updated

6 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Updated

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

Updated

6 years ago
Keywords: uiwanted
(Assignee)

Comment 8

6 years ago
Created attachment 537352 [details] [diff] [review]
patch v2
Attachment #535012 - Attachment is obsolete: true
Attachment #537352 - Flags: feedback?(raymond)
Comment on attachment 537352 [details] [diff] [review]
patch v2

Looks good! I've also tried the patch and it works great.  Thanks Tim!
Attachment #537352 - Flags: feedback?(raymond) → feedback+
(Assignee)

Comment 10

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

Passed try:

http://tbpl.mozilla.org/?tree=Try&pusher=tim.taubert@gmx.de

If anyone wants to try the new behavior, the builds are here:

https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/tim.taubert@gmx.de-bc1e4cd0e616/
Attachment #537352 - Flags: review?(ehsan)
Comment on attachment 537352 [details] [diff] [review]
patch v2

I'm not sure I know enough about the Panorama code to be able to review this...  Sorry!
Attachment #537352 - Flags: review?(ehsan)
See bug 628061 comment 21 abut removing the "zero" state of the tabview button image
(Assignee)

Comment 13

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

(In reply to comment #11) 
> I'm not sure I know enough about the Panorama code to be able to review
> this...  Sorry!

Np, that's a really big core patch. That might be a perfect job for Ian, hope he has the time to do it :)
Attachment #537352 - Flags: review?(ian)
(In reply to comment #13)
> Np, that's a really big core patch. That might be a perfect job for Ian,
> hope he has the time to do it :)

I'll take a look at it; hopefully tomorrow.
Comment on attachment 537352 [details] [diff] [review]
patch v2

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

Ok, I've reviewed everything but the tests so far... looking good! Thanks for making sure to clean up the comments and the CSS as you go.

UX definitely needs a chance to play with it, so I've flagged faaborg. Perhaps you can point him to a try build?

Hopefully I'll get to the tests tomorrow.

::: browser/base/content/tabview/items.js
@@ +165,5 @@
>        },
>        stop: function() {
> +        if (!this.isAGroupItem && !this.parent) {
> +          let groupItem = new GroupItem([drag.info.$el], {immediately: true});
> +          groupItem.pushAway();

new GroupItem() already calls pushAway (down at the bottom of that routine); you shouldn't need to do it here.

::: browser/base/content/tabview/tabitems.js
@@ +291,4 @@
>  
>        if (tabData.groupID) {
> +        groupItem = GroupItems.groupItem(tabData.groupID);
> +        self.setBounds(tabData.bounds, true);

It's possible we don't need to save the bounds for a tab any more, as tab layout is handled by the parent group. I believe there was some talk at some point about using the saved bounds as an optimization (so we didn't have to recalculate at start-up) but I don't think we got around to that, and at any rate, I think there are issues with that approach.

@@ +295,3 @@
>  
> +        if (Utils.isPoint(tabData.userSize))
> +          self.userSize = new Point(tabData.userSize);

I don't think TabItems need userSize any more, since the user can't resize them manually.

@@ +301,5 @@
> +        if (Utils.isPoint(tabData.userSize))
> +          groupItem.userSize = new Point(tabData.userSize);
> +      }
> +
> +      if (groupItem) {

groupItem should definitely exist at this point, so no need to check for it, right?

::: browser/base/content/tabview/ui.js
@@ +205,3 @@
>  
> +              let opts = {immediately: true, bounds: box};
> +              let groupItem = new GroupItem([], opts);

How does the tab get created? All I see is a groupItem getting created.

@@ +205,4 @@
>  
> +              let opts = {immediately: true, bounds: box};
> +              let groupItem = new GroupItem([], opts);
> +              groupItem.pushAway(true);

Do we need this pushAway or will new GroupItem take are of it?

@@ +421,5 @@
>    setActive: function UI_setActive(item, options) {
> +    Utils.assert(item, "item must be given");
> +
> +    if (item.isATabItem) {
> +      if (item.parent)

Will it be possible for an item not to have a parent?

::: browser/base/content/test/tabview/head.js
@@ +294,5 @@
>    let win = window.openDialog(getBrowserURL(), "_blank", opts);
>  
>    whenWindowLoaded(win, function () {
> +    let ss = Cc["@mozilla.org/browser/sessionstore;1"]
> +             .getService(Ci.nsISessionStore);

Good catch!
Attachment #537352 - Flags: ui-review?(faaborg)
One comment from a UX standpoint: maybe when you drag a tab out of a group, the new resulting group should be a bit bigger, so the tab doesn't appear to shrink so much.
Comment on attachment 537352 [details] [diff] [review]
patch v2

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

Ok, now I've reviewed the tests. Definitely looks like you're on the right track, but I'll hold r+ until you've addressed my comments and UX has had a chance to chime in.

::: browser/base/content/test/tabview/browser_tabview_bug597399.js
@@ +40,5 @@
>      "tabviewsearchenabled", onSearchEnabled, false);
>    contentWindow.addEventListener(
>      "tabviewsearchdisabled", onSearchDisabled, false);
>  
>    onSearchDisabled();

Doesn't seem to be anything wrong with the clean up on this test, but it also doesn't seem to be necessary for this patch, which is already very long. 

Don't bother taking these changes out, but next time, please keep the patch focused.

::: browser/base/content/test/tabview/browser_tabview_bug654721.js
@@ +9,5 @@
> +      extData: {"tabview-tab": '{"url":"about:home","groupID":1,"bounds":{"left":20,"top":20,"width":20,"height":20}}'}
> +    },{
> +      entries: [{ url: "about:home" }],
> +      hidden: false,
> +      extData: {"tabview-tab": '{"url":"about:home","groupID":0,"bounds":{"left":300,"top":300,"width":200,"height":200}}'},

groupID 0? Is that a mistake, or testing to see what happens with people's existing orphan tabs? If the latter (which is a good idea), please comment as such.

@@ +49,5 @@
> +      EventUtils.synthesizeMouse(target, 10, 10, {type: 'mousedown'}, cw);
> +      EventUtils.synthesizeMouse(target, 20, -200, {type: 'mousemove'}, cw);
> +      EventUtils.synthesizeMouse(target, 10, 10, {type: 'mouseup'}, cw);
> +
> +      is(groupItems.length, 3, "two groupItems");

"two" should be "three"
Attachment #537352 - Flags: review?(ian) → review-

Updated

6 years ago
Blocks: 665002
(Assignee)

Comment 18

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

(In reply to comment #17)
> Doesn't seem to be anything wrong with the clean up on this test, but it
> also doesn't seem to be necessary for this patch, which is already very
> long. 
> 
> Don't bother taking these changes out, but next time, please keep the patch
> focused.

These are changes I forgot to revert after some searching for the failing test. I took them out.

> groupID 0? Is that a mistake, or testing to see what happens with people's
> existing orphan tabs? If the latter (which is a good idea), please comment
> as such.

It's the latter, comment added.

> "two" should be "three"

Fixed.

I'll push it to the ux-branch and flag it then for ui-review again.
Attachment #537352 - Attachment is obsolete: true
Attachment #537352 - Flags: ui-review?(faaborg)
Attachment #542407 - Flags: review?(ian)
(Assignee)

Comment 19

6 years ago
Created attachment 542436 [details] [diff] [review]
patch v4
Attachment #542407 - Attachment is obsolete: true
Attachment #542407 - Flags: review?(ian)
Attachment #542436 - Flags: review?(ian)
Comment on attachment 542436 [details] [diff] [review]
patch v4

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

Looks good. I'm giving it an r+, but please don't land it on trunk until ux has had a chance to check it out.
Attachment #542436 - Flags: review?(ian) → review+
(Assignee)

Comment 21

6 years ago
(In reply to comment #20)
> Looks good. I'm giving it an r+, but please don't land it on trunk until ux
> has had a chance to check it out.

Promise! :)
(Assignee)

Comment 22

6 years ago
Comment on attachment 542436 [details] [diff] [review]
patch v4

Pushed to ux-branch.
Attachment #542436 - Flags: ui-review?(limi)
Comment on attachment 542436 [details] [diff] [review]
patch v4

Works great!

There's one case that doesn't seem to be handled, though:

1. Have an existing group with a couple of tabs in it
2. Drag one of those to a new group, a new group gets created, name gets focused (all this is exactly as it should be!)
3. Avoid naming the group, and drag the tab to the previous group again, you are now left with an empty, unnamed group.

I think the rule should be that if you empty a group, and it has no name assigned to it, that group should go away.

I remember that we removed the "flush empty group on enter/exit of Panorama" — the difference here is that we're doing an explicit action to empty out the group ("cleaning up"), whereas if I created an empty group and just left it there, it should probably stay, since I'm planning to put something in it. I realize this difference is very slight!

(and I wouldn't hold the patch for this, just something to keep in mind if it's an easy fix)
Attachment #542436 - Flags: ui-review?(limi) → ui-review+
(Assignee)

Comment 24

6 years ago
(In reply to comment #23)
> There's one case that doesn't seem to be handled, though:
> 
> 1. Have an existing group with a couple of tabs in it
> 2. Drag one of those to a new group, a new group gets created, name gets
> focused (all this is exactly as it should be!)
> 3. Avoid naming the group, and drag the tab to the previous group again, you
> are now left with an empty, unnamed group.
> 
> I think the rule should be that if you empty a group, and it has no name
> assigned to it, that group should go away.
> 
> I remember that we removed the "flush empty group on enter/exit of Panorama"
> — the difference here is that we're doing an explicit action to empty out
> the group ("cleaning up"), whereas if I created an empty group and just left
> it there, it should probably stay, since I'm planning to put something in
> it. I realize this difference is very slight!

Will be fixed by 663421.
(Assignee)

Comment 25

6 years ago
Created attachment 543738 [details] [diff] [review]
patch v5

Unrotted the patch.
Attachment #542436 - Attachment is obsolete: true
(Assignee)

Comment 26

6 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/95a023903ae0
Whiteboard: [inbound]
http://hg.mozilla.org/mozilla-central/rev/95a023903ae0
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 7
(Assignee)

Updated

6 years ago
Depends on: 669694

Comment 28

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

Verified issue on Ubuntu 11.04 x86, WinXP, Win7 x86 and Mac OS X 10.6 using the following steps:
 1. Start Firefox and open several websites
 2. Enter Panorama
 3. Drag one of the tabs into the background

A new group is created containing the tab -> orphan tab concept no longer present. Setting status to Verified Fixed.
Status: RESOLVED → VERIFIED
(Assignee)

Updated

6 years ago
Depends on: 670035
(Assignee)

Updated

6 years ago
Depends on: 673196

Updated

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