524469 - Opening enough tabs to create additional columns cuts off the initial column

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Aakash Desai [:aakashd]]

Build Id:

Mozilla/5.0 (X11; U; Linux armv7l; en-US; rv:1.9.2b1pre) Gecko/20091026
Fennec/1.0b5pre

and

Mozilla/5.0 (X11; U; Linux armv6l; en-US; rv:1.9.3a1pre) Gecko/20091026
Fennec/1.0b5pre


Steps to Reproduce:
1. Go to http://popuptest.com/popuptest1.html
2. Click on "Show"


Actual Results:
The tab sidebar will grow to additional columns (in most cases, try a couple times to get an instance where it creates enough pop-ups) that the newly created columns show up on the screen

Expected Results:
The tab sidebar should not show up

Screen Shot: http://www.flickr.com/photos/42893104@N04/4047070826/

Comment 1

[Brad Lassey [:blassey] (use needinfo?)]

your screen shot isn't publicly visible

Comment 2

[Aakash Desai [:aakashd]]

Sorry about that, apparently the n900 automatically sets uploaded pictures via flickr as private. try again.

Comment 3

[Stuart Parmenter]

We probably just need to call onAfterVisibleMove when new tabs get added dynamically

Comment 4

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch

This is a workaround which call Browser.hideSidebars (which itself call onAfterVisibleMove) when a new tab is added and the tabs sidebar is not fully visible (>0 && <1)

The workaround works on popuptest and also when we add new tab through Ctrl+T

Comment 5

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 413944 [details] [diff] [review]
Patch

I know that this is probably not the best way to do it, but I've not found any other clean way for that.

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

As I think about this more, I wish we didn't have this code in .addTab

We could put some code in the tabs.xml resize method or even the "TabOpen" event handler.

Comment 7

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to comment #6)
> As I think about this more, I wish we didn't have this code in .addTab
> 
> We could put some code in the tabs.xml resize method or even the "TabOpen"
> event handler.

Practically I'm agree with you, this code will more easily fit in tabs.xml. I can move it here but I've not previously done it for 2 reasons:
 * It is a workaround, it did not correct the real problem, it just hide the bug
 * I want to avoid use references to the Browser object in tabs.xml

I guess since it's only a workaround we can move it into tabs.xml because is lifetime _should_ be short (I hope)

Comment 9

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch v0.2

Move the workaround to TabOpen.

The patch also resolves bug 528526 - yeah, our notifications code was broken :/

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 415487 [details] [diff] [review]
Patch v0.2

Thanks for moving to "TabOpen". It puts the work around away from code that shouldn't need to worry about it, which is a good thing.

Comment 11

[Mark Finkle (:mfinkle) (use needinfo?)]

pushed:
https://hg.mozilla.org/mobile-browser/rev/a1d558f49eab

(removed some C.reportError() that I left in by mistake too)

Comment 12

[Aakash Desai [:aakashd]]

verified FIXED on builds:
Mozilla/5.0 (X11; U; Linux armv7l; Nokia N900; en-US; rv:1.9.2b5pre) Gecko/20091202 Firefox/3.6b5pre Fennec/1.0b6pre

and

Mozilla/5.0 (X11; U; Linux armv6l; Nokia N8xx; en-US; rv:1.9.3a1pre) Gecko/20091202 Firefox/3.7a1pre Fennec/1.0b5

Comment 13

[Al Billings [:abillings - ex-MoCo]]

Added litmus testcase: https://litmus.mozilla.org/show_test.cgi?searchType=by_id&id=9832