Adding a keyword for a search with an URL containing '?' yield invalid quicksearch bookmark

RESOLVED FIXED in Firefox 35

Status

()

defect
RESOLVED FIXED
9 years ago
5 years ago

People

(Reporter: mithi, Assigned: hussain237)

Tracking

3.6 Branch
Firefox 35
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug], URL)

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

9 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.9.2.13) Gecko/20101203 Firefox/3.6.13
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.9.2.13) Gecko/20101203 Firefox/3.6.13

When trying to set up a quicksearch bookmark for the search function of http://lyrics.wikia.com/Main_Page the resulting bookmark contains a bogus URL. The relevant part of the website is <form action="index.php?title=Special:Search" method="get">. Note the '?' in the action. Rightclicking the search text field, selecting "add a keyword for this search" from the pop-up menu and saving it yields the following bookmark: "http://lyrics.wikia.com/index.php?title=Special:Search?search=%s&fulltext=0". This fails upon use of the newly created bookmark due to the duplicate '?' in the URL. Manually replacing the second '?' with '&' yields the desired result.

Reproducible: Always

Steps to Reproduce:
1. Go to http://lyrics.wikia.com/Main_Page
2. Locate the search field to the right.
3. Rightclick the text field
4. Select "add a keyword for this search" from the pop-up menu
5. Assign a keyword
6. Click "Save"
7. Inspect the properties (in particular URL) of the newly created bookmark.
8. Use that quicksearch
Actual Results:  
7. http://lyrics.wikia.com/index.php?title=Special:Search?search=%s&fulltext=0
8. Get an error page due to the duplicate '?'.

Expected Results:  
7. http://lyrics.wikia.com/index.php?title=Special:Search&search=%s&fulltext=0
8. Actual search results.

Suggestion: Subsitute the second '?' by '&' when creating the quicksearch bookmark. I'm not familiar enough with the details of URL composition to know if a simple algorithm like "If URL already contains a '?' append '&', otherwise append '?'" is sufficient for this kind of problem or if a more elaborate approach is necessary.

Comment 1

8 years ago
I think this is an issue with Wika. Doing the same on Wikipedia http://en.wikipedia.org/wiki/Special:Search creates a correct bookmark keyword.
Component: Bookmarks & History → Search
QA Contact: bookmarks → search
The form elemens action attribute already specifies a question mark for the target: "index.php?title=Special:Search". I would assume that we do not check if a question mark is already present in the URL and add it either way. We should check if a '?' is present and use an ampersand in such a case.
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows XP → All
Hardware: x86 → All
Version: unspecified → 3.6 Branch
(Reporter)

Comment 3

8 years ago
I agree with Henrik, checking for '?' should be sufficient. In the interest of refactoring/reusing existing code: The code for building the correct URL should already be available, as using the search field manually works as intended. It just needs to be applied when a search bookmark is generated.
The problem is here, when the URL is not encoded:
http://hg.mozilla.org/mozilla-central/annotate/7f2b60765d01/browser/base/content/browser.js#l6154

> spec += "?" + formData.join("&");

We should check the value of spec first, and only appending "?" if it's really necessary. Looks like a trivial patch.
Whiteboard: [good first bug]
Posted patch Add check for "?" in form URL (obsolete) — Splinter Review
First, this is my first patch, sorry if I've done anything incorrectly.

Patch checks for "?" in URL and and creates URL with "&" instead.
Attachment #523763 - Flags: review?(hskupin)
Comment on attachment 523763 [details] [diff] [review]
Add check for "?" in form URL

Hey Matt, you probably want Gavin to review this patch instead of Henrik.
(You can see module owners and peers here: https://wiki.mozilla.org/Modules/Firefox )
Attachment #523763 - Flags: review?(hskupin) → review?(gavin.sharp)
Comment on attachment 523763 [details] [diff] [review]
Add check for "?" in form URL

Matthew: thanks for the patch! Looks good.

This method is somewhat buggy/hacky in general and could use a rewrite, but this fixes the immediate problem in the interim.

We should add a test for this - this is testable using the browser chrome test harness. The test just needs to load a page with a form whose action contains a "?", then set document.popupNode to one of that form's inputs, and then call AddKeywordForSearchField directly and check the result (it might be helpful to factor out the code that produces the spec/postData as a helper so that you could just call it and easily test the result). browser_bug423833.js is an example of a test that does similar things.

Matthew, if you wanted to take on writing the test I'd be happy to provide guidance (as I'm sure would Mehdi, or anyone else on #fx-team or #developers on IRC). Otherwise I can try to do it myself and then check this in some time soon.
Attachment #523763 - Flags: review?(gavin.sharp) → review+
I'll look into testing later this week. Mehdi has offered to assist me with that, so I'll ping him. Thanks for the pointers on how to get started, Gavin.
Whiteboard: [good first bug] → [good first bug][has review, needs test]
Matthew, would you still like to give this a try? It looks like it is super close to done.
Flags: needinfo?(mozilla)
(Assignee)

Comment 11

5 years ago
Hi 
Can you please assign thus bug to me?

Comment 12

5 years ago
There is already a patch on this bug. It may still work. Though it is likely to need to be updated to fit in the current code base (unbitrotted). The main bit here is to create a test for the patch.

https://developer.mozilla.org/en-US/docs/Browser_chrome_tests
Assignee: nobody → hussain237

Comment 13

5 years ago
Hello all. I would love to write the test for this patch, but I will need a little help with the process. Anyone willing to mentor a newbie to the project?
Flags: needinfo?(mozilla)
I may get around to this eventually but that looks unlikely
Hi, there!
Here is my attempt to fix this issue.

First, I could reproduce the situation described in the comment 0 with firefox from the trunk. However, Wikia Lyrics (http://lyrics.wikia.com/Main_Page) has updated its page and the search for does not include ? any more in its action. Nevertheless, I could reproduce this with a following, manually crafted piece of HTML.

data:text/html,<!DOCTYPE html><html lang="en-US"><head><meta charset="utf-8" /></head><body><div id="question-mark"><form method="get" action="http://www.google.com/search?oe=utf-8"><input type="text" name="q" value="Search google"></form></div><br /><div id="no-question-mark"><form method="get" action="http://www.google.com/search"><input type="text" name="q" value="Search google"></form></div></body></html>

I could not apply the previously offered patch (comment 6), because the code was relocated. I could, however, locate the functionality in browser.js and update it manually just like in the patch. Instead of creating new test as suggested in comment 8, I updated the test file that corresponds to the keyword search functions (browser_addKeywordSearch.js). I extended the test to cover the '?' use case. The tests pass on my local build (Ubuntu 14.04 x86).
Attachment #8490680 - Flags: review?(MattN+bmo)
Comment on attachment 8490680 [details] [diff] [review]
The fix and a test for handling question marks in the action attribute of a form

Review of attachment 8490680 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks! This looks good. I just have some style comments below:

::: browser/base/content/browser.js
@@ +6110,5 @@
>  
>    if (isURLEncoded)
>      postData = formData.join("&");
>    else
> +  {

The brace should go on the same line as the else: else {

See https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Control_Structures

@@ +6111,5 @@
>    if (isURLEncoded)
>      postData = formData.join("&");
>    else
> +  {
> +    // determining correct separator (bug 620565)

This comment doesn't add value over version-control blame so please remove it.

@@ +6112,5 @@
>      postData = formData.join("&");
>    else
> +  {
> +    // determining correct separator (bug 620565)
> +    var separator = (spec.indexOf("?") == -1 ? "?" : "&" );

You can now use |spec.contains("?") ? …| but note that you need to reverse the last two operands. The braces are unnecessary and you should use |let| instead of |var| for new code.

::: browser/base/content/test/general/browser_addKeywordSearch.js
@@ +34,5 @@
>        [ 'http://example.com/', 'q', 'http://example.com/?q=%s' ],
>        [ 'http://example.com/new-path-here/', 'q', 'http://example.com/new-path-here/?q=%s' ],
>        [ '', 'q', 'http://example.org/browser/browser/base/content/test/general/dummy_page.html?q=%s' ],
> +      // Tests for proper behaviour when called on a form whose action contains a question mark. (bug620565)
> +      [ 'http://example.com/search?oe=utf-8', 'q', 'http://example.com/search?oe=utf-8&q=%s' ],

Nit: I don't think the bug number adds value.
Attachment #8490680 - Flags: review?(MattN+bmo) → feedback+
Thanks for the detailed review, Metthew! Coding Style was a good reading, I wasn't aware of it before. I revised the code according to your feedback and updated the patch. Tests pass on my setup.

Are there guidelines on how to refer code to the corresponding bug numbers?
Attachment #8490680 - Attachment is obsolete: true
Attachment #8491376 - Flags: review?(MattN+bmo)
Comment on attachment 8491376 [details] [diff] [review]
The fix and a test for handling question marks in the action attribute of a form

Review of attachment 8491376 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Iaroslav Sheptykin from comment #17)
> Are there guidelines on how to refer code to the corresponding bug numbers?

Since every bug fix is required to have a bug number in the commit message, you would use something like hg annotate[2] to find out the bug number for a specific line.

Could you update your commit message to fit the conventions? (you would use "r=MattN" for the review portion). You can then attach that, mark review+ by yourself (to carry forward my r+) and then add the checkin-needed keyword.

Thanks

[1] https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#Commit_Message_Conventions
[2] http://www.selenic.com/mercurial/hg.1.html#annotate

::: browser/base/content/browser.js
@@ +6080,5 @@
>  
>    if (isURLEncoded)
>      postData = formData.join("&");
> +  else {
> +    let separator = spec.contains("?")? "&" : "?";

The space before the ? ternary operator is missing.
Attachment #8491376 - Flags: review?(MattN+bmo) → review+
Whiteboard: [good first bug][has review, needs test] → [good first bug]
https://hg.mozilla.org/integration/fx-team/rev/87b3c243b764

I ended up addressing the comments for you. Thanks for the patch. Do you need help finding another that interests you?
Flags: in-testsuite+
Whiteboard: [good first bug] → [good first bug][fixed-in-fx-team]
(In reply to Matthew N. [:MattN] from comment #19)

> I ended up addressing the comments for you. Thanks for the patch. 
Thanks, Matthew, and sorry for the trouble. I believe we are far apart, I cannot react fast because I sleep when you work. Or you don't sleep.

> Do you need help finding another that interests you?
I would love having a suggestion about the next bug. Maybe one of super-trivial ones assigned to you, which you find boring? I could work under you mentorship then. I have some experience coding in c++, php, js. I made a couple of apps for android. But I am a total noob in Mozilla codebase. I only have contributed to this bug and the bug 407821. If you recently came across anything that fits this profile and is useful for Mozilla I would appreciate a hint.
https://hg.mozilla.org/mozilla-central/rev/87b3c243b764
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug][fixed-in-fx-team] → [good first bug]
Target Milestone: --- → Firefox 35
You need to log in before you can comment on or make changes to this bug.