403263 - deleting a separator in menus deletes all items preceding it

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect, P1)

Description

[Dietrich Ayala (:dietrich)]

from mardeg in #testday:

STR:

1. add a separator to a folder on the toolbar
2. add some other items above it
3. open the folder on the toolbar, and delete the separator via it's context menu

Expected results: separator is removed

Actual results: all items preceding the separator are removed

Note: I haven't tried to reproduce this yet. Will ask for blocking once confirmed.

Comment 1

[Ria Klaassen (not reading all bugmail)]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b2pre) Gecko/2007110909 Minefield/3.0b2pre

When I add separators in sub folders of the bookmarks toolbar the new separators are added to the root bookmarks toolbar, all side by side.
Deleting them works normally.
Separators added to the menu are added on the right place, so this works correctly. I can only delete them in the root menu. Deleting them in subfolders of the menu doesn't succeed; it deletes bookmarks instead (one by one).

I only tried my existing bookmarks.html, imported in a new profile.

Comment 2

[Mardeg]

Note that only the item *immediately above* the separator is removed, though if the separator is the top item in the folder, the folder itself is deleted!
This can happen if you have multiple separators underneath eachother with no links in between them, right-click -> delete on the very bottom separator in that group will remove the link immediately above the topmost separator, so the cursor is not near that link when it happens.

Comment 3

[Ria Klaassen (not reading all bugmail)]

So what I did:
- Open bookmarks menu
- Open sub folder of bookmarks menu
- Right click: New Separator
- Right click: Delete
And in this case the regression range is: http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=1193275080&maxdate=1193277959

Comment 4

[Dietrich Ayala (:dietrich)]

thanks for confirmation.

Comment 5

[Ria Klaassen (not reading all bugmail)]

Also the other issue has the same regression range:
- On Bookmarks Toolbar, open a folder
- In this folder, right click: New Separator
Result: the separator is added ON the Bookmarks Toolbar, next to previous new separator, not in the folder where it was created. I don't know if this is a different bug.

Comment 6

[Mardeg]

Nice catch there with the New Separator ON the Bookmarks Toolbar when trying to add it in the folder right-click.
The opposite is also true
- right-click the folder on the toolbar without opening it -> New Separator
Result: the separator is add IN the folder, not next to the folder on the Bookmarks Toolbar where it was intended.

Comment 7

[Ria Klaassen (not reading all bugmail)]

Here's some more odd behaviour:

In builds before 24 Oct, when you tried to delete the separator in a bookmarks-menu folder above "Open All in Tabs" (that is there by nature), you got an error in the console:
 
Error: [Exception... "Component returned failure code: 0x804b000a [nsIIOService.newURI]"  nsresult: "0x804b000a (<unknown>)"  location: "JS frame :: file:///C:/Docume~1/Ria/Bureau~1/firefox/components/nsScriptableIO.js :: ScriptableIO_newURI :: line 229"  data: no]
Source File: file:///C:/Docume~1/Ria/Bureau~1/firefox/components/nsScriptableIO.js
Line: 229

This is normal behaviour I think; it should not be deletable.

In builds after 24 Oct, I see no error; it deletes the bookmark above this separator instead. But when you just keep retrying, it deletes the whole remaining folder after the third, fourth, fifths or sixth attempt, still without any error.

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Marking blocking per bug 417594 comment 2

Comment 10

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

On first analysis: 
we don't have a valid selection for separators on popups, so in the bookmarks menu we have the contextmenu handler that takes care of setting the selection. We should do that for all menupopups. This should be quite simpler after Bug 389290 lands since we will have a new base binding for all places menus.

Comment 11

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch

i've moved the contextmenu and DOMMenuItemActive to places-popup-base, so they are fired for every menupopup.
this way right clicking on a separator puts the correct selection in.

with contextmenu i'm serving menuseparators only since for disabled items DOMMenuItemActive is called correctly (a previous comment was saying the opposite...).

Mano, we are setting the selection to popupNode.node || popupNode.parentNode._resultNode, this way selecting delete on an invalid target into a menupopup will delete the parent menu. 
This feels like correct (having a selection generally) but actually we are showing the "delete" option when right-clicking for example on "Open all in tabs" or on separators before that, selecting that deletes the entire folder and its contents. We should clearly fix context menu to not show at all on items out of startMarker, endMarker, but is really necessary to set the selection to the containing menu here?

Comment 12

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

somehow related bugs: Bug 384968, Bug 405776

Comment 13

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

for Bug 405776 probably we could make contextmenu served for all nodes, not only separators

Comment 14

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

nevermind last comment, clearly invalid

Comment 15

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

probably we can also remove contextmenu handler from toolbar.xml since there we already have mousedown that set the selection

Comment 16

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch (mousedown)

this uses mousedown event instead of contextmenu, the advantage is that it solves Bug 405776 too.

Comment 17

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Bug 418813 could be connected, with this patch i cannot reproduce that anymore...

Comment 18

[Asaf Romano (gone)]

Attachment: another approach

Comment 19

[Asaf Romano (gone)]

Attachment: don't break d&d

Comment 20

[Asaf Romano (gone)]

Attachment: take 3

Comment 21

[Asaf Romano (gone)]

Attachment: fix "empty" menuitem issue

Comment 22

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

functionality test is going on IRC

first review:

           if (aEvent.ctrlKey)
             aDragAction.action = Ci.nsIDragService.DRAGDROP_ACTION_COPY;
+
+          // set selection and activate the view
+          document.popupNode = aEvent.target;

comment is set selection, and we are setting popupNode, what about commenting on why we are doing that?

       <!-- nsIPlacesView -->
       <method name="getSelectionNodes">
         <body><![CDATA[
-          return this.hasSelection ? [this.selectedNode] : [];
+          var selectedNode = this.selectedNode;
+          if (selectedNode)
+            return [selectedNode];
+          return [];
         ]]></body>
       </method>

return selectedNode ? [selectedNode] : [];
 
       <!-- nsIPlacesView -->
       <method name="getSelectionNodes">
         <body><![CDATA[
-          return this.hasSelection ? [this.selectedNode] : [];
+          var selectedNode = this.selectedNode;
+          if (selectedNode)
+            return [selectedNode];
+          return [];
         ]]></body>
       </method>

as before 

         onDragStart: function TBV_DO_onDragStart(event, xferData, dragAction) {
+
+          // activate the view and set selection
+          document.popupNode = event.target;

as before

onDragExit in toolbar.xml is still called for toolbar submenus, is that needed?

Comment 23

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

i've already put this on IRC, but in case you have lost that, about those errors i was getting when removing a separator from a submenu

ASSERT: null node
Stack Trace:
0:PU_getConcreteItemId(null)
1:get_insertionPoint()
2:PC__canInsert()
3:PC_isCommandEnabled(placesCmd_new:folder)
4:isCommandEnabled(placesCmd_new:folder)
5:updatePlacesCommand(placesCmd_new:folder)
6:goUpdatePlacesCommands()
7:oncommandupdate([object Event])


ASSERT: null node
Stack Trace:
0:PU_getConcreteItemId(null)
1:get_insertionPoint()
2:PC__canInsert()
3:PC_isCommandEnabled(cmd_paste)
4:isCommandEnabled(cmd_paste)
5:goUpdateCommand(cmd_paste)
6:goUpdateGlobalEditMenuItems()
7:oncommandupdate([object Event])
8:focus()
9:XPCNativeWrapper function wrapper()
10:destroyContextMenu()
11:onpopuphiding([object MouseEvent])


the problem is in InsertionPoint getter in menu.xml, folderId = PlacesUtils.getConcreteItemId(this.selectedNode.parent), this.selectedNode.parent is null, maybe popupNode.node is still pointing to the removed item?
a solution could be make hasSelection return this.selectedNode.parent != null && this.selectedNode != null;

Comment 24

[Asaf Romano (gone)]

Attachment: patch

Comment 25

[Asaf Romano (gone)]

Attachment: patch

Comment 26

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 305493 [details] [diff] [review]
patch

functionality test and code review have been done on IRC.

this is going to fix also: Bug 418813, Bug 405776, Bug 413977 (i can confirm this at least on Windows)

Comment 27

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 305493 [details] [diff] [review]
patch

argh still some problem in the Library, clicking a folder in the left pane:

ASSERT: null node
Stack Trace: 
0:PU_nodeIsURI(null)
1:get_selectedURINode()
2:PC_isCommandEnabled(placesCmd_open)
3:isCommandEnabled(placesCmd_open)
4:updatePlacesCommand(placesCmd_open)
5:goUpdatePlacesCommands()
6:oncommandupdate([object Event])
7:updateCommands(sort)
8:PTV__sortingChanged(0)
9:PTV__finishInit()
10:PTV_setTree([object BoxObject])
11:view([object Object])
12:set_view([object Object])
13:load([xpconnect wrapped nsINavHistoryQuery],[xpconnect wrapped nsINavHistoryQueryOptions])
14:set_place(place:folder=3&expandQueries=0)
15:PO_onPlaceSelected(true)
16:onselect([object Event])
17:select(3)
18:onxblmousedown([object MouseEvent])

Comment 28

[Dietrich Ayala (:dietrich)]

Comment on attachment 305493 [details] [diff] [review]
patch


>@@ -1031,21 +1025,24 @@ PlacesController.prototype = {
>    * @returns A TransferDataSet object that can be dragged and dropped 
>    *          elsewhere.
>    */
>   getTransferData: function PC_getTransferData(dragAction) {
>     var result = this._view.getResult();
>     var oldViewer = result.viewer;
>     try {
>       result.viewer = null;
>-      var nodes = null;
>-      if (dragAction == Ci.nsIDragService.DRAGDROP_ACTION_COPY)
>-        nodes = this._view.getCopyableSelection();
>-      else
>-        nodes = this._view.getDragableSelection();
>+      var nodes = this._view.getDragableSelection();
>+      if (dragAction == Ci.nsIDragService.DRAGDROP_ACTION_MOVE) {
>+        nodes.filter(function(node) {
>+          var parent = node.parent;
>+          return parent && !PlacesUtils.nodeIsReadOnly(parent);
>+        });
>+      }
>+

iirc, Array.filter() doesn't change the array it's called on.

Comment 29

[Asaf Romano (gone)]

Attachment: more cleanup

Comment 30

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 305558 [details] [diff] [review]
more cleanup

testing has been fine with this versione, code appear correct. let's put this in!

Comment 31

[Asaf Romano (gone)]

mozilla/browser/components/places/content/controller.js 1.206
mozilla/browser/components/places/content/history-panel.js 1.24
mozilla/browser/components/places/content/menu.xml 1.104
mozilla/browser/components/places/content/places.js 1.130
mozilla/browser/components/places/content/toolbar.xml 1.126
mozilla/browser/components/places/content/tree.xml 1.96
mozilla/browser/components/preferences/selectBookmark.js 1.8

Comment 32

[pal-moz]

filed a bug 419549

Comment 33

[Tracy Walker [:tracy]]

verified with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b4pre)
Gecko/2008022704 Minefield/3.0b4pre 

Comment 34

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv