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

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.

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.

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?

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

bugspam

bugspam

Comment on attachment 537352 [details] [diff] [review] patch v2 Looks good! I've also tried the patch and it works great. Thanks Tim!

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/

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!

See bug 628061 comment 21 abut removing the "zero" state of the tabview button image

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 :)

(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!

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"

(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.

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.

(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! :)

Comment on attachment 542436 [details] [diff] [review] patch v4 Pushed to ux-branch.

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)

(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.

Unrotted the patch.

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.