686654 - "Almost stacked" group might switch between stacked and unstacked display

Status: VERIFIED FIXED

(Firefox Graveyard :: Panorama, defect)

Description

[Andreas Jung]

Attachment: Screencast of the bug

Steps to reproduce:
1) Create a (new) tab group with two tabs in it.
2) Switch to Panorama and resize the group to be as small as possible without
   causing the tab group to switch to stacked view.
   => The group contains two tabs and is not stacked
3) Leave Panorama and create a third tab in this group.
4) Close the third tab again and switch back to Panorama.
   => The group contains two tabs and is stacked
5) Click the expand button and dismiss the expanded view.
   => The group is not stacked again

Since this sounds complicated I will attach a screencast.

Comment 1

[Virgil Dicu [:virgil] [QA]]

Mozilla/5.0 (X11; Linux x86_64; rv:9.0a1) Gecko/20110913 Firefox/9.0a1
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:9.0a1) Gecko/20110913 Firefox/9.0a1
Mozilla/5.0 (Windows NT 6.1; rv:9.0a1) Gecko/20110913 Firefox/9.0a1

Reproduced using the steps in the description.

Regression range (the two tabs getting stacked(step 4)):

Last good nightly: 2011-08-09
First bad nightly: 2011-08-10

Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=87e3ea12ed5d&tochange=04dfb49d3a3d

The result from step 5 seems to enter in a separate range. Willing to search if necessary.

Comment 2

[Raymond Lee [:raymondlee]]

Attachment: v1

The regression is caused by this change set
http://hg.mozilla.org/mozilla-central/rev/248aed2b337d

We shouldn't check the this.isStacked() in GroupItem_getContentBounds() because GroupItem_shouldStack() uses GroupItem_getContentBounds() which expects it to return the content bounds for non-stacked group.

Comment 3

[Dietrich Ayala (:dietrich)]

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

Review of attachment 561400 [details] [diff] [review]:
-----------------------------------------------------------------

code changes look fine. looking at the STR, this should be testable. can you add a test?

Comment 4

[Raymond Lee [:raymondlee]]

Attachment: v2

With test

Comment 5

[Dietrich Ayala (:dietrich)]

Comment on attachment 561689 [details] [diff] [review]
v2

Review of attachment 561689 [details] [diff] [review]:
-----------------------------------------------------------------

looks good, thanks!

Comment 6

[Raymond Lee [:raymondlee]]

Attachment: Patch for checkin

Push to try and waiting for the results
https://tbpl.mozilla.org/?tree=Try&rev=53f3f5aeddd2

Comment 7

[Raymond Lee [:raymondlee]]

(In reply to Raymond Lee [:raymondlee] from comment #6)
> Created attachment 561952 [details] [diff] [review] [diff] [details] [review]
> Patch for checkin
> 
> Push to try and waiting for the results
> https://tbpl.mozilla.org/?tree=Try&rev=53f3f5aeddd2

Passed Try

Comment 8

[Ed Morley [:emorley]]

Thanks for the nicely formatted patch + try run URL, makes pushing much easier :-)

https://hg.mozilla.org/integration/mozilla-inbound/rev/35e3a5f3c07b

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/35e3a5f3c07b

Comment 10

[Virgil Dicu [:virgil] [QA]]

Mozilla/5.0 (X11; Linux x86_64; rv:9.0a1) Gecko/20110925 Firefox/9.0a1

Verified on Ubuntu 11.04, Windows XP, 7 and Mac OS 10.6 using the steps in the description. Tab group is no longer stacked as in step 4.