Last Comment Bug 475045 - Can't drag unlinkified URL to bookmarks toolbar (regression from Firefox 3)
: Can't drag unlinkified URL to bookmarks toolbar (regression from Firefox 3)
Status: VERIFIED FIXED
[qa!]
: regression
Product: Firefox
Classification: Client Software
Component: Bookmarks & History (show other bugs)
: Trunk
: All All
: -- normal with 4 votes (vote)
: Firefox 9
Assigned To: Brian R. Bondy [:bbondy]
:
Mentors:
data:text/html,http://www.mozilla.org
: 495211 500106 501927 659548 665297 673912 (view as bug list)
Depends on: 478070 670587 673080
Blocks: 701647 356295
  Show dependency treegraph
 
Reported: 2009-01-23 10:06 PST by Ria Klaassen (not reading all bugmail)
Modified: 2011-11-17 07:16 PST (History)
24 users (show)
mak77: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
?


Attachments
Patch for bookmarking unlinked URLs (2.41 KB, patch)
2011-07-25 09:04 PDT, Brian R. Bondy [:bbondy]
mak77: feedback+
Details | Diff | Splinter Review
Patch for bookmarking unlinked URLs + review comments (1.61 KB, patch)
2011-07-26 21:36 PDT, Brian R. Bondy [:bbondy]
no flags Details | Diff | Splinter Review
atch for bookmarking unlinked URLs + review comments (1.61 KB, patch)
2011-07-27 07:17 PDT, Brian R. Bondy [:bbondy]
no flags Details | Diff | Splinter Review
Patch for bookmarking unlinked URLs + review comments + chrome mochitest (6.45 KB, patch)
2011-07-28 06:28 PDT, Brian R. Bondy [:bbondy]
no flags Details | Diff | Splinter Review
Patch for bookmarking unlinked URLs + review comments + chrome mochitest (6.46 KB, patch)
2011-07-28 06:33 PDT, Brian R. Bondy [:bbondy]
mak77: review+
Details | Diff | Splinter Review
Patch for bookmarking unlinked URLs + review comments + chrome mochitest (5.05 KB, patch)
2011-08-02 10:54 PDT, Brian R. Bondy [:bbondy]
no flags Details | Diff | Splinter Review
Patch for bookmarking unlinked URLs + review comments + chrome mochitest (5.02 KB, patch)
2011-08-02 10:59 PDT, Brian R. Bondy [:bbondy]
mak77: review+
Details | Diff | Splinter Review
Patch for bookmarking unlinked URLs + review comments + chrome mochitest (4.86 KB, patch)
2011-08-03 03:51 PDT, Brian R. Bondy [:bbondy]
no flags Details | Diff | Splinter Review
Bug 475045 - Can't drag unlinkified URL to bookmarks toolbar (regression from Firefox 3) + OS X fix (5.96 KB, patch)
2011-08-21 17:55 PDT, Brian R. Bondy [:bbondy]
mak77: review+
Details | Diff | Splinter Review
Can't drag unlinkified URL to bookmarks toolbar (regression from Firefox 3) + OS X fix (6.04 KB, patch)
2011-08-22 07:42 PDT, Brian R. Bondy [:bbondy]
enndeakin: review+
Details | Diff | Splinter Review

Description Ria Klaassen (not reading all bugmail) 2009-01-23 10:06:29 PST
STR:

- Go to URL
- Scroll down to http://willemzwijgtnietmeer.web-log.nl/ (use findbar to locate it)
- Select and attempt to drag this URL to bookmarks toolbar

Expected result: dragged URL becomes bookmark.
Actual result: none.

This regressed on 27 Aug 2008: http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2008-08-25+22%3A00%3A00&enddate=2008-08-28+13%3A00%3A00
between 20080827052043 and 20080827061801 so probably from Bug 356295.
Comment 1 Dorus Peelen 2009-02-11 08:56:37 PST
And neither can you drag a url (or partial url) from the navigation toolbar and drop it on the bookmark toolbar. (do not confuse this with dragging the favicon to the bookmark toolbar)


All working:
Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b1pre)
Gecko/20080919032843 Minefield/3.1b1pre

Cursor 'animation' still working in this one, but doesn't create a bookmark.
Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b1pre)
Gecko/20080920033605 Minefield/3.1b1pre
Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b2pre) Gecko/20081011
Minefield/3.1b2pre

In this one, i can't even drag the text in the url bar at all
Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b2pre) Gecko/20081018
Minefield/3.1b2pre
Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b2pre) Gecko/20081020
Minefield/3.1b2pre

In this one, i can drag again, but not onto bookmark bar, shows blocked cursor
(= current behaviour also):
Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b2pre) Gecko/20081022
Minefield/3.1b2pre
Comment 2 Marco Bonardo [::mak] 2009-05-28 03:55:21 PDT
*** Bug 495211 has been marked as a duplicate of this bug. ***
Comment 3 Marco Bonardo [::mak] 2009-06-18 03:30:40 PDT
comment 1 is now fixed in bug 478070.
Comment 4 Kevin Brosnan 2009-06-23 22:56:47 PDT
*** Bug 500106 has been marked as a duplicate of this bug. ***
Comment 5 Nochum Sossonko [:Natch] 2009-07-12 19:35:47 PDT
This is a pretty serious regression, just updated a friend of mine to 3.5, this was the first thing he ran into. It's an extremely common action to drag a url from the bar to the bookmark toolbar, IMO, putting this on the radar.

Note: this probably belongs in Browser::<something> as it doesn't really seem to be a Core bug, rather a bookmark or Navigation Bar bug...
Comment 6 Mardeg 2009-08-04 08:05:26 PDT
*** Bug 501927 has been marked as a duplicate of this bug. ***
Comment 7 eyal gruss (eyaler) 2009-08-10 00:55:27 PDT
while this bug is still relevant for 3.5.2,
it seems to be fixed in 3.6a2
Comment 8 eyal gruss (eyaler) 2009-08-13 17:27:25 PDT
my bad - i was referring to comment #1.
the original bug is still broken in 3.6a2
Comment 9 eyal gruss (eyaler) 2009-09-15 20:42:14 PDT
can the mechanism of Bug 454518 be of use here?
Comment 10 CoJaBo 2011-05-12 16:24:04 PDT
Ran into this upgrading systems from 3.x to 4.
This is a rather serious oversight, as many users bookmark exclusively by dragging the text from the location bar to the bookmarks toolbar-
It isn't obvious there is a work-around, it just appears that bookmarking is no longer supported in the new version.
Comment 11 Alice0775 White 2011-05-24 20:34:39 PDT
*** Bug 659548 has been marked as a duplicate of this bug. ***
Comment 12 Pavel Cvrcek [:JasnaPaka] 2011-06-19 03:44:50 PDT
*** Bug 665297 has been marked as a duplicate of this bug. ***
Comment 13 Brian R. Bondy [:bbondy] 2011-07-25 07:38:47 PDT
*** Bug 673912 has been marked as a duplicate of this bug. ***
Comment 14 Brian R. Bondy [:bbondy] 2011-07-25 09:04:13 PDT
Created attachment 548198 [details] [diff] [review]
Patch for bookmarking unlinked URLs

The problem was that we were doing a string compare on the transferable flavor mime type.  

If we were given something of type text/plain we should have made the best matched flavor to text/unicode (conversion would be made internally).
Comment 15 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-07-25 09:50:29 PDT
Comment on attachment 548198 [details] [diff] [review]
Patch for bookmarking unlinked URLs

If it's not to be generally used, it's probably best to avoid adding a PlacesUtils constant for it (just hardcode it in the one place where you need it).

:mak and/or Mano should probably review this, flagging them both and they can sort it out :)
Comment 16 Brian R. Bondy [:bbondy] 2011-07-25 10:08:13 PDT
OK thanks for the feedback Gavin.  I'll wait for the additional feedback from others before doing any changes here.
Comment 17 Marco Bonardo [::mak] 2011-07-25 13:04:37 PDT
Comment on attachment 548198 [details] [diff] [review]
Patch for bookmarking unlinked URLs

Review of attachment 548198 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/places/content/controller.js
@@ +1378,5 @@
>      }
> +
> +    // We couldn't find a supported flavor, we need to ensure that we aren't
> +    // trying to use text/plain though and if so indicate to use text/unicode
> +    // A conversion internally will be made automatically to text/unicode.

let's compress this coment a bit.
"If no supported flavor is found, check if data includes text/plain contents.  If so request them as text/unicode, conversion will happen automatically."

@@ +1381,5 @@
> +    // trying to use text/plain though and if so indicate to use text/unicode
> +    // A conversion internally will be made automatically to text/unicode.
> +    if (aFlavors.contains(PlacesUtils.TYPE_PLAIN_TEXT) != -1) {
> +        return PlacesUtils.TYPE_UNICODE;
> +    }

I agree with Gavin, this is not useful to be exposed from utils for now, just hardcode "text/plain" here.

@@ +1383,5 @@
> +    if (aFlavors.contains(PlacesUtils.TYPE_PLAIN_TEXT) != -1) {
> +        return PlacesUtils.TYPE_UNICODE;
> +    }
> +
> +    // We couldn't find a flavor match

This comment doesn't seem that useful, just remove it
Comment 18 Marco Bonardo [::mak] 2011-07-25 13:08:48 PDT
A couple things yet, do we really have a contains() method on arrays?
Also making a test for this should be possible and most likely required for a positive review.
Comment 19 Brian R. Bondy [:bbondy] 2011-07-25 13:13:49 PDT
Thanks for the review Marco.

aFlavors is of type DOMStringList:
https://developer.mozilla.org/En/DOM/DOMStringList

I'll implement those changes and look into a chrome mochitest
Comment 20 Marco Bonardo [::mak] 2011-07-25 13:28:45 PDT
(In reply to comment #19)
> aFlavors is of type DOMStringList:
> https://developer.mozilla.org/En/DOM/DOMStringList

ugh! my memory fails, please add a note in the javadoc above getFirstValidFlavor, on the aFlavors param.
Comment 21 Brian R. Bondy [:bbondy] 2011-07-26 21:36:00 PDT
Created attachment 548688 [details] [diff] [review]
Patch for bookmarking unlinked URLs + review comments

Just updating with review comments.  Before I ask for another review though I'll implement the chrome mochitest.
Comment 22 Marco Bonardo [::mak] 2011-07-27 05:23:23 PDT
according to http://mxr.mozilla.org/mozilla-central/source/dom/interfaces/core/nsIDOMDOMStringList.idl#51 contains() returns a boolean, so checking it against -1 sounds still wrong.
Also, please use nsIDOMDOMStringList in the javadoc, since it's what we really get
Comment 23 Brian R. Bondy [:bbondy] 2011-07-27 07:17:21 PDT
Created attachment 548772 [details] [diff] [review]
atch for bookmarking unlinked URLs + review comments

Fixed, it was left over from me also thinking originally it was an array and using indexOf.  Pending test case.
Comment 24 Brian R. Bondy [:bbondy] 2011-07-28 06:28:51 PDT
Created attachment 549092 [details] [diff] [review]
Patch for bookmarking unlinked URLs + review comments + chrome mochitest

- The mochitest passes with the new code and fails before the new code. 
- It tests mainly for text/plain but I also tried for unicode and text/x-moz-url
- It tests for each drop effect type as well
Comment 25 Brian R. Bondy [:bbondy] 2011-07-28 06:33:44 PDT
Created attachment 549094 [details] [diff] [review]
Patch for bookmarking unlinked URLs + review comments + chrome mochitest

Just had to qrefresh a comment change
Comment 26 Marco Bonardo [::mak] 2011-08-01 12:45:04 PDT
Comment on attachment 549094 [details] [diff] [review]
Patch for bookmarking unlinked URLs + review comments + chrome mochitest

Review of attachment 549094 [details] [diff] [review]:
-----------------------------------------------------------------

the patch is fine, the test needs some minor cleanup, r=me with the following fixed (as usual nits are not needed to get a positive review, but appreciated if you fix them).
if you want a further final check, feel free to ask it.

::: browser/components/places/tests/browser/browser_475045.js
@@ +13,5 @@
> + *
> + * The Original Code is Places test code.
> + *
> + * The Initial Developer of the Original Code is
> + * Mozilla Foundation.

nit: "the Mozilla Foundation." but see below

@@ +32,5 @@
> + * and other provisions required by the GPL or the LGPL. If you do not delete
> + * the provisions above, a recipient may use your version of this file under
> + * the terms of any one of the MPL, the GPL or the LGPL.
> + *
> + * ***** END LICENSE BLOCK ***** */

nit: if you wish you may use the PD license in tests (http://www.mozilla.org/MPL/boilerplate-1.1/pd-c) you don't have to, if you don't want to. The clear advantage is it's shorter and allows any kind of code reuse.

@@ +41,5 @@
> +  let toolbar = document.getElementById("PersonalToolbar");
> +  let wasCollapsed = toolbar.collapsed;
> +  if (wasCollapsed) {
> +    setToolbarVisibility(toolbar, true);
> +  }

if the test timeouts or fails for whatever reason, it may leave toolbar in an unwanted status, so rather use registerCleanupFunction here to restore it at original status after test run

@@ +46,5 @@
> +
> +  // Setup a node we will use to be dropped. The actual node used does not
> +  // matter, because we will set its data, effect, and mimeType manually
> +  let placesItems = document.getElementById("PlacesToolbarItems");
> +  var srcElement = placesItems.childNodes[0];

I think it's safer if you test that is not null and its localName, since previous tests may do funny stuff.
nit: please don't mix up let and var unless you have really good reasons to, let is usually fine.

@@ +49,5 @@
> +  let placesItems = document.getElementById("PlacesToolbarItems");
> +  var srcElement = placesItems.childNodes[0];
> +  
> +  // simulateDragDrop will simulate a drop of a URI onto the bookmarks bar
> +  // using the specified effect and mime type

use a real javadoc here, no reason to use single-lines comments to document a function

@@ +51,5 @@
> +  
> +  // simulateDragDrop will simulate a drop of a URI onto the bookmarks bar
> +  // using the specified effect and mime type
> +  var simulateDragDrop = function(aEffect, aMimeType) {
> +    // Simulate a dorp of a URI

typo: dorp
nit: we prefer to make comments read like phrases, so punctuation may help readability (uc first, end with period, and such)

@@ +52,5 @@
> +  // simulateDragDrop will simulate a drop of a URI onto the bookmarks bar
> +  // using the specified effect and mime type
> +  var simulateDragDrop = function(aEffect, aMimeType) {
> +    // Simulate a dorp of a URI
> +    let uriSpec = "http://www.mozilla.org/D1995729-A152-4e30-8329-469B01F30AA7";

nit: you can use a const URL = "..." here

@@ +66,5 @@
> +    let bookmarkIds = PlacesUtils.bookmarks
> +                      .getBookmarkIdsForURI(uri);
> +
> +    // Verify that there is exactly one URI only
> +    ok(bookmarkIds.length == 1, "There should be exactly one bookmark");

I think the second check here (getBookmarksIdsForURI) makes the first check (isBookmarked) pretty much useless, I'd remove the isBookmarked check.

@@ +68,5 @@
> +
> +    // Verify that there is exactly one URI only
> +    ok(bookmarkIds.length == 1, "There should be exactly one bookmark");
> +    
> +    // Verify that we removed the bookmark successfully

nit: we have not yet removed it, we are about to do that.

@@ +78,5 @@
> +  // Simulate a bookmark drop for all of the mime types and effects
> +  var mimeTypes = ["text/plain", "text/unicode", "text/x-moz-url"];
> +  var effects = ["move", "copy", "link"];
> +  for (e in effects) {
> +    for (m in mimeTypes) {

per good code practices, don't use for...in on arrays, rather use .forEach or a plain for/while loop. While that doesn't matter here, we know lots of addons developers use tests to get inspiration for their code, so we should avoid common pitfalls.
Comment 27 Brian R. Bondy [:bbondy] 2011-08-01 12:48:38 PDT
Thanks for the review, I'll implement all comments and r? you again to be sure.
Comment 28 Brian R. Bondy [:bbondy] 2011-08-02 10:54:59 PDT
Created attachment 550125 [details] [diff] [review]
Patch for bookmarking unlinked URLs + review comments + chrome mochitest

Implemented review comments on the chrome mochitest.
Comment 29 Brian R. Bondy [:bbondy] 2011-08-02 10:59:26 PDT
Created attachment 550128 [details] [diff] [review]
Patch for bookmarking unlinked URLs + review comments + chrome mochitest

Minor change from last patch I forgot to qrefresh
Comment 30 Marco Bonardo [::mak] 2011-08-03 02:37:23 PDT
Comment on attachment 550128 [details] [diff] [review]
Patch for bookmarking unlinked URLs + review comments + chrome mochitest

Review of attachment 550128 [details] [diff] [review]:
-----------------------------------------------------------------

only nits, thanks for taking care of this.

::: browser/components/places/tests/browser/browser_475045.js
@@ +15,5 @@
> +  registerCleanupFunction(function() {
> +    if (wasCollapsed) {
> +      setToolbarVisibility(toolbar, false);
> +    }
> +  });

nit: you may directly

if (toolbar.collapsed) {
  setToolbarVisibility(toolbar, true);
  registerCleanupFunction(function() {
    setToolbarVisibility(toolbar, false);
  });
}

@@ +48,5 @@
> +    PlacesUtils.bookmarks.removeItem(bookmarkIds[0]);
> +
> +    // Verify that we removed the bookmark successfully.
> +    let bookmarked = PlacesUtils.bookmarks.isBookmarked(uri);
> +    ok(!bookmarked, "URI should be removed");

nit: no need for the local, you can just ok(!PlacesUtils...
Comment 31 Brian R. Bondy [:bbondy] 2011-08-03 03:51:34 PDT
Created attachment 550345 [details] [diff] [review]
Patch for bookmarking unlinked URLs + review comments + chrome mochitest

Fixed nits, thanks for the suggestions.
Comment 32 Marco Bonardo [::mak] 2011-08-03 05:07:16 PDT
Comment on attachment 550345 [details] [diff] [review]
Patch for bookmarking unlinked URLs + review comments + chrome mochitest

it's fine, doesn't need a further review
Comment 33 Brian R. Bondy [:bbondy] 2011-08-03 05:08:50 PDT
Does it need an r+ flag on the patch for someone to pick it up for checkin?
Comment 34 Marco Bonardo [::mak] 2011-08-03 05:15:23 PDT
(In reply to comment #33)
> Does it need an r+ flag on the patch for someone to pick it up for checkin?

nope, it just needs a r+ somewhere in the bug and a checkin-needed in the whiteboard
Comment 35 Marco Bonardo [::mak] 2011-08-03 05:16:06 PDT
ops, I meant in the keywords field :)
Comment 38 Brian R. Bondy [:bbondy] 2011-08-05 13:52:20 PDT
Comment on attachment 550345 [details] [diff] [review]
Patch for bookmarking unlinked URLs + review comments + chrome mochitest

Sorry :( Will try again locally with pulling recent changes and will run through try after that.
Comment 39 Brian R. Bondy [:bbondy] 2011-08-05 14:07:23 PDT
Mentioned in IRC already but I think the dependent bug needed to be in first.  Before asking again for a checkin though I'll definitely investigate more and run through try :S
Comment 40 Marco Bonardo [::mak] 2011-08-05 15:00:08 PDT
hm, the test had some green run too, looks like it's a frequent random failure, not a permanent failure.
Comment 41 Brian R. Bondy [:bbondy] 2011-08-05 15:19:12 PDT
> the test had some green run too,

Where was the test run? Do you have a link?
Comment 42 Marco Bonardo [::mak] 2011-08-05 15:33:37 PDT
hm I may have been wrong, it was permafailure on osx, green on others...
http://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=a8bca81215ea

but note I had to backout bug 673301 for similar reasons, there may be a D&D bug on OSX that hits both.
Comment 43 Marco Bonardo [::mak] 2011-08-18 06:36:52 PDT
bug 673301 is in inbound now, with a possible workaround for the exception, may you try to push this to try on top of inbound tip, checking mochitest-oth on osx?
Comment 44 Brian R. Bondy [:bbondy] 2011-08-18 09:08:42 PDT
Thanks for letting me know Marco, I'll run it through try today and if it passes I'll put it back onto inbound.
Comment 45 Brian R. Bondy [:bbondy] 2011-08-18 10:04:16 PDT
I wanted to ensure everything worked so I pushed to all tests and platforms here:
http://tbpl.mozilla.org/?tree=Try&rev=a4e2a8487328
Comment 46 Brian R. Bondy [:bbondy] 2011-08-18 11:15:27 PDT
Sorry I realized that I pushed to mozilla-central try so I cancelled that try request, I'll put another one out.
Comment 47 Brian R. Bondy [:bbondy] 2011-08-18 11:22:48 PDT
OK I repushed to try from a fully updated mozilla-inbound.  I think that should work now.
Comment 48 Marco Bonardo [::mak] 2011-08-18 15:54:28 PDT
you probably want to use tbpl.allizom.org for now (it's the temporary replacement for the official tbpl, due to excessive load on Tinderbox):
http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=aae42b831c3b
Comment 49 Brian R. Bondy [:bbondy] 2011-08-18 15:59:21 PDT
Ya sorry someone told me on IRC after I posted, thanks though.
Comment 50 Brian R. Bondy [:bbondy] 2011-08-20 20:27:10 PDT
If I put the synthesizeDrop in a try/catch statement the test succeeds fine and the bookmarks do get created as they should on OS X.  I setup my dev environment on OS X and debugged a bit so far and found that the nsBaseDragService::EndDragSession method is returning NS_ERROR_FAILURE because of a NULL mDoingDrag.  (See code snippet below)

I still need to investigate the cause, for example maybe it is being called twice. 
Also it consistently fails on EndDragSession no matter what the mime type and drop effect is on OS X.

On Windows nsBaseDragService::EndDragSession is returning fine so mDoingDrag is non NULL.
Note: We actually ignore the return result on Windows of EndDragSession but if I properly return the real return result is succeeds anyway.
 
nsBaseDragService::EndDragSession(PRBool aDoneDrag)
{
  if (!mDoingDrag) {
    return NS_ERROR_FAILURE;
}


I'll investigate more and update again.
Comment 51 Brian R. Bondy [:bbondy] 2011-08-21 17:55:00 PDT
Created attachment 554776 [details] [diff] [review]
Bug 475045 - Can't drag unlinkified URL to bookmarks toolbar (regression from Firefox 3) + OS X fix

So the problem on OS X was that a window move was happening which cancels the drag, and then another end drag was issued a second time which threw the exception in JS from EndDragSession.



Call stack:

nsBaseDragService::EndDragSession
nsDragService::EndDragSession
nsBaseDragService::Suppress
nsGlobalWindow::CanMoveResizeWindows


Problem stems from the call stack from this:
nsGlobalWindow::CanMoveResizeWindows {
  if (ds) {
      gDragServiceDisabled = PR_TRUE;
      ds->Suppress(); <--
  }

There were 2 changes I needed to do so that the window wouldn't move. 
1) EventUtils.synthesizeDrop needed to use the source element mouse down from center point instead of (2,2)
2) I needed to use a bookmark as the source element instead of the places toolbar.


I ran this on try as well and I think it's good:
http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=4585f77734fa
Comment 52 Marco Bonardo [::mak] 2011-08-22 04:33:01 PDT
Comment on attachment 554776 [details] [diff] [review]
Bug 475045 - Can't drag unlinkified URL to bookmarks toolbar (regression from Firefox 3) + OS X fix

Review of attachment 554776 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/places/tests/browser/browser_475045.js
@@ +29,5 @@
> +   */
> +  let simulateDragDrop = function(aEffect, aMimeType) {
> +    const uriSpec = "http://www.mozilla.org/D1995729-A152-4e30-8329-469B01F30AA7";
> +    let uri = makeURI(uriSpec);
> +    EventUtils.synthesizeDrop(placesItems.childNodes[0], 

Can you sanity check that this is not null above, where you test for placesItems?

::: testing/mochitest/tests/SimpleTest/EventUtils.js
@@ +637,5 @@
>  
>    try {
>      // need to use real mouse action
>      aWindow.addEventListener("dragstart", trapDrag, true);
> +    synthesizeMouseAtCenter(srcElement, { type: "mousedown" }, aWindow);

Imo this is fine, but just in case I'd prefer Neil to approve the change to EventUtils
Comment 53 Neil Deakin 2011-08-22 04:51:06 PDT
Does comment 51 describe an underlying bug?
Comment 54 Brian R. Bondy [:bbondy] 2011-08-22 07:22:19 PDT
> Does comment 51 describe an underlying bug?

Ya even if I supply a bookmark button in synthesizeDrop as the source element it would move the whole window on OS X.  When it moves the window instead of the bookmark it cancels the drag.  Setting the mouse down to be the center of the source element instead fixes the problem.
Comment 55 Marco Bonardo [::mak] 2011-08-22 07:27:28 PDT
may be due to rounded corners on those buttons? dragging the toolbar moves the window, isn't it? so if you hit the space out of the button border, you are dragging the window.
Comment 56 Brian R. Bondy [:bbondy] 2011-08-22 07:29:28 PDT
> may be due to rounded corners on those buttons?

That sounds possible ya.  I thought those coordinates 2,2 were relative to the source element so it didn't make sense to me why.  But that would explain it.

> dragging the toolbar moves the window, isn't it?

On OS X yes, on Windows no.

> so if you hit the space out of the button border, you are dragging the window

Ya so I think dragging from the center of the source element is best.
Comment 57 Brian R. Bondy [:bbondy] 2011-08-22 07:42:45 PDT
Created attachment 554861 [details] [diff] [review]
Can't drag unlinkified URL to bookmarks toolbar (regression from Firefox 3) + OS X fix

Updated test case for no children check.
Review needed for the EventUtils change.
Comment 58 Ed Morley [:emorley] 2011-08-24 17:18:37 PDT
http://hg.mozilla.org/mozilla-central/rev/bab51f46b02f
Comment 59 Pavel Cvrcek [:JasnaPaka] 2011-09-02 00:22:22 PDT
Thanks for the fix! I think bug 670587 is related to this bug because without it it doesn't work for majority of users.
Comment 60 eyal gruss (eyaler) 2011-11-11 02:08:58 PST
submitted Bug 701647 to allow dragging when the leading protocol is missing
Comment 61 Virgil Dicu [:virgil] [QA] 2011-11-17 07:16:38 PST
Mozilla/5.0 (X11; Linux x86_64; rv:9.0) Gecko/20100101 Firefox/9.0
Mozilla/5.0 (X11; Linux x86_64; rv:10.0a2) Gecko/20111117 Firefox/10.0a2
Mozilla/5.0 (X11; Linux x86_64; rv:11.0a1) Gecko/20111117 Firefox/11.0a1
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:9.0) Gecko/20100101 Firefox/9.0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:10.0a2) Gecko/20111116 Firefox/10.0a2
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:11.0a1) Gecko/20111117 Firefox/11.0a1
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:9.0) Gecko/20100101 Firefox/9.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:10.0a2) Gecko/20111116 Firefox/10.0a2
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:11.0a1) Gecko/20111117 Firefox/11.0a1
Mozilla/5.0 (Windows NT 5.1; rv:9.0) Gecko/20100101 Firefox/9.0
Mozilla/5.0 (Windows NT 5.1; rv:10.0a2) Gecko/20111116 Firefox/10.0a2
Mozilla/5.0 (Windows NT 5.1; rv:11.0a1) Gecko/20111117 Firefox/11.0a1

Verified on F9 Beta 2, Aurora and Nightly.

STR used:
1. Load "data:text/html,http://www.mozilla.org" (any site)
2. Select the unlinkified text.
3. Drag the text to the bookmark toolbar.

As expected, the dragged URL becomes a bookmark.

Note You need to log in before you can comment on or make changes to this bug.