Closed Bug 629665 ([regression]) Opened 9 years ago Closed 9 years ago

Tab bar covers full screen

Categories

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

ARM
Android
defect

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: kbrosnan, Assigned: vingtetun)

References

Details

Attachments

(3 files, 2 obsolete files)

Attached image 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
Attached patch Patch (obsolete) — Splinter Review
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)
Attachment #507893 - Flags: review?(mark.finkle)
hmm, I can probably move 
+    event = document.createEvent("Events");
+    event.initEvent("TabClosed", true, false);
+    window.dispatchEvent(event);

before the this.selectedTab call!
Priority: -- → P3
Mark,
if you don't like the TabClosed event I can remove it from this patch and open an other bug for it.
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"
(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.
I want "TabClose" to work like it does on desktop
Attached patch Patch v0.2 (obsolete) — Splinter Review
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.
Assignee: nobody → 21
Attachment #507893 - Attachment is obsolete: true
Attachment #509380 - Flags: review?(mark.finkle)
Attachment #507893 - Flags: review?(mark.finkle)
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.
Let's go with your first approach, but call the event "TabRemove"
(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.
Attached patch Patch v0.2Splinter Review
Before landing I'm re-asking review to be sure we agree on the solution.
Attachment #509380 - Attachment is obsolete: true
Attachment #509474 - Flags: review?(mark.finkle)
Attachment #509380 - Flags: review?(mark.finkle)
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
Attachment #509474 - Flags: review?(mark.finkle) → review+
Attached image 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.
http://hg.mozilla.org/mobile-browser/rev/aa4eaa1f3916
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
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
Status: RESOLVED → VERIFIED
Duplicate of this bug: 632201
You need to log in before you can comment on or make changes to this bug.