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.

I'll do A).

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.

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

-> comment 3

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?

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

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

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

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

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

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

(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 on attachment 280535 [details] [diff] [review] patch v3 patch doesn't apply anymore

(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 on attachment 299806 [details] [diff] [review] patch a1.9+=damons

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

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