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

RESOLVED FIXED in seamonkey2.17

Status

SeaMonkey
Location Bar
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: Philip Chee, Assigned: neil@parkwaycc.co.uk)

Tracking

Trunk
seamonkey2.17

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

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

5 years ago
Created attachment 676526 [details] [diff] [review]
WIP patch.
Attachment #676526 - Flags: feedback?(philip.chee)
(Reporter)

Comment 2

5 years ago
Created attachment 677079 [details] [diff] [review]
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

5 years ago
Created attachment 677311 [details] [diff] [review]
Maintain URL encoding when loading url by pressing ENTER. (v1)
Attachment #676526 - Attachment is obsolete: true
Attachment #677079 - Attachment is obsolete: true
Attachment #676526 - Flags: feedback?(philip.chee)
Attachment #677311 - Flags: review?(neil)
(Assignee)

Comment 4

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

Comment 5

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

Comment 6

5 years ago
But it would be nice not to drift too far away from Firefox.
(Assignee)

Comment 7

5 years ago
Created attachment 684938 [details] [diff] [review]
Possible patch
Attachment #684938 - Flags: review?(philip.chee)
Attachment #684938 - Flags: feedback?(ewong)
(Reporter)

Comment 8

5 years ago
> +    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;
?
(Assignee)

Comment 9

5 years ago
(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.

Updated

5 years ago
Attachment #684938 - Flags: feedback?(ewong) → feedback+

Updated

5 years ago
Assignee: ewong → neil

Updated

5 years ago
Attachment #677311 - Attachment is obsolete: true
Attachment #677311 - Flags: review?(neil)
(Reporter)

Comment 11

5 years ago
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+
(Assignee)

Comment 12

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

Comment 13

5 years ago
Created attachment 690191 [details] [diff] [review]
Addressed review comments
Attachment #684938 - Attachment is obsolete: true
Attachment #690191 - Flags: review?(philip.chee)
(Reporter)

Comment 14

5 years ago
Comment on attachment 690191 [details] [diff] [review]
Addressed review comments

r=me based on the interdiff
Attachment #690191 - Flags: review?(philip.chee) → review+
(Assignee)

Comment 15

5 years ago
Pushed comm-central changeset c169838bf614.
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

4 years ago
Target Milestone: --- → seamonkey2.17
You need to log in before you can comment on or make changes to this bug.