Closed Bug 313027 Opened 19 years ago Closed 18 years ago

Can't drag plain text url to tab bar

Categories

(Camino Graveyard :: Drag & Drop, defect)

Product:
Camino Graveyard
Component:
Drag & Drop
Platform:
PowerPC
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Camino1.5

People

(Reporter: sfraser_bugs, Assigned: sfraser_bugs)

Details

(Keywords: fixed1.8.1)

Attachments

(1 file)

Patch to make url parsing from text less strict
1.03 KB, patch
stuart.morgan+bugzilla
: review+
mikepinkerton
: superreview+
Details | Diff | Splinter Review 
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.
Smokey mentioned this was a regression from 0.8.4.
Keywords: regression
The same goes for the bookmark bar: you can't create a bookmark via drag&drop.
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
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
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
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.

(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
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.
(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
Is this fixable for 1.0?  It's a regression that's currently untargeted.
Wevah, any chance you can look into/fix this for 1.0, since it's a regression?

Targeting for 1.1 now.
Target Milestone: --- → Camino1.1
Hm, dragging plaintext to the tab bar background leaves the drag hilight there too.
Assignee: mikepinkerton → sfraser_bugs
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
(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.
(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.

(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?
> 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!

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

(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.
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.
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 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.
Attachment #205462 - Flags: review?(stuart.morgan)
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 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.
Attachment #205462 - Flags: superreview?
Attachment #205462 - Flags: review?(stuart.morgan)
Attachment #205462 - Flags: review+
Attachment #205462 - Flags: superreview? → superreview?(mikepinkerton)
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?
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 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.
Attachment #205462 - Flags: superreview?(mikepinkerton) → superreview+
Whiteboard: [needs checkin]
Fixed trunk and branch.
Status: NEW → RESOLVED
Closed: 18 years ago
Keywords: regression → fixed1.8.1
Resolution: --- → FIXED
Whiteboard: [needs checkin]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: