475045 - Can't drag unlinkified URL to bookmarks toolbar (regression from Firefox 3)

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Ria Klaassen (not reading all bugmail)]

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]

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 3

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

comment 1 is now fixed in bug 478070.

Comment 5

[Nochum Sossonko [:Natch]]

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 7

[eyal gruss]

while this bug is still relevant for 3.5.2,
it seems to be fixed in 3.6a2

Comment 8

[eyal gruss]

my bad - i was referring to comment #1.
the original bug is still broken in 3.6a2

Comment 9

[eyal gruss]

can the mechanism of Bug 454518 be of use here?

Comment 10

[CoJaBo]

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 14

[Brian R. Bondy [:bbondy]]

Attachment: 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: [e-mail]]]

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]]

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] (Away Apr 25 - May 5)]

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] (Away Apr 25 - May 5)]

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]]

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] (Away Apr 25 - May 5)]

(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]]

Attachment: 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] (Away Apr 25 - May 5)]

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]]

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

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

Attachment: Patch for bookmarking unlinked URLs + review comments + chrome mochitest

Just had to qrefresh a comment change

Comment 26

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

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]]

Thanks for the review, I'll implement all comments and r? you again to be sure.

Comment 28

[Brian R. Bondy [:bbondy]]

Attachment: Patch for bookmarking unlinked URLs + review comments + chrome mochitest

Implemented review comments on the chrome mochitest.

Comment 29

[Brian R. Bondy [:bbondy]]

Attachment: Patch for bookmarking unlinked URLs + review comments + chrome mochitest

Minor change from last patch I forgot to qrefresh

Comment 30

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

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]]

Attachment: Patch for bookmarking unlinked URLs + review comments + chrome mochitest

Fixed nits, thanks for the suggestions.

Comment 32

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

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]]

Does it need an r+ flag on the patch for someone to pick it up for checkin?

Comment 34

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

(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] (Away Apr 25 - May 5)]

ops, I meant in the keywords field :)

Comment 36

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

http://hg.mozilla.org/integration/mozilla-inbound/rev/a8bca81215ea

Comment 37

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

backed out, the test is permaorange :(

http://tinderbox.mozilla.org/showlog.cgi?log=Mozilla-Inbound/1312574380.1312575673.18633.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Mozilla-Inbound/1312574263.1312575676.18642.gz

Comment 38

[Brian R. Bondy [:bbondy]]

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]]

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] (Away Apr 25 - May 5)]

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]]

> the test had some green run too,

Where was the test run? Do you have a link?

Comment 42

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

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] (Away Apr 25 - May 5)]

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]]

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]]

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]]

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]]

OK I repushed to try from a fully updated mozilla-inbound.  I think that should work now.

Comment 48

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

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]]

Ya sorry someone told me on IRC after I posted, thanks though.

Comment 50

[Brian R. Bondy [:bbondy]]

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]]

Attachment: 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] (Away Apr 25 - May 5)]

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]

Does comment 51 describe an underlying bug?

Comment 54

[Brian R. Bondy [:bbondy]]

> 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] (Away Apr 25 - May 5)]

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]]

> 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]]

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

http://hg.mozilla.org/mozilla-central/rev/bab51f46b02f

Comment 59

[Pavel Cvrcek [:JasnaPaka]]

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]

submitted Bug 701647 to allow dragging when the leading protocol is missing

Comment 61

[Virgil Dicu [:virgil] [QA]]

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.