Closed Bug 403263 Opened 13 years ago Closed 13 years ago

deleting a separator in menus deletes all items preceding it

Categories

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

defect

Tracking

()

VERIFIED FIXED
Firefox 3 beta4

People

(Reporter: dietrich, Assigned: mano)

References

Details

(Keywords: dataloss)

Attachments

(1 file, 8 obsolete files)

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.
Keywords: dataloss
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.
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.
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
thanks for confirmation.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: qawanted
Priority: -- → P1
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.
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.
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.
Target Milestone: Firefox 3 beta3 → ---
Marking blocking per bug 417594 comment 2
Flags: blocking-firefox3+
Assignee: nobody → mak77
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.
Status: NEW → ASSIGNED
Attached patch patch (obsolete) — Splinter Review
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?
Attachment #304719 - Flags: review?(mano)
somehow related bugs: Bug 384968, Bug 405776
for Bug 405776 probably we could make contextmenu served for all nodes, not only separators
nevermind last comment, clearly invalid
probably we can also remove contextmenu handler from toolbar.xml since there we already have mousedown that set the selection
Attachment #304719 - Flags: review?(mano)
Attached patch patch (mousedown) (obsolete) — Splinter Review
this uses mousedown event instead of contextmenu, the advantage is that it solves Bug 405776 too.
Bug 418813 could be connected, with this patch i cannot reproduce that anymore...
Attached patch another approach (obsolete) — Splinter Review
Assignee: mak77 → mano
Attachment #305144 - Flags: review?(mak77)
Attached patch don't break d&d (obsolete) — Splinter Review
Attachment #304719 - Attachment is obsolete: true
Attachment #304726 - Attachment is obsolete: true
Attachment #305144 - Attachment is obsolete: true
Attachment #305146 - Flags: review?(mak77)
Attachment #305144 - Flags: review?(mak77)
Attached patch take 3 (obsolete) — Splinter Review
Attachment #305146 - Attachment is obsolete: true
Attachment #305150 - Flags: review?(mak77)
Attachment #305146 - Flags: review?(mak77)
Target Milestone: --- → Firefox 3 beta4
Attached patch fix "empty" menuitem issue (obsolete) — Splinter Review
Attachment #305150 - Attachment is obsolete: true
Attachment #305151 - Flags: review?(mak77)
Attachment #305150 - Flags: review?(mak77)
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?
Target Milestone: Firefox 3 beta4 → ---
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;
Whiteboard: [needs new patch]
Attached patch patch (obsolete) — Splinter Review
Attachment #305151 - Attachment is obsolete: true
Attachment #305426 - Flags: review?(dietrich)
Attachment #305151 - Flags: review?(mak77)
Attachment #305426 - Flags: review?(dietrich) → review?(mak77)
Target Milestone: --- → Firefox 3 beta4
Attached patch patch (obsolete) — Splinter Review
Attachment #305426 - Attachment is obsolete: true
Attachment #305493 - Flags: review?(mak77)
Attachment #305426 - Flags: review?(mak77)
OS: Windows XP → All
Hardware: PC → All
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)
Attachment #305493 - Flags: review?(mak77) → review+
Whiteboard: [needs new patch]
Blocks: 405776, 413977, 418813
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])
Attachment #305493 - Flags: review+ → review-
Whiteboard: [needs new patch]
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.
Attached patch more cleanupSplinter Review
Attachment #305493 - Attachment is obsolete: true
Attachment #305558 - Flags: review?(mak77)
Comment on attachment 305558 [details] [diff] [review]
more cleanup

testing has been fine with this versione, code appear correct. let's put this in!
Attachment #305558 - Flags: review?(mak77) → review+
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
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [needs new patch]
Blocks: 419549
filed a bug 419549
No longer blocks: 419549
Depends on: 419549
verified with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b4pre)
Gecko/2008022704 Minefield/3.0b4pre 
Status: RESOLVED → VERIFIED
Depends on: 421061
Depends on: 424115
No longer depends on: 449170
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
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in before you can comment on or make changes to this bug.