345258 - Removal of the corresponding menuitem of the all tabs menu popup if a tab is closed while the menu is open

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[(not reading, please use [e-mail] instead)]

Removal of the corresponding menuitem of the all tabs menu popup if a tab is closed while the menu is open

need a test case for this.

Comment 1

[John Mellor (Jomel)]

What about if tabs are opened while the menu is showing? (e.g. a popup opened from a timer started by a user clicking a link). Probably not hugely important though, admittedly.

Comment 2

[Michael Ventnor]

Attachment: Add more listeners to the menu

This patch should fix what remains of any case where the menu doesn't update itself. It adds three new listeners:

a) The menu didn't change the bold entry when the tab changed focus. Open the menu then mouse-scroll through the tabs, watch what happens. This was just an innocent oversight on whoever originally made the all tabs menu dynamic.
b) The menu will check for when a tab is removed and remove the menu item, and...
c) The menu will refresh itself when a new tab is opened.

I have tested these by adding delayed tab removals and additions within the code during development, and they work. I don't know how to test this in the wild.

Comment 3

[Michael Ventnor]

Attachment: Test case

A test case which will create a new tab then close it.

Comment 4

[(not reading, please use [e-mail] instead)]

Sorry for the delay.  thanks for the patch and the test case, michael.  

I'll try it out and do the initial reviews, and if all goes well, I'll seek a second review from Asaf.

Comment 5

[Michael Ventnor]

Comment on attachment 241009 [details] [diff] [review]
Add more listeners to the menu

Seth is concentrating on Places now so I don't think I can count on a review any time soon. Mano, can I transfer the review to you?

Comment 6

[Mano (::mano, needinfo? for any questions; not reading general bugmail)]

Comment on attachment 241009 [details] [diff] [review]
Add more listeners to the menu

Use the TabClose event instead. Did you actually test appending a menuitem to the menu from the tabopen listener?

Comment 7

[Michael Ventnor]

Attachment: Patch refactoring some of the all tabs menu

OK, I've used the TabClose event and I've made the code more efficient. I did test all parts of this patch in as many ways I could think of and it all works.

Comment 8

[Mano (::mano, needinfo? for any questions; not reading general bugmail)]

Comment on attachment 242378 [details] [diff] [review]
Patch refactoring some of the all tabs menu

Don't introduce additional complexity here, the isVisible attribute is out of the scope of this bug.

Comment 9

[Michael Ventnor]

Attachment: Patch refactoring some of the all tabs menu 2

Sorry, I thought I could kill two birds with one stone.

Comment 10

[Michael Ventnor]

Attachment: Improve the menu even further

This patch will add listeners for the tab position. Now tab movements will be picked up by the menu. In addition, undo close tab can now work while the menu is open. I did extensive testing on each part of this patch.
I don't think another bug on the menu will ever be filed again. :)

Comment 11

[Mano (::mano, needinfo? for any questions; not reading general bugmail)]

Comment on attachment 242948 [details] [diff] [review]
Improve the menu even further

>Index: tabbrowser.xml
>===================================================================

>@@ -2958,90 +2959,152 @@
>           var menuItem = aEvent.target.mCorrespondingMenuitem;
>           if (menuItem) {
>             var attrName = aEvent.attrName;
>             switch (attrName) {
>               case "label":
>               case "crop":
>               case "busy":
>               case "image":
>+              case "selected":
>                 if (aEvent.attrChange == aEvent.REMOVAL)
>                   menuItem.removeAttribute(attrName);
>                 else
>                   menuItem.setAttribute(attrName, aEvent.newValue);
>             }
>           }
>         ]]></body>
>       </method>
> 
>+      <method name="_tabOnTabClose">
>+        <parameter name="aEvent"/>
>+        <body><![CDATA[
>+          var menuItem = aEvent.target.mCorrespondingMenuitem;
>+          if (menuItem)
>+            menuItem.parentNode.removeChild(menuItem);

this.removeChild(menuItem) should work too

>+        ]]></body>
>+      </method>
>+
>       <method name="handleEvent">
>         <parameter name="aEvent"/>
>         <body><![CDATA[
>           if (!aEvent.isTrusted)
>             return;
> 
>           switch (aEvent.type) {
>             case "command":
>               this._menuItemOnCommand(aEvent);
>               break;
>             case "DOMAttrModified":
>               this._tabOnAttrModified(aEvent);
>+              break;
>+            case "TabClose":
>+              this._tabOnTabClose(aEvent);
>+              break;
>+            case "TabOpen":
>+            case "TabMove":

Tabs cannot be moved while the menu is open, so this is not necessary.

>+              this.createTabMenuItem(aEvent.originalTarget);
>+              break;
>           }
>         ]]></body>
>       </method>
>-    </implementation>
> 
>-    <handlers>
>-      <handler event="popupshowing">
>-      <![CDATA[
>-        // set up the menu popup
>-        var tabcontainer = document.getBindingParent(this);
>-        var tabs = tabcontainer.childNodes;
>+      <method name="appendMenuItem">
>+        <parameter name="aMenuItem"/>
>+        <body><![CDATA[
>+          // Append the menu item so its position is consistent with the tab's position.
>+          // This allows the menu to now accomodate undo close tab and tab moving while open.
> 
>-        // if an animation is in progress and the user
>-        // clicks on the "all tabs" button, stop the animation
>-        tabcontainer._stopAnimation();
>+          // Do some basic garbage collection first.
>+          for (var i = 0; i < this.childNodes.length; i++) {
>+            if (this.childNodes[i].tab == aMenuItem.tab) {
>+              this.removeChild(this.childNodes[i]);
>+              i--;
>+            }
>+          }
>+
>+          var posOfEndTab = document.getBindingParent(this).childNodes.length - 1;
>+          var tabPos = aMenuItem.tab._tPos;
>+          if (tabPos == posOfEndTab) {
>+            this.appendChild(aMenuItem);
>+          } else {
>+            this.insertBefore(aMenuItem, this.childNodes[tabPos]);
>+          }

and since tabs cannot be moved while the menu is open, the additional complexity here isn't necessary as well. 

>+        ]]></body>
>+      </method>
>+
>+      <method name="createTabMenuItem">
>+        <parameter name="aTab"/>
>+        <body><![CDATA[
>+          if (aTab.localName != "tab")
>+            return false;

Does this ever happen?

Comment 12

[Peter Kasting]

> and since tabs cannot be moved while the menu is open, the additional
> complexity here isn't necessary as well. 

Couldn't this change if/when we do smooth scrolling for tabs?  What about things extensions might do?  Being overly safe seems OK to me...

Comment 13

[Mano (::mano, needinfo? for any questions; not reading general bugmail)]

XUL menus are closed when you mousedown anything else in the opening window...

Comment 14

[Michael Ventnor]

(In reply to comment #13)
> XUL menus are closed when you mousedown anything else in the opening window...

But what if a call to moveTabTo() happens while the menu is open? What's wrong with making this patch future-proof?
Nonetheless I'll address these comments when I get home.

Comment 15

[Mano (::mano, needinfo? for any questions; not reading general bugmail)]

Just for the record, I don't see the value of changing the position of menu-items in the menu, even if there was such a case.

Comment 16

[Michael Ventnor]

Attachment: Patch, review comments addressed

Addresses Mano's comments.

Comment 17

[Mano (::mano, needinfo? for any questions; not reading general bugmail)]

Comment on attachment 243563 [details] [diff] [review]
Patch, review comments addressed

>Index: tabbrowser.xml
>===================================================================


>+
>+      <method name="createTabMenuItem">

nit: this is a pseudo-private method and should therefore be prefixed with an underscore.

>+        <parameter name="aTab"/>
>+        <body><![CDATA[
>+          var menuItem = document.createElementNS(
>+            "http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul", 
>+            "menuitem");
>+
>+          if (aTab.selected)
>+            menuItem.setAttribute("selected", "true");

nit: do so either right after or right before we set the busy attribute.

>+          menuItem.tab.removeEventListener("TabClose", this, false);
>           menuItem.tab.mCorrespondingMenuitem = null;
>           this.removeChild(menuItem);
>         }
>+        var tabroot = 

s/tabroot/tabbrowser.


r=mano with those addressed.

Comment 18

[Michael Ventnor]

Attachment: Patch, nits addressed

Nits addressed.

Comment 19

[Mano (::mano, needinfo? for any questions; not reading general bugmail)]

Comment on attachment 243755 [details] [diff] [review]
Patch, nits addressed

r=mano

Comment 20

[Mano (::mano, needinfo? for any questions; not reading general bugmail)]

mozilla/toolkit/content/widgets/tabbrowser.xml 1.212