634672 - Minimum tab size is no longer being respected

Status: VERIFIED FIXED

(Firefox Graveyard :: Panorama, defect, P3)

Description

[Kevin Hanes]

When resizing a group, tabs now are able to get barely larger than their favicon before we stack them. This is a regression and it is possible to create really unsightly unstacked groups.

Comment 1

[Kevin Hanes]

Attachment: screen shot of tiny, tiny tabs

Comment 2

[Michael Yoshitaka Erlewine [:mitcho]]

I just started seeing this as well, just in the past nightly or two.

Comment 3

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: Non-standard aspect in expanded stack view

Relatedly: some tabs in some grid arrangements in general get non-standard aspect ratios. The aspect ratio is sacred!

Comment 4

[Ian Gilman [:iangilman]]

(In reply to comment #3)
> Created attachment 513020 [details]
> Non-standard aspect in expanded stack view
> 
> Relatedly: some tabs in some grid arrangements in general get non-standard
> aspect ratios. The aspect ratio is sacred!

I imagine that's a separate bug. I believe this bug is just a matter of tweaking our "should I stack?" threshold.

Comment 5

[Michael Yoshitaka Erlewine [:mitcho]]

(In reply to comment #4)
> (In reply to comment #3)
> > Created attachment 513020 [details]
> > Non-standard aspect in expanded stack view
> > 
> > Relatedly: some tabs in some grid arrangements in general get non-standard
> > aspect ratios. The aspect ratio is sacred!
> 
> I imagine that's a separate bug. I believe this bug is just a matter of
> tweaking our "should I stack?" threshold.

I actually think this bug isn't just a "should I stack" threshold issue, as the resulting tabs in groups (seen in the first screenshot attachment) get a non-standard aspect ratio.

Comment 6

[Ian Gilman [:iangilman]]

(In reply to comment #5)
> I actually think this bug isn't just a "should I stack" threshold issue, as the
> resulting tabs in groups (seen in the first screenshot attachment) get a
> non-standard aspect ratio.

The resulting tabs are like 2x2 pixels, with the favicon hanging out over top of it. The fact is we should just never let tabs get that small. Making sure 2 pixel tall tabs have the right aspect ratio isn't going to do us much good.

Comment 7

[Raymond Lee [:raymondlee]]

Attachment: Screen shot

I couldn't reproduce it (see screenshot - that's the min. size I get before the stack is displayed).  Any STR?

http://hg.mozilla.org/mozilla-central/file/fe2375f12819/browser/base/content/tabview/tabitems.js#l1275

It looks like the TabItems__getWidthForHeight() and TabItems__getHightForWidth() would set the min size of tab item.  However, I don't quite understand why we have if size.x == -1 and else if size.y == -1 blocks.  What situation do we have size.x == -1 or size.y == -1? on tab item initialization?

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

(In reply to comment #7)
> I couldn't reproduce it (see screenshot - that's the min. size I get before the
> stack is displayed).  Any STR?

I can't reproduce this either.

> It looks like the TabItems__getWidthForHeight() and
> TabItems__getHightForWidth() would set the min size of tab item.  However, I
> don't quite understand why we have if size.x == -1 and else if size.y == -1
> blocks.  What situation do we have size.x == -1 or size.y == -1? on tab item
> initialization?

These special values are used in Items.pushAway() - https://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabview/items.js#440

Comment 9

[Michael Yoshitaka Erlewine [:mitcho]]

I also can no longer reproduce this, even though I was able to on the nightly a few days ago.

Comment 10

[Kevin Hanes]

Nope. I can definitely still hit this. It's some magical group aspect ratio combined with the number of tags. It took me a little bit of fiddling, but I can get this to happen with any group (see attached). 

Our minimum size should be something reasonable (see other attached).

Comment 11

[Kevin Hanes]

Attachment: Still tiny

Comment 12

[Kevin Hanes]

Attachment: Proposed min-size

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v1

Comment 14

[Michael Yoshitaka Erlewine [:mitcho]]

(In reply to comment #11)
> Created attachment 514046 [details]
> Still tiny

I just reproduced this again in my nightly... I have an STR, but it's kind of dependent on the layout of my top level items in my default profile... :(

Comment 15

[Michael Yoshitaka Erlewine [:mitcho]]

(In reply to comment #14)
> (In reply to comment #11)
> > Created attachment 514046 [details]
> > Still tiny
> 
> I just reproduced this again in my nightly... I have an STR, but it's kind of
> dependent on the layout of my top level items in my default profile... :(

FWIW, I can't reproduce this by manually resizing groups. It only happens when I resize the entire window. Kevin, is that how you get this as well?

Comment 16

[Michael Yoshitaka Erlewine [:mitcho]]

Comment on attachment 514102 [details] [diff] [review]
patch v1

>-    let arrObj = Items.arrange(null, bb, options);
>+    let arrObj = Items.arrange(this._children, bb, options);

We discussed this on IRC: this change was made so that Items.arrange can compute the appropriate itemMargin value. Previously we wouldn't have wanted to do this, because Items.arrange would actually mess with these children, but more recently I've made it so Items.arrange doesn't actually move anything so this is actually safe. Glad I checked. :)

Please open a followup to check all uses of Items.arrange to try to catch these types of issues.

>-  _getWidthForHeight: function TabItems__getWidthForHeight(height, options) {    
>-    let titleSize = (options !== undefined && options.hideTitle === true) ? 
>+  _getWidthForHeight: function TabItems__getWidthForHeight(height, options) {
>+    let titleSize = (options !== undefined && options.hideTitle === true) ?

This is just cleaning up trailing whitespace, right?

>-    return Math.max(0, Math.max(TabItems.minTabHeight, height - titleSize)) * 
>+    return Math.max(TabItems.minTabHeight, height - titleSize) *

Thanks for getting these, as well as the whitespace.

>+        retSize.y = this._getHeightForWidth(size.x, options);
>+        retSize.x = this._getWidthForHeight(retSize.y, options);

What is the point of changing retSize.x here? Why don't we just set retSize.x = size.x? This seems like a needless confusion/potential for getting a little off.

Otherwise looks great! Please just address that last question.

Comment 17

[Tim Taubert [:ttaubert] (inactive)]

(In reply to comment #16)
> Please open a followup to check all uses of Items.arrange to try to catch these
> types of issues.

Filed bug 635943.

> >-  _getWidthForHeight: function TabItems__getWidthForHeight(height, options) {    
> >-    let titleSize = (options !== undefined && options.hideTitle === true) ? 
> >+  _getWidthForHeight: function TabItems__getWidthForHeight(height, options) {
> >+    let titleSize = (options !== undefined && options.hideTitle === true) ?
> 
> This is just cleaning up trailing whitespace, right?

Yup.

> >+        retSize.y = this._getHeightForWidth(size.x, options);
> >+        retSize.x = this._getWidthForHeight(retSize.y, options);
> 
> What is the point of changing retSize.x here? Why don't we just set retSize.x =
> size.x? This seems like a needless confusion/potential for getting a little
> off.

This fixes the problem with orphan tabs getting way too small. Because _getHeightForWidth() and _getWidthForHeight() use Math.max() we can get a value that doesn't match the aspect ratio constraint. So we need to recalculate retSize.x for the y we got.

Comment 18

[Michael Yoshitaka Erlewine [:mitcho]]

(In reply to comment #17)
> This fixes the problem with orphan tabs getting way too small. Because
> _getHeightForWidth() and _getWidthForHeight() use Math.max() we can get a value
> that doesn't match the aspect ratio constraint. So we need to recalculate
> retSize.x for the y we got.

Sorry, that sort of makes sense, but I'm not following why it needs to be that way. I.e., why can't _getHeightForWidth return a height such that it matches the input width exactly (in terms of aspect)?

Comment 19

[Michael Yoshitaka Erlewine [:mitcho]]

[bugspam: betaN -> final]

Has patch, we can hopefully wrap it up soon, and it's lightweight. Wouldn't be surprised if we get a-, though.

Comment 20

[Ian Gilman [:iangilman]]

Comment on attachment 514102 [details] [diff] [review]
patch v1

These all look like good changes. I assume they fix the issue (I don't have a build setup here on the road)? Is it something we can have a test for?

Comment 21

[Raymond Lee [:raymondlee]]

Please see bug 625666 comment 16

Comment 22

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v2

(In reply to comment #21)
> Please see bug 625666 comment 16

Calculating fontSize now correctly and updated test.

(In reply to comment #20)
> These all look like good changes. I assume they fix the issue (I don't have a
> build setup here on the road)? Is it something we can have a test for?

Yeah that fixes it and a test is included. It was there before but it had the wrong name :)

(In reply to comment #18)
> Sorry, that sort of makes sense, but I'm not following why it needs to be that
> way. I.e., why can't _getHeightForWidth return a height such that it matches
> the input width exactly (in terms of aspect)?

I simplified and restructured the calcValidSize() method and I think it looks clearer. Does that make sense now? I thought about it and think we need the re-calculation because we cannot rely on the given width/height values to constrain to the standard aspect.

Comment 23

[Ian Gilman [:iangilman]]

Comment on attachment 515900 [details] [diff] [review]
patch v2

The patch fixes the issue, but it's grown to the point where it's no longer a quick fix, so less likely for Fx4 RC2 (if there is one). 

In particular, this change: 

>     if (rect.height != this.bounds.height || options.force) {
>-      if (!this.isStacked)
>-          css.height = rect.height - TabItems.tabItemPadding.y -
>-                       TabItems.fontSizeRange.max;
>-      else
>-        css.height = rect.height - TabItems.tabItemPadding.y;
>+      css.height = rect.height - TabItems.tabItemPadding.y;
>+      if (!this.isStacked) {
>+        css.height -= parseInt(css.fontSize ||
>+            TabItems.getFontSizeFromWidth(rect.width - TabItems.tabItemPadding.x));
>+      }
>     }

... is tightening the vertical spacing of the tab grid, which doesn't seem related to the bug. I like the idea in general, but it needs some work: it's now possible to get the title of a tab above to be close to the tab below. This is due to some extent to the fact that the gap between the bottom of a thumbnail and the top of its title is a constant size, no matter how big the thumbnail is. Both of those issues (tightening the vertical grid, and making the thumbnail/title gap related to thumbnail size) seem like good follow-up bugs, rather than rolling them into here. 

Regarding the test: 

* The 634672 test doesn't seem to actually test the scenario that started this bug thread: resizing a group so that its tabs are really small but not stacked. Is it possible to write a test for that?

Comment 24

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v3

(In reply to comment #23)
> ... is tightening the vertical spacing of the tab grid, which doesn't seem
> related to the bug. I like the idea in general, but it needs some work: it's
> now possible to get the title of a tab above to be close to the tab below. This
> is due to some extent to the fact that the gap between the bottom of a
> thumbnail and the top of its title is a constant size, no matter how big the
> thumbnail is. Both of those issues (tightening the vertical grid, and making
> the thumbnail/title gap related to thumbnail size) seem like good follow-up
> bugs, rather than rolling them into here.

Ok, instead of using the real title size I reverted the patch to use the constant title size (TabItems.fontSizeRange.max) again. I'll file follow-up bugs soon.

> * The 634672 test doesn't seem to actually test the scenario that started this
> bug thread: resizing a group so that its tabs are really small but not stacked.
> Is it possible to write a test for that?

I rewrote the whole test to shrink a groupItem with some tabs until it stacks. The smallest possible size at which the groupItem does _not_ stack should show close buttons for its tabItems.

I ensured that both tests (for this and for bug 625666) fail without the patch applied.

Comment 25

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v4

Removed unneeded test changes.

Comment 26

[Ian Gilman [:iangilman]]

Comment on attachment 522379 [details] [diff] [review]
patch v4

Thank you; I look forward to those follow-up bugs. 

The test seems as close an approximation as we're going to get.

Comment 27

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 522379 [details] [diff] [review]
patch v4

Passed try:

http://tbpl.mozilla.org/?tree=MozillaTry&pusher=[e-mail]&rev=d55f9ec09b06

Comment 28

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch for checkin

Comment 29

[(no longer active)]

I landed this on cedar <http://hg.mozilla.org/projects/cedar/rev/7fbc721aa455>, but I had to back it out <http://hg.mozilla.org/projects/cedar/rev/841d44530d2f> because of test failures on Mac <http://tinderbox.mozilla.org/showlog.cgi?log=Cedar/1301580028.1301583272.7407.gz>.

Comment 30

[Kevin Hanes]

bugspam

Comment 31

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v5

Corrected test for bug 610208.

Comment 32

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 524310 [details] [diff] [review]
patch v5

Added fixes for browser_tabview_bug610208.js

Passed try:

http://tbpl.mozilla.org/?tree=MozillaTry&pusher=[e-mail]&rev=70b2e8eebe0b

Comment 33

[Ian Gilman [:iangilman]]

Comment on attachment 524310 [details] [diff] [review]
patch v5

Looks good.

Comment 34

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch for checkin

Comment 35

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/3f0b5355069a

Comment 36

[George Carstoiu]

Verified with Mozilla/5.0 (Windows NT 5.1; rv:2.2a1pre) Gecko/20110410 Firefox/4.2a1pre