411165 - Pasting multiline text into the location bar ignores returns

Status: RESOLVED FIXED

(Camino Graveyard :: Location Bar & Autocomplete, defect)

Description

[Martin Girschick]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en; rv:1.8.1.11) Gecko/20071128 Camino/1.5.4
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en; rv:1.8.1.11) Gecko/20071128 Camino/1.5.4

When I copy multiline text from a PDF document within Acrobat, the pasteboard contains return characters after each line. When I paste this into the location bar these returns are simply ignored and the two adjacent words merged. Combined with keyword based searches this is quite annoying. I therefore suggest to replace return characters with spaces. As it seems, the text field of the find dialog does this.

One may argue that this is a bug on Acrobat's side as Preview.app seems to add spaces before the return character when copying text. But I guess that other applications are affected as well.

Reproducible: Always

Steps to Reproduce:
1. Open PDF in Acrobat
2. Copy two lines of text
3. Paste in Camino's location bar
Actual Results:  
The words on each side of the line break are combined.

Expected Results:  
Both words are separated by a space.

Comment 1

[Stuart Morgan]

Confirming; we may as well convert newlines and tabs to spaces, rather than stripping them out.

This is just a simple change to the paste: method of AutoCompleteTextFieldEditor (in BWC).

Comment 2

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

Someone want to tell me why 

>-      NSString *newText = [oldText stringByRemovingCharactersInSet:[NSCharacterSet controlCharacterSet]];
>+      NSString *newText = [oldText stringByReplacingCharactersInSet:[NSCharacterSet controlCharacterSet] withString:@" "];

doesn't work?

Comment 3

[Stuart Morgan]

Heh, cute. Because we implement stringByReplacingCharactersInSet:withString: with NSScanner, which skips over whitespace by default. We should probably turn that off:
  [cleanerScanner setCharactersToBeSkipped:[NSCharacterSet characterSetWithCharactersInString:@""]];

Note that we'll want to do a second pass over the paste: string with stringByTrimmingCharactersInSet: so that trailing spaces don't hork the URL, and this change should be replicated in performDragOperation:'s cleanup as well so the behavior is consistent (or, better, a little utility method should be added to the class that does the string cleanup we want, and both should use it).

Comment 4

[Chris Lawson (gone)]

Attachment: fix v1

Comment 5

[Martin Girschick]

> this change should be replicated in performDragOperation

Does this also include calls via the Service Menu ("Open URL in Camino")? Might be an idea to aggregate all these sources and pass the "raw" text to a common function. Did anyone check, how Firefox handles this?

Comment 6

[Stuart Morgan]

(In reply to comment #5)
> Did anyone check, how Firefox handles this?

To what end? We should do what is most useful, not whatever Firefox happens to do.

Comment 7

[Martin Girschick]

(In reply to comment #6)
> (In reply to comment #5)
> > Did anyone check, how Firefox handles this?
> 
> To what end?

Ah sorry, I rather meant to move this functionality to a more common part in order to be useable by other Mac and Non-Mac-Browser, although I guess that this might be too complicated.

Still, my main comment about checking the Service- (or Applescript) URL-calls as well might be worth a check. Anyway I would be absolutely satisfied if copy/paste works, that's really something, which annoys me every day :-).

Comment 8

[Chris Lawson (gone)]

(In reply to comment #5)
> > this change should be replicated in performDragOperation
> 
> Does this also include calls via the Service Menu ("Open URL in Camino")?

It doesn't, but I also don't think the Services menu has this problem.

I can't seem to test it to determine whether it does for certain, because LaunchServices seems to think that Camino 0.8.4 is the only copy of Camino on my system as far as the Services menu is concerned, but looking at the code, I don't think this would be an issue.

Comment 9

[Chris Lawson (gone)]

If you find that the Services menu *does* have this problem, by the way, please file a separate bug on that and I'll fix it too.

Comment 10

[Martin Girschick]

(In reply to comment #9)
> If you find that the Services menu *does* have this problem, by the way, please
> file a separate bug on that and I'll fix it too.

As the service menu (correctly) reads "Open URL in Camino", I think the current behavior is okay, as it simply removes any returns without replacing them. The "Search with Google" Service entry seems to be hardwired to Safari, so it doesn't work with Camino anyway. 

Comment 11

[Chris Lawson (gone)]

Attachment: fix v1.01

Minor typo fix from previous patch, r=hendy on IRC.

Comment 12

[Stuart Morgan]

Comment on attachment 333840 [details] [diff] [review]
fix v1.01

>+-(NSString*)stringByCleaningAndTrimmingControlCharacters
>+{
>+  return [[self stringByReplacingCharactersInSet:[NSCharacterSet controlCharacterSet] withString:@" "] stringByTrimmingWhitespace];
>+}

What is the purpose of this method? Replacing a single line that has a clear effect with a shorter line doesn't seem worth making a new method; it's not abstracting a concept.

If it stays it really needs a new name. It doesn't clean and trim control characters, as the name says; it cleans control characters and trims whitespace. Also, I would have no idea that "cleaning" means replacing with a space without reading the implementation. I think the difficulty of giving this a name that's much shorter than the implementation line is another argument against having it.

Comment 13

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

The flip side of this change (I think) is that if I'm pasting a URL which has been broken up from somewhere else, then I end up with a broken URL space in the middle of it, whereas before I'd end up with a valid URL.

I'm not sure which case is more common or would annoy people most, but given that it's a *location* bar, I think we should prefer producing valid URLs on pasting if we can't adequately distinguish between types of pastes (no matter how much this bug annoys me when I want to use a copied 3-line address with a bookmark shortcut).

Comment 14

[Stuart Morgan]

We could try some heuristics; something along the lines of:
1) if it starts (after whitespace trimming) with [a-zA-Z]://, it's a URL; eat space
2) if a space is just after a '/', assume it's a broken URL, and eat spaces
3) like 2, but after '-'. Similarly with any other common break point in URL wrapping that's very unlikely to be the last character of a shortcut

Alternately, we could try to resolve anything with a space in it as a shortcut, and remove the spaces if nothing comes back. That means we'll double-resolve pasted shortcuts (on paste, and on submit), but that shouldn't actually matter.

Comment 15

[Chris Lawson (gone)]

(In reply to comment #13)
> The flip side of this change (I think) is that if I'm pasting a URL which has
> been broken up from somewhere else, then I end up with a broken URL space in
> the middle of it, whereas before I'd end up with a valid URL.

Right.

I like Stuart's second idea, possibly combined with the first heuristic, since "://" shouldn't be allowed in shortcuts anyway. (I know we currently do, but that doesn't seem to be something people are likely to put in shortcuts, nor does it seem like something they ought to be putting in shortcuts.)

cl

Comment 16

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

(In reply to comment #14)
> Alternately, we could try to resolve anything with a space in it as a shortcut,
> and remove the spaces if nothing comes back. That means we'll double-resolve
> pasted shortcuts (on paste, and on submit), but that shouldn't actually matter.

If I understand this correctly, doing this would break my most common use-case that hits this bug as filed, which is pasting a multi-line postal address from a non-editable location into the location bar (either with my shortcut already sitting in the bar, or going back to add the shortcut in front of it after pasting).  If we try to resolve my pasteboard (a postal address), see that it doesn't match a shortcut, then stip the spaces, I still end up with this bug (a postal address all run together).

The heuristics sound reasonable, though.

Comment 17

[Stuart Morgan]

True; we'd probably need to figure out what the whole string would be after pasting, run the heuristic on that, then handle the pasted text accordingly.

Comment 18

[Chris Lawson (gone)]

Attachment: fix v1.1

Addresses sr comments, re-requesting review as a sanity check.

Comment 19

[Stuart Morgan]

Comment on attachment 335903 [details] [diff] [review]
fix v1.1

>+  NSString* cleanString = [aString stringByTrimmingCharactersInSet:[NSCharacterSet whitespaceAndNewlineCharacterSet]];

Since you are just looking for :// anywhere, you don't need to trim before the "we think it's a URL case", since those characters will be removed anyway. Move this to inside the else block.

>+  if ([cleanString rangeOfString:@"://"].location != NSNotFound) // we assume anything containing "://" is a URL and eat spaces
>+    cleanString = [cleanString stringByRemovingCharactersInSet:whitespaceIllegalAndControlCharacterSet];
>+
>+  else // it's not a URL, so replace illegals/control characters with spaces and leave existing spaces alone  
>+    cleanString = [cleanString stringByReplacingCharactersInSet:illegalAndControlCharacterSet withString:@" "];

Don't separate |if| and |else| blocks with horizontal whitespace. Also, there's enough comment tet that it's crowded to put inline like that, so pull them up into one comment block before the whole if/else that explains what you are doing--and the "why"; it's easy not to think of bookmarklets when looking at this code and wonder why we would ever leave spaces.

sr=smorgan with those changes.

Comment 20

[Chris Lawson (gone)]

Attachment: fix v.1.11

sr+ comments addressed, ready for the chickenwranglers.

Comment 21

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

Landed on cvs trunk.