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

RESOLVED FIXED in Firefox 3 alpha1

Status

()

RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: moco, Assigned: ventnor.bugzilla)

Tracking

Trunk
Firefox 3 alpha1
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 5 obsolete attachments)

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

12 years ago
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.
(Assignee)

Comment 2

12 years ago
Created attachment 241009 [details] [diff] [review]
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.
Attachment #241009 - Flags: review?(sspitzer)
(Assignee)

Comment 3

12 years ago
Created attachment 241039 [details]
Test case

A test case which will create a new tab then close it.
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.
(Assignee)

Comment 5

12 years ago
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?
Attachment #241009 - Flags: review?(sspitzer) → review?(mano)
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?
Attachment #241009 - Flags: review?(mano) → review-
(Assignee)

Comment 7

12 years ago
Created attachment 242378 [details] [diff] [review]
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.
Attachment #241009 - Attachment is obsolete: true
Attachment #242378 - Flags: review?(mano)
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.
Attachment #242378 - Flags: review?(mano)
(Assignee)

Comment 9

12 years ago
Created attachment 242400 [details] [diff] [review]
Patch refactoring some of the all tabs menu 2

Sorry, I thought I could kill two birds with one stone.
Attachment #242378 - Attachment is obsolete: true
Attachment #242400 - Flags: review?(mano)
(Assignee)

Updated

12 years ago
Attachment #242400 - Flags: review?(mano)
(Assignee)

Updated

12 years ago
Attachment #242400 - Flags: review?(mano)
(Assignee)

Comment 10

12 years ago
Created attachment 242948 [details] [diff] [review]
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. :)
Attachment #242400 - Attachment is obsolete: true
Attachment #242948 - Flags: review?(mano)
Attachment #242400 - Flags: review?(mano)
(Assignee)

Updated

12 years ago
Attachment #242948 - Flags: review?(mano) → review?(gavin.sharp)
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?
Attachment #242948 - Flags: review?(gavin.sharp) → review-

Comment 12

12 years ago
> 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...
XUL menus are closed when you mousedown anything else in the opening window...
(Assignee)

Comment 14

12 years ago
(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.
Assignee: sspitzer → ventnor.bugzilla
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.
(Assignee)

Comment 16

12 years ago
Created attachment 243563 [details] [diff] [review]
Patch, review comments addressed

Addresses Mano's comments.
Attachment #242948 - Attachment is obsolete: true
Attachment #243563 - Flags: review?(mano)
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.
Attachment #243563 - Flags: review?(mano) → review+
(Assignee)

Comment 18

12 years ago
Created attachment 243755 [details] [diff] [review]
Patch, nits addressed

Nits addressed.
Attachment #243563 - Attachment is obsolete: true
Attachment #243755 - Flags: review?(mano)
Comment on attachment 243755 [details] [diff] [review]
Patch, nits addressed

r=mano
Attachment #243755 - Flags: review?(mano) → review+
mozilla/toolkit/content/widgets/tabbrowser.xml 1.212
Status: NEW → RESOLVED
Last Resolved: 12 years ago
OS: Windows XP → All
Hardware: PC → All
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3 alpha1
You need to log in before you can comment on or make changes to this bug.