454667 - Add ability to close tabs

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Madhava Enros [:madhava]]

Right now, there's now visible way to close a tab in Fennec (there may be a key combination).  We should have one!

Along these lines: http://blog.seanmartell.com/wp-content/uploads/2008/09/fennec_screen_4.jpg

Comment 1

[Madhava Enros [:madhava]]

Attachment: close button for the tabs

Comment 2

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

Attachment: WIP 1: first pass patch

This patch adds the (x) image over the tab thumbnail and support clicking the (x) to close the tab.

The patch has some problems: Clicking (x) for the selected tab seems to work ok, but trying to close a tab that is not selected (the current tab) results in some "this.browser = null" errors. For some reason the display list is not updating fast enough(?)

Comment 3

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

Attachment: full patch

This patch can remove the selected or any unselected tab. It also hides the "remove" button if there is only 1 tab

Comment 4

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 339948 [details] [diff] [review]
full patch

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

>+            if (!nextTab)
>+              return;

We probably want to remove the current tab and add a new blank one in this case to match tabbrowser behavior, right? Can do in a followup (we have a bunch of other tabbrowser-compat bugs to fix).

>+      <constructor><![CDATA[
>+        let close = document.getAnonymousElementByAttribute(this, "anonid", "close");
>+        let closefn = new Function("event", parentNode.getAttribute("onclosetab"));

explicit "this.parentNode" would be clearer.

>+        var self = this;
>+        close.addEventListener("mousedown", function(event) { closefn.call(self, event); event.stopPropagation(); }, true);
>+      ]]></constructor>

Can you use a <handler> for this instead?

Comment 5

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

(In reply to comment #4)
> (From update of attachment 339948 [details] [diff] [review])
> >diff --git a/chrome/content/deckbrowser.xml b/chrome/content/deckbrowser.xml
> 
> >+            if (!nextTab)
> >+              return;
> 
> We probably want to remove the current tab and add a new blank one in this case
> to match tabbrowser behavior, right? Can do in a followup (we have a bunch of
> other tabbrowser-compat bugs to fix).

Followup sounds good

> 
> >+      <constructor><![CDATA[
> >+        let close = document.getAnonymousElementByAttribute(this, "anonid", "close");
> >+        let closefn = new Function("event", parentNode.getAttribute("onclosetab"));
> 
> explicit "this.parentNode" would be clearer.

Done

> 
> >+        var self = this;
> >+        close.addEventListener("mousedown", function(event) { closefn.call(self, event); event.stopPropagation(); }, true);
> >+      ]]></constructor>
> 
> Can you use a <handler> for this instead?

I'll test out a patch to get it working. The handler is currently on the image, not the richlistitem, but we probably do stuff like that in the tree.

Followup bug 457635

http://hg.mozilla.org/mobile-browser/rev/385a8f06a09c (missed a review comment)
http://hg.mozilla.org/mobile-browser/rev/f18cbb0c115a (main landing)

Comment 6

[Joel Maher ( :jmaher ) (UTC -8)]

verified in beta3