Titles of tab items get clipped off within groups

VERIFIED FIXED in Firefox 4.0b11

Status

defect
P1
normal
VERIFIED FIXED
9 years ago
3 years ago

People

(Reporter: tchung, Assigned: seanedunn)

Tracking

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

Firefox Tracking Flags

(blocking2.0 .x+, status2.0 wanted)

Details

(Whiteboard: [b8][visual][softblocker])

Attachments

(2 attachments, 16 obsolete attachments)

37.24 KB, image/png
Details
v14
43.48 KB, patch
iangilman
: review+
beltzner
: approval2.0+
Details | Diff | Splinter Review
(Reporter)

Description

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

See screenshot.
Assignee: nobody → ian

Comment 1

9 years ago
Mass moving all Tab Candy bugs from Mozilla Labs to Firefox::Tab Candy.  Filter the bugmail spam with "tabcandymassmove".
Component: TabCandy → TabCandy
Product: Mozilla Labs → Firefox
Target Milestone: -- → ---
Version: unspecified → Trunk
QA Contact: tabcandy → tabcandy
Same on Linux, OS=>ALL. This is both annoying and looks bad, so requesting blocking.
blocking2.0: --- → ?
OS: Mac OS X → All
Priority: -- → P2
Hardware: x86 → All
blocking2.0: ? → final+
Duplicate of this bug: 594954
Summary: Titles get clipped off → Titles of tab items get clipped off within groups
Duplicate of this bug: 596035
Duplicate of this bug: 587733

Updated

9 years ago
Duplicate of this bug: 597516

Comment 7

9 years ago
We should aim to get this done for b8.
Priority: P2 → P1
Whiteboard: [b8][visual]
Target Milestone: --- → Firefox 4.0

Updated

9 years ago
Blocks: 597763
(Assignee)

Updated

9 years ago
Assignee: ian → seanedunn
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.
(Assignee)

Comment 9

9 years ago
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.
Makes sense to me.
(Assignee)

Comment 11

9 years ago
Posted patch v1 (obsolete) — Splinter Review
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.
Attachment #477771 - Flags: feedback?(ian)
(Assignee)

Comment 12

9 years ago
(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.
Attachment #477771 - Flags: feedback?(ian)
(Assignee)

Comment 13

9 years ago
Posted patch v2 (obsolete) — Splinter Review
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.
Attachment #477771 - Attachment is obsolete: true
Attachment #478736 - Flags: feedback?(ian)
Status: NEW → ASSIGNED
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.
Attachment #478736 - Flags: feedback?(ian) → feedback-
(Assignee)

Comment 15

9 years ago
Posted patch v3 (obsolete) — Splinter Review
Attachment #478736 - Attachment is obsolete: true
Attachment #478882 - Flags: feedback?(ian)
Comment on attachment 478882 [details] [diff] [review]
v3

As discussed in IRC, remove Item.setBounds.
Attachment #478882 - Flags: feedback?(ian) → feedback+
(Assignee)

Comment 17

9 years ago
Posted patch v4 (obsolete) — Splinter Review
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.
Attachment #478882 - Attachment is obsolete: true
Attachment #478911 - Flags: review?
Attachment #478911 - Flags: feedback?(ian)
(Assignee)

Updated

9 years ago
Attachment #478911 - Flags: review? → review?(gavin.sharp)
Attachment #478911 - Flags: feedback?(ian) → feedback+
(Assignee)

Updated

9 years ago
Attachment #478911 - Flags: review?(gavin.sharp) → review?(dolske)
(Assignee)

Updated

9 years ago
Attachment #478911 - Flags: review?(dolske) → review?(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!
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?
Attachment #478911 - Flags: review?(dietrich)
(Assignee)

Comment 20

9 years ago
Posted patch v5 (w/ test) (obsolete) — Splinter Review
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.
Attachment #478911 - Attachment is obsolete: true
Attachment #488335 - Flags: review?(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.
Attachment #488335 - Flags: review?(dietrich) → review+
(Assignee)

Comment 22

9 years ago
Added as bug 611328.
Sean, please package for check-in. Also, does it need a try run?
(Assignee)

Comment 24

9 years ago
Posted patch v5 (w/test) (obsolete) — Splinter Review
Unrotted
Attachment #488335 - Attachment is obsolete: true
(Assignee)

Comment 27

9 years ago
Try was successful.
(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.
Sean, bump! :D
(Assignee)

Comment 30

9 years ago
Posted patch v6 (w/test) for check-in (obsolete) — Splinter Review
Bit rot removed!
Attachment #490265 - Attachment is obsolete: true
(Assignee)

Updated

9 years ago
Keywords: checkin-needed
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?
Keywords: checkin-needed
(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
(Assignee)

Comment 33

9 years ago
Posted patch v7 (w/test) for checkin (obsolete) — Splinter Review
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();
       }
Attachment #493898 - Attachment is obsolete: true
(Assignee)

Comment 34

9 years ago
Sent to try: http://hg.mozilla.org/try/rev/003f1befc477
(Assignee)

Updated

9 years ago
Blocks: 606148
(Assignee)

Comment 35

9 years ago
Posted patch v8 (w/test) for checkin (obsolete) — Splinter Review
Unrotted. Again.
Attachment #494884 - Attachment is obsolete: true

Comment 36

9 years ago
Sean, is this ready for a checkin-needed?
(Assignee)

Comment 37

9 years ago
No, it's still leaking.

Updated

9 years ago
Blocks: 598154

Comment 38

9 years ago
Beta8 has 1 bug left on it, moving our blockers to b9
We really *want* this for Firefox 4, but we can let it slip if necessary.
blocking2.0: final+ → .x
status2.0: --- → wanted

Comment 40

8 years ago
bugspam (moving b9 to b10)
Blocks: 608028

Comment 41

8 years ago
bugspam (removing b9)
No longer blocks: 598154
(Assignee)

Comment 42

8 years ago
Posted patch v9: Unrotted (obsolete) — Splinter Review
Unrotted so mitcho can test for leaks.
Attachment #495033 - Attachment is obsolete: true
(Assignee)

Comment 43

8 years ago
Posted patch v10: Unrotted for real. (obsolete) — Splinter Review
Attachment #502703 - Attachment is obsolete: true
Posted image Weird stacking? (obsolete) —
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?
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...
Depends on: 624722
(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?
Bug 624722 indeed fixes the leak for me, locally.

Pushed to try for confirmation.

Sean, please package for checkin!
Target Milestone: Firefox 4.0 → Firefox 3
(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 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?
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.
Whiteboard: [b8][visual] → [b8][visual][softblocker]
(Assignee)

Comment 51

8 years ago
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.
(Assignee)

Updated

8 years ago
Depends on: 609685
No longer depends on: 624722
Duplicate of this bug: 626271
(Assignee)

Comment 53

8 years ago
Posted patch v11 (obsolete) — Splinter Review
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
Attachment #502707 - Attachment is obsolete: true
Attachment #502713 - Attachment is obsolete: true
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.
Attachment #504726 - Flags: review?(ian)
Posted video Weird zoomout size adjustment (obsolete) —
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.
(Assignee)

Comment 56

8 years ago
Posted patch v12 (obsolete) — Splinter Review
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.
Attachment #504726 - Attachment is obsolete: true
Attachment #504777 - Attachment is obsolete: true
Attachment #505005 - Flags: review?(ian)
Attachment #505005 - Flags: feedback?(mitcho)
Attachment #504726 - Flags: review?(ian)
(Assignee)

Comment 57

8 years ago
Sent to try: http://hg.mozilla.org/try/rev/fe7b6062fc4b
(Assignee)

Comment 58

8 years ago
Looks like the try passed, except for what really appear to be unrelated errors.
Comment on attachment 505005 [details] [diff] [review]
v12

Looks great!
Attachment #505005 - Flags: feedback?(mitcho) → feedback+
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?
Attachment #505005 - Flags: review?(ian) → review-

Comment 61

8 years ago
bugspam. Moving b10 to b11
Blocks: 627096

Comment 62

8 years ago
bugspam. Removing b10
No longer blocks: 608028
(Assignee)

Comment 63

8 years ago
Posted patch v13 (obsolete) — Splinter Review
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.
Attachment #505005 - Attachment is obsolete: true
Attachment #506442 - Flags: review?(ian)
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.
Attachment #506442 - Flags: review?(ian) → review+
(Assignee)

Comment 65

8 years ago
Posted patch v14Splinter Review
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
Attachment #506442 - Attachment is obsolete: true
Attachment #506858 - Flags: review?(ian)
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.
Attachment #506858 - Flags: review?(ian)
Attachment #506858 - Flags: review+
Attachment #506858 - Flags: approval2.0?
Comment on attachment 506858 [details] [diff] [review]
v14

a=beltzner
Attachment #506858 - Flags: approval2.0? → approval2.0+
(Assignee)

Comment 68

8 years ago
Passed try.
(Assignee)

Updated

8 years ago
Keywords: checkin-needed
Whiteboard: [b8][visual][softblocker] → [b8][visual][softblocker][land before 606148]

Comment 69

8 years ago
http://hg.mozilla.org/mozilla-central/rev/e8a37c179e5e
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [b8][visual][softblocker][land before 606148] → [b8][visual][softblocker]
Target Milestone: Firefox 3 → Firefox 4.0b11
verified with recent minefield nightly build
Status: RESOLVED → VERIFIED
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.