The default bug view has changed. See this FAQ.

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

4 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

4 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

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

Comment 7

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

Comment 8

4 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

4 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

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

Updated

4 years ago
Assignee: ewong → neil

Updated

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

Comment 11

4 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

4 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

4 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

4 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

4 years ago
Pushed comm-central changeset c169838bf614.
Status: ASSIGNED → RESOLVED
Last Resolved: 4 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.