633074 - Zoom in animation is not shown for tabs deep within stacks

Status: VERIFIED FIXED

(Firefox Graveyard :: Panorama, defect, P2)

Description

[Michael Yoshitaka Erlewine [:mitcho]]

Seeing this in the nightly: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b12pre) Gecko/20110209 Firefox/4.0b12pre

STR:
1. Create a stacked group with at least six or so tabs. Select one of the last ones.
2. Go in and out of Panorama.

Expected: zoom animation happens.
Actual: no zoom animation. Zoom animation still works on non-stacks and top six or so tabs of stacks.

Note that this doesn't seem to just be an issue of the TabCanvas not being updated... if you open the group up with the expander or resize the group and make sure the TabCanvas updates and then follow the STR again, you can still reproduce it.

My thought: pretty sure this happened because of bug 606148 landing its setHidden... we probably just need to unhide the canvas when doing zoom animations.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v1

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

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

Passed try:

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

Comment 4

[Michael Yoshitaka Erlewine [:mitcho]]

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

>diff --git a/browser/base/content/tabview/tabitems.js b/browser/base/content/tabview/tabitems.js
>+      if (this.getHidden())

We should only ever be hidden if we're in a stack, but just in case, should we check that this.parent exists here?

>+        this.parent.setTopChild(this);

So, this works, but I'm confused about the topChild spec. It seems (at least in my mind) that there's been some confusion as to what our policy is. Here are two natural options:

1. topChild is always the first tab in the group, i.e. the lowest-tab-index (so leftmost in ltr) tab
2. the most recently accessed tab.

Comment 5

[Michael Yoshitaka Erlewine [:mitcho]]

CC faaborg to answer the ux question in comment 4.

Comment 6

[Michael Yoshitaka Erlewine [:mitcho]]

Nevermind... bug 634085 makes clear our spec is option 2:

2. topChild is the most recently accessed tab.

Thanks Tim for pointing this out to me on IRC.

Comment 7

[Michael Yoshitaka Erlewine [:mitcho]]

Note bug 635737, which is due to a very similar cause. I posted a patch there which is a slight (more general) revision on the patch here.

Comment 8

[Michael Yoshitaka Erlewine [:mitcho]]

Bug 635737 landed. This should be fixed.

Comment 9

[George Carstoiu]

Mozilla/5.0 (X11; Linux i686; rv:2.0b12) Gecko/20100101 Firefox/4.0b12

I am still able to reproduce issue using beta 12.

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

(In reply to comment #9)
> I am still able to reproduce issue using beta 12.

Yeah, that didn't make it into b12. Will be in rc1.

Comment 11

[George Carstoiu]

Verified with: Mozilla/5.0 (X11; Linux i686; rv:2.0) Gecko/20100101 Firefox/4.0