Closed Bug 625666 Opened 9 years ago Closed 9 years ago

Resize code can magically change aspect ratio of orphan tabs

Categories

(Firefox Graveyard :: Panorama, defect, P3)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 5

People

(Reporter: mitcho, Unassigned)

References

Details

(Keywords: polish, Whiteboard: [visual][polish])

Attachments

(2 files, 4 obsolete files)

Attached image 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.
Blocks: 627096
Assignee: nobody → tim.taubert
Version: unspecified → Trunk
Assignee: tim.taubert → nobody
Any way to reproduce this?
Let me see if I can still reproduce this.
I'm unable to reproduce this... though I don't think the pushAway and resize code has changed recently, which makes me uncomfortable.
Whiteboard: [visual][polish][good first bug] → [visual][polish][WFM?]
Tim has a mochitest which can consistently reproduce the issue. Tim, could you post it here?
Whiteboard: [visual][polish][WFM?] → [visual][polish]
Attached patch Test patch v2 (obsolete) — Splinter Review
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.
Attachment #514048 - Attachment is obsolete: true
Attachment #514149 - Flags: feedback?(tim.taubert)
Depends on: 634672
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?
Attachment #514149 - Flags: feedback?(tim.taubert) → feedback+
Attached patch v3 (obsolete) — Splinter Review
(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?
Attachment #514149 - Attachment is obsolete: true
Attachment #514398 - Flags: review?
Attachment #514398 - Attachment is patch: true
Attachment #514398 - Attachment mime type: application/octet-stream → text/plain
[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.
Blocks: 585689
No longer blocks: 627096
(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 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.
Attachment #514398 - Flags: review? → review+
Attached patch v4 (obsolete) — Splinter Review
Added code to verify that the tab's bounds indeed change.
Attachment #514398 - Attachment is obsolete: true
Attachment #515554 - Flags: review?(ian)
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?
(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?
(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.
Attached patch v5Splinter Review
(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?
Attachment #515554 - Attachment is obsolete: true
Attachment #515554 - Flags: review?(ian)
No longer blocks: 585689
Blocks: 603789
bugspam
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
bug spam
Target Milestone: --- → Firefox 5
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.