567029 - Titles of tab items get clipped off within groups

Status: VERIFIED FIXED

(Firefox Graveyard :: Panorama, defect, P1)

Description

[Tony Chung [:tchung]]

Attachment: CLipped titles screenshot

Depending on the size of the tab window, the titles of the websites can get clipped off.

See screenshot.

Comment 1

[(no longer active)]

Mass moving all Tab Candy bugs from Mozilla Labs to Firefox::Tab Candy.  Filter the bugmail spam with "tabcandymassmove".

Comment 2

[Dietrich Ayala (:dietrich)]

Same on Linux, OS=>ALL. This is both annoying and looks bad, so requesting blocking.

Comment 7

[Aza Raskin [:aza]]

We should aim to get this done for b8.

Comment 8

[Ian Gilman [:iangilman]]

The root of the problem here is that a TabItem's bounds doesn't include its title; this should be fixed. Note that doing so will make TabItem.getBoundsWithTitle() obsolete.

Comment 9

[Sean Dunn]

On investigation, this doesn't appear to be the case. The items.js:Items.arrange() method makes an assumption about the size of the tab based on the aspect ratio of tabitems.js:TabItems.{tabHeight|tabWidth} applied to the desired width.

Most modern GUI toolkits follow some variant of the 2-pass measure/arrange for doing this layout. That may be overkill here, but I think we should still give objects a chance to come up with a size for themselves based on some constrained input in the same way, without the parent having to much knowledge about how it does it. This will let the tab use its knowledge about what font title size it will use and the aspect it wants to maintain.

I propose a getSizeForWidth() call which will pass back the requested tab size given an input width. Maybe later we can have a measure() and arrange() if we need it.

Comment 10

[Aza Raskin [:aza]]

Makes sense to me.

Comment 11

[Sean Dunn]

Attachment: v1

I made the title height static, to replace the scaling factor. The arrangement code needs some future cleanup, and I think for the sake of b8, this is a fix that doesn't introduce any new ways of doing things, but improves what was there.

Comment 12

[Sean Dunn]

(In reply to comment #10)
> Makes sense to me.

Aza, I started building the above call, but found myself rat-holing in the code a bit. I think the above that you commented on should be the way it's done in the future.

Comment 13

[Sean Dunn]

Attachment: v2

The prevailing change in this patch is that all estimation of item size has been been unified with the addition of the measureBounds method. This will return the best fit of the item given the inputs (which can be incomplete, using -1 for an unknown dimension). For TabItems, this includes the tab title, if the tab title is meant to be shown. This simplifies codepaths quite a bit, and removes a lot of boilerplate for calculating tab size. It also sets us up for a modern measure/arrange layout system in the future.

Comment 14

[Ian Gilman [:iangilman]]

Comment on attachment 478736 [details] [diff] [review]
v2

The approach looks good. Comments:

* The strategy I've used so far with methods that descendants of Item need to provide is to not write a version for Item and then assert in Item._init that those methods exist (and were therefore added by the subclass). By providing versions of measureBounds and setBounds in Item, you're subverting this check (and not really adding anything, as everyone overwrites those anyway). My preference would be to remove the Item versions of measureBounds and setBounds, and add an assert for measureBounds in Item._init, but of course I'm open to counter argument. 

* Now that you've got a measureBounds routine, we don't have to provide a fixed height for the title; it can be based on the actual size the title text will be. 

* Needs test, and a TPS report.

Nits:

+  // Function: measureBounds
+  // Basic measure rules. Makes square out of incomplete size spec. 

Doesn't look like it actually makes a square out of incomplete size (this is the GroupItem version); please update the comment.

+
+
   // ----------
   // Function: setBounds
   // Sets the bounds with the given <Rect>, animating unless "immediately" is false.
-  setBounds: function InfoItem_setBounds(rect, immediately) {
+  setBounds: function InfoItem_setBounds(rect, immediately, options) {

Are the extra new lines above this routine intentional?

+  // Function: _getWidthForHeight
+  // Private method that returns the tabitem width given a height.
+  // Set forceTitle to force whether titles are measured (true) or force
+  // them to not show (false).
+  _getWidthForHeight: function Item__getWidthForHeight(height, options) {    

It's not clear from the comment that forceTitle is supposed to be a property on options. 	

-      let tabData = Storage.getTabData(item.tab);
+      let tabData = Storage.getTabData(item.tab);      

Looks like all you did here was add some space after the line; this is frowned upon.

Comment 15

[Sean Dunn]

Attachment: v3

Comment 16

[Ian Gilman [:iangilman]]

Comment on attachment 478882 [details] [diff] [review]
v3

As discussed in IRC, remove Item.setBounds.

Comment 17

[Sean Dunn]

Attachment: v4

I haven't written a test yet because I'm not sure I should, or what I should be spending my time testing. Any suggestions would be appreciated.

Comment 18

[Dietrich Ayala (:dietrich)]

Sorry for the delay. My search I was using to filter review requests was not including those blocking final+, which meant I didn't see this :(

Ian, can you answer Sean's question in comment #17 before I review this? Thanks!

Comment 19

[Ian Gilman [:iangilman]]

For a test, how about you start a browser_tabview_layout.js as a possible place for future layout tests, and stick in it a test that creates a group with four tabs, sized so that it arranges in a two by two grid, and then verify that the titles from the top two tabs don't overlap the thumbnails from the bottom two tabs?

Comment 20

[Sean Dunn]

Attachment: v5 (w/ test)

Updated to defeat bitrot, added test which creates 4 items in a group and ensures that titles do not intersect any part of other items.

Comment 21

[Dietrich Ayala (:dietrich)]

Comment on attachment 488335 [details] [diff] [review]
v5 (w/ test)

r=me.

not related to this patch really, but those trace() calls should be assert()s, no? and if we don't need to throw an error, then we certainly shouldn't ship with those. please file a followup for it, either way.

Comment 22

[Sean Dunn]

Added as bug 611328.

Comment 23

[Ian Gilman [:iangilman]]

Sean, please package for check-in. Also, does it need a try run?

Comment 24

[Sean Dunn]

Attachment: v5 (w/test)

Unrotted

Comment 25

[Sean Dunn]

Try: http://hg.mozilla.org/try/pushloghtml?changeset=e0de7b96d2a8

Comment 26

[Ian Gilman [:iangilman]]

(In reply to comment #25)
> Try: http://hg.mozilla.org/try/pushloghtml?changeset=e0de7b96d2a8

Was the try successful?

Comment 27

[Sean Dunn]

Try was successful.

Comment 28

[Ian Gilman [:iangilman]]

(In reply to comment #27)
> Try was successful.

Excellent.  

Sorry to say, there's more rot.  It's gonna be like this for a bit while we get our backlog landed.  Please unrot again and add "a=blocking" to the commit message.

Comment 29

[Michael Yoshitaka Erlewine [:mitcho]]

Sean, bump! :D

Comment 30

[Sean Dunn]

Attachment: v6 (w/test) for check-in

Bit rot removed!

Comment 31

[Raymond Lee [:raymondlee]]

Submitted to try twice and there is always a leak on mochitest-o for the windows debug.  Could you take a look at it please?

Comment 32

[Raymond Lee [:raymondlee]]

(In reply to comment #31)
> Submitted to try twice and there is always a leak on mochitest-o for the
> windows debug.  Could you take a look at it please?

Changeset: 71b8da5c1e3f
http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTry/1291219455.1291223408.27953.gz

Comment 33

[Sean Dunn]

Attachment: v7 (w/test) for checkin

Made the following change in TabItems._update(). If _update() was called before the save(), it would set the URL even if it didn't properly connect. This would make the URL test above fail subsequently, and never save the tab. My patch didn't cause the bug, but it did change the size of tabs earlier than previously, which caused the early update.

       if (tabUrl != tabItem.url) {
-        let oldURL = tabItem.url;
-        tabItem.url = tabUrl;
-
         if (!tabItem.reconnected)
           this.reconnect(tabItem);
+
+        if (tabItem.reconnected)
+          tabItem.url = tabUrl;

         tabItem.save();
       }

Comment 34

[Sean Dunn]

Sent to try: http://hg.mozilla.org/try/rev/003f1befc477

Comment 35

[Sean Dunn]

Attachment: v8 (w/test) for checkin

Unrotted. Again.

Comment 36

[Kevin Hanes]

Sean, is this ready for a checkin-needed?

Comment 37

[Sean Dunn]

No, it's still leaking.

Comment 38

[Kevin Hanes]

Beta8 has 1 bug left on it, moving our blockers to b9

Comment 39

[Benjamin Smedberg]

We really *want* this for Firefox 4, but we can let it slip if necessary.

Comment 40

[Kevin Hanes]

bugspam (moving b9 to b10)

Comment 41

[Kevin Hanes]

bugspam (removing b9)

Comment 42

[Sean Dunn]

Attachment: v9: Unrotted

Unrotted so mitcho can test for leaks.

Comment 43

[Sean Dunn]

Attachment: v10: Unrotted for real.

Comment 44

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: Weird stacking?

Just applied this patch to take a look at the leak, but the first thing I noticed was this. This happened by adjusting the group; it's not an artifact of session restore. This is just m-c with this patch applied. Nothing else.

Can others reproduce? Sean?

Comment 45

[Michael Yoshitaka Erlewine [:mitcho]]

Just applied the patch, and the tabview tests indeed leak on my machine (Mac). I tried to find a minimal set of tests which leak and landed upon the exact same set as in 587503:

browser_tabview_rtl.js
browser_tabview_startup_transitions.js
browser_tabview_undo_group.js

In fact, here's the leak signature:

TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 2077571 bytes during test execution
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1412 instances of AtomImpl with size 40 bytes each (56480 bytes total)
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of BackstagePass with size 48 bytes
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of BodyRule with size 32 bytes
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 6 instances of CSSImportRuleImpl with size 80 bytes each (480 bytes total)
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 44 instances of CSSImportantRule with size 32 bytes each (1408 bytes total)

Hmm, where have I seen this before? OH! THIS IS THE SAME LEAK AS IN BUG 587503!

Tim also confirmed on his machine. Just pushed this test setup (with three tabview tests) to try to verify there too.

The quest continues...

Comment 46

[Ian Gilman [:iangilman]]

(In reply to comment #44)
> Created attachment 502713 [details]
> Weird stacking?
> 
> Just applied this patch to take a look at the leak, but the first thing I
> noticed was this. This happened by adjusting the group; it's not an artifact of
> session restore. This is just m-c with this patch applied. Nothing else.
> 
> Can others reproduce? Sean?

Mitcho, does this happen if you apply bug 622835 as well?

Comment 47

[Michael Yoshitaka Erlewine [:mitcho]]

Bug 624722 indeed fixes the leak for me, locally.

Pushed to try for confirmation.

Sean, please package for checkin!

Comment 48

[Michael Yoshitaka Erlewine [:mitcho]]

(In reply to comment #46)
> (In reply to comment #44)
> > Created attachment 502713 [details]
> > Weird stacking?
> > 
> > Just applied this patch to take a look at the leak, but the first thing I
> > noticed was this. This happened by adjusting the group; it's not an artifact of
> > session restore. This is just m-c with this patch applied. Nothing else.
> > 
> > Can others reproduce? Sean?
> 
> Mitcho, does this happen if you apply bug 622835 as well?

Just checked: yes, this still happens even once 622835 is applied. Let me take a quick look at this patch.

Comment 49

[Michael Yoshitaka Erlewine [:mitcho]]

Comment on attachment 502707 [details] [diff] [review]
v10: Unrotted for real.

Alright, looking at this patch, I think we're going to have some problems landing this beside some other recent work we've done.

>+  // Function: measureBounds
>+  // Basic measure rules. Assures that item is a minimum size.
>+  measureBounds: function Item_measureBounds(size, options) {
>+    Utils.assert(Utils.isPoint(size), 'input is a Point');
>+    let retSize = new Point(0,0);
>+    Utils.assert((size.x>0 || size.y>0) && (size.x!=0 && size.y!=0), "dimensions are valid:"+size.x+","+size.y);
>+    // any dimension of -1 gets sized to the min
>+    retSize.x = Math.max(GroupItems.minWidth, size.x);
>+    retSize.y = Math.max(GroupItems.minHeight, size.y);
>+    return retSize;
>+  },

The point of this function is essentially the same as the enforceMinSize functions in bug 622631, which in my mind is a more expressive name. Also, nits: the function name is wrong and the options argument isn't used.

>+    // first, find the right of the rightmost tab! luckily, they're in order.
>+    var rightMostRight = 0;
>+    if (UI.rtl) {
>+      rightMostRight = rects[0].right;
>+    } else {
>+      for each (var rect in rects) {
>+        if (rect.right > rightMostRight)
>+          rightMostRight = rect.right;
>+        else
>+          break;
>+      }
>+    }

This code is scheduled for removal as part of the bug 587503 patch, fyi.

>-    // compute h and w. h and w are the dimensions of each of the tabs... in other words, the
>-    // height and width of the entire stack, modulo rotation.
>-    if (bbAspect > itemAspect) { // Tall, thin groupItem
>-      w = bb.width * scale;
>-      h = w * itemAspect;
>-      // let's say one, because, even though there's more space, we're enforcing that with scale.
>-    } else { // Short, wide groupItem
>-      h = bb.height * scale;
>-      w = h * (1 / itemAspect);
>-    }
>+    // compute size of the entire stack, modulo rotation.
>+
>+    let size = proto.measureBounds(new Point(bb.width * scale, -1));
>+    let itemAspect = size.y / size.x;

This is the crux of the problem with the "exploding stack". Because we only give measureBounds the width of the available space, not the height, it's no longer properly constrained in this dimension.

Finally, the merge of Item_arrange here with the one in 587503 also looks to be non-trivial.

Ian, how do you think we should resolve these?

Comment 50

[Michael Yoshitaka Erlewine [:mitcho]]

Sean, if you'd like me to, I'd be happy to try to reincorporate your fix for this bug soon, as bug 587503 and 622631 hopefully will land soon.

Comment 51

[Sean Dunn]

It's ok, I'll take care of it when it lands. I've got several bugs in my mq stack that depend on 567029. Thanks for the offer.

Comment 53

[Sean Dunn]

Attachment: v11

I removed enforceMinSize, and renamed my function to enforceValidSize, which does what enforceMinSize did, in addition to keeping track of whether the title should be showing. It takes and returns a Point.

Sent to try: http://hg.mozilla.org/try/rev/e3378366e649

Comment 54

[Michael Yoshitaka Erlewine [:mitcho]]

Comment on attachment 504726 [details] [diff] [review]
v11

Thanks for cleaning this up again. The patch works well and doesn't produce the weird stacks extending beyond the top and bottom of the group. Some quick feedback before Ian takes a look at this:

>+  // Function: enforceValidSize
>+  // Basic measure rules. Assures that item is a minimum size.
>+  enforceValidSize: function Item_enforceValidSize(size, options) {

I'm not sure why this is in GroupItem instead of GroupItems, as it's not specific to the group at all... also, the function name is wrong either way. (Actually, I do know why you put it in GroupItem/TabItem, but I'll talk about that below...)

>@@ -1125,59 +1145,62 @@ GroupItem.prototype = Utils.extend(new I
>     if (!options)
>       options = {};
>     var animate = "animate" in options ? options.animate : true;
> 
>     var count = childrenToArrange.length;
>     if (!count)
>       return;
> 
>+    // Use the first child as a prototype
>+    let proto = this._children[0];
>+
>+    if (typeof options == 'undefined')
>+      options = {};

Isn't this part redundant? See the if (!options) above. Also, we're trying to replace |typeof x == 'undefined'| checks with |x === undefined| (Bug 606824)

>     if (bbAspect > itemAspect) { // Tall, thin groupItem
>-      w = bb.width * scale;
>-      h = w * itemAspect;
>-      // let's say one, because, even though there's more space, we're enforcing that with scale.
>+      size = proto.enforceValidSize(new Point(bb.width * scale, -1),
>+        {forceTitle:false});
>     } else { // Short, wide groupItem
>-      h = bb.height * scale;
>-      w = h * (1 / itemAspect);
>+      size = proto.enforceValidSize(new Point(-1, bb.height * scale),
>+        {forceTitle:false});

Here we know the children are going to be tabs, right? So why not TabItems.enforceValidSize?

>-        child.setBounds(box, !animate);
>+        child.isStacked = true;
>+        child.setBounds(box, !animate, {force:true});

Why do we need force=true here?

>+        var startAspect;

Why was this added?

>@@ -941,54 +947,66 @@ let Items = {
>   arrange: function Items_arrange(items, bounds, options) {

>+    let proto=null;
>+    if(typeof options.proto != "undefined") {
>+      proto = options.proto;
>+    } else {
>+      proto = items[0];
>+    }
>+    Utils.assert(proto!=null,
>+      "there should be a valid prototype object in arrange");

I'm not thrilled by this passing of a "proto". For one, it's confusing as you're not necessarily talking about the JS prototype. Second, why not just check whether the items are tabs or groups? Or have a flag which can specify, if necessary?

In fact, looking at the code overall now, I'm pretty sure Items.arrange is only ever used for arranging some tabs. I don't think it's ever used for groups. Making this TabItems.enforceValidSize should work fine, then.


>+  _getFontSizeFromWidth: function Item__getFontSizeFromWidth(width) {

TabItem__

>+  _getWidthForHeight: function Item__getWidthForHeight(height, options) {    

TabItem__

>+  _getHeightForWidth: function Item__getHeightForWidth(width, options) {

TabItem__

>+  enforceValidSize: function TabItem_enforceValidSize(size, options) {
>+    Utils.assert(Utils.isPoint(size), 'input is a Point');
>+    let retSize = new Point(0,0);
>+    if (size.x==-1) {
>+      retSize.x = this._getWidthForHeight(size.y, options);
>+      retSize.y = size.y;
>+    } else if (size.y==-1) {
>+      retSize.x = size.x;
>+      retSize.y = this._getHeightForWidth(size.x, options);
>+    } else {
>+      let fitHeight = this._getHeightForWidth(size.x, options);
>+      let fitWidth = this._getWidthForHeight(size.y, options);
>+
>+      // Go with the smallest final dimension.
>+      if (fitWidth < size.x) {
>+        retSize.x = fitWidth;
>+        retSize.y = size.y;
>+      } else {
>+        retSize.x = size.x;
>+        retSize.y = fitHeight;
>+      }
>+    }
>+    return retSize;
>+  },

So, as far as I can tell, this is the only reason to move these functions to the individual tabs and groups: for tabs, _getHeightForWidth (and vice versa) looks at this.isStacked. Why not keep these {TabItems,GroupItems}.ensureValidSize and let one of those options values be an isStacked flag?


>-  // ----------
>   // Function: inStack
>   // Returns true if this item is in a stacked groupItem.
>   inStack: function TabItem_inStack() {
>-    return iQ(this.container).hasClass("stacked");
>+    return this.isStacked;
>   },

Yeah, keeping this flag around is a good idea.

>+    this.tabAspect = this.tabHeight / this.tabWidth;
>+    this.invTabAspect = 1.0 / this.tabAspect;

No .0 needed, right? We're all float all the time!


>-        let oldURL = tabItem.url;
>-        tabItem.url = tabUrl;
>+        if (tabItem._reconnected)
>+          tabItem.url = tabUrl;

??? Is this a related change?

>+    window.addEventListener("tabviewshown", onTabViewVisibleReset, false);
>+    EventUtils.synthesizeKey("e", {accelKey : true}, contentWindow);    
>+  };
>+
>+  window.addEventListener("tabviewhidden", onTabViewHiddenReset, false);
>+  EventUtils.synthesizeKey("e", {accelKey : true}, contentWindow);
>+}

Haven't looked carefully at test logic, but this bit will need to be changed. In fact, we shouldn't tie it to the shortcut seeing as our shortcut is changing every week or so. why not just TabView.toggle()?

So, overall, I just wonder whether we really need to move ensureValidSize into the individual tabs and groups instead of staying in TabItems/GroupItems. Also, if it's returning an updated Point, why not call it getValidSize or makeValidSize?

Flagging Ian for review.

Comment 55

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: Weird zoomout size adjustment

Actually, patch v11 does introduce a weird artifact I just noticed. When zooming out of a tab into Panorama, the size of the tab is for some reason taller than it was originally, then snaps back into its original size. This doesn't happen for me without this patch.

Comment 56

[Sean Dunn]

Attachment: v12

Mitcho, I believe I addressed all the concerns you had in the last patch. I've changed the name to calcValidSize, and it's now a non-inherited member of TabItems and GroupItems. For some reason I had a bunch of lines that didn't belong in that patch that had made their way in from when I was trying to figure out what was causing the test memory leaks. This should be much cleaner.

Comment 57

[Sean Dunn]

Sent to try: http://hg.mozilla.org/try/rev/fe7b6062fc4b

Comment 58

[Sean Dunn]

Looks like the try passed, except for what really appear to be unrelated errors.

Comment 59

[Michael Yoshitaka Erlewine [:mitcho]]

Comment on attachment 505005 [details] [diff] [review]
v12

Looks great!

Comment 60

[Ian Gilman [:iangilman]]

Comment on attachment 505005 [details] [diff] [review]
v12

When I apply the patch, I get these issues: 

* Orphans creep to the left whenever I move anything else
* If I create a small group (by dragging on the background) far enough away from others that they don't block its growth, it'll grow a little more next time something else gets dragged.

Other comments:

>-          TabItems.enforceMinSize(bounds);
>           if (sizeStep.y > sizeStep.x) {
>-            var newWidth = bounds.height * (TabItems.tabWidth / TabItems.tabHeight);
>-            bounds.left += (bounds.width - newWidth) / 2;
>-            bounds.width = newWidth;
>+            validSize = TabItems.calcValidSize(new Point(-1, bounds.height));
>           } else {
>-            var newHeight = bounds.width * (TabItems.tabHeight / TabItems.tabWidth);
>-            bounds.top += (bounds.height - newHeight) / 2;
>-            bounds.height = newHeight;
>+            validSize = TabItems.calcValidSize(new Point(bounds.width, -1));
>           }
>         }
>+        bounds.width = validSize.x;
>+        bounds.height = validSize.y;

This is squish code, where an item gets squished down because another item is jamming it into the window wall. With the old version, items would squish along their center axis, which I think looks more natural. This new version squishes them along their left or top axis. Please fix.

>+    // force the input size to be valid
>+    let validSize = TabItems.calcValidSize(
>+      new Point(inRect.width, inRect.height), 
>+      {hideTitle: (this.isStacked || 
>+        (options !== undefined && options.hideTitle === true))});
>+    let rect = new Rect(inRect.left, inRect.top, 
>+      validSize.x, validSize.y);
>+
>     if (!options)
>       options = {};

If you put the new lines below the existing !options bit, you don't need the options !== undefined check. 

>   zoomOut: function TabItem_zoomOut(complete) {
>     var $tab = iQ(this.container);
>+    var self = this;
> 
>-    var box = this.getBounds();
>-    box.width -= this.sizeExtra.x;
>-    box.height -= this.sizeExtra.y;
>-
>-    var self = this;
>+    // Set initial condition
>+    this.setZoomPrep(true);

Why the new setZoomPrep here? Doesn't seem like it belongs.

>+  let onTabViewHiddenReset = function() {
>+    window.removeEventListener("tabviewhidden", onTabViewHiddenReset, false);
>+    // assert that we're no longer in tab view
>+    ok(!TabView.isVisible(), "Tab View is hidden");
>+
>+    let onTabViewVisibleReset = function() {
>+      window.removeEventListener("tabviewshown", onTabViewVisibleReset, false);
>+      ok(TabView.isVisible(), "Tab View is visible");
>+      mainTestFunc();
>+    };
>+  
>+    window.addEventListener("tabviewshown", onTabViewVisibleReset, false);
>+    TabView.toggle();    
>+  };
>+
>+  window.addEventListener("tabviewhidden", onTabViewHiddenReset, false);
>+  TabView.toggle();  
>+}

What's the purpose of this? Looks like you go into tabview, set some stuff up, then exit and reenter. Why not just stay in there?

Comment 61

[Kevin Hanes]

bugspam. Moving b10 to b11

Comment 62

[Kevin Hanes]

bugspam. Removing b10

Comment 63

[Sean Dunn]

Attachment: v13

When I apply the patch, I get these issues: 

>* Orphans creep to the left whenever I move anything else
>* If I create a small group (by dragging on the background) ... it'll grow ...

These issues should be fixed. There was still code in the unsquish() that set improper sizes.

> This new version squishes them along their left or top axis. Please fix.

Fixed.

> If you put the new lines below the existing !options bit, you don't need the
> options !== undefined check. 

Fixed.

> Why the new setZoomPrep here? Doesn't seem like it belongs.

It's required to ensure that the css has been set correctly (half sized) before zooming out. Otherwise, it's set to the original tab bounds.

> What's the purpose of this? Looks like you go into tabview, set some stuff up,
> then exit and reenter. Why not just stay in there?

It exits tabview so that the closed group gets flushed, and the number of group items is asserted correctly on exit.

Comment 64

[Ian Gilman [:iangilman]]

Comment on attachment 506442 [details] [diff] [review]
v13

(In reply to comment #63)
> > Why the new setZoomPrep here? Doesn't seem like it belongs.
> 
> It's required to ensure that the css has been set correctly (half sized) before
> zooming out. Otherwise, it's set to the original tab bounds.

I guess what I'm asking is what changed to make this necessary? This was working fine before without this line, right? It's supposed to be set up in UI.onTabSelect, so it doesn't need to be called here. Seems like that's still working. I just tried removing that line and doing various zoom in/out operations, and I don't see any problems.

Of course, looks like we're contemplating removing zoomPrep entirely (in but 624931, for instance), so it may become moot soon.

I'd say please remove the line, unless you're aware of somewhere the existing system isn't working.

> > What's the purpose of this? Looks like you go into tabview, set some stuff up,
> > then exit and reenter. Why not just stay in there?
> 
> It exits tabview so that the closed group gets flushed, and the number of group
> items is asserted correctly on exit.

Gotcha. Please use this pattern for that purpose instead:

      groupItem.addSubscriber(groupItem, "groupHidden", function() {
        groupItem.removeSubscriber(groupItem, "groupHidden");
        groupItem.closeHidden();
      });
      groupItem.closeAll();

>-            var newWidth = bounds.height * (TabItems.tabWidth / TabItems.tabHeight);
>-            bounds.left += (bounds.width - newWidth) / 2;
>-            bounds.width = newWidth;
>+//            var newWidth = bounds.height * (TabItems.tabWidth / TabItems.tabHeight);
>+//            bounds.left += (bounds.width - newWidth) / 2;
>+//            bounds.width = newWidth;            
>+            validSize = TabItems.calcValidSize(new Point(-1, bounds.height));
>+            bounds.left += (bounds.width - validSize.x) / 2;
>+            bounds.width = validSize.x;
>           } else {
>-            var newHeight = bounds.width * (TabItems.tabHeight / TabItems.tabWidth);
>-            bounds.top += (bounds.height - newHeight) / 2;
>-            bounds.height = newHeight;
>+//            var newHeight = bounds.width * (TabItems.tabHeight / TabItems.tabWidth);
>+//            bounds.top += (bounds.height - newHeight) / 2;
>+//            bounds.height = newHeight;
>+            validSize = TabItems.calcValidSize(new Point(bounds.width, -1));
>+            bounds.top += (bounds.height - validSize.y) / 2;
>+            bounds.height = validSize.y;        

Please remove the code rather than commenting it out. 

r+ with the above addressed.

Comment 65

[Sean Dunn]

Attachment: v14

All issues addressed. The hidden group code was actually unnecessary, as you were right, that whole test section at the end wasn't needed.

Sent to try to be sure: http://hg.mozilla.org/try/rev/5577b38d89fa

Comment 66

[Ian Gilman [:iangilman]]

Comment on attachment 506858 [details] [diff] [review]
v14

Looks great! 

Note that since it blocks .x, which is post Fx4, it still needs approval. Flagging as such.

Comment 67

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 506858 [details] [diff] [review]
v14

a=beltzner

Comment 68

[Sean Dunn]

Passed try.

Comment 69

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/e8a37c179e5e

Comment 70

[Tracy Walker [:tracy]]

verified with recent minefield nightly build