625666 - Resize code can magically change aspect ratio of orphan tabs

Status: RESOLVED FIXED

(Firefox Graveyard :: Panorama, defect, P3)

Description

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: Screenshot

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.

Comment 1

[Raymond Lee [:raymondlee]]

Any way to reproduce this?

Comment 2

[Michael Yoshitaka Erlewine [:mitcho]]

Let me see if I can still reproduce this.

Comment 3

[Michael Yoshitaka Erlewine [:mitcho]]

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

Comment 4

[Michael Yoshitaka Erlewine [:mitcho]]

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

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

Attachment: Testcase reproducing the issue

Comment 6

[Raymond Lee [:raymondlee]]

Attachment: Test patch v2

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.

Comment 7

[Michael Yoshitaka Erlewine [:mitcho]]

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?

Comment 8

[Raymond Lee [:raymondlee]]

Attachment: v3

(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?

Comment 9

[Michael Yoshitaka Erlewine [:mitcho]]

[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.

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

(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.

Comment 11

[Ian Gilman [:iangilman]]

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.

Comment 12

[Raymond Lee [:raymondlee]]

Attachment: v4

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

Comment 13

[Michael Yoshitaka Erlewine [:mitcho]]

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?

Comment 14

[Raymond Lee [:raymondlee]]

(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?

Comment 15

[Michael Yoshitaka Erlewine [:mitcho]]

(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.

Comment 16

[Raymond Lee [:raymondlee]]

Attachment: v5

(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?

Comment 17

[Kevin Hanes]

bugspam

Comment 18

[Tim Taubert [:ttaubert] (inactive)]

bug spam