623911 - First tab in stack changes when expand button is opened and then closed

Status: VERIFIED FIXED

(Firefox Graveyard :: Panorama, defect, P3)

Description

[George Carstoiu]

User-Agent:       Mozilla/5.0 (Windows NT 5.1; rv:2.0b9pre) Gecko/20110106 Firefox/4.0b9pre
Build Identifier: Mozilla/5.0 (Windows NT 5.1; rv:2.0b9pre) Gecko/20110106 Firefox/4.0b9pre

By opening and closing the stacked list with tabs, using the expand button, the first tab from the pile changes to the first tab present in tab view.


Reproducible: Always

Steps to Reproduce:
1.Open at least three websites in same group (ex: www.google.com, www.nasa.gov, www.cnn.com)
2.Go to Panorama and minimize group until you get stacked pile.
3.Press expand button and select www.nasa.gov
4.Go to Panorama and observe stacked pile (nasa.gov should be the first tab)
5.Press expand button
6.Dismiss opened window

Actual Results:  
First tab from tab view (www.google.com) is shown as first tab in pile.


Expected Results:  
Previous accessed window is shown as first in pile (our case nasa.gov).

Also, when switching for the first time from tab view to panorama and minimizing in order to obtain the stack, the first tab in the pile is not the last accessed one (in the above example www.nasa.gov).

Comment 1

[Michael Yoshitaka Erlewine [:mitcho]]

Would this improve/be fixed by bug 616729, which should better enforce that the tab order in groups (and therefore which tab is the topmost in a stack) better sync the real tab order?

Comment 2

[Ian Gilman [:iangilman]]

Bug 616729 has now landed; can you see if this is still happening, George?

Comment 3

[George Carstoiu]

Mozilla/5.0 (Windows NT 6.1; rv:2.0b11pre) Gecko/20110127 Firefox/4.0b11pre

I can still reproduce the issue.

Comment 4

[Raymond Lee [:raymondlee]]

Attachment: v1

Comment 5

[Ian Gilman [:iangilman]]

Comment on attachment 510235 [details] [diff] [review]
v1

In addition to removing the topChild = null from _gridArrange(), you need to add a topChild = null to arrange() right before it calls _gridArrange(). topChild is supposed to be relatively temporary, and shouldn't survive a group getting unstacked (though I'm fine with it surviving expand/collapse).

Otherwise looks good.

Comment 6

[Raymond Lee [:raymondlee]]

Attachment: v1

(In reply to comment #5)
> Comment on attachment 510235 [details] [diff] [review]
> v1
> 
> In addition to removing the topChild = null from _gridArrange(), you need to
> add a topChild = null to arrange() right before it calls _gridArrange().
> topChild is supposed to be relatively temporary, and shouldn't survive a group
> getting unstacked (though I'm fine with it surviving expand/collapse).
> 
Put topChild = null right before it calls _gridArrange() would simply bring back the bug.  I've moved the topChild = null to the else block in _gridArrange() so topChild would get cleared when a group is not stacked.

Comment 7

[Ian Gilman [:iangilman]]

Comment on attachment 510500 [details] [diff] [review]
v1

Good call

Comment 8

[Michael Yoshitaka Erlewine [:mitcho]]

Raymond, please make sure that this bug doesn't make the bug 633074 regression worse... just want to make sure, as this touches similar code paths.

Comment 9

[Raymond Lee [:raymondlee]]

(In reply to comment #8)
> Raymond, please make sure that this bug doesn't make the bug 633074 regression
> worse... just want to make sure, as this touches similar code paths.

I am certain that it would make the bug 633074 regression worse.

Comment 10

[Raymond Lee [:raymondlee]]

(In reply to comment #9)
> (In reply to comment #8)
> > Raymond, please make sure that this bug doesn't make the bug 633074 regression
> > worse... just want to make sure, as this touches similar code paths.
> 
> I am certain that it would make the bug 633074 regression worse.

Oops! I mean it would not.

Comment 11

[Raymond Lee [:raymondlee]]

Passed try.  http://tbpl.mozilla.org/?tree=MozillaTry&rev=5929e502ab00

Comment 12

[Michael Yoshitaka Erlewine [:mitcho]]

(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #8)
> > > Raymond, please make sure that this bug doesn't make the bug 633074 regression
> > > worse... just want to make sure, as this touches similar code paths.
> > 
> > I am certain that it would make the bug 633074 regression worse.
> 
> Oops! I mean it would not.

lol good. That's what I thought. :)

Comment 13

[Dietrich Ayala (:dietrich)]

Comment on attachment 510500 [details] [diff] [review]
v1

low footprint, has tests, improves behavioral consistency in Panorama -> a+

Comment 14

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: Patch for checkin

Comment 15

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/de0d135053fc

Comment 16

[George Carstoiu]

Mozilla/5.0 (Windows NT 6.1; rv:2.0b12pre) Gecko/20110215 Firefox/4.0b12pre

Issue no longer present.