273466 - Can't drag favicon from url bar into bookmark sidebar to create bookmark

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Ed Chan]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8a6) Gecko/20041206 Firefox/1.0+ (bangbang023)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8a6) Gecko/20041206 Firefox/1.0+

Whenever I try to drag a url by the favicon from the URL bar into the bookmark
sidebar, a bookmark is not created.  In previous versions (1.7.5) I was able to
create bookmarks in this fashion and when dragged over over the sidebar, a line
will in between current bookmarks to show where the bookmark will be created. 
This line no longer appers.

Reproducible: Always
Steps to Reproduce:
1. Open the Bookmarks sidebar
2. Visit a url
3. Drag the favicon from the url bar and drop it in the bookmark sidebar

Actual Results:  
Nothing, no bookmark was created

Expected Results:  
A line should have been visible showing where the bookmark will be created and a
bookmark should have been created once the icon is dropped.

default theme

Comment 1

[Ed Chan]

I'm currently using bangbang023's fx but this bug was found on the official
20041206 trunk as well.

Comment 2

[Phil Ringnalda (:philor)]

Working in 20041129, not working by 20041203 - sounds like aviary-landing to me.

Comment 3

[Ryan Polk (Quark)]

bug 235001 was the same thing for the Aviary branch.  It's likely whatever broke
this in aviary got landed, but not the fix for it.

This is the checkin Blake made on aviary to fix:
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&subdir=mozilla/toolkit/components/history/src&command=DIFF_FRAMESET&file=nsGlobalHistory.cpp&rev1=1.33.2.1.2.5&rev2=1.33.2.1.2.6&root=/cvsroot

A quick check shows the latest version of that file doesn't have the change
(skip to line 1800):
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&subdir=mozilla/toolkit/components/history/src&command=DIFF_FRAMESET&file=nsGlobalHistory.cpp&rev1=1.48&rev2=1.33.2.1.2.6&root=/cvsroot

Comment 4

[nulldomain]

This also appears on linux, but a little differently.  You can drag the favicon
to bookmarks and place it there, but the line showing where you place the
bookmark does not appear, and you can't place the link in subfolders.

Also I think this is related to me not being able to order my bookmarks by just
dragging them up or down in the bookmark menu tab.  You currently can't select a
bookmark and drag it anywhere.

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8a6) Gecko/20041210 Firefox/1.0+

Comment 5

[Ian Neal]

Attachment: Patched based on fix for bug 235001

jag's patch as applied to aviary branch with bug 235001

Comment 6

[Peter Lairo]

This may also be related to not being able to drag a bookmark into a bookmark
subdirectory, and to the bookmarks list not auto-scrolling while dragging a new
BM into it if the list is longer than the screen height.

Comment 7

[Benjamin Smedberg]

Comment on attachment 168450 [details] [diff] [review]
Patched based on fix for bug 235001

I'm going to mark r+ not because I understand the code, but because the cvs
history shows that it's probably correct, and because the original bug is a
useless piece of shit.

Comment 8

[Ian Neal]

Comment on attachment 168450 [details] [diff] [review]
Patched based on fix for bug 235001

Checking in nsGlobalHistory.cpp;
/cvsroot/mozilla/toolkit/components/history/src/nsGlobalHistory.cpp,v  <-- 
nsGlobalHistory.cpp
new revision: 1.51; previous revision: 1.50
done

Comment 9

[Martin Meyer]

Does this need to be reopened maybe, or am I the only one still having a
problem? I don't see any line indicating where a bookmark would be created when
I drag the icon over the bookmark sidebar, but I do see the cursor indicating
that the item is dropable.  Nothing happens though when I drop the icon in the
sidebar.

I've tried this on beast builds from 20041226 and 20041228.  It doesn't seem to
work on either.  I've tried this also in safemode, no luck.  I haven't tried a
fresh profile, but that shouldn't be necessary I think.

Comment 10

[Michael Lefevre]

This doesn't seem to be working for me either (20041228 trunk on win2k), so
reopening...

Comment 11

[Ed Chan]

Doesn't work for me either.  I tried 20041227 with a new profile and it didn't
work so I decided to try 20041225 (when it was apparently fixed) as well with a
new profile and that didn't work either.

Comment 12

[Ian Neal]

Part of the problem could be to do with the fix for bug 235243 being reversed
out when aviary landed on the trunk

Comment 13

[Ian Neal]

Having looked through the code canDrop part of bookmarksTree.xml
http://lxr.mozilla.org/seamonkey/source/browser/components/bookmarks/content/bookmarksTree.xml#670
returns false if the selection contains an immutable

The new code from the aviary-landing in bookmarks.js
http://lxr.mozilla.org/seamonkey/source/browser/components/bookmarks/content/bookmarks.js#1266
decides the selection is an immutableBookmark and thus canDrop returns false

Two options I can see at the moment:
1. Remove the new code which will probably break Aggregation support - landed in
bug 235665 and bug 256362 
2. Fix the code so it doesn't think they're immutable bookmarks.

cc'ing vladimir as he's on the blame for this code.

Comment 14

[Worcester12345]

I thought there was going to be a big rewrite of the bookmarks code for the next
version after 1.0. Wouldn't that take all these into account?

Comment 15

[Peter Lairo]

Bug 276017, bug 273466 and bug 271891 are essentially all the same bug. Could we
just pick one and dupe the others to get some focus? This is an important bug.

Comment 16

[David Stone]

They're actually not the same. This one refers to the sidebar. Those refer to
the bookmarks toolbar...you know, the one right under the location bar?

Comment 17

[Ian Neal]

and the fix on bug 271891 does not fix this bug

Comment 18

[Peter Lairo]

(In reply to comment #16)
> They're actually not the same. This one refers to the sidebar. Those refer to
> the bookmarks toolbar...you know, the one right under the location bar?

Yeah, "same" was a poor choice of words. I should have said "seemingly very
similar". I mainly wanted to point out the *existence* of the other very similar
bugs ("dragging URLs into bookmarks") that *might* have the same or a similar
solution, in *case* some of the developers were not aware of the other bugs.
Sorry for not being more clear.

(In reply to comment #17)
> and the fix on bug 271891 does not fix this bug

That's really too bad.

Comment 19

[E. M. Ryan]

I thought I would mention that this bug does appear on both Windows and OSX
(10.3.7), even though the mention of it (and the other bug <a
href="https://bugzilla.mozilla.org/show_bug.cgi?id=273466">Bug 273466</a>) in
the "Official Build" bug list only lists Windows.

It also applies equally to dragging the favicon from the Address bar to the
"Manage Bookmarks" window (for lack of a better term).

Comment 20

[Ian Neal]

-> All/All

Comment 21

[Marcelo Simon]

Bug still present in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2)
Gecko/20050317 Firefox/1.0+.

Did the patch was checked in ?

Sorry for the spam, but this bug really bugs me!

Comment 22

[Ronny Perinke]

(In reply to comment #21)
> Bug still present in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2)
> Gecko/20050317 Firefox/1.0+.
> 
> Did the patch was checked in ?
> 
> Sorry for the spam, but this bug really bugs me!

yes, but it does not seem to work.

Comment 23

[Gary van der Merwe]

Attachment: Remove containsImmutable from canDrop

Ian: why can't we just do this? canDrop was not there on Avary 1.0, so I guess
that it was allways true?

Comment 24

[Gary van der Merwe]

Attachment: above patch with -u8pN

Sorry - forgot the context info

Comment 25

[Gary van der Merwe]

Attachment: Above patch Fixed

Soo sorry about the spam. I'm new to the whole patch concept.

Comment 26

[Ian Neal]

Next step is to ask someone for a review - perhaps mconnor or beng?

Comment 27

[Gary van der Merwe]

Comment on attachment 179453 [details] [diff] [review]
Above patch Fixed

Please review with skepticism. I don’t understand why it was added
(http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&
root=/cvsroot&subdir=mozilla/browser/components/bookmarks/content&command=DIFF_
FRAMESET&file=bookmarksTree.xml&rev2=1.43&rev1=1.42 ), and hence, I an not sure
that this change is not going to cause other bugs.

Comment 28

[Gary van der Merwe]

Here is that url again for the patch for the code that my patch removes. Not
sure why the url wraped in the prev comment.
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/browser/components/bookmarks/content&command=DIFF_FRAMESET&file=bookmarksTree.xml&rev2=1.43&rev1=1.42

Comment 29

[Chris]

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b2) Gecko/20050410
Firefox/1.0+

confirmed

Comment 30

[Anbo Motohiko]

WFM on 2005-04-18-10-trunk/Win32.
Since bug 288406 is fixed, we can also close this bug.

Comment 31

[Mike Connor [:mconnor]]

Comment on attachment 179453 [details] [diff] [review]
Above patch Fixed

this isn't the right fix, this breaks proper canDrop handling of Livemarks.

Better fix coming shortly.

Comment 32

[Mike Connor [:mconnor]]

Attachment: containsImmutable is only really valid if its a bookmark

This actually fixes dragging from the content area as well, which isn't
mentioned here, but was also broken.

Comment 33

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Comment on attachment 181097 [details] [diff] [review]
containsImmutable is only really valid if its a bookmark

filtering on moz/rdfitem is a little scary, but no more than the rest of this
code. r=me

Comment 34

[Mike Connor [:mconnor]]

Comment on attachment 181097 [details] [diff] [review]
containsImmutable is only really valid if its a bookmark

Anbo, I still see this on 04-18 nightly win32, on multiple profiles.  In fact,
I can't see how that fix would change this bug at all, since dragging
non-bookmarks into the sidebar will fail, because checkSelection thinks they're
ImmutableBookmarks.

Comment 35

[Asa Dotzler [:asa]]

Comment on attachment 181097 [details] [diff] [review]
containsImmutable is only really valid if its a bookmark

a=asa

Comment 36

[Mike Connor [:mconnor]]

Fixed on trunk.

Comment 37

[Gregor Larson]

Change to bookmarksTree.xml worked for me (on win32). Thanks Mike!!

Comment 38

[Mike Connor [:mconnor]]

sorry for bugspam, long-overdue mass reassign of ancient QA contact bugs, filter on "beltznerLovesGoats" to get rid of this mass change