857963 - The search engine added from Fandango.com does not work

Status: RESOLVED INCOMPLETE

(Firefox for Android Graveyard :: General, defect)

Description

[Adrian Tamas (:AdrianT)]

Nightly 23.0a1 2013-04-03/ Aurora 22.0a2 2013-04-02
Acre Iconia Tab A500 (Android 3.2.1)

Steps to reproduce:
1. Go to fandango.com continue to the website
2. Long tap on the search field and choose to add as search engines
3. Open a new tab and try to search for a movie using the new fandango search engine

Expected results:
The results for the search are displayed on fandango.com

Actual results:
The fandango page is opened but no search is performed

Comment 1

[Aaron Train [:aaronmt]]

This is likely a site redirect issue. I can reproduce the same issue on desktop Firefox when adding the search engine via keyword. I get redirected back on attempt to search.

Comment 2

[Lawrence Mandel [:lmandel] (use needinfo)]

I wouldn't be so quick to put this on Fandango. Presumably, the way that Firefox pulls out a search engine is not a standard. Perhaps there is a case here that Firefox should recognize when adding a search engine. (i.e. Perhaps this is on us.)

Comment 3

[:Margaret Leibovic]

I'm finding the same issue happens on desktop if you go to http://mobile.fandango.com. It's probably an issue with the way we hack together the search engine parameters from the DOM (unlike search plugins, this is something that we put together ourselves).

Comment 4

[Hemantha Holla]

The 'Go' button in mobile.fandango.com itself adds a parameter - 'submit-searchresults-SEARCH' - apart from the 'QUERY' param added by the search bar and another <input type='hidden'> param.  The page in question seems to have multiple forms targeting the same action URL and they seem to decide on appropriate content based on which submit button was pressed. As the previous comment suggested, the logic that puts together query URL fails to pick up parameters added by buttons resulting in an incomplete request to the server.

GetSearchFieldBookmarkData() in browser/base/content/browser.js, called when adding a keyword search, looks only at specific input elements (textarea,hidden,radio,checkbox and select) to pick form data. It doesn't examine buttons. But since the HTML spec does include buttons in 'successful controls' i.e those elements whose name,value pair gets sent on form submission (http://www.w3.org/TR/html401/interact/forms.html#successful-controls), it seems the fandango page is within accepted standards.

Comment 5

[Hemantha Holla]

Attachment: Pick up name/value pair from buttons too when adding keyword searches

Small patch to add parameters from button elements to search queries constructed for keyword searches. 

In cases where the form has multiple buttons (as in fandango's case and even Google's), only first one's name and value needs to be picked up since it is considered the default (HTML5 makes it explicit - http://stackoverflow.com/a/925353). Additional check for !el.disabled isn't necessary for this bug, but avoids adding empty name-value strings (like 'q=%s&=&=&param=value' - which happens with Google since the page includes two disabled text inputs)

Comment 6

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 810532 [details] [diff] [review]
Pick up name/value pair from buttons too when adding keyword searches

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

::: browser/base/content/browser.js
@@ +6155,5 @@
>            formData.push(escapeNameValuePair(el.name, el.options[j].value,
>                                              isURLEncoded));
>        }
> +    } else if (!seenSubmitButton && 
> +        (type == "submit" || el instanceof HTMLButtonElement)) {

drive-by comment: is the instanceof HTMLButtonElement check needed? i thought <button> elements don't default to be submit buttons unless they have type="submit", so your type == "submit" check should catch those cases too.

Comment 7

[Hemantha Holla]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #6)

> drive-by comment: is the instanceof HTMLButtonElement check needed? i
> thought <button> elements don't default to be submit buttons unless they
> have type="submit", so your type == "submit" check should catch those cases
> too.

Right, HTMLButtonElement check shouldn't be necessary.

Comment 8

[Wesley Johnston (:wesj)]

Comment on attachment 810532 [details] [diff] [review]
Pick up name/value pair from buttons too when adding keyword searches

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

This is the desktop browser.js and won't have any effect on Firefox for Android.  Android does this here: http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#6607

Comment 9

[BMO Automation]

We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.