Closed
Bug 634672
Opened 14 years ago
Closed 14 years ago
Minimum tab size is no longer being respected
Categories
(Firefox Graveyard :: Panorama, defect, P3)
Firefox Graveyard
Panorama
Tracking
(Not tracked)
VERIFIED
FIXED
Firefox 5
People
(Reporter: khanes, Assigned: ttaubert)
References
Details
(Keywords: regression, Whiteboard: [visual][ux])
Attachments
(6 files, 6 obsolete files)
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•14 years ago
|
||
Reporter | ||
Updated•14 years ago
|
Comment 2•14 years ago
|
||
I just started seeing this as well, just in the past nightly or two.
Updated•14 years ago
|
Whiteboard: [visual][ux]
Comment 3•14 years ago
|
||
Relatedly: some tabs in some grid arrangements in general get non-standard aspect ratios. The aspect ratio is sacred!
Updated•14 years ago
|
Attachment #513020 -
Attachment is patch: false
Attachment #513020 -
Attachment mime type: text/plain → image/png
Comment 4•14 years ago
|
||
(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•14 years ago
|
||
(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•14 years ago
|
||
(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•14 years ago
|
||
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?
Assignee | ||
Comment 8•14 years ago
|
||
(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•14 years ago
|
||
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•14 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•14 years ago
|
||
Reporter | ||
Comment 12•14 years ago
|
||
Assignee | ||
Updated•14 years ago
|
Assignee: nobody → tim.taubert
Assignee | ||
Updated•14 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Updated•14 years ago
|
Whiteboard: [visual][ux][WFM?] → [visual][ux]
Assignee | ||
Comment 13•14 years ago
|
||
Attachment #514102 -
Flags: feedback?(ian)
Comment 14•14 years ago
|
||
(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•14 years ago
|
||
(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?
Reporter | ||
Updated•14 years ago
|
Attachment #514102 -
Flags: feedback?(mitcho)
Comment 16•14 years ago
|
||
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+
Assignee | ||
Comment 17•14 years ago
|
||
(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•14 years ago
|
||
(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•14 years ago
|
||
[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•14 years ago
|
||
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+
Comment 21•14 years ago
|
||
Please see bug 625666 comment 16
Assignee | ||
Comment 22•14 years ago
|
||
(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 23•14 years ago
|
||
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)
Assignee | ||
Comment 24•14 years ago
|
||
(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)
Assignee | ||
Comment 25•14 years ago
|
||
Removed unneeded test changes.
Attachment #522354 -
Attachment is obsolete: true
Attachment #522379 -
Flags: review?(ian)
Attachment #522354 -
Flags: review?(ian)
Comment 26•14 years ago
|
||
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+
Assignee | ||
Comment 27•14 years ago
|
||
Comment on attachment 522379 [details] [diff] [review]
patch v4
Passed try:
http://tbpl.mozilla.org/?tree=MozillaTry&pusher=tim.taubert@gmx.de&rev=d55f9ec09b06
Assignee | ||
Comment 28•14 years ago
|
||
Attachment #522379 -
Attachment is obsolete: true
Assignee | ||
Updated•14 years ago
|
Keywords: checkin-needed
Comment 29•14 years ago
|
||
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>.
Keywords: checkin-needed
Reporter | ||
Comment 30•14 years ago
|
||
bugspam
Assignee | ||
Comment 31•14 years ago
|
||
Corrected test for bug 610208.
Attachment #522542 -
Attachment is obsolete: true
Assignee | ||
Comment 32•14 years ago
|
||
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 33•14 years ago
|
||
Comment on attachment 524310 [details] [diff] [review]
patch v5
Looks good.
Attachment #524310 -
Flags: review?(ian) → review+
Assignee | ||
Comment 34•14 years ago
|
||
Attachment #524310 -
Attachment is obsolete: true
Assignee | ||
Updated•14 years ago
|
Keywords: checkin-needed
Comment 35•14 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox4.2
Comment 36•14 years ago
|
||
Verified with Mozilla/5.0 (Windows NT 5.1; rv:2.2a1pre) Gecko/20110410 Firefox/4.2a1pre
Status: RESOLVED → VERIFIED
Updated•14 years ago
|
Target Milestone: Firefox5 → Firefox 5
Updated•9 years ago
|
Product: Firefox → Firefox Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•