Closed Bug 801287 Opened 7 years ago Closed 7 years ago

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

Categories

(SeaMonkey :: Location Bar, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.17

People

(Reporter: philip.chee, Assigned: neil)

References

()

Details

Attachments

(1 file, 4 obsolete files)

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
Attached patch WIP patch. (obsolete) — Splinter Review
Attachment #676526 - Flags: feedback?(philip.chee)
Attached patch WIP patch (working) (obsolete) — Splinter Review
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.
Attachment #676526 - Attachment is obsolete: true
Attachment #677079 - Attachment is obsolete: true
Attachment #676526 - Flags: feedback?(philip.chee)
Attachment #677311 - Flags: review?(neil)
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...
Hmm, so strictly speaking we don't actually need the openUILinkIn changes because we don't call that from handleURLBarCommand()...
But it would be nice not to drift too far away from Firefox.
Attached patch Possible patch (obsolete) — Splinter Review
Attachment #684938 - Flags: review?(philip.chee)
Attachment #684938 - Flags: feedback?(ewong)
> +    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;
?
(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 :-(
(In reply to neil@parkwaycc.co.uk 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.
Attachment #684938 - Flags: feedback?(ewong) → feedback+
Assignee: ewong → neil
Attachment #677311 - Attachment is obsolete: true
Attachment #677311 - Flags: review?(neil)
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?
Attachment #684938 - Flags: review?(philip.chee) → review+
(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.
Attachment #684938 - Attachment is obsolete: true
Attachment #690191 - Flags: review?(philip.chee)
Comment on attachment 690191 [details] [diff] [review]
Addressed review comments

r=me based on the interdiff
Attachment #690191 - Flags: review?(philip.chee) → review+
Pushed comm-central changeset c169838bf614.
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.17
You need to log in before you can comment on or make changes to this bug.