Michael Yoshitaka Erlewine [:mitcho] Reporter
	Description • 14 years ago

Orphan tab aspect ratio should be sacred, but is not respected in the resize code.

This is actually a pretty nasty problem because we don't let users modify tabs' aspect ratios when resizing, preserving this weird aspect ratio.

	Raymond Lee [:raymondlee]
	Comment 1 • 14 years ago

Any way to reproduce this?

	Michael Yoshitaka Erlewine [:mitcho] Reporter
	Comment 2 • 14 years ago

Let me see if I can still reproduce this.

	Michael Yoshitaka Erlewine [:mitcho] Reporter
	Comment 3 • 14 years ago

I'm unable to reproduce this... though I don't think the pushAway and resize code has changed recently, which makes me uncomfortable.

	Michael Yoshitaka Erlewine [:mitcho] Reporter
	Comment 4 • 14 years ago

Tim has a mochitest which can consistently reproduce the issue. Tim, could you post it here?

	Raymond Lee [:raymondlee]
	Comment 6 • 14 years ago

Patch for bug 634672 fixes this bug as well.  I've updated the test so it calculates the aspect ratio of tabview item without the title.

	Michael Yoshitaka Erlewine [:mitcho] Reporter
	Comment 7 • 14 years ago

Comment on attachment 514149 [details] [diff] [review]
Test patch v2


>+  let getTabAspect = function (tabItem, win) {

win here isn't being used. Please remove.

>+    contentWindow = win.document.getElementById("tab-view").contentWindow;
>+    let cw = win.TabView.getContentWindow();

Don't these do the same thing? Just use cw and make that have test-function-scope.

Otherwise looks good.

Have you confirmed that this test fails on trunk and passes with the patch for bug 634672?

	Raymond Lee [:raymondlee]
	Comment 8 • 14 years ago

(In reply to comment #7)
> Comment on attachment 514149 [details] [diff] [review]
> Test patch v2
> 
> 
> >+  let getTabAspect = function (tabItem, win) {
> 
> win here isn't being used. Please remove.

OK removed it.

> 
> >+    contentWindow = win.document.getElementById("tab-view").contentWindow;
> >+    let cw = win.TabView.getContentWindow();
> 
> Don't these do the same thing? Just use cw and make that have
> test-function-scope.
> 

Removed contentWindow.

> Otherwise looks good.
> 
> Have you confirmed that this test fails on trunk and passes with the patch for
> bug 634672?

Yes, the test fails without bug 634672.


May we should just merge this with the patch for bug 634672?

	Michael Yoshitaka Erlewine [:mitcho] Reporter
	Comment 9 • 14 years ago

[bugspam: betaN -> final]

This is just a test change, so could be made during final though, of course, we wouldn't want to do that if we don't get approval for 634672.

	Tim Taubert [:ttaubert] (inactive)
	Comment 10 • 14 years ago

(In reply to comment #8)
> Yes, the test fails without bug 634672.
> May we should just merge this with the patch for bug 634672?

Yeah let's do that. I'll include that test in the patch.

	Ian Gilman [:iangilman]
	Comment 11 • 14 years ago

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

The test should verify that the tab's bounds did indeed change, even though the aspect ratio did not.

Otherwise looks good. Please fix that and combine into bug 634672.

	Raymond Lee [:raymondlee]
	Comment 12 • 14 years ago

Added code to verify that the tab's bounds indeed change.

	Michael Yoshitaka Erlewine [:mitcho] Reporter
	Comment 13 • 14 years ago

Comment on attachment 515554 [details] [diff] [review]
v4


>+    let titleSize = cw.TabItems.fontSizeRange.max;

The title size isn't always the same as the max of fontSizeRange, is it?

>+    return Math.round(bounds.width / (bounds.height - titleSize));

>+
>+    is(getTabAspect(tabItem, cw), aspect, "orphaned tab's aspect didn't change");

I feel like I said this before, though perhaps on another bug... Shouldn't we be comparing this to the expected standard aspect, TabItems.tabHeight/TabItems.tabWidth, instead of just checking that it's the same? We could add like a 2% wiggle room for rounding-based error, but still. Ian, what do you think?

	Raymond Lee [:raymondlee]
	Comment 14 • 14 years ago

(In reply to comment #13)
> Comment on attachment 515554 [details] [diff] [review]
> v4
> 
> 
> >+    let titleSize = cw.TabItems.fontSizeRange.max;
> 
> The title size isn't always the same as the max of fontSizeRange, is it?
> 

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabview/tabitems.js#1278

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabview/tabitems.js#1290

The TabItems__getWidthForHeight and TabItems__getWidthForHeight are using the fontSizeRange.max so the same is used in the test.  I think the title size isn't the same always so we probably should file a bug to fix the code itself.  What do you think?

	Michael Yoshitaka Erlewine [:mitcho] Reporter
	Comment 15 • 14 years ago

(In reply to comment #14)
> The TabItems__getWidthForHeight and TabItems__getWidthForHeight are using the
> fontSizeRange.max so the same is used in the test.  I think the title size
> isn't the same always so we probably should file a bug to fix the code itself. 
> What do you think?

I believe this is a "feature"... if the tab is small, we make the title text smaller too.

That's why I think we should check for an actual titleSize based on the title div from the tab, instead of just using the max fontsize, in the test.

	Raymond Lee [:raymondlee]
	Comment 16 • 14 years ago

(In reply to comment #15)
> (In reply to comment #14)
> > The TabItems__getWidthForHeight and TabItems__getWidthForHeight are using the
> > fontSizeRange.max so the same is used in the test.  I think the title size
> > isn't the same always so we probably should file a bug to fix the code itself. 
> > What do you think?
> 
> I believe this is a "feature"... if the tab is small, we make the title text
> smaller too.

Yes, I agree with that.

> 
> That's why I think we should check for an actual titleSize based on the title
> div from the tab, instead of just using the max fontsize, in the test.

It's not a problem that we use the actual titleSize in the test.  We need to change the code in TabItems__getWidthForHeight and TabItems__getWidthForHeight to use the actual height of tabitem's title to calculate the width and height for an tabItem instead of using fontSizeRange.max.

@tim: could you update your patch for bug 634672 to use the actual height of tabItem's title in TabItems__getWidthForHeight() and TabItems__getWidthForHeight() please?

	Kevin Hanes
	Comment 17 • 14 years ago

bugspam

	Tim Taubert [:ttaubert] (inactive)
	Comment 18 • 14 years ago

bug spam