Last Comment Bug 801287 - Loading URL by pressing ENTER on already present URL in location bar doesn't maintain URL encoding (Port Bug 461304)
: Loading URL by pressing ENTER on already present URL in location bar doesn't ...
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: Location Bar (show other bugs)
: Trunk
: All All
: -- normal (vote)
: seamonkey2.17
Assigned To: neil@parkwaycc.co.uk
:
:
Mentors:
http://www.stockholm.se/-/Nyheter/?q=...
Depends on: 461304
Blocks:
  Show dependency treegraph
 
Reported: 2012-10-13 06:14 PDT by Philip Chee
Modified: 2013-01-08 08:39 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
WIP patch. (2.34 KB, patch)
2012-10-30 02:40 PDT, Edmund Wong (:ewong)
no flags Details | Diff | Splinter Review
WIP patch (working) (6.08 KB, patch)
2012-10-31 11:22 PDT, Philip Chee
no flags Details | Diff | Splinter Review
Maintain URL encoding when loading url by pressing ENTER. (v1) (6.09 KB, patch)
2012-11-01 01:56 PDT, Edmund Wong (:ewong)
no flags Details | Diff | Splinter Review
Possible patch (8.18 KB, patch)
2012-11-25 03:06 PST, neil@parkwaycc.co.uk
philip.chee: review+
ewong: feedback+
Details | Diff | Splinter Review
Addressed review comments (8.55 KB, patch)
2012-12-09 07:41 PST, neil@parkwaycc.co.uk
philip.chee: review+
Details | Diff | Splinter Review

Description Philip Chee 2012-10-13 06:14:39 PDT
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) 2012-10-30 02:40:09 PDT
Created attachment 676526 [details] [diff] [review]
WIP patch.
Comment 2 Philip Chee 2012-10-31 11:22:36 PDT
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 Edmund Wong (:ewong) 2012-11-01 01:56:41 PDT
Created attachment 677311 [details] [diff] [review]
Maintain URL encoding when loading url by pressing ENTER. (v1)
Comment 4 neil@parkwaycc.co.uk 2012-11-17 16:25:06 PST
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 neil@parkwaycc.co.uk 2012-11-24 13:58:10 PST
Hmm, so strictly speaking we don't actually need the openUILinkIn changes because we don't call that from handleURLBarCommand()...
Comment 6 Philip Chee 2012-11-25 00:22:22 PST
But it would be nice not to drift too far away from Firefox.
Comment 7 neil@parkwaycc.co.uk 2012-11-25 03:06:58 PST
Created attachment 684938 [details] [diff] [review]
Possible patch
Comment 8 Philip Chee 2012-11-25 21:22:48 PST
> +    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 neil@parkwaycc.co.uk 2012-11-26 00:34:28 PST
(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) 2012-12-02 20:54:27 PST
(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.
Comment 11 Philip Chee 2012-12-03 05:41:39 PST
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 neil@parkwaycc.co.uk 2012-12-09 07:37:52 PST
(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 neil@parkwaycc.co.uk 2012-12-09 07:41:46 PST
Created attachment 690191 [details] [diff] [review]
Addressed review comments
Comment 14 Philip Chee 2012-12-10 08:03:35 PST
Comment on attachment 690191 [details] [diff] [review]
Addressed review comments

r=me based on the interdiff
Comment 15 neil@parkwaycc.co.uk 2012-12-13 14:17:15 PST
Pushed comm-central changeset c169838bf614.

Note You need to log in before you can comment on or make changes to this bug.