618995 - Dragging a link with a line break to the bookmark bar produces two-line bookmark

Status: RESOLVED FIXED

(Camino Graveyard :: Bookmarks, defect)

Description

[Lisa Thompson]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en; rv:1.9.2.14pre) Gecko/20101209 Camino/2.1a1pre (like Firefox/3.6.14pre)
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.6; en; rv:1.9.2.14pre) Gecko/20101209 Camino/2.1a1pre (like Firefox/3.6.14pre)

STR: If you have an account, go to Google Reader and drag any headline to your bookmark bar.

AR: The resulting bookmark looks mis-aligned because the bookmark title has a trailing line break in it. 

ER: The trailing break could be removed, so the bookmark appears in the bar the same as if you had right-clicked on the link, chosen "Bookmark This Link...", and bookmarked it to the bar.

The Google Reader source is too complex for me, so I don't know for sure what the cause is there. But I see the same thing with any link that has a line-break in it. In case you don't have Google reader, I put such a link on my test page at http://homepage.mac.com/lthompson.22/test.html. There's also a screenshot that you can reproduce by dragging the multi-line link to your bookmark bar.

Allowing two-line bookmarks doesn't really bother me, it's easy for users to fix. But the odd case of bookmarks for Google Reader headlines, where there's no text on the second line, prompts me to report this for your consideration.


Reproducible: Always

Comment 1

[Lisa Thompson]

I meant to include in the report that neither Safari nor Chrome exhibit the issue with Google Reader headlines, and both remove line breaks from links dragged to the bookmark bar. (Firefox does not allow dragging links to the bookmark bar?)

> But the odd case of bookmarks for Google Reader headlines, 
> where there's no text on the second line

...and no indication that one should expect a two-line bookmark (as there is in my home-cooked example). When I first saw it, I thought I had found some UI-drawing glitch; it didn't immediately occur to me that it was due to a line break.

Comment 2

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #1)
> I meant to include in the report that neither Safari nor Chrome exhibit the
> issue with Google Reader headlines, and both remove line breaks from links
> dragged to the bookmark bar.

We probably need to hook up more sanitizing; specifically, we need to call |cleanedStringWithPasteboardString| on all the title flavors (and not just URLs) in NSPasteboard+Utils's |getURLs:andTitles|.

> (Firefox does not allow dragging links to the bookmark bar?)

Would not surprise me at all. :P

Comment 3

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: Expanded testcase for line-breaks in link titles

This is Lisa's testcase, expanded to include leading and trailing linebreaks.

I guess we need to strip leading and trailing breaks, and convert internal breaks to spaces?

Comment 4

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #2)
> > (Firefox does not allow dragging links to the bookmark bar?)
> 
> Would not surprise me at all. :P

No, it seems to just reject drags whose titles have returns in them; I was able to drag other links OK.  Also, it appears to sanitize when using the "Bookmark This Link" context menu item.

At any rate, it doesn't surprise Firefox and Safari behave differently, since we share 0 code with them in this area.  It surprises me a little bit the Chrome doesn't, since they share our Pasteboard code (but it's entirely possible they've sanitized at a different place).

Comment 5

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: Partial fix

This sort-of fixes the problem by using |cleanedStringWithPasteboardString|.  The only issue is that it simply strips and doesn't convert internal returns into spaces, if we want to do that (I'm not sure where our fancy location-bar return-sanitizing code is).

We'd need a sister method to |cleanedStringWithPasteboardString| that instead calls |stringByReplacingCharactersInSet:withString:| and then |stringByTrimmingWhitespace|.  |cleanedTitleStringWithPasteboardString| anyone?  Or is there a method that already does that I can't find?

Comment 6

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(For reference, the fancy location-bar sanitizing code is apparently BWC's |cleanPastedText:| http://mxr.mozilla.org/camino/source/camino/src/browser/BrowserWindowController.mm#296  That code, while not directly useful here, raises another question, however: why aren't our control character-cleaning methods in NSString+Utils also cleaning illegals?  Why is |cleanPastedText:| the only place that does that?)

It's too late for my brain to think usefully about the issues in the last few comments; hopefully cl will have some thoughts for me tomorrow ;)

Comment 7

[Chris Lawson (gone)]

How is this different, exactly, from bug 313079, other than that one having a semi-obscure title? :-p

Comment 8

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #7)
> How is this different, exactly, from bug 313079, other than that one having a
> semi-obscure title? :-p

Well, that one seemed focused on editing bookmarks, whereas this one is drags, and then there's the obscure title, and then the fact that I'd forgotten all about that one :P

(Note also that the dragging case mentioned briefly in bug 313079 no longer reproduces, since it was a raw URL, which have since been sanitized.)

Comment 9

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

The Bookmark This Link… CM (and the AddBookmarkDialogController in general) solve this problem via http://mxr.mozilla.org/camino/source/camino/src/bookmarks/AddBookmarkDialogController.mm#57

That's probably the correct solution here, too (although maybe not for links with the URL-as-link-text; I'd have to check)--but it also raises the question of bug 619166 once again.

Comment 10

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: Fix

Nothing like saying what the fix is over and over again but not implementing it.

Here's |cleanedTitleStringWithPasteboardString:|, which does the same thing as AddBookmarkDialogController's |BookmarkTitleForItem| cleaning part and as HTMLBookmarkConverter's |cleanedStringFromHTMLBookmarkImport|.

Should we rename the existing |cleanedStringWithPasteboardString:| to |cleanedURLStringWithPasteboardString:| for parity and clarity?

Comment 11

[Wevah (Nate Weaver)]

Comment on attachment 512010 [details] [diff] [review]
Fix

r+; looks fine to me o_O

Comment 12

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Comment on attachment 512010 [details] [diff] [review]
Fix

(In reply to comment #10)
> Should we rename the existing |cleanedStringWithPasteboardString:| to
> |cleanedURLStringWithPasteboardString:| for parity and clarity?

cl thought we should; Wevah said he could go either way, since that could be used on non-URLs (though it's currently not)

Comment 13

[Stuart Morgan]

Comment on attachment 512010 [details] [diff] [review]
Fix

sr=smorgan

Yes, let's do the rename.

I'm wondering if we should make the new method cleanedTitleStringFromPasteboardType:, and have it do both the fetching and cleaning, since that's what every call site does, but whichever way you prefer is fine.

Comment 14

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

http://hg.mozilla.org/camino/rev/4418fe40feeb with the renaming and refactoring of the two methods.