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

Status: RESOLVED FIXED

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

Description

[Ria Klaassen (not reading all bugmail)]

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

[Ria Klaassen (not reading all bugmail)]

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

Comment 2

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: patch!

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.

Comment 3

[Annie Sullivan]

Comment on attachment 217363 [details] [diff] [review]
patch!


>+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();
>+          }
>+#endif
>         },

Can you add a comment explaining this?

Comment 4

[Ben Goodger (use ben at mozilla dot org for email)]

Attachment: 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.

Comment 5

[Annie Sullivan]

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.

Comment 6

[Ben Goodger (use ben at mozilla dot org for email)]

fixed-on-trunk, fixed-1.8-branch

I will track the mac drag over folder thing separately.

Comment 7

[Ben Goodger (use ben at mozilla dot org for email)]

fixed-on-trunk, fixed-1.8-branch

I will track the mac drag over folder thing separately.

Comment 8

[Steffen Wilberg]

This caused bug 333651.

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

Compare:
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=1.268.2.30&mark=476#466

Comment 9

[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