Minimum tab size is no longer being respected

VERIFIED FIXED in Firefox 5

Status

defect
P3
normal
VERIFIED FIXED
8 years ago
3 years ago

People

(Reporter: khanes, Assigned: ttaubert)

Tracking

({regression})

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [visual][ux])

Attachments

(6 attachments, 6 obsolete attachments)

Reporter

Description

8 years ago
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.
Reporter

Comment 1

8 years ago
Reporter

Updated

8 years ago
Blocks: 627096
Keywords: regression
Priority: -- → P3
Version: unspecified → Trunk
I just started seeing this as well, just in the past nightly or two.
Whiteboard: [visual][ux]
Relatedly: some tabs in some grid arrangements in general get non-standard aspect ratios. The aspect ratio is sacred!
Attachment #513020 - Attachment is patch: false
Attachment #513020 - Attachment mime type: text/plain → image/png
(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.
(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.
(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.
Posted image 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?
(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
I also can no longer reproduce this, even though I was able to on the nightly a few days ago.
Whiteboard: [visual][ux] → [visual][ux][WFM?]
Reporter

Comment 10

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

Comment 11

8 years ago
Posted image Still tiny
Reporter

Comment 12

8 years ago
Posted image Proposed min-size
Assignee: nobody → tim.taubert
Status: NEW → ASSIGNED
Whiteboard: [visual][ux][WFM?] → [visual][ux]
Posted patch patch v1 (obsolete) — Splinter Review
Attachment #514102 - Flags: feedback?(ian)
(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... :(
(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?
Blocks: 625666
Reporter

Updated

8 years ago
Attachment #514102 - Flags: feedback?(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.
Attachment #514102 - Flags: feedback?(mitcho)
Attachment #514102 - Flags: feedback?(ian)
Attachment #514102 - Flags: feedback+
(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.
(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)?
[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.
Blocks: 585689
No longer blocks: 627096
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?
Attachment #514102 - Flags: review+
Posted patch patch v2 (obsolete) — Splinter Review
(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.
Attachment #514102 - Attachment is obsolete: true
Attachment #515900 - Flags: review?(ian)
Attachment #515900 - Flags: feedback?(mitcho)
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?
Attachment #515900 - Flags: review?(ian)
Attachment #515900 - Flags: review-
Attachment #515900 - Flags: feedback?(mitcho)
No longer blocks: 585689
Posted patch patch v3 (obsolete) — Splinter Review
(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.
Attachment #515900 - Attachment is obsolete: true
Attachment #522354 - Flags: review?(ian)
Posted patch patch v4 (obsolete) — Splinter Review
Removed unneeded test changes.
Attachment #522354 - Attachment is obsolete: true
Attachment #522379 - Flags: review?(ian)
Attachment #522354 - Flags: review?(ian)
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.
Attachment #522379 - Flags: review?(ian) → review+
Posted patch patch for checkin (obsolete) — Splinter Review
Attachment #522379 - Attachment is obsolete: true
Reporter

Comment 30

8 years ago
bugspam
Posted patch patch v5 (obsolete) — Splinter Review
Corrected test for bug 610208.
Attachment #522542 - Attachment is obsolete: true
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=tim.taubert@gmx.de&rev=70b2e8eebe0b
Attachment #524310 - Flags: review?(ian)
Comment on attachment 524310 [details] [diff] [review]
patch v5

Looks good.
Attachment #524310 - Flags: review?(ian) → review+
Attachment #524310 - Attachment is obsolete: true

Comment 35

8 years ago
http://hg.mozilla.org/mozilla-central/rev/3f0b5355069a
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox4.2

Comment 36

8 years ago
Verified with Mozilla/5.0 (Windows NT 5.1; rv:2.2a1pre) Gecko/20110410 Firefox/4.2a1pre
Status: RESOLVED → VERIFIED
Target Milestone: Firefox5 → Firefox 5
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.