629665 - ([regression]) Tab bar covers full screen

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P3)

Description

[Kevin Brosnan]

Attachment: screenshot of the issue

My tab bar expanded to cover all the screen.

Mozilla/5.0 (Android; Linux armv7l; rv:2.0b11pre) Gecko/20110127 Firefox/4.0b11pre Fennec/4.0b5pre ID:20110127162904

Comment 1

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

Attachment: Patch

This bug is a regression from bug 628732. I've done a mistake by substracting the height of the undo tab box.

This patch fixes that and also fix some funny bugs encountered when adding tests, like: 
 * the scroll position can be wrong when the undo tab is removed from the content and the sidebar is hidden
 * the scroll position can be wrong when a tab is closed (which is the reason for the new event, that fire _after_ the tab has been removed from the DOM)

Comment 2

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

hmm, I can probably move 
+    event = document.createEvent("Events");
+    event.initEvent("TabClosed", true, false);
+    window.dispatchEvent(event);

before the this.selectedTab call!

Comment 3

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

Mark,
if you don't like the TabClosed event I can remove it from this patch and open an other bug for it.

Comment 4

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

Can we drop "TabClosed" and use "TabClose", but setTimeout(..., 0) in the event handler. That should allow the teardown of the Tab to complete and should act like a "TabClosed"

Comment 5

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

(In reply to comment #4)
> Can we drop "TabClosed" and use "TabClose", but setTimeout(..., 0) in the event
> handler. That should allow the teardown of the Tab to complete and should act
> like a "TabClosed"

My problem the tab.chromeTab element (on which the event is dispatched) would be unexistent at this point :/

Another possibility is to rename the TabClose event TabWillClose and fired a new TabClosed event on the window? This is basically the same as what i already do in the patch except the name is closer to the truth.

Comment 6

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

I want "TabClose" to work like it does on desktop

Comment 7

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

Attachment: Patch v0.2

This is not adding any new event. I'm just keeping a reference to the dom element to have it alive for the duration of the function.

Comment 8

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

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

We are changing the way TabClose works here. The "browser" is now dead by the
time TabClose is fired. The chromeTab is now removed from the document too. I'd
worry that the event wouldn't even fire correctly, since the element is no
longer in the DOM.

I think this will break:
http://mxr.mozilla.org/mobile-browser/source/components/SessionStore.js#212
which wants access to the "browser"

Let me think a bit more about the previous idea of adding a "TabRemove" or something event.

Comment 9

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

Let's go with your first approach, but call the event "TabRemove"

Comment 10

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

(In reply to comment #9)
> Let's go with your first approach, but call the event "TabRemove"

Ok, I will update the patch during landing.

Comment 11

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

Attachment: Patch v0.2

Before landing I'm re-asking review to be sure we agree on the solution.

Comment 14

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

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


>     Elements.tabs.addEventListener("TabSelect", this, true);
>     Elements.tabs.addEventListener("TabOpen", this, true);
>+    window.addEventListener("TabRemove", this, true);

I think we should fire the event on the chromeTab or the parent of the chrometab, so Elements.tabs.addEventListener should work for "TabRemove" too

>     event.initEvent("TabClose", true, false);
>     tab.chromeTab.dispatchEvent(event);
>     tab.browser.messageManager.sendAsyncMessage("Browser:TabClose");
> 
>     tab.destroy();
>     this._tabs.splice(tabIndex, 1);
> 
>     this.selectedTab = nextTab;
>+
>+    event = document.createEvent("Events");
>+    event.initEvent("TabRemove", true, false);
>+    window.dispatchEvent(event);

Hold onto the chromeTab until the end of this function and fire the TabRemove event on it, or if that won't work, use the parent of the chromeTab. But let's not use the window.

r+ with that change

Comment 15

[Ioana Chiorean]

Attachment: double column

for Mozilla /5.0 (Maemo;Linux armv7l;rv:2.0b12pre)Gecko/20110204 Firefox/4.0b12pre Fennec /4.0b5pre 
Build ID: 20110204004947, Platform: Linux-maemo, Operating System: Linux-arm

having three tabs and closing one of them, will double the tab panel.

Comment 16

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

http://hg.mozilla.org/mobile-browser/rev/aa4eaa1f3916

Comment 17

[Ioana Chiorean]

Build Identifier: Mozilla/5.0 (Android; Linux armv7l; rv:2.0b8pre)
Gecko/20101104 Firefox/4.0b8pre Fennec/4.0b3pre 
Device: HTC Desire

The columns are not duplicated anymore, so I will set the bug to Verified Fixed