Last Comment Bug 313027 - Can't drag plain text url to tab bar
: Can't drag plain text url to tab bar
Status: RESOLVED FIXED
: fixed1.8.1
Product: Camino Graveyard
Classification: Graveyard
Component: Drag & Drop (show other bugs)
: Trunk
: PowerPC Mac OS X
-- normal (vote)
: Camino1.5
Assigned To: Simon Fraser
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2005-10-19 09:46 PDT by Simon Fraser
Modified: 2006-08-12 11:24 PDT (History)
8 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Patch to make url parsing from text less strict (1.03 KB, patch)
2005-12-09 21:25 PST, Simon Fraser
stuart.morgan+bugzilla: review+
mikepinkerton: superreview+
Details | Diff | Splinter Review

Description User image Simon Fraser 2005-10-19 09:46:14 PDT
You can't drag plain text (like "www.mozilla.org") to the background area in the
tab bar and have it load.

Seems like the tab bar isn't accepting plain text drags.
Comment 1 User image Samuel Sidler (old account; do not CC) 2005-10-19 09:57:33 PDT
Smokey mentioned this was a regression from 0.8.4.
Comment 2 User image JK 2005-10-19 10:25:12 PDT
The same goes for the bookmark bar: you can't create a bookmark via drag&drop.
Comment 3 User image Chris Lawson (gone) 2005-10-19 18:16:00 PDT
Our current behaviour is only halfway a regression from 0.8.4. Here's what 0.8.4
allows you to do:

If you have a URL minus the protocol handler (e.g., www.example.com) in plain
text, you can highlight it and drag it to the bookmarks bar, where a new
bookmark will be created with the title of www.example.com and the URL of (no
protocol) www.example.com.

If you drag this to a blank space in the tab bar, you get a new tab with
www.example.com as the URL. Again, no protocol.

If the URL has a protocol associated with it (e.g., http://www.example.com),
dragging it to the bookmarks bar does nothing. Dragging it to the tab bar works
as expected, loading http://www.example.com in a new tab.

That's what 0.8.4 does.

The 02 June nightly works for drag-n-drop of URLs into either the bookmarks bar
or tab bar, but IF AND ONLY IF the URL has a protocol associated with it. In
other words, www.example.com no longer works.

In addition, when dropping http://www.example.com onto the bookmarks bar, the
bookmark title is left blank/empty.

The current nightly does nothing with text URLs dropped on either the bookmarks
bar OR tab bar, whether or not there's a protocol associated with them.

So we have at least three different regressions here, and what I would consider
to be an accidental bugfix.

1) Dropping protocol-less URLs on bookmarks and tab bar. Regressed sometime
between spin-off of 0.8 branch and 02 May.

2) Dropping URLs with protocols on bookmarks and tab bar. Unknowingly fixed
between 0.8.4 and 02 May. Regressed sometime between 02 June and a week ago,
when it was first noticed.

3) Title is left blank when dropping a URL with protocol on the bookmarks bar.
Regression window appears to be the same as for (1).

I'll post more when I've narrowed the window further. This looks pretty messy,
though.

cl
Comment 4 User image Chris Lawson (gone) 2005-10-19 18:21:20 PDT
Ugh. Regression 2 is actually two different regressions.

The 03 July nightly exhibits the URL-with-protocol behaviour of 02 May in the
tab bar, but exhibits the current nightly behaviour in the bookmarks bar.

No bookmark is created by dragging plain text to the bookmarks bar at all, but
the tab bar drags work (but only with URLs that have a protocol specified).

cl
Comment 5 User image Chris Lawson (gone) 2005-10-19 18:47:26 PDT
I've isolated a 48-hour regression window for (2), bookmarks bar case.

22 June 2005 nightly allows dropping of plain-text URL (protocol required) on
bookmarks bar, but gives bookmark no title.

24 June 2005 nightly does not allow this -- no bookmark is created, period.

The only seemingly related patch I can see that was checked in during this time
is the checkin for bug 298547.

cl
Comment 6 User image Simon Fraser 2005-10-19 19:53:36 PDT
We call -[NSPasteboard containsURLData] when deciding to accept text drags in
various places. This does:

  if ([types containsObject:NSStringPboardType]) {
    NSURL* testURL = [NSURL URLWithString:[self stringForType:NSStringPboardType]];
    return (testURL != nil) && ([[testURL scheme] length] > 0);
  }

which would appear to be too strict.(In reply to comment #3)

> The current nightly does nothing with text URLs dropped on either the bookmarks
> bar OR tab bar, whether or not there's a protocol associated with them.

That's not what I see. Dragging text-with-protocol to bookmarks bar or tab bar
gives me a new bookmark or tab.
Comment 7 User image Chris Lawson (gone) 2005-10-19 20:09:03 PDT
(In reply to comment #6)

> > The current nightly does nothing with text URLs dropped on either the bookmarks
> > bar OR tab bar, whether or not there's a protocol associated with them.
> 
> That's not what I see. Dragging text-with-protocol to bookmarks bar or tab bar
> gives me a new bookmark or tab.

My apologies. I must have gotten confused; this definitely works in the latest
nightly, but only with a protocol. Absent a protocol, I get nothing. (I also
discovered a rather entertaining mostly unrelated bug in the process.)

cl
Comment 8 User image Smokey Ardisson (offline for a while; not following bugs - do not email) 2005-10-19 21:55:09 PDT
I think something's awry in your tests of 0.8.4 in Comment 3 :-)

plain text, no protocol:
BM Bar: creates bookmark entitled "www.mozilla.org" with URL "www.mozilla.org"
Tab Bar: loads "www.mozilla.org" (which gets the http:// protocol prepended
during loading)

plain text with protocol:
*BM Bar: creates bookmark entitled "http://www.mozilla.org" with URL
"http://www.mozilla.org"
Tab Bar: loads "http://www.mozilla.org"

In other words, everything did work in 0.8.4.

I don't see regression 3 at all; url+protocol to BM bar creates bookmark
entitled "http://www.mozilla.org" with URL "http://www.mozilla.org" just as
0.8.4.  Nor any part of regression 2, as I think others have determined.

The only thing that's broken is regression 1, protocol-less URLs don't create
new tabs or new bookmarks; the drag always springs back.  (You do get the green
globe plus over the tab bar before it springs back, but no | cursor over the BM
bar).

That said, maybe I'm the only one who wasn't clear on all of that yet, and it
sounds like Simon has identified the problem.
Comment 9 User image Chris Lawson (gone) 2005-10-19 22:04:24 PDT
(In reply to comment #8)
> I think something's awry in your tests of 0.8.4 in Comment 3 :-)

Yeah. I can't figure out where what went wrong, but I certainly see how I could
have gotten confused. :)

At any rate, I agree with you that regression 1 is the only one that seems to be
a real problem.

Is there any simple way to make that check less strict? (I'll have a look at the
NSURL stuff tomorrow.)

cl
Comment 10 User image Smokey Ardisson (offline for a while; not following bugs - do not email) 2005-10-31 03:15:41 PST
Is this fixable for 1.0?  It's a regression that's currently untargeted.
Comment 11 User image Smokey Ardisson (offline for a while; not following bugs - do not email) 2005-12-07 23:58:53 PST
Wevah, any chance you can look into/fix this for 1.0, since it's a regression?

Targeting for 1.1 now.
Comment 12 User image Simon Fraser 2005-12-08 09:03:58 PST
Hm, dragging plaintext to the tab bar background leaves the drag hilight there too.
Comment 13 User image Simon Fraser 2005-12-09 21:25:53 PST
Created attachment 205462 [details] [diff] [review]
Patch to make url parsing from text less strict
Comment 14 User image Chris Lawson (gone) 2006-05-06 11:56:46 PDT
Simon, is there a reason you never asked for r or sr on this patch? It'd be nice to get this in, as it's one our few 1.0 regressions.

cl
Comment 15 User image Simon Fraser 2006-05-06 16:10:08 PDT
(In reply to comment #14)
> Simon, is there a reason you never asked for r or sr on this patch? It'd be
> nice to get this in, as it's one our few 1.0 regressions.

Only that this code is called from a bunch of places, and it needs some thorough testing.
Comment 16 User image Smokey Ardisson (offline for a while; not following bugs - do not email) 2006-05-06 20:03:04 PDT
(In reply to comment #15)
> (In reply to comment #14)
> > Simon, is there a reason you never asked for r or sr on this patch? It'd be
> > nice to get this in, as it's one our few 1.0 regressions.
> 
> Only that this code is called from a bunch of places, and it needs some
> thorough testing.

Sounds like a job for a smorgan review ;)

That said, I can also build with this and see if I notice anything funny.
Comment 17 User image krmathis@gmail.com 2006-05-07 04:06:10 PDT
(In reply to comment #13)
> Created an attachment (id=205462) [edit]
> Patch to make url parsing from text less strict
> 

I just compiled a 1.8 branch Camino build with this patch included.
Drag plain text url to the tab bar opens the url in a new tab, which it did not do earlier.

Drag the same url to the bookmarks bar creates a new bookmark, with a title and location matching the url. Same goes when I drag the url from the location bar into the bookmarks bar.

As discussed on IRC, with ardissone, it seems fine. If no one else have something for me to test though?
Comment 18 User image Smokey Ardisson (offline for a while; not following bugs - do not email) 2006-05-07 04:09:56 PDT
> As discussed on IRC, with ardissone, it seems fine. If no one else have
> something for me to test though?

I also had Kai test the scenarios in bug 298547 for regressions from this patch, since making them work properly caused this regression!
Comment 19 User image Smokey Ardisson (offline for a while; not following bugs - do not email) 2006-05-07 07:50:21 PDT
The "problem" with this patch is that we'll now blithely accept drags of *anything* to the tab bar or the bookmark bar (or the Bookmarks Manager to create new bookmarks there); not only things like caminobrowser.org/start (which is ok) or www.foo.bar or sample.txt but even single words or characters like !  (Fx allows this as well...)

We'll load the error page for everything drug to the tab bar that's invalid (or do fixup if possible), and eventually end up doing that when a user tries to use the bookmark he's created with the invalid URL.  (This also means text drags from other apps are also treated as if they could be URLs by the tab bar and the bookmark bar.)

It's not the end of the world that we allow everything and go to the error page, but do we want to be this loose?  If there's an easily obtainable middle ground, I'd prefer that, but if not, too loose is better than too strict.

http://landfill.mozilla.org/mxr-test/mozilla/search?string=containsURLData&find=&filter=mozilla%2Fcamino
Comment 20 User image Stuart Morgan 2006-07-01 16:11:47 PDT
One possible middle ground would be to require either a scheme or a '.' character (or perhaps '.[A-Za-z]', to prevent 46.9 from being counted).  "http://mozilla" is clearly a URL, "mozilla.org" is probably a URL, but "mozilla" probably isn't (although it will work because of fixup).  I would think that would do the right thing from the user's perspective in the vast majority of cases.
Comment 21 User image froodian (Ian Leue) 2006-07-02 11:02:04 PDT
I've been looking at this, and I'm a bit worried about '.[A-Za-z]'  Wouldn't it be valid to have foo.com/folder2/ ?  I mean, I guess we could search for '.[A-Za-z]until'/'' but it seems safer just to look for '.' and take the risk of somebody dragging a float.
Comment 22 User image Stuart Morgan 2006-07-02 11:32:27 PDT
(In reply to comment #21)
> I've been looking at this, and I'm a bit worried about '.[A-Za-z]'  Wouldn't it
> be valid to have foo.com/folder2/ ?  I mean, I guess we could search for
> '.[A-Za-z]until'/'' but it seems safer just to look for '.' and take the risk
> of somebody dragging a float.
> 

foo.com/folder2 contains '.c', which matches '\.[A-Za-z]' (a period followed by a letter).  AFAIK, there are no TLDs starting with a number.
Comment 23 User image Peter Jaros (:peeja) 2006-07-02 11:34:46 PDT
(In reply to comment #21)
> I've been looking at this, and I'm a bit worried about '.[A-Za-z]'  Wouldn't it
> be valid to have foo.com/folder2/ ?  I mean, I guess we could search for
> '.[A-Za-z]until'/'' but it seems safer just to look for '.' and take the risk
> of somebody dragging a float.

To clarify, 'foo.com/folder2/' does match '.[A-Za-z]'.  The pattern only needs a dot followed by a letter.

So, to reword smorgan's proposal, a string is validated if it begins with a schema OR it contains at least one dot followed by at least one letter.  Since every URL should end in a TLD, and no TLD begins with a non-letter, this should work.
Comment 24 User image Stuart Morgan 2006-07-02 12:42:41 PDT
Except that I was forgetting IP addresses, which brings it back to just '.'

And another possibility is file paths: /Volumes/User/..., which means it should probably be (has_scheme or starts_with_/ or contains_.).  Or it should just accept everything, which I'm beginning to think may be better.
Comment 25 User image Wevah 2006-07-02 12:55:02 PDT
I think "everything" is not a bad choice, especially given that the user can *type* anything into the location bar and it may or may not work. The only thing we should definitely do is some light fixup, like removing surrounding whitespace and the like.
Comment 26 User image Stuart Morgan 2006-07-02 15:28:34 PDT
Comment on attachment 205462 [details] [diff] [review]
Patch to make url parsing from text less strict

Well, "everything" isn't completely accurate.  It's actually "everything which NSURL doesn't reject", which means that strings that contain whitespace and other invalid characters will be rejected--no cleanup should be required.

I'll test this out when I get a chance.
Comment 27 User image Wevah 2006-07-02 15:41:45 PDT
I don't think they should be rejected if they contain surrounding whitespace; if I accidentally include a leading space when dragging from a text document I think it should still work.
Comment 28 User image Stuart Morgan 2006-07-11 21:29:00 PDT
Comment on attachment 205462 [details] [diff] [review]
Patch to make url parsing from text less strict

This seems like the way to go, and works in my testing.

r=me (although the commented out |&& ...| should be removed before checkin.
Comment 29 User image Mike Pinkerton (not reading bugmail) 2006-08-02 07:40:51 PDT
so if there's *any* whitespace (leading or trailing) this will now reject a perfectly good url? Can we trim it before we pass it along?
Comment 30 User image Stuart Morgan 2006-08-02 08:02:24 PDT
In my tests it looked like something was trimming it at an earlier stage of the process.  It could be explicitly trimmed again here if we don't want to rely on that.
Comment 31 User image Mike Pinkerton (not reading bugmail) 2006-08-11 19:39:52 PDT
Comment on attachment 205462 [details] [diff] [review]
Patch to make url parsing from text less strict

sr=pink

yes, please remove the commented out bits of the expression.
Comment 32 User image Nick Kreeger 2006-08-12 11:24:46 PDT
Fixed trunk and branch.

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