631752 - Tab aspect ratio gets changed on drag-drop orphaning

Status: VERIFIED FIXED

(Firefox Graveyard :: Panorama, defect, P2)

Description

[Michael Yoshitaka Erlewine [:mitcho]]

See attached video. I'm pretty sure this didn't happen before.

Nightly: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b11pre) Gecko/20110202 Firefox/4.0b11pre

The tab thumbnail aspect ratio should be sacred.

Comment 1

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: Video

Comment 2

[Ian Gilman [:iangilman]]

Presumably this is the result of bug 625666? Should one be a duplicate of the other?

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v1

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 513902 [details] [diff] [review]
patch v1

Passed try: http://tbpl.mozilla.org/?tree=MozillaTry&pusher=[e-mail]&rev=a68c8e825be6

Comment 5

[Michael Yoshitaka Erlewine [:mitcho]]

Comment on attachment 513902 [details] [diff] [review]
patch v1

>-    return Math.max(0, Math.max(TabItems.minTabHeight, height - titleSize)) * 
>+    return Math.max(0, Math.max(TabItems.minTabHeight, height + titleSize)) * 

Why do we need Math.max(0, ...) here? Tabitems.minTabHeight is always positive, no?

Same question goes for getHeightForWidth, too.

Comment 6

[Michael Yoshitaka Erlewine [:mitcho]]

Also, in general: I'm not convinced that this is the only fix required to solve this bug. For example, try dragging a tab out of an expanded stack. This is a consistent way right now to mangle the tab aspect.

Because it's expanded, it should be displaying its title when you drag it out, but because it's isStacked, we call calcValidSize with hideTitle = true (see TabItem_setBounds). I think that's part of the issue here as well, at least for that STR.

Comment 7

[Ian Gilman [:iangilman]]

Comment on attachment 513902 [details] [diff] [review]
patch v1

Please address Mitcho's comments.

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v2

(In reply to comment #5)
> Why do we need Math.max(0, ...) here? Tabitems.minTabHeight is always positive,
> no?
> 
> Same question goes for getHeightForWidth, too.

Removed.

(In reply to comment #6)
> Also, in general: I'm not convinced that this is the only fix required to solve
> this bug. For example, try dragging a tab out of an expanded stack. This is a
> consistent way right now to mangle the tab aspect.

Tested when dragging tabs out of a stacked group and an expanded stacked group. Works for both.

Comment 9

[Ian Gilman [:iangilman]]

Comment on attachment 514072 [details] [diff] [review]
patch v2

Does it need a test?

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

This is testable so I should write one.

Comment 11

[Michael Yoshitaka Erlewine [:mitcho]]

Tim, as we discussed briefly on IRC, please also add some better comments, particularly in the headers of _getHeightForWidth and _getWidthForHeight which makes clear whether the height values expected/being dealt with include title space or not.

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

(In reply to comment #11)
> Tim, as we discussed briefly on IRC, please also add some better comments,
> particularly in the headers of _getHeightForWidth and _getWidthForHeight which
> makes clear whether the height values expected/being dealt with include title
> space or not.

Sorry I forgot about that :/ Will add them with the next patch.

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v3

So this is a completely new (and correct) approach. I don't know why the former one worked but Mitcho's intuition was right :)

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v3 (alternative approach)

Instead of setting the tab's height with every call to setBounds() we can just force the re-calculation once when removing the tabItem from a stacked group.

Comment 15

[Michael Yoshitaka Erlewine [:mitcho]]

Comment on attachment 514110 [details] [diff] [review]
patch v3 (alternative approach)

I think I prefer this alternative approach. Is there an advantage to making the fix in patch v3 as well?

A couple nits:

>+      // force tab item resize if we're stacked. the tab's title will be visible
>+      // and that's why we need to recalculate its height.
>+      if (this._isStacked)

We should use the method this.isStacked() here instead of accessing _isStacked directly.

Also, empirical question: do we need this if the group is expanded? I don't think we do. I think we only need this if the group is stacked and not expanded. If that's the case, while it shouldn't hurt to run this regardless of expanded status, we might as well check for expanded-ness and only run when it's needed.

Do you want me to feedback the other patch as well? I guess that depends on your answer to my first question.

Comment 16

[Tim Taubert [:ttaubert] (inactive)]

(In reply to comment #15)
> I think I prefer this alternative approach. Is there an advantage to making the
> fix in patch v3 as well?

Well I think v3b makes not much sense when v3 is applied. While v3b may have less performance impact, v3 will make sure this glitch will never happen when transitioning from stacked mode to non-stacked mode and the other way around - so that is a bit safer. What do you think? :)

> We should use the method this.isStacked() here instead of accessing _isStacked
> directly.

Ok, that's better.

> Also, empirical question: do we need this if the group is expanded? I don't
> think we do. I think we only need this if the group is stacked and not
> expanded. If that's the case, while it shouldn't hurt to run this regardless of
> expanded status, we might as well check for expanded-ness and only run when
> it's needed.

Yep, we need this, otherwise the tab aspect still gets invalid when dragging a tab out of an expanded stacked group.

> Do you want me to feedback the other patch as well? I guess that depends on
> your answer to my first question.

Please do if you think we should take the safer path or even do both.

Comment 17

[Michael Yoshitaka Erlewine [:mitcho]]

[bugspam: betaN -> final]

Patch in progress, hopefully pretty trivial. Let's try to see if we can get it approved soon.

Comment 18

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v4

Used the approach from v3b and added a test.

Comment 19

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 514761 [details] [diff] [review]
patch v4

Passed try (though it doesn't look like that because of the many oranges):

http://tbpl.mozilla.org/?tree=MozillaTry&pusher=[e-mail]&rev=ead62885292d

Comment 20

[Michael Yoshitaka Erlewine [:mitcho]]

Comment on attachment 514761 [details] [diff] [review]
patch v4

Patch looks good. Comments on the test:

>+    return Math.floor(bounds.height / bounds.width * 100) / 100;

Is computing this to one decimal point a good metric? Should we have an "acceptable range" of wiggle room, like plus or minus 2% or something instead? Not sure...

>+  let dragTabItem = function (tabItem) {

>+    let aspect = getTabItemAspect(tabItem);

The expected tab aspect should be the standard one: TabItems.tabHeight/TabItems.tabWidth, not computed for each individual tab.

>+    EventUtils.synthesizeMouseAtCenter(container, {type: "mousedown"}, cw);
>+    for (let x = 200; x <= 400; x += 100)
>+      EventUtils.synthesizeMouse(doc, x, 100, {type: "mousemove"}, cw);
>+    is(getTabItemAspect(tabItem), aspect, "tabItem aspect didn't change");
>+
>+    EventUtils.synthesizeMouseAtCenter(container, {type: "mouseup"}, cw);
>+    is(getTabItemAspect(tabItem), aspect, "tabItem aspect didn't change");

Make sure that the tab actually (a) moved physically and (b) has the right parent (or no parent), both before and after the move.

>+  showTabView(function () {

This test has the potential to move things around, snap, etc. Use newWindowWithTabView instead.

>diff --git a/browser/base/content/test/tabview/browser_tabview_bug634158.js b/browser/base/content/test/tabview/browser_tabview_bug634158.js

>+      tabItem.setSize(200, 200, true);

What's this about?

Comment 21

[Ian Gilman [:iangilman]]

Comment on attachment 514761 [details] [diff] [review]
patch v4

I like this fix better; I'm concerned about the performance impact with version 3.

Why the options.dontArrange check?

Please address Mitcho's test comments, of course.

Comment 22

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v5

(In reply to comment #20)
> Is computing this to one decimal point a good metric? Should we have an
> "acceptable range" of wiggle room, like plus or minus 2% or something instead?
> Not sure...

I use TabItems.tabAspect now and allow a variance of +-1%.

> The expected tab aspect should be the standard one:
> TabItems.tabHeight/TabItems.tabWidth, not computed for each individual tab.

Fixed (see above).

> Make sure that the tab actually (a) moved physically and (b) has the right
> parent (or no parent), both before and after the move.

Done.

> This test has the potential to move things around, snap, etc. Use
> newWindowWithTabView instead.

Fixed.

> >diff --git a/browser/base/content/test/tabview/browser_tabview_bug634158.js b/browser/base/content/test/tabview/browser_tabview_bug634158.js
> 
> >+      tabItem.setSize(200, 200, true);
> 
> What's this about?

This is because the .expander element has a specific size and if the tabItem gets too small this does not pass the bounds.contains() check. I rewrote that test to use newWindowWithTabView(), so we always get the same tab size and the test doesn't move things around.

(In reply to comment #21)
> Why the options.dontArrange check?

setBounds() causes a groupItem.arrange() call that we don't want at this point because the groupItem gets closed.

Comment 23

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 516229 [details] [diff] [review]
patch v5

Passed try:

http://tbpl.mozilla.org/?tree=MozillaTry&pusher=[e-mail]&rev=8b6e59cbcec8

Comment 24

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v6

Raised the variance to +-1.5% to let the patch for bug 634672 pass.

Comment 25

[Ian Gilman [:iangilman]]

Comment on attachment 516322 [details] [diff] [review]
patch v6

>+      if (!options.dontArrange && this.isStacked())
>+        item.setBounds(item.getBounds(), true, {force: true});

As discussed on IRC, please remove the options.dontArrange, and figure out what the error you were seeing was. 

The test looks good.

Comment 26

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v7

(In reply to comment #25)
> As discussed on IRC, please remove the options.dontArrange, and figure out what
> the error you were seeing was.

The condition is now (item.isDragging && this.isStacked()) because we only need to cover the case when dragging a tabItem out of a stacked groupItem (expanded and not expanded).

The error occured because setBounds() was called on an unlinked tabItem when groupItem.remove(unlinkedTabItem) was called.

Comment 27

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 522054 [details] [diff] [review]
patch v7

Passed try:

http://tbpl.mozilla.org/?tree=MozillaTry&pusher=[e-mail]&rev=c942a4196b2f

Comment 28

[Ian Gilman [:iangilman]]

Comment on attachment 522054 [details] [diff] [review]
patch v7

Looks good

Comment 29

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch for checkin

Comment 30

[Kevin Hanes]

bugspam

Comment 31

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/22fe7cd1264b

Comment 32

[George Carstoiu]

Verified issue on Mozilla/5.0 (X11; Linux i686; rv:2.2a1pre) Gecko/20110404 Firefox/4.2a1pre.