Bug 419544 - "places menupopups options ("open all in tabs", "open <feed website>") do not update correctly" [p=mak77@supereva.it (Marco Bonardo [mak77]) r=Mano a=blocking-firefox3+]
--- a/browser/base/content/browser-places.js
+++ b/browser/base/content/browser-places.js
@@ -632,79 +632,96 @@ var BookmarksEventHandler = {
* Handler for popupshowing event for an item in bookmarks toolbar or menu.
* If the item isn't the main bookmarks menu, add an "Open All in Tabs"
* menuitem to the bottom of the popup.
* @param event
* DOMEvent for popupshowing
*/
onPopupShowing: function BM_onPopupShowing(event) {
var target = event.originalTarget;
- if (target.localName == "menupopup" &&
- target.id != "bookmarksMenuPopup" &&
- target.getAttribute("anonid") != "chevronPopup") {
- // Add the "Open All in Tabs" menuitem if there are
- // at least two menuitems with places result nodes.
- // Add the "Open (Feed Name)" menuitem if it's a livemark with a siteURI.
- var numNodes = 0;
- var hasMultipleEntries = false;
- var currentChild = target.firstChild;
- while (currentChild) {
- if (currentChild.localName == "menuitem" && currentChild.node)
- numNodes++;
-
- // If the menuitem already exists, do nothing.
- if (currentChild.getAttribute("openInTabs") == "true")
- return;
- if (currentChild.hasAttribute("siteURI"))
- return;
-
- currentChild = currentChild.nextSibling;
- }
- if (numNodes > 1)
- hasMultipleEntries = true;
+ if (!target.hasAttribute("placespopup"))
+ return;
- var itemId = target._resultNode.itemId;
- var siteURIString = "";
- if (itemId != -1 && PlacesUtils.livemarks.isLivemark(itemId)) {
- var siteURI = PlacesUtils.livemarks.getSiteURI(itemId);
- if (siteURI)
- siteURIString = siteURI.spec;
- }
-
- if (hasMultipleEntries || siteURIString) {
- var separator = document.createElement("menuseparator");
- target.appendChild(separator);
-
- if (siteURIString) {
- var openHomePage = document.createElement("menuitem");
- openHomePage.setAttribute("siteURI", siteURIString);
- openHomePage.setAttribute("oncommand",
- "openUILink(this.getAttribute('siteURI'), event);");
- // If a user middle-clicks this item we serve the oncommand event
- // We are using checkForMiddleClick because of Bug 246720
- // Note: stopPropagation is needed to avoid serving middle-click
- // with BT_onClick that would open all items in tabs
- openHomePage.setAttribute("onclick",
- "checkForMiddleClick(this, event); event.stopPropagation();");
- openHomePage.setAttribute("label",
- PlacesUtils.getFormattedString("menuOpenLivemarkOrigin.label",
- [target.parentNode.getAttribute("label")]));
- target.appendChild(openHomePage);
- }
-
- if (hasMultipleEntries) {
- var openInTabs = document.createElement("menuitem");
- openInTabs.setAttribute("openInTabs", "true");
- openInTabs.setAttribute("oncommand",
- "PlacesUtils.openContainerNodeInTabs(this.parentNode._resultNode, event);");
- openInTabs.setAttribute("label",
- gNavigatorBundle.getString("menuOpenAllInTabs.label"));
- target.appendChild(openInTabs);
+ // Check if the popup contains at least 2 menuitems with places nodes
+ var numNodes = 0;
+ var hasMultipleURIs = false;
+ var currentChild = target.firstChild;
+ while (currentChild) {
+ if (currentChild.localName == "menuitem" && currentChild.node) {
+ if (++numNodes == 2) {
+ hasMultipleURIs = true;
+ break;
}
}
+ currentChild = currentChild.nextSibling;
+ }
+
+ var itemId = target._resultNode.itemId;
+ var siteURIString = "";
+ if (itemId != -1 && PlacesUtils.livemarks.isLivemark(itemId)) {
+ var siteURI = PlacesUtils.livemarks.getSiteURI(itemId);
+ if (siteURI)
+ siteURIString = siteURI.spec;
+ }
+
+ if (!siteURIString && target._endOptOpenSiteURI) {
+ target.removeChild(target._endOptOpenSiteURI);
+ target._endOptOpenSiteURI = null;
+ }
+
+ if (!hasMultipleURIs && target._endOptOpenAllInTabs) {
+ target.removeChild(target._endOptOpenAllInTabs);
+ target._endOptOpenAllInTabs = null;
+ }
+
+ if (!(hasMultipleURIs || siteURIString)) {
+ // we don't have to show any option
+ if (target._endOptSeparator) {
+ target.removeChild(target._endOptSeparator);
+ target._endOptSeparator = null;
+ target._endMarker = -1;
+ }
+ return;
+ }
+
+ if (!target._endOptSeparator) {
+ // create a separator before options
+ target._endOptSeparator = document.createElement("menuseparator");
+ target._endOptSeparator.setAttribute("builder", "end");
+ target._endMarker = target.childNodes.length;
+ target.appendChild(target._endOptSeparator);
+ }
+
+ if (siteURIString && !target._endOptOpenSiteURI) {
+ // Add "Open (Feed Name)" menuitem if it's a livemark with a siteURI
+ target._endOptOpenSiteURI = document.createElement("menuitem");
+ target._endOptOpenSiteURI.setAttribute("siteURI", siteURIString);
+ target._endOptOpenSiteURI.setAttribute("oncommand",
+ "openUILink(this.getAttribute('siteURI'), event);");
+ // If a user middle-clicks this item we serve the oncommand event
+ // We are using checkForMiddleClick because of Bug 246720
+ // Note: stopPropagation is needed to avoid serving middle-click
+ // with BT_onClick that would open all items in tabs
+ target._endOptOpenSiteURI.setAttribute("onclick",
+ "checkForMiddleClick(this, event); event.stopPropagation();");
+ target._endOptOpenSiteURI.setAttribute("label",
+ PlacesUtils.getFormattedString("menuOpenLivemarkOrigin.label",
+ [target.parentNode.getAttribute("label")]));
+ target.appendChild(target._endOptOpenSiteURI);
+ }
+
+ if (hasMultipleURIs && !target._endOptOpenAllInTabs) {
+ // Add the "Open All in Tabs" menuitem if there are
+ // at least two menuitems with places result nodes.
+ target._endOptOpenAllInTabs = document.createElement("menuitem");
+ target._endOptOpenAllInTabs.setAttribute("oncommand",
+ "PlacesUtils.openContainerNodeInTabs(this.parentNode._resultNode, event);");
+ target._endOptOpenAllInTabs.setAttribute("label",
+ gNavigatorBundle.getString("menuOpenAllInTabs.label"));
+ target.appendChild(target._endOptOpenAllInTabs);
}
},
fillInBTTooltip: function(aTipElement) {
// Fx2XP: Don't show tooltips for bookmarks under sub-folders
if (aTipElement.localName != "toolbarbutton")
return false;
--- a/browser/components/places/content/menu.xml
+++ b/browser/components/places/content/menu.xml
@@ -73,16 +73,18 @@
<!-- markers for start and end of valid places items -->
<field name="_startMarker">-1</field>
<field name="_endMarker">-1</field>
<!-- This is the view that manage the popup -->
<field name="_rootView">PlacesUtils.getViewForNode(this);</field>
+ <field name="_built">false</field>
+
<method name="onDragOver">
<parameter name="aEvent"/>
<parameter name="aFlavour"/>
<parameter name="aDragSession"/>
<body><![CDATA[
PlacesControllerDragHelper.currentDropTarget = aEvent.target;
// check if we have a valid dropPoint
var dropPoint = this._getDropPoint(aEvent);
@@ -226,24 +228,24 @@
<parameter name="aEvent"/>
<body><![CDATA[
var target = aEvent.target;
// in some view we have _startMarker and _endMarker, we should not
// draw the drop indicator outside of them
var betweenMarkers = true;
if (this._startMarker != -1 &&
- target.boxObject.y < this.childNodes[this._startMarker].boxObject.y)
+ target.boxObject.y <= this.childNodes[this._startMarker].boxObject.y)
betweenMarkers = false;
if (this._endMarker != -1 &&
- target.boxObject.y > this.childNodes[this._endMarker].boxObject.y)
+ target.boxObject.y >= this.childNodes[this._endMarker].boxObject.y)
betweenMarkers = false;
// hide the dropmarker if current node is not a places bookmark item
- return !(target && betweenMarkers && this.canDrop());
+ return !(target && target.node && betweenMarkers && this.canDrop());
]]></body>
</method>
<!-- This function returns information about where to drop when
dragging over this popup insertion point -->
<method name="_getDropPoint">
<parameter name="aEvent"/>
<body><![CDATA[
@@ -489,24 +491,23 @@
this._initialized = true;
]]></body>
</method>
<property name="controller"
readonly="true"
onget="return this._controller;"/>
- <field name="_built">false</field>
-
<method name="onPopupShowing">
<parameter name="aEvent"/>
<body><![CDATA[
var popup = aEvent.target;
var resultNode = popup._resultNode;
- resultNode.containerOpen = true;
+ if (!resultNode.containerOpen)
+ resultNode.containerOpen = true;
if (!popup._built)
this._rebuild(popup);
]]></body>
</method>
<!-- nsIPlacesView -->
<method name="getResult">
<body><![CDATA[
@@ -517,68 +518,16 @@
<!-- nsIPlacesView -->
<method name="getResultNode">
<body><![CDATA[
this._ensureInitialized();
return this._resultNode;
]]></body>
</method>
- <method name="_cleanMenu">
- <parameter name="aPopup"/>
- <body><![CDATA[
- // Find static menuitems that should go at the start
- // and end of the menu, marked by builder="start" and
- // builder="end" attributes, and keep track of their indices.
- // All of the items between the start and end should be removed.
- var items = [];
- aPopup._startMarker = -1;
- aPopup._endMarker = -1;
- for (var i = 0; i < aPopup.childNodes.length; ++i) {
- var item = aPopup.childNodes[i];
- if (item.getAttribute("builder") == "start") {
- aPopup._startMarker = i;
- continue;
- }
- if (item.getAttribute("builder") == "end") {
- aPopup._endMarker = i;
- continue;
- }
- if ((aPopup._startMarker != -1) && (aPopup._endMarker == -1))
- items.push(item);
- }
-
- // If static items at the beginning were found, remove all items between
- // them and the static content at the end.
- for (var i = 0; i < items.length; ++i) {
- // skip the empty menu item
- if (aPopup._emptyMenuItem != items[i]) {
- aPopup.removeChild(items[i]);
- if (this._endMarker > 0)
- --this._endMarker;
- }
- }
-
- // If no static items were found at the beginning, remove all items before
- // the static items at the end.
- if (aPopup._startMarker == -1) {
- var end = aPopup._endMarker == -1 ?
- aPopup.childNodes.length - 1 : aPopup._endMarker - 1;
- for (var i = end; i >=0; i--) {
- // skip the empty menu item
- if (aPopup._emptyMenuItem != aPopup.childNodes[i]) {
- aPopup.removeChild(aPopup.childNodes[i]);
- if (aPopup._endMarker > 0)
- --aPopup._endMarker;
- }
- }
- }
- ]]></body>
- </method>
-
<method name="removeItem">
<parameter name="child"/>
<body><![CDATA[
if (PlacesUtils.nodeIsContainer(child.node)) {
for (var i=0; i < this._containerNodesMap.length; i++) {
if (this._containerNodesMap[i].resultNode == child.node) {
this._containerNodesMap.splice(i, 1);
break;
@@ -632,17 +581,17 @@
aPopup._emptyMenuItem.setAttribute("disabled", true);
aPopup.appendChild(aPopup._emptyMenuItem);
]]></body>
</method>
<method name="_rebuild">
<parameter name="aPopup"/>
<body><![CDATA[
- this._cleanMenu(aPopup);
+ PlacesUtils.cleanPlacesPopup(aPopup);
var cc = aPopup._resultNode.childCount;
if (cc > 0) {
if (aPopup._emptyMenuItem)
aPopup._emptyMenuItem.hidden = true;
for (var i = 0; i < cc; ++i) {
var child = aPopup._resultNode.getChild(i);
--- a/browser/components/places/content/toolbar.xml
+++ b/browser/components/places/content/toolbar.xml
@@ -978,53 +978,60 @@
<method name="insertNewItemToPopup">
<parameter name="aChild"/>
<parameter name="aParentPopup"/>
<parameter name="aBefore"/>
<body><![CDATA[
var element =
PlacesUtils.createMenuItemForNode(aChild, this._containerNodesMap);
+
if (aBefore)
aParentPopup.insertBefore(element, aBefore);
- else
- aParentPopup.appendChild(element);
+ else {
+ // Add the new element to the menu. If there is static content at
+ // the end of the menu, add the element before that. Otherwise,
+ // just add to the end.
+ if (aParentPopup._endMarker != -1) {
+ aParentPopup.insertBefore(element,
+ aParentPopup.childNodes[aParentPopup._endMarker++]);
+ }
+ else
+ aParentPopup.appendChild(element);
+ }
]]></body>
</method>
<method name="_containerPopupShowing">
<parameter name="aPopup"/>
<body><![CDATA[
if (aPopup._built)
return;
- // remove previous menu items
- while (aPopup.hasChildNodes())
- aPopup.removeChild(aPopup.firstChild);
- // restore the empty-menu item if has been created already
- if (aPopup._emptyMenuItem)
- aPopup.appendChild(aPopup._emptyMenuItem);
+ PlacesUtils.cleanPlacesPopup(aPopup);
var resultNode = aPopup._resultNode;
if (!resultNode.containerOpen)
resultNode.containerOpen = true;
var cc = resultNode.childCount;
if (cc > 0) {
if (aPopup._emptyMenuItem)
aPopup._emptyMenuItem.hidden = true;
for (var i = 0; i < cc; ++i) {
var child = resultNode.getChild(i);
this.insertNewItemToPopup(child, aPopup, null);
}
}
else {
- // add element to show it is empty.
- this._showEmptyMenuItem(aPopup);
+ // This menu is empty. If there is no static content, add
+ // an element to show it is empty.
+ if (aPopup._startMarker == -1 && aPopup._endMarker == -1)
+ this._showEmptyMenuItem(aPopup);
}
aPopup._built = true;
]]></body>
</method>
<method name="_isChevronChild">
<parameter name="aChild"/>
<body><![CDATA[
--- a/browser/components/places/content/utils.js
+++ b/browser/components/places/content/utils.js
@@ -1891,16 +1891,64 @@ var PlacesUtils = {
element.setAttribute("image", iconURISpec);
}
element.node = aNode;
element.node.viewIndex = 0;
return element;
},
+ cleanPlacesPopup: function PU_cleanPlacesPopup(aPopup) {
+ // Find static menuitems at the start and at the end of the menupopup,
+ // marked by builder="start" and builder="end" attributes, and set
+ // markers to keep track of their indices.
+ var items = [];
+ aPopup._startMarker = -1;
+ aPopup._endMarker = -1;
+ for (var i = 0; i < aPopup.childNodes.length; ++i) {
+ var item = aPopup.childNodes[i];
+ if (item.getAttribute("builder") == "start") {
+ aPopup._startMarker = i;
+ continue;
+ }
+ if (item.getAttribute("builder") == "end") {
+ aPopup._endMarker = i;
+ continue;
+ }
+ if ((aPopup._startMarker != -1) && (aPopup._endMarker == -1))
+ items.push(item);
+ }
+
+ // If static items at the beginning were found, remove all items between
+ // them and the static content at the end.
+ for (var i = 0; i < items.length; ++i) {
+ // skip the empty menu item
+ if (aPopup._emptyMenuItem != items[i]) {
+ aPopup.removeChild(items[i]);
+ if (this._endMarker > 0)
+ --this._endMarker;
+ }
+ }
+
+ // If no static items were found at the beginning, remove all items before
+ // the static items at the end.
+ if (aPopup._startMarker == -1) {
+ var end = aPopup._endMarker == -1 ?
+ aPopup.childNodes.length - 1 : aPopup._endMarker - 1;
+ for (var i = end; i >= 0; i--) {
+ // skip the empty menu item
+ if (aPopup._emptyMenuItem != aPopup.childNodes[i]) {
+ aPopup.removeChild(aPopup.childNodes[i]);
+ if (aPopup._endMarker > 0)
+ --aPopup._endMarker;
+ }
+ }
+ }
+ },
+
getBestTitle: function PU_getBestTitle(aNode) {
var title;
if (!aNode.title && this.uriTypes.indexOf(aNode.type) != -1) {
// if node title is empty, try to set the label using host and filename
// this._uri() will throw if aNode.uri is not a valid URI
try {
var uri = this._uri(aNode.uri);
var host = uri.host;