Can't drag plain text url to tab bar

RESOLVED FIXED in Camino1.5

Status

Camino Graveyard
Drag & Drop
RESOLVED FIXED
12 years ago
11 years ago

People

(Reporter: Simon Fraser, Assigned: Simon Fraser)

Tracking

({fixed1.8.1})

Trunk
Camino1.5
PowerPC
Mac OS X
fixed1.8.1

Details

Attachments

(1 attachment)

1.03 KB, patch
Stuart Morgan
: review+
Mike Pinkerton (not reading bugmail)
: superreview+
Details | Diff | Splinter Review
(Assignee)

Description

12 years ago
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

Comment 2

12 years ago
The same goes for the bookmark bar: you can't create a bookmark via drag&drop.

Comment 3

12 years ago
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

12 years ago
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

12 years ago
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
(Assignee)

Comment 6

12 years ago
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

12 years ago
(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.

Comment 9

12 years ago
(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
(Assignee)

Comment 12

12 years ago
Hm, dragging plaintext to the tab bar background leaves the drag hilight there too.
(Assignee)

Updated

12 years ago
Assignee: mikepinkerton → sfraser_bugs
(Assignee)

Comment 13

12 years ago
Created attachment 205462 [details] [diff] [review]
Patch to make url parsing from text less strict

Comment 14

11 years ago
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
(Assignee)

Comment 15

11 years ago
(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.

Comment 17

11 years ago
(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

Comment 20

11 years ago
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

11 years ago
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

11 years ago
(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

11 years ago
(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

11 years ago
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

11 years ago
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

11 years ago
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)

Comment 27

11 years ago
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

11 years ago
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+

Updated

11 years ago
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?

Comment 30

11 years ago
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+

Updated

11 years ago
Whiteboard: [needs checkin]

Comment 32

11 years ago
Fixed trunk and branch.
Status: NEW → RESOLVED
Last Resolved: 11 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.