801287 - Loading URL by pressing ENTER on already present URL in location bar doesn't maintain URL encoding (Port Bug 461304)

Status: RESOLVED FIXED

(SeaMonkey :: Location Bar, defect)

Description

[Philip Chee]

From Bug 461304 Comment 0:

> When you load a page in Firefox the location bar makes some magic and "decodes" 
> the URL, making %c3%b6 for instance appearing as "ö". All this is fine, but when 
> I just go to the location bar and press ENTER the encoding gets screwed up.
> 
> What I think Firefox does wrong: 
> It doesn't remember what encoding the page loaded had (UTF-8) and therefore just 
> encodes it as ISO-8859-1 or something like this. It should "remember" the 
> encoding.
> 
> Reproducible: Always
> 
> Steps to Reproduce:
> 1.Load the URL in the URL field, see that the textbox to the right contains 
> "höjning"
> 2.Go to the location bar and press ENTER
> 3.See how the text box changes to h�jning
> Actual Results:  
> The URL encoding changed
> 
> Expected Results:  
> The URL encoding stays the same

Comment 1

[Edmund Wong (:ewong)]

Attachment: WIP patch.

Comment 2

[Philip Chee]

Attachment: WIP patch (working)

OK I've managed to get it working:
You need to port more stuff than just the tabbrowser.xml changes:

1. The Firefox urlbarBindings.xml->handleCommand()
Corresponds to our navigator.js->handleURLBarCommand()

2. Copy the changes in the Firefox utilityOverlay.js->openLinkIn()
to our utilityOverlay.js->openUILinkIn()
[Firefox split their openUILinkIn() into two parts with the new part called openLinkIn() which we haven't done.

Comment 3

[Edmund Wong (:ewong)]

Attachment: Maintain URL encoding when loading url by pressing ENTER. (v1)

Comment 4

[[e-mail]]

Comment on attachment 677311 [details] [diff] [review]
Maintain URL encoding when loading url by pressing ENTER. (v1)

>   var w = getTopWin();
>+  var browser = w.getBrowser();
This is in the wrong place.

>-    w.loadURI(url, aReferrerURI, aPostData, aAllowThirdPartyFixup);
I wonder whether we should add an additional parameter here...

Comment 5

[[e-mail]]

Hmm, so strictly speaking we don't actually need the openUILinkIn changes because we don't call that from handleURLBarCommand()...

Comment 6

[Philip Chee]

But it would be nice not to drift too far away from Firefox.

Comment 7

[[e-mail]]

Attachment: Possible patch

Comment 8

[Philip Chee]

> +    if (isUTF8)
> +      flags |= nsIWebNavigation.LOAD_FLAGS_URI_IS_UTF8;
>      if (allowThirdPartyFixup) {
>        flags = nsIWebNavigation.LOAD_FLAGS_ALLOW_THIRD_PARTY_FIXUP;
Shouldn't this be:
       flags != nsIWebNavigation.LOAD_FLAGS_ALLOW_THIRD_PARTY_FIXUP;
?

Comment 9

[[e-mail]]

(In reply to Philip Chee from comment #8)
> > +    if (isUTF8)
> > +      flags |= nsIWebNavigation.LOAD_FLAGS_URI_IS_UTF8;
> >      if (allowThirdPartyFixup) {
> >        flags = nsIWebNavigation.LOAD_FLAGS_ALLOW_THIRD_PARTY_FIXUP;
> Shouldn't this be:
>        flags != nsIWebNavigation.LOAD_FLAGS_ALLOW_THIRD_PARTY_FIXUP;
> ?

Yeah, I originally had the lines in reverse order and forgot to switch the operators :-(

Comment 10

[Edmund Wong (:ewong)]

(In reply to [e-mail] from comment #9)
> (In reply to Philip Chee from comment #8)
> > > +    if (isUTF8)
> > > +      flags |= nsIWebNavigation.LOAD_FLAGS_URI_IS_UTF8;
> > >      if (allowThirdPartyFixup) {
> > >        flags = nsIWebNavigation.LOAD_FLAGS_ALLOW_THIRD_PARTY_FIXUP;
> > Shouldn't this be:
> >        flags != nsIWebNavigation.LOAD_FLAGS_ALLOW_THIRD_PARTY_FIXUP;
> > ?
> 
> Yeah, I originally had the lines in reverse order and forgot to switch the
> operators :-(

With this changed to |flags != nsIWebNavigation.LOAD_FLAGS_ALLOW_THIRD_PARTY_FIXUP;|,
the patch works.

so fb+ with this change.

Comment 11

[Philip Chee]

Comment on attachment 684938 [details] [diff] [review]
Possible patch

> With this changed to |flags != nsIWebNavigation.LOAD_FLAGS_ALLOW_THIRD_PARTY_FIXUP;|,
> the patch works.
I meant of course flags |= ...
Also perhaps you could arrange the flags here in the same order as in tabbrowser.xml.

r=with this fixed.

I noticed that in openUILinkIn() you didn't address this part:

>   if (!w || where == "window") {
>     return window.openDialog(getBrowserURL(), "_blank", "chrome,all,dialog=no", url,
>                              null, null, aPostData, aAllowThirdPartyFixup);
Is it not needed here?

Comment 12

[[e-mail]]

(In reply to Philip Chee from comment #11)
> > With this changed to |flags != nsIWebNavigation.LOAD_FLAGS_ALLOW_THIRD_PARTY_FIXUP;|,
> > the patch works.
> I meant of course flags |= ...
> Also perhaps you could arrange the flags here in the same order as in
> tabbrowser.xml.
Actually I need to rewrite that block, it doesn't do what I was intending.

> I noticed that in openUILinkIn() you didn't address this part:
> >   if (!w || where == "window") {
> >     return window.openDialog(getBrowserURL(), "_blank", "chrome,all,dialog=no", url,
> >                              null, null, aPostData, aAllowThirdPartyFixup);
> Is it not needed here?
Oops, I overlooked it, thanks for spotting it.

Comment 13

[[e-mail]]

Attachment: Addressed review comments

Comment 14

[Philip Chee]

Comment on attachment 690191 [details] [diff] [review]
Addressed review comments

r=me based on the interdiff

Comment 15

[[e-mail]]

Pushed comm-central changeset c169838bf614.