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)
Firefox
Tabbed Browser
Tracking
()
RESOLVED
FIXED
Firefox 3 alpha1
People
(Reporter: moco, Assigned: ventnor.bugzilla)
References
Details
Attachments
(2 files, 5 obsolete files)
536 bytes,
text/html
|
Details | |
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.
Comment 1•19 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•19 years ago
|
||
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•19 years ago
|
||
A test case which will create a new tab then close it.
Reporter | ||
Comment 4•19 years ago
|
||
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•19 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 6•19 years ago
|
||
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•19 years ago
|
||
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 8•19 years ago
|
||
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•19 years ago
|
||
Sorry, I thought I could kill two birds with one stone.
Attachment #242378 -
Attachment is obsolete: true
Attachment #242400 -
Flags: review?(mano)
Assignee | ||
Updated•19 years ago
|
Attachment #242400 -
Flags: review?(mano)
Assignee | ||
Updated•19 years ago
|
Attachment #242400 -
Flags: review?(mano)
Assignee | ||
Comment 10•19 years ago
|
||
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•19 years ago
|
Attachment #242948 -
Flags: review?(mano) → review?(gavin.sharp)
Comment 11•19 years ago
|
||
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•19 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...
Comment 13•19 years ago
|
||
XUL menus are closed when you mousedown anything else in the opening window...
Assignee | ||
Comment 14•19 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
Comment 15•19 years ago
|
||
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•19 years ago
|
||
Addresses Mano's comments.
Attachment #242948 -
Attachment is obsolete: true
Attachment #243563 -
Flags: review?(mano)
Comment 17•19 years ago
|
||
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•19 years ago
|
||
Nits addressed.
Attachment #243563 -
Attachment is obsolete: true
Attachment #243755 -
Flags: review?(mano)
Comment 19•19 years ago
|
||
Comment on attachment 243755 [details] [diff] [review]
Patch, nits addressed
r=mano
Attachment #243755 -
Flags: review?(mano) → review+
Comment 20•19 years ago
|
||
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.
Description
•