bugzilla.mozilla.org will be intermittently unavailable on Saturday, March 24th, from 16:00 until 20:00 UTC.

After dragging bookmark to folder on bookmarks toolbar, bookmarks-menu does not close after clicking in the page

RESOLVED FIXED in Firefox 2 alpha2



Bookmarks & History
12 years ago
8 years ago


(Reporter: Ria Klaassen (not reading all bugmail), Assigned: Ben Goodger (use ben at mozilla dot org for email))


Firefox 2 alpha2
Windows XP

Firefox Tracking Flags

(Not tracked)



(1 attachment, 1 obsolete attachment)

Steps to reproduce:
1. Open bookmarks-menu.
2. Drag a bookmark to the bookmarks toolbar.
3. Now try to get rid of the bookmarks-menu popup.
Expected behaviour: popup should disappear after a click in the page.
NB: I don't see this behaviour when dragging within the bookmarks-menu.

Comment 1

12 years ago
Only when dragging from bookmarks-menu into a folder on the bookmarks toolbar.


12 years ago
Summary: After dragging operation bookmarks-menu popup sticks → After dragging bookmark to folder on bookmarks toolbar, bookmarks-menu does not close after clicking in the page
Assignee: nobody → annie.sullivan
Priority: -- → P2
Target Milestone: --- → Firefox 2 alpha2
Created attachment 217363 [details] [diff] [review]

Because of bugs in the back end (see bug 332845) we need to have special case hackery to make this work. I had to reintroduce some of the nasty timer force-closing from bookmarksMenu.js

I moved a bunch of the Places code from browser.js to browser-places.js for neatness.

I also made sure you could drop on the bookmarks menu item itself, and alt+drag folders on your personal toolbar around.
Assignee: annie.sullivan → bugs
Attachment #217363 - Flags: review?(annie.sullivan)

Comment 3

12 years ago
Comment on attachment 217363 [details] [diff] [review]

>+BookmarkAllTabsCommand.NAME = "Browser:BookmarkAllTabs";
>+BrowserController.commands[BookmarkAllTabsCommand.NAME] = 
>+  new BookmarkAllTabsCommand();
>+BrowserController.events[BrowserController.EVENT_TABCHANGE] = 
>+  [BookmarkAllTabsCommand.NAME];

Maybe put a comment explaining this?  When I searched through the code, I saw that it updates the command when the tab change event happens, but the first time I looked at this code I didn't get it.  Might be more confusing now that BrowserController is in a separate file.

>+  onDrop: function BMDH_onDrop(event, data, session) {
>+    var view = document.getElementById("bookmarksMenuPopup");
>+    PlacesController.activeView = view;
>+    PlacesControllerDragHelper.onDrop(null, view, view.insertionPoint, 1);
>+    view._rebuild();
>+  }

view.insertionPoint depends on the view's current selection.  But if the user drops onto the label of the bookmarks menu, we always want to insert the item in the insertionPoint the menu would have if there was no selection.  Is there a guarantee that the view won't have a selection at this point?  If so, can you add a comment explaining?  If not, should we add a method for this to the views?

>+  _setDragTimer: function PMDC__setDragTimer(prop, meth, delay, args) {

Can you add comments above this method explaining what it does and what the arguments are?

>+    if (!this._dragSupported)
>+      return;

So the mac definitely still doesn't support dragging?  It was working fine for me.

>+    if (this._timersSupported) {

I'm not clear on why we can't use the same workaround for timers not firing that menu.xml uses (see the _setTimer method of it's DNDObserver).  If we definitely can't use it, then why isn't the code in menu.xml being changed, too?  And why do we have to have two implementations, one for timers supported and one for timers not supported?  Can't we simplify the code by always using the timers not supported implementation?

>+  /**
>+   * Close a menupopup and all of its children.
>+   * @param   menupopup
>+   *          A menupopup to close.
>+   */
>+  _closeMenu: function PMDC__closeMenu(menupopup) {

It looks like this only gets called for places menus.  If so, why not just use the hidePopupAndChildPopups() method?

>         onDragStart: function TBV_DO_onDragStart(event, xferData, dragAction) {
>           PlacesController.activeView = this._self;
>           if (event.ctrlKey) {
>             dragAction.action = Ci.nsIDragService.DRAGDROP_ACTION_COPY;
>           }
>           xferData.data = PlacesController.getTransferData(dragAction.action);
>+#ifdef XP_WIN
>+          if (event.target.localName == "toolbarbutton" &&
>+              event.target.getAttribute("type") == "menu") {
>+            if (!event.shiftKey && !event.altKey && !event.ctrlKey)
>+              return false;
>+            event.target.firstChild.hidePopup();
>+          }
>         },

Can you add a comment explaining this?
Attachment #217363 - Flags: review?(annie.sullivan) → review-
Created attachment 217697 [details] [diff] [review]
new patch

Annie: I tested D&D on mac and while it "works" there are numerous repainting issues. I wouldn't call it shipping quality. 

This patch changes a few things:

- More documentation for BrowserController
- Gets rid of manual timer impl (for some reason the idle = false thing didn't work for me before)... I would have built the regular thing into toolbar.xml but that wouldn't help the toplevel bookmarks menu. I think we just need to fix the actual menu XUL problem in the long term.
- Share more code with menu.xml and PlacesControllerDragHelper (draggingOverChildNode)

Note: includes the fix for 332873. Brett is reviewing that.
Attachment #217363 - Attachment is obsolete: true
Attachment #217697 - Flags: review?(annie.sullivan)

Comment 5

12 years ago
Comment on attachment 217697 [details] [diff] [review]
new patch

Shouldn't you disable auto-opening toolbarbutton folders when they're dragged over on mac as well?  To do this, you need to #ifdef out the stuff that acts on _overFolder in toolbar.xml.
Attachment #217697 - Flags: review?(annie.sullivan) → review+
fixed-on-trunk, fixed-1.8-branch

I will track the mac drag over folder thing separately.
fixed-on-trunk, fixed-1.8-branch

I will track the mac drag over folder thing separately.
Last Resolved: 12 years ago
Resolution: --- → FIXED

Comment 8

12 years ago
This caused bug 333651.

Error: BrowserController.onTabSwitch is not a function
Source file: chrome://browser/content/browser.xul
Line: 1

Trunk: http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/browser/base/content/browser.xul&rev=1.295&mark=476#466
Branch: http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/browser/base/content/browser.xul&rev=
Depends on: 333651
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.

Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in before you can comment on or make changes to this bug.