Browser-laptop: spaces in URLs

[LaurenWags]

• Did you search for similar issues before submitting this one?
  Yes

• Describe the issue you encountered:
  When opening a php link from http://prototiposis.com/hackers/ brave is removing the '%20' and showing spaces in the URL bar. In Brave, this link works fine. However, if you copy the URL (with spaces) an paste it elsewhere (like facebook), the link no longer works because the %20 is missing.

• Platform (Win7, 8, 10? macOS? Linux distro?):
  macOS

• Brave Version (revision SHA):
  Brave | 0.18.14
  rev | ad92d02
  Muon | 4.3.6

• Steps to reproduce:

  1. Navigate to http://prototiposis.com/hackers/
  2. Click on a link.
  3. Spaces display in the URL bar.
  4. Copy this URL from the URL bar and try to post it in Facebook, link is not recognized.

• Actual result:
  %20s are stripped out so you cannot copy this link into facebook.

• Expected result:
  %20s are not stripped out (this is how Chrome works)

• Will the steps above reproduce in a fresh profile? If not what other info can be added?
  Yes

• Is this an issue in the currently released version?
  Yes

• Can this issue be consistently reproduced?

• Extra QA steps:

  1. Navigate to http://prototiposis.com/hackers/
  2. Hover over a link, you can see that the URL contains %20 for the spaces.
  3. Right click on the link, copy link address.
  4. Paste link into URL bar, %20s are shown.
  5. Hit Enter. %20s are stripped out, spaces display in URL.

• Screenshot if needed:
  

• Any related issues:

[luixxiul]

I noticed it as well when opening https://github.com/brave/browser-laptop/issues?utf8=✓&q=is%3Aissue is%3Aopen label%3Aperf

STR:

1. Open the issue tab above
2. Search is:issue is:open label:bug
3. Copy the URL
4. Open a new tab
5. Paste and go

Actual result: search result of the URL is displayed

Expected result: the URL should be directly opened

[luixxiul]

I moved the milestone just in case as this is a regression of the feature that should always pass through general regression tests.

[diracdeltas]

related issue: if you type 'http://google.com/ abc' into the URL bar, it will go to the URL 'http://google.com/%20abc' instead of interpreting it as a search query like it used to.

[bsclifton]

Findings so far (having a really hard time narrowing this down):

• 0.17.19 works as expected
• all commits in 0.18.x have the issue
• 0.19.x also has the issue

I think 0.17.x received a patch which was not copied to 0.18/0.19. I'm doing a diff now to try and find it

[bsclifton]

I believe I found the source of the problem. It seems to be this commit:
https://github.com/brave/browser-laptop/commit/b332d8e69f987a9e3e82c1334299446b2e92e0dc#diff-7b5b041c814805dd8496488bc6a53d3cL343

Specifically the replaced code doesn't do the same thing as urlUtil.getDisplayLocation (which was removed).

I was able to "fix" the behavior by applying this patch:

diff --git a/app/browser/tabs.js b/app/browser/tabs.js
index a2d897312..fdbb7f681 100644
--- a/app/browser/tabs.js
+++ b/app/browser/tabs.js
@@ -24,6 +24,7 @@ const appStore = require('../../js/stores/appStore')
 const appConfig = require('../../js/constants/appConfig')
 const siteTags = require('../../js/constants/siteTags')
 const {newTabMode} = require('../common/constants/settingsEnums')
+const urlFormat = require('url').format
 const {cleanupWebContents, currentWebContents, getWebContents, updateWebContents} = require('./webContentsCache')
 const {FilterOptions} = require('ad-block')
 const {isResourceEnabled} = require('../filtering')
@@ -320,7 +321,7 @@ const fixDisplayURL = (navigationEntry, controller) => {
     navigationEntry.virtualURL = ''
   }

-  navigationEntry.virtualURL = muon.url.formatForDisplay(navigationEntry.virtualURL)
+  navigationEntry.virtualURL = urlFormat(muon.url.formatForDisplay(navigationEntry.virtualURL))

   const parsedURL = muon.url.parse(navigationEntry.virtualURL)
   navigationEntry = Object.assign(parsedURL, navigationEntry)

@bridiver can you help me look at this? I believe the problem is that muon.url.formatForDisplay is not performing URL encoding

[bsclifton]

Code that potentially needs updating:
https://github.com/brave/muon/blob/master/brave/common/extensions/url_bindings.cc#L312-L314

[bridiver]

if that is the behavior that we want then change net::UnescapeRule::SPACES to net::UnescapeRule::NONE, but it's probably better to add a parameter for it so we can pass whatever we want (default to NONE)

[bsclifton]

Awesome, thanks @bridiver! I've got a fix- just need to finish compiling Muon to test it and will PR

[bsclifton]

Fixed with https://github.com/brave/muon/pull/274

This won't be available until we get a new Muon build (4.3.10)