Pasting multiline text into the location bar ignores returns

RESOLVED FIXED

Status

Camino Graveyard
Location Bar & Autocomplete
--
minor
RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: Martin Girschick, Assigned: Chris Lawson (gone))

Tracking

Details

(Whiteboard: [good first bug])

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

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

11 years ago
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).
Status: UNCONFIRMED → NEW
Ever confirmed: true
Someone want to tell me why 

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

doesn't work?

Comment 3

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

Comment 4

10 years ago
Created attachment 333584 [details] [diff] [review]
fix v1
Assignee: nobody → cl-bugs-new
Status: NEW → ASSIGNED
Attachment #333584 - Flags: review?(murph)
(Assignee)

Updated

10 years ago
Hardware: Macintosh → All
(Reporter)

Comment 5

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

10 years ago
(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.
(Reporter)

Comment 7

10 years ago
(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 :-).
(Assignee)

Comment 8

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

Comment 9

10 years ago
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.
(Reporter)

Comment 10

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

Updated

10 years ago
Attachment #333584 - Flags: review?(murph) → review?(trendyhendy2000)
(Assignee)

Comment 11

10 years ago
Created attachment 333840 [details] [diff] [review]
fix v1.01

Minor typo fix from previous patch, r=hendy on IRC.
Attachment #333584 - Attachment is obsolete: true
Attachment #333840 - Flags: superreview?(mikepinkerton)
Attachment #333840 - Flags: review+
Attachment #333584 - Flags: review?(trendyhendy2000)

Comment 12

10 years ago
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.
Attachment #333840 - Flags: superreview?(mikepinkerton) → superreview-
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

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

Comment 15

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

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

Comment 18

10 years ago
Created attachment 335903 [details] [diff] [review]
fix v1.1

Addresses sr comments, re-requesting review as a sanity check.
Attachment #333840 - Attachment is obsolete: true
Attachment #335903 - Flags: superreview?(stuart.morgan+bugzilla)
Attachment #335903 - Flags: review?(trendyhendy2000)
(Assignee)

Updated

10 years ago
Blocks: 452677

Updated

10 years ago
Attachment #335903 - Flags: review?(trendyhendy2000) → review+

Updated

10 years ago
Attachment #335903 - Flags: superreview?(stuart.morgan+bugzilla) → superreview+

Comment 19

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

Comment 20

10 years ago
Created attachment 336424 [details] [diff] [review]
fix v.1.11

sr+ comments addressed, ready for the chickenwranglers.
Attachment #335903 - Attachment is obsolete: true
Landed on cvs trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED

Updated

10 years ago
Depends on: 453623
(Assignee)

Updated

10 years ago
No longer depends on: 453623
You need to log in before you can comment on or make changes to this bug.