394667 - Copying javascript: URL from location bar replaces spaces with %20

Status: VERIFIED FIXED

(Firefox :: Address Bar, defect)

Description

[Jesse Ruderman]

Steps to reproduce:
1. Type (or paste) into the address bar:
     javascript:alert(1 + 2)
2. Cmd+A
3. Cmd+C

Result: what's copied to the clipboard has %20 in place of spaces.

This is annoying because when I start writing a bookmarklet, I often type a bit into the address bar and copy it for a "backup" just before executing it.  If it includes %20 instead of spaces, it's much harder to read and edit.

Possible fixes:
A) Leave javascript: and data: URLs alone when copying.
B) Leave URLs being typed (as opposed to URLs of pages being displayed) alone when copying.

Comment 1

[Dão Gottwald [:dao]]

I'll do A).

Comment 2

[Dão Gottwald [:dao]]

Attachment: patch

Sigh. Fixing the copy command handler wasn't enough. nsBrowserStatusHandler.onLocationChange cardinally assigns encoded addresses to the urlbar, so data: URIs mustn't be excluded from decoding -- that doesn't apply to javascript:, as it doesn't result in a location change.

Comment 3

[Jesse Ruderman]

javascript: URLs do result in a location change if the last statement's value is not |undefined|.

Example:

javascript:1 + 1;

Comment 4

[Dão Gottwald [:dao]]

Attachment: patch v2

-> comment 3

Comment 5

[:Gavin Sharp [email: [e-mail]]]

I'm not sure I understand the patch. The first two hunks don't seem to make any functional difference. Am I missing something? (Also don't know why you're nesting try/catch blocks that don't actually handle any exceptions).

I also don't like hard-coding schemes into the location bar binding like this. Why are data/javascript special? Because users frequently type them in and edit them directly?

Comment 6

[Dão Gottwald [:dao]]

(In reply to comment #5)
> I'm not sure I understand the patch. The first two hunks don't seem to make any
> functional difference. Am I missing something?

The current situation is: If _uri.host is not defined, _uri is set to null, which prevents _prettyView from handling those addresses.
Now I want to decode all valid URIs (not only those that have a host) and I also want to use _uri in doCommand. So instead of setting it to null, I save the URI-has-host state and access it in _prettyView.

> (Also don't know why you're
> nesting try/catch blocks that don't actually handle any exceptions).

Um ... newURI can throw and I actually count on .host throwing.

> I also don't like hard-coding schemes into the location bar binding like this.
> Why are data/javascript special? Because users frequently type them in and edit
> them directly?

Yes.

Comment 7

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #6)
> > (Also don't know why you're
> > nesting try/catch blocks that don't actually handle any exceptions).
> 
> Um ... newURI can throw and I actually count on .host throwing.

Yeah, but why two nested try/catch blocks instead of one?

Comment 8

[Dão Gottwald [:dao]]

Oh, yes, that doesn't make a difference. Anything else that needs an update?

Comment 9

[:Gavin Sharp [email: [e-mail]]]

I don't see anything offhand, I'm leaving now though so I won't be able to finish the review tonight.

Comment 10

[Dão Gottwald [:dao]]

Attachment: patch v3

Comment 11

[Jesse Ruderman]

I think we should also do

B) Leave URLs being typed (as opposed to URLs of pages being displayed) alone
when copying.

Yesterday I put something like "ASSERTION: grandparent should be root box" into the address bar temporarily.  (Maybe I did this to check what was in my clipboard, I'm not sure).  Later, I found that the text on my clipboard had been changed to "assertion:%20grandparent%20should%20be%20root%20box".

Comment 12

[Dão Gottwald [:dao]]

(In reply to comment #11)
> I think we should also do
> 
> B) Leave URLs being typed (as opposed to URLs of pages being displayed) alone
> when copying.
> 
> Yesterday I put something like "ASSERTION: grandparent should be root box" into
> the address bar temporarily.

Some thoughts:

1. The real problem here is that newURI takes that as a valid URI, which seems to be a rare edge case, i.e. your random string has to match /^[a-z]+:/.
2. It's not the task of the Location Bar to act as a clipboard for strings that look like URIs but actually aren't.
3. It's not obvious to me how a sane fix for this would look like. For example, autocompleting also dispatches an "input" event.

All in all, I don't think it's worthwhile.

Comment 13

[Dão Gottwald [:dao]]

(In reply to comment #12)
> 3. It's not obvious to me how a sane fix for this would look like. For example,
> autocompleting also dispatches an "input" event.

Also, if I paste one of the famous unreadable Wikipedia URLs into the Location Bar, it certainly shouldn't be left alone. So we'd probably need a scheme whitelist, which doesn't sound like a great solution to me either.

Comment 14

[Dão Gottwald [:dao]]

Comment on attachment 280535 [details] [diff] [review]
patch v3

patch doesn't apply anymore

Comment 15

[Dão Gottwald [:dao]]

(In reply to comment #12)
> (In reply to comment #11)
> > I think we should also do
> > 
> > B) Leave URLs being typed (as opposed to URLs of pages being displayed) alone
> > when copying.
> > 
> > Yesterday I put something like "ASSERTION: grandparent should be root box" into
> > the address bar temporarily.
> 
> 3. It's not obvious to me how a sane fix for this would look like. For example,
> autocompleting also dispatches an "input" event.

We now have the pageproxystate attribute on the location bar, which we can use.

Comment 16

[Dão Gottwald [:dao]]

Attachment: patch

Comment 17

[Damon Sicore (:damons)]

Comment on attachment 299806 [details] [diff] [review]
patch

a1.9+=damons

Comment 18

[Reed Loden [:reed]]

Checking in browser/base/content/urlbarBindings.xml;
/cvsroot/mozilla/browser/base/content/urlbarBindings.xml,v  <--  urlbarBindings.xml
new revision: 1.53; previous revision: 1.52
done

Comment 19

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED using:

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9pre) Gecko/2008050806 Minefield/3.0pre

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9pre) Gecko/2008050804 Minefield/3.0pre

-and-

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9pre) Gecko/2008050804 Minefield/3.0pre