Steps To Reproduce: 1. Open Bookmarks menu 2. Right click on bookmark item of "last 5 Recently Bookmarked" Actual Results: Open it in tab And Context menu has only 5 menu items Expectedd Results: Should not open it in tab. Regular placescontext should pop up. i.e. Context menu should be 12 items

As for the "Context menu should be 12 items" - I already also poked an another related bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1248268 It seems that there is a mix of bookmarks related issues in the latest Nightly.

probably for now right click should do nothing. Implementing the whole Places context menu is a complex task.

Can you simply make it not open the link? It is poor user experience to open links when the user didn't intend to do so.

This is the current context menu in the "recently bookmarked" section. Those are the options Firefox normally shows for folders and NOT for links which makes this a bit confusing for users. It would make sense to disable the context menu entirely in this section until a more appropriate one is added.

yes, the contextual menu on places menus static content refers to the root folder... It's confusing and we already evaluated removing contextual menus on static content many years ago (bug 429469)

I don't understand the title of this bug. I think this is a reference to when right clicking used to open the link. My bug 1261660 was about adding a proper context menu. Are you sure it is a duplicate?

(In reply to Hussam Al-Tayeb from comment #8) > I don't understand the title of this bug. I think this is a reference to > when right clicking used to open the link. > My bug 1261660 was about adding a proper context menu. Are you sure it is a > duplicate? See the comment 0, although it is vague, but it means the type of context menu.

(In reply to Marco Bonardo [::mak] from comment #6) > yes, the contextual menu on places menus static content refers to the root > folder... It's confusing and we already evaluated removing contextual menus > on static content many years ago (bug 429469) So when I set `item._placesNode = node` in `BookmarkingUI#_updateRecentBookmarks()`, I get half-way there. Of course, the drag-n-drop controller starts barfing because it can't find a parent node, yadayada, but is this remotely a direction worth pursuing nonetheless? As a stop-gap measure, I'd like to disable all pointer interactions for these five items (including drag-n-drop), except left click. After that I'd like to learn how I can implement an interaction model properly. Marco, can you give me a pointer where I should be looking?

Hi Mike, please check https://bugzilla.mozilla.org/show_bug.cgi?id=1248268#c48 I don't think we'll ever be able to have a complete places context menu here, since it would be hackish (lots of "simulated" nodes). Since these entries support a much smaller subset of actions than common bookmarks (atm), I'd rather suggest to remove the existing places contextual menu on anything that doesn't have a _placesNode attached and build a new contextual menu for these items. I think you should coordinate with Dao about that.

Regarding drag & drop, places also supports unicode drag flavors that IIRC are something like "url\ntitle" so you may be able to create a datatransfer accepted by places views "easily".

(In reply to Mike de Boer [:mikedeboer] from comment #10) > As a stop-gap measure, I'd like to disable all pointer interactions for > these five items (including drag-n-drop), except left click. The patch in bug 1248267 basically does that. It makes "Hide Recently Bookmarked" the only context menu entry for these items.

Dão, I tried to do it this way around by not disabling context menus after all. This can be extended to bug 1248616 for DnD. It looks like the controller disables/ hides the correct items for us this way. The setTimeout() needs to be introduced for the bookmarks menu as a subview (try it!)

Comment on attachment 8745350 [details] [diff] [review] Patch v1: allow Recently Bookmarked items to be special root nodes to allow primitive contextual actions Linux (and prolly Windows too) support context menus on items inside the main menu too. So this is not a proper fix.

Comment on attachment 8745403 [details] [diff] [review] Patch v2: allow Recently Bookmarked items to be special root nodes to allow primitive contextual actions >--- a/browser/base/content/browser-places.js >+++ b/browser/base/content/browser-places.js >@@ -1359,28 +1359,27 @@ var BookmarkingUI = { > footer: "panel-subview-footer" > }, > insertionPoint: ".panel-subview-footer" > }); > }, > > _updateRecentBookmarks: function(aHeaderItem, extraCSSClass = "") { > const kMaxResults = 5; >+ let parent = aHeaderItem.parentNode; > > let options = PlacesUtils.history.getNewQueryOptions(); > options.excludeQueries = true; > options.queryType = options.QUERY_TYPE_BOOKMARKS; > options.sortingMode = options.SORT_BY_DATEADDED_DESCENDING; > options.maxResults = kMaxResults; > let query = PlacesUtils.history.getNewQuery(); > >- while (aHeaderItem.nextSibling && >- aHeaderItem.nextSibling.localName == "menuitem") { >- aHeaderItem.nextSibling.remove(); >- } >+ for (let item of Array.from(parent.querySelectorAll("menuitem[special-recents]"))) >+ item.remove(); > > let onItemCommand = function (aEvent) { > let item = aEvent.target; > openUILink(item.getAttribute("targetURI"), aEvent); > CustomizableUI.hidePanelForNode(item); > }; > > let fragment = document.createDocumentFragment(); >@@ -1392,26 +1391,29 @@ var BookmarkingUI = { > let title = node.title; > let icon = node.icon; > > let item = > document.createElementNS("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul", > "menuitem"); > item.setAttribute("label", title || uri); > item.setAttribute("targetURI", uri); >+ item.setAttribute("special-recents", true); Those changes all seem unrelated? Marco will want to review the PlacesUIUtils.jsm, browserPlacesViews.js and controller.js changes, I don't have a very informed opinion there.

(In reply to Dão Gottwald [:dao] from comment #17) > Those changes all seem unrelated? They are! But not obviously so :-) What I mean is that these changes are needed once I did `item._placesNode = node;`, because that opens up a couple of code paths that weren't trodden before.

(In reply to Mike de Boer [:mikedeboer] from comment #18) > (In reply to Dão Gottwald [:dao] from comment #17) > > Those changes all seem unrelated? > > They are! But not obviously so :-) What I mean is that these changes are > needed once I did `item._placesNode = node;`, because that opens up a couple > of code paths that weren't trodden before. I don't understand. What part of what code path makes those changes necessary?

Also, if you could do this on top of the patch in bug 1248268 such that I don't have to rebase my older and larger patch, that would be appreciated.

(In reply to Dão Gottwald [:dao] from comment #20) > Also, if you could do this on top of the patch in bug 1248268 such that I > don't have to rebase my older and larger patch, that would be appreciated. Of course. It's not a race. I was merely quite enthusiastic due to having the idea that I'm starting to grok the Places observer and controller code to the extent that I can make changes comfortably (whilst understanding what it's doing, that is).

Comment on attachment 8745403 [details] [diff] [review] Patch v2: allow Recently Bookmarked items to be special root nodes to allow primitive contextual actions Review of attachment 8745403 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Dão Gottwald [:dao] from comment #19) ::: browser/base/content/browser-places.js @@ +1373,5 @@ > options.maxResults = kMaxResults; > let query = PlacesUtils.history.getNewQuery(); > > + for (let item of Array.from(parent.querySelectorAll("menuitem[special-recents]"))) > + item.remove(); Making the removal of the 'old' items dependent on selecting them explicitly through the use of a unique property is handy, because we needn't rely on a menuseparator to be present in various XUL files. This means we can keep the control flow in this single file. @@ +1396,5 @@ > "menuitem"); > item.setAttribute("label", title || uri); > item.setAttribute("targetURI", uri); > + item.setAttribute("special-recents", true); > + item.setAttribute("outside-container", true); `special-recents` means that these items are part of a special 'Recently Bookmarked' group. Useful for the items-selector above. `outside-container` means that these items are not part of a (live) places container, so the menu view may know to leave them alone. See PlacesViewBase#_cleanPopup() in this patch. @@ +1403,5 @@ > item.addEventListener("command", onItemCommand); > if (icon) { > item.setAttribute("image", icon); > } > + item._placesNode = node; This makes the item 'visible' to the Places controller. In other words: linking the item to the places db node allows for a context menu to be present and for other input control methods, like drag 'n drop. Normally this would be done by the specific view, but since we're generating the items here custom, we need to take this step as well. ::: browser/components/places/PlacesUIUtils.jsm @@ +614,5 @@ > let parentNode = aNode.parent; > + if (!parentNode) { > + Cu.reportError(new Error("canUserRemove doesn't accept root nodes")); > + return false; > + } When the controller selects which commands to disable for the context menu, it'll be rough and throw an error for root nodes here. Since our recently bookmarked items and respective places nodes are outside their container, they have in fact become root nodes of sorts. But we do want a context menu. Hence I make this part friendlier whilst still disabling the command for root nodes. ::: browser/components/places/content/browserPlacesViews.js @@ +206,5 @@ > // In all other cases the insertion point is before that node. > container = selectedNode.parent; > + // ...except when the node is placed outside its root! > + if (!container) > + return null; Since our recently bookmarked items and their respective places nodes are placed outside their container(s), there is no container. (Enter Jedi hand wave gesture.) Therefore bailing out here is the right thing to do in this case. @@ +239,5 @@ > // Remove Places nodes from the popup. > let child = aPopup._startMarker; > while (child.nextSibling != aPopup._endMarker) { > let sibling = child.nextSibling; > + let isPlacesNode = (sibling._placesNode && !sibling.hasAttribute("outside-container")); This code extends the `._placesNode` property detection with the `outside-container` property, because we want to be in full control of the recently bookmarked items and not have the view remove the nodes from the menu! @@ +676,5 @@ > containerStateChanged: > function PVB_containerStateChanged(aPlacesNode, aOldState, aNewState) { > + // We may safely ignore root nodes here. > + if (!aPlacesNode.parent) > + return; This is all about live bookmarks, so container-less nodes don't have any value here. In fact, if we let this through, `this.invalidateContainer(...)` will throw an error. ::: browser/components/places/content/controller.js @@ -455,5 @@ > nodeData["link"] = true; > uri = NetUtil.newURI(node.uri); > if (PlacesUtils.nodeIsBookmark(node)) { > nodeData["bookmark"] = true; > - PlacesUtils.nodeIsTagQuery(node.parent) Someone forgot to remove this statement during the last refactor here. Since our nodes are parent-less, this obviously throws. Removing this line does _not_ have side-effects, because it's also invoked three lines down (line 463 here).

(In reply to Mike de Boer [:mikedeboer] from comment #22) > (In reply to Dão Gottwald [:dao] from comment #19) > > ::: browser/base/content/browser-places.js > @@ +1373,5 @@ > > options.maxResults = kMaxResults; > > let query = PlacesUtils.history.getNewQuery(); > > > > + for (let item of Array.from(parent.querySelectorAll("menuitem[special-recents]"))) > > + item.remove(); > > Making the removal of the 'old' items dependent on selecting them explicitly > through the use of a unique property is handy, because we needn't rely on a > menuseparator to be present in various XUL files. > This means we can keep the control flow in this single file. The menuseparator is there to stay, so this sounds like a solution looking for a problem. Anyway, this change is just completely unrelated to this bug.

(In reply to Dão Gottwald [:dao] from comment #23) > The menuseparator is there to stay, so this sounds like a solution looking > for a problem. Anyway, this change is just completely unrelated to this bug. Nope, it's not. In fact, the other places view code messes up and draws duplicate items for us if we don't take of this. In other words: remove this code and the patch won't work. :(

Then I still don't get how exactly this change helps places view code. Does the loop without querySelector remove items it shouldn't remove, or leave back items it should remove? What's going on?

Comment on attachment 8745403 [details] [diff] [review] Patch v2: allow Recently Bookmarked items to be special root nodes to allow primitive contextual actions Review of attachment 8745403 [details] [diff] [review]: ----------------------------------------------------------------- Please rebase on top of bug 1248268 and let's see where we are.

I left the 'hideRecentlyBookmarked' in there, because it's still handy for the header text context menu.

(In reply to Dão Gottwald [:dao] from comment #25) > Then I still don't get how exactly this change helps places view code. Does > the loop without querySelector remove items it shouldn't remove, or leave > back items it should remove? What's going on? Can you please explain this?

(In reply to Dão Gottwald [:dao] from comment #25) > Then I still don't get how exactly this change helps places view code. Does > the loop without querySelector remove items it shouldn't remove, or leave > back items it should remove? What's going on? I'm afraid I don't have to 100% grasp on the situation, but here's what I've found out so far: As we're now setting `._placesNode` on the menuitem, they are now considered proper bookmark nodes for `PlacesViewBase` in browserPlacesView.js. This means that `_rebuildPopup` is called for our recent bookmark items as well, which invokes `_cleanPopup`, which is where we added the guarding code for items with an 'outside-container' attribute set. But _rebuildPopup does lots more and I believe `_insertNewItemToPopup` is what ultimately causes the situation in the screenshot: it shows the state of the 'Recently Bookmarked' items when you open the popup for the _second_ time - it's the same for the main menu on any platform; complete duplicate items. Thus the insertion point can't be deduced from the places node correctly anymore, since they live outside their respective containers.

Are you sure the while loop ran at all? Have you accidentally removed or otherwise broken it? Because it looks like all old items remained and then the new ones were added on top of them.

(In reply to Dão Gottwald [:dao] from comment #30) > Are you sure the while loop ran at all? Have you accidentally removed or > otherwise broken it? Because it looks like all old items remained and then > the new ones were added on top of them. I'm _very_ sure. And the small patch is all the code I have applied.

Comment on attachment 8747045 [details] [diff] [review] Patch v3: allow Recently Bookmarked items to be special root nodes to allow primitive contextual actions Review of attachment 8747045 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for late, my priorities are in a good shape, so I'm now looking into this, and will follow you until this is merged. I think your problem with the menu rebuilding is that by adding _placesNode you confused the code that is inserting the _startMarker and _endMarker in the menu. To better explain this, each places menu has a _startMarker and _endMarker special menuseparators that are hidden and indicate the beginning and the end of the dinamically generated content in the menu. These are used to properly draw drop indicators. the _startMarker is found by walking the menu until the first node with a _placesNode is found. And this is where your patch fails. See _ensureMarkers at http://mxr.mozilla.org/mozilla-central/source/browser/components/places/content/browserPlacesViews.js#864 and in particular 897 if (child._placesNode && !firstNonStaticNodeFound) { I think you want to add an hasAttribute check here and skip your fake nodes. The attribute should be more generically named than special-recents, so we can reuse it, something like "static-places-node" or "simulated-places-node" This should help reducing the reach of your patch.

(In reply to Marco Bonardo [::mak] from comment #32) > Sorry for late, my priorities are in a good shape, so I'm now looking into > this, and will follow you until this is merged. I didn't expect any different from you! I know your queue is long nowadays, so I calculated the waiting time in... here's to hoping for better days. > See _ensureMarkers at > http://mxr.mozilla.org/mozilla-central/source/browser/components/places/ > content/browserPlacesViews.js#864 > and in particular > 897 if (child._placesNode && !firstNonStaticNodeFound) { Awesome, thanks! I think this info will help finish things here. I was on PTO yesterday and not feeling well today, but next week for sure.

Comment on attachment 8750265 [details] [diff] [review] Patch v4: allow Recently Bookmarked items to be special root nodes to allow primitive contextual actions Review of attachment 8750265 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/places/PlacesUIUtils.jsm @@ +611,5 @@ > * @return true if the aNode represents a removable entry, false otherwise. > */ > canUserRemove: function (aNode) { > let parentNode = aNode.parent; > + if (!parentNode) { could you please tell me the stack trace reaching this point? Is it just used by commands updating? If so, probably we should not reportError, or it would happen at every contextual menu opening. ::: browser/components/places/content/browserPlacesViews.js @@ +206,5 @@ > // In all other cases the insertion point is before that node. > container = selectedNode.parent; > + // ...except when the node is placed outside its root! > + if (!container) > + return null; to maintain the current behavior, we should instead end up in the previews branch 198 if (!popup._placesNode || popup._placesNode == this._resultNode || 199 popup._placesNode.itemId == -1) { 200 // If a static menuitem is selected, or if the root node is selected, 201 // the insertion point is inside the folder, at the end. 202 container = selectedNode; 203 orientation = Ci.nsITreeView.DROP_ON; Ideally these nodes should not have a _placesNode, thus we would enter this branch. you should add a check for the attribute here and take this path rather than the else. @@ +239,5 @@ > // Remove Places nodes from the popup. > let child = aPopup._startMarker; > while (child.nextSibling != aPopup._endMarker) { > let sibling = child.nextSibling; > + let isPlacesNode = (sibling._placesNode && !sibling.hasAttribute("simulated-places-node")); Looks like these changes should not be needed anymore, now that we fixed _startMarker. This code starts cleaning from the startMarker so it should not even touch our nodes. @@ +676,5 @@ > containerStateChanged: > function PVB_containerStateChanged(aPlacesNode, aOldState, aNewState) { > + // We may safely ignore root nodes here. > + if (!aPlacesNode.parent) > + return; Could you please tell me the stack trace reaching this point?

(In reply to Marco Bonardo [::mak] from comment #35) > ::: browser/components/places/PlacesUIUtils.jsm > @@ +611,5 @@ > > * @return true if the aNode represents a removable entry, false otherwise. > > */ > > canUserRemove: function (aNode) { > > let parentNode = aNode.parent; > > + if (!parentNode) { > > could you please tell me the stack trace reaching this point? Is it just > used by commands updating? > If so, probably we should not reportError, or it would happen at every > contextual menu opening. TypeError: aNode is null PU_nodeIsQuery()PlacesUtils.jsm:333 this.PlacesUIUtils.canUserRemove()PlacesUIUtils.jsm:630 _hasRemovableSelection()controller.js:336 PC_isCommandEnabled()controller.js:162 updatePlacesCommand()controller.js:1683 goUpdatePlacesCommands()controller.js:1694 oncommandupdate()browser.xul:1 PVB_buildContextMenu()browserPlacesViews.js:230 onpopupshowing()browser.xul:1 ... I've removed the Cu.reportError and added a comment there instead. > > ::: browser/components/places/content/browserPlacesViews.js > @@ +206,5 @@ > > // In all other cases the insertion point is before that node. > > container = selectedNode.parent; > > + // ...except when the node is placed outside its root! > > + if (!container) > > + return null; So true! I changed it. > @@ +676,5 @@ > > containerStateChanged: > > function PVB_containerStateChanged(aPlacesNode, aOldState, aNewState) { > > + // We may safely ignore root nodes here. > > + if (!aPlacesNode.parent) > > + return; > > Could you please tell me the stack trace reaching this point? I'm not hitting this anymore with the changes applied that you requested, so it works just fine now.

Comment on attachment 8750727 [details] [diff] [review] Patch v5: allow Recently Bookmarked items to be special root nodes to allow primitive contextual actions Review of attachment 8750727 [details] [diff] [review]: ----------------------------------------------------------------- I'm forwarding a feedback to dao, cause I think we don't need anymore 2 separate context menus and I hope we could remove the "reduced" one now Since he added it, maybe I'm missing some context. ::: browser/base/content/browser-places.js @@ +1483,5 @@ > fragment.appendChild(item); > } > root.containerOpen = false; > aHeaderItem.parentNode.insertBefore(fragment, aHeaderItem.nextSibling); > aHeaderItem.setAttribute("context", "hideRecentlyBookmarked"); this thing still confuses me, the contextual menu on "Recently Bookmarked" header is exotic and different from anything else. I think it should look the same as the menu on any other static item. Shouldn't we remove the "hideRecentlyBookmarked" menupopup and stop setting it as a context attribute here? I'm not sure what you decided in this regard, but it looks an over-complication we don't need anymore... ::: browser/components/places/PlacesUIUtils.jsm @@ +612,5 @@ > */ > canUserRemove: function (aNode) { > let parentNode = aNode.parent; > + if (!parentNode) { > + // canUserRemove doesn't accept root nodes. "root or simulated nodes"

Comment on attachment 8750727 [details] [diff] [review] Patch v5: allow Recently Bookmarked items to be special root nodes to allow primitive contextual actions (In reply to Marco Bonardo [::mak] from comment #38) > I'm forwarding a feedback to dao, cause I think we don't need anymore 2 > separate context menus and I hope we could remove the "reduced" one now > Since he added it, maybe I'm missing some context. It depends on whether it's feasible to use the places context menu for the "Recently Bookmarked" header menu item. If so, I'd prefer getting rid of the custom context menu.

I think it will just work as it works on Show all bookmarks or any other menuitem there.

With custom menupopup removal. Carrying over r=mak.

https://hg.mozilla.org/integration/fx-team/rev/ec7fe414e5892563cd7343836431d662222a4068 Bug 1248267 - allow Recently Bookmarked items to be special root nodes to allow primitive contextual actions. r=mak,dao https://hg.mozilla.org/integration/fx-team/rev/e8112fba80d64c7c6615c9019178e68f60b5d718 Bug 1248267 - fix unit test to deal with recent bookmarks static items. r=dao

(In reply to Mike de Boer [:mikedeboer] from comment #44) > https://treeherder.mozilla.org/#/jobs?repo=try&revision=e0b7b2c4ef71 Well, Windows seems to be busted in its own spectacular way that I find hard to relate to my patches. Giving it a shot on integration.

https://hg.mozilla.org/mozilla-central/rev/ec7fe414e589 https://hg.mozilla.org/mozilla-central/rev/e8112fba80d6

I have reproduced this bug with 47.0a1 (2016-02-14) on Windows 8.1 64 bit! This bug's fix is verified on Latest Aurora 49.0a2. Build ID : 20160723004004 User Agent : Mozilla/5.0 (Windows NT 6.3; WOW64; rv:49.0) Gecko/20100101 Firefox/49.0 [testday-20160722]

Thanks Azmina for the testing! I also verified this using latest DevEdition 49.0a2 on Mac OS X 10.10.5 and Ubuntu 16.04 64-bit. Marking as verified fixed on 49 and also changing the status of the bug to Verified since the Target Milestone has been reached.