When searching, spaces are replaced with "+" instead of "%20"

[alex-gunning]

Describe the bug

As per the title, spaces are replaced with "+" while some websites require "%20" for space delimiters.

One example is https://www.rottentomatoes.com/search?search=alice%20in%20wonderland

To Reproduce

1. Configure vimium custom search engine with

   r: https://www.rottentomatoes.com/search?search=%s Rotten Tomatoes

2. Search Rottentomatoes with "Alice in Wonderland"

Incorrect results given.

Brave Browser 1.8.90 and Vimium 1.66

I have a branch with a working fix for this already if this is something you guys would like addressed :)

Edit: Created a PR

[gdh1995]

Maybe we can make %S join query strings with <space>, instead of JavaScript's <comma>.

[alex-gunning]

I'm not sure I follow. It currently joins them with a +. I made a pull request to create a new %su option to allow for spaces to be rewritten with %20

[gdh1995]

%s uses +, while %S (upper-case of "S") means to skip encodeURIComponent and join queries using Array.prototype.toString.

I think the current behavior of joining using Array::toString is just a typo bug and we can make %S join with <space> instead. Then Vimium will send http://foo.com/?q=a b c to a browser, and the browser will auto encode <space>s with %20.

Added:

Of course my idea of modifying behavior of "%S" may cause a web server gets wrong queries, if a query includes +, ? or some other special punctuation characters (e.g.: a query of 1?abc=2).

While your %su might somehow break some old search engine rules of other users, which works well up to now.

[alex-gunning]

Aah, okay. I see what you're saying now. Yes, indeed the <space> makes a lot more sense.

[alex-gunning]

I relooked at this again and updated my PR and I now have it encoding %S with an encodeURL. This seems to be giving the correct results given the unit test description in utils.test.js but still encoding <space> correctly.

should("replace %S without encoding", () => {
    assert.equal("https://www.github.com/philc/vimium/pulls", Utils.createSearchUrl("vimium/pulls", "https://www.github.com/philc/%S"))
}),

(maintaining special characters - see how it maintains the /)

I tried sending the URL to the browser with <space> as per your suggestion but Chrome seems to encodeURIComponent and use it as a search query to Google - which is what we're currently seeing when using %S.

[gdh1995]

Um, it's a little strange for me. chrome.tabs.create({url: 'https://www.google.com/search?ie=utf-8&q=t1 t2' }) will make Chrome open https://www.google.com/search?ie=utf-8&q=t1%20t2 as I expect.

Maybe Vimium does further translations in some other places, like convertToUrl.

[alex-gunning]

My apologies for the last comment. I was wrong about the browser encoding it automatically (I did not read further in the codebase)

The problem lies in utils.js line 225

 ...
 } else if (Utils.isUrl(string)) {
      return Utils.createFullUrl(string);
 } else {
     return Utils.createSearchUrl(string);
 }

and line 122

// Tries to detect if :str is a valid URL.
isUrl(str) {
  // Must not contain spaces
  if (str.includes(' ')) { return false; }
  ...

It thinks (correctly) that the spaces constitute a non-url and so chooses the (incorrect) createSearchUrl path.

Since we're using language like isUrl, I think we should actually have a valid URL by this point and with the surrounding language, my interpretation leads me to believe that the encoding should be done from within Vimium. This would let allow the Utils.createFullUrl branch to be selected correctly.

What do you think?

[gdh1995]

The current commit of the PR is also enough, of course.

Just for your Information: my customized extension, Vimium C, will split the text, get the part before a first space, and then use it to check whether the whole string is a valid URL.

---Original--- From: "Alex Gunning"<notifications@github.com> Date: Wed, Jun 17, 2020 23:09 PM To: "philc/vimium"<vimium@noreply.github.com>; Cc: "Comment"<comment@noreply.github.com>;"Dahan Gong"<gdh1995@qq.com>; Subject: Re: [philc/vimium] When searching, spaces are replaced with "+" instead of "%20" (#3583)

My apologies for the last comment. I was wrong about the browser encoding it automatically (I did not read further in the codebase)

The problem lies in utils.js line 225 ... } else if (Utils.isUrl(string)) { return Utils.createFullUrl(string); } else { return Utils.createSearchUrl(string); }
and line 122 // Tries to detect if :str is a valid URL. isUrl(str) { // Must not contain spaces if (str.includes(' ')) { return false; } ...
It thinks (correctly) that the spaces constitute a non-url and so chooses the (incorrect) createSearchUrl path.

Since we're using language like isUrl, I think we should actually have a valid URL by this point and with the surrounding language, my interpretation leads me to believe that the encoding should be done from within Vimium. This would let allow the Utils.createFullUrl branch to be selected correctly.

What do you think?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe.

[alex-gunning]

Yeah I think the current PR should be fine.

I will keep :eyes: on that fork.