Ghost tab in the undo panel when opening a local page

VERIFIED FIXED

Status

Firefox for Android Graveyard
General
VERIFIED FIXED
8 years ago
8 years ago

People

(Reporter: vingtetun, Assigned: vingtetun)

Tracking

Bug Flags:
in-testsuite ?

Details

Attachments

(1 attachment, 2 obsolete attachments)

Created attachment 493731 [details] [diff] [review]
WIP v0.1

Steps to reproduce:
 * launch fennec
 * open a new tab
 * load about:home
 * slide to the right to see the left sidebar

Actual result:
 * There is 2 tabs and a black tab in the undo panel

Expected result:
 * there is 2 tabs
Created attachment 493953 [details] [diff] [review]
Patch + tests

The patch add a new parameter to the Browser.closeTab function in order to prevent a tab to appears in the undo box in the UI. The "cant undo" boolean value is also sent into a UIEvent used by tab.
Assignee: nobody → 21
Attachment #493731 - Attachment is obsolete: true
Attachment #493953 - Flags: review?(mark.finkle)
Comment on attachment 493953 [details] [diff] [review]
Patch + tests

>diff --git a/chrome/content/browser.js b/chrome/content/browser.js
>--- a/chrome/content/browser.js
>+++ b/chrome/content/browser.js
>@@ -529,17 +529,17 @@ var Browser = {
>     // new "remote" tabs if opening web pages from a "local" about: page.
>     let currentURI = browser.currentURI.spec;
>     let useLocal = Util.isLocalScheme(aURI);
>     let hasLocal = Util.isLocalScheme(currentURI);
> 
>     if (hasLocal != useLocal) {
>       let oldTab = this.selectedTab;
>       if (currentURI == "about:blank" && !browser.canGoBack && !browser.canGoForward) {
>-        this.closeTab(oldTab);
>+        this.closeTab(oldTab, true);

I'd rather add a property to the "tabs" binding than pass a parameter. The parameter just needs to be passed to the "tabs" binding anyway, so let's do it more explicitly:

        Elements.tabs.allowUndo = false;
        try {
          this.closeTab(oldTab);
        } catch (e) { Cu.reportError(e); }
        Elements.tabs.allowUndo = true;

>-  closeTab: function(tab) {
>-    if (tab instanceof XULElement)
>-      tab = this.getTabFromChrome(tab);
>+  closeTab: function(aTab, aCantUndo) {

No need for aCantUndo

>+    let tab = aTab;
>+    if (aTab instanceof XULElement)
>+      tab = this.getTabFromChrome(aTab);
> 
>     if (!tab)
>       return;

Can you change this check to:

     if (!tab || this._tabs.length < 2)
       return;

That would help fix bug 615404

>-    let event = document.createEvent("Events");
>-    event.initEvent("TabClose", true, false);
>+    let event = document.createEvent("UIEvents");
>+    event.initUIEvent("TabClose", true, false, window, !!aCantUndo);

No need for this change

>   destroy: function destroy() {
>-    document.getElementById("tabs").removeTab(this._chromeTab);

Leave this. I don't want to depend on the event. You could add "tabs" to the Elements object ( Elements.tabs.removeTab(...) )

>diff --git a/chrome/content/tabs.xml b/chrome/content/tabs.xml

>+    <handlers>
>+      <handler event="TabClose">
>+        <![CDATA[
>+          this._container.removeTab(this, event.detail);
>+        ]]>
>+      </handler>
>+    </handlers>

Don't do this. But add the "allowUndo" property instead

>       <method name="removeTab">
>         <parameter name="aTab"/>
>+        <parameter name="aCantUndo"/>

No need for aCantUndo. Check the this.allowUndo property instead

Update any tests that were depending on the extra param in closeTab or the TabClose event
Attachment #493953 - Flags: review?(mark.finkle) → review-
I've hesitate to add a property to the tab object itself first, I guess it make things much simpler.
Created attachment 494371 [details] [diff] [review]
Patch v0.2 + tests

The patch addressed comments but do not use Elements.tabs since it is already used for the tab sidebar.
Attachment #493953 - Attachment is obsolete: true
Attachment #494371 - Flags: review?(mark.finkle)
Attachment #494371 - Flags: review?(mark.finkle) → review+
Whiteboard: [fennec-checkin-postb3]
Flags: in-testsuite?
Blocks: 617795
pushed:
http://hg.mozilla.org/mobile-browser/rev/d1a74d739c0c
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Whiteboard: [fennec-checkin-postb3]
verified FIXED on builds:
Mozilla/5.0 (Maemo; Linux armv71; rv:2.0b8pre) Gecko/20101210 Namoroka/4.0b8pre Fennec/4.0b3pre

and

Mozilla/5.0 (Android; Linux armv71; rv:2.0b8pre) Gecko/20101210 Namoroka/4.0b8pre Fennec/4.0b3pre
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.