Closed Bug 345258 Opened 19 years ago Closed 19 years ago

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

Categories

(Firefox :: Tabbed Browser, defect)

Product:
Firefox
Component:
Tabbed Browser
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 3 alpha1

People

(Reporter: moco, Assigned: ventnor.bugzilla)

References

Details

Attachments

(2 files, 5 obsolete files)

Test case
536 bytes, text/html
Details
Patch, nits addressed
6.57 KB, patch
asaf
: review+
Details | Diff | Splinter Review
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.
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.
Attached patch Add more listeners to the menu (obsolete) — — Splinter Review
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)
Attached file 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.
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-
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)
Sorry, I thought I could kill two birds with one stone.
Attachment #242378 - Attachment is obsolete: true
Attachment #242400 - Flags: review?(mano)
Attachment #242400 - Flags: review?(mano)
Attachment #242400 - Flags: review?(mano)
Attached patch Improve the menu even further (obsolete) — — Splinter Review
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)
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-
> 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...
(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.
Attached patch Patch, review comments addressed (obsolete) — — Splinter Review
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+
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
Closed: 19 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.

Attachment

General

Creator:
Created:
Updated:
Size: