Search engine "ask.com" couldn't be added to the existing list of search engines.

VERIFIED FIXED in Firefox 54

Status

()

Firefox
Search
P3
normal
VERIFIED FIXED
2 years ago
3 months ago

People

(Reporter: Ovidiu, Assigned: standard8)

Tracking

44 Branch
Firefox 54
Other
Windows 10
Points:
---

Firefox Tracking Flags

(firefox44 affected, firefox54 verified)

Details

(Whiteboard: [fxsearch])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Reporter)

Description

2 years ago
Reproduce this bug on Windows 10 x64 with Firefox latest nightly 44.0.a1
and also Windows 7 professional x64 with Firefox 41.0.2


Steps to reproduce:

1 Open Firefox
    
2 Navigate to page   ask.com     who can be added to Firefox search engines list.     
3 Select the magnifying glass from the search toolbar and add to the search engines list.


Expected results: Ask.com is added in the list of search engines
Actual results: An error appears and ask.com can't be added on the search engines list.
(Reporter)

Comment 1

2 years ago
Created attachment 8678778 [details]
Ask.com Error
(Reporter)

Comment 2

2 years ago
Hi,
 I made some regression test and I notes that this problem appears in version 36 Firefox, in this version this feature was implemented.
I can't reproduce this when I load www.ask.com as I get redirected to a French page, but using http://us.ask.com/ instead works.

The page contains:
<link REL="search" type="application/opensearchdescription+xml" HREF="http://www.ask.com/ask-search.xml" title="Ask Search">

The url http://www.ask.com/ask-search.xml can correctly be downloaded. The file contains:

<?xml version="1.0" encoding="UTF-8"?>
<OpenSearchDescription>
    <ShortName>Ask Search</ShortName>
    <Url type="text/html" template="http://www.ask.com/web?q={searchTerms}&amp;qsrc=997&amp;o=34851&amp;l=dir&amp;ad=dirN"/>
</OpenSearchDescription>

There may be a reason why this file is invalid (is it missing some required field?), but an error message saying we couldn't download the file definitely seems wrong.
Priority: -- → P3
Whiteboard: [fxsearch]
Taking a look at this, the core issue is that 

<OpenSearchDescription>

should be

<OpenSearchDescription xmlns="http://a9.com/-/spec/opensearch/1.1/">

according to the spec: http://www.opensearch.org/Specifications/OpenSearch/1.1#Examples

I have just submitted feedback to ask.com to see if we can get them to fix their site.

Fixing the dialog message should be fairly easy, so I'll take a look at that.
Assignee: nobody → standard8
Comment hidden (mozreview-request)
Any idea who I ping to check the new string is ok?
Flags: needinfo?(florian)
(In reply to Mark Banner (:standard8, limited time in Dec) from comment #6)
> Any idea who I ping to check the new string is ok?

If you want to check the wording/phrasing, you needinfo mheubusch. If you want to check that the string is OK with l10n (I see no problem) you needinfo :flod.
Flags: needinfo?(florian)

Comment 8

8 months ago
mozreview-review
Comment on attachment 8818548 [details]
Bug 1218324 - Correctly report when a search engine could not be installed due to an invalid format.

https://reviewboard.mozilla.org/r/98602/#review98892

::: toolkit/locales/en-US/chrome/search/search.properties:18
(Diff revision 1)
>  error_duplicate_engine_msg=%S could not install the search plugin from “%S” because an engine with the same name already exists.
>  
>  error_invalid_engine_title=Install Error
>  # LOCALIZATION NOTE (error_invalid_engine_msg): %S = brandShortName
>  error_invalid_engine_msg=This search engine isn’t supported by %S and can’t be installed.
> +# LOCALIZATION NOTE (error_invalid_engine_format_msg): %1$S = brandShortName, %2$S = location (url)

The l10n bits look good, but I can't help wondering about the mixed terminology in that file (search engine, search plugin), and how this message might overlap with error_invalid_engine_msg

Comment 9

7 months ago
It looks as the idea of the proposed patch can work for bug 1313856 too.
Blocks: 1313856
(Assignee)

Comment 10

7 months ago
mozreview-review-reply
Comment on attachment 8818548 [details]
Bug 1218324 - Correctly report when a search engine could not be installed due to an invalid format.

https://reviewboard.mozilla.org/r/98602/#review98892

> The l10n bits look good, but I can't help wondering about the mixed terminology in that file (search engine, search plugin), and how this message might overlap with error_invalid_engine_msg

I've fixed the terminolgy to "search engine". You're right, there is some overlap with the other message. Maybe it is right to combine those and put specifics to the error console?
Comment on attachment 8818548 [details]
Bug 1218324 - Correctly report when a search engine could not be installed due to an invalid format.

https://reviewboard.mozilla.org/r/98602/#review99804

The code change looks good. One nit on the string.

::: toolkit/locales/en-US/chrome/search/search.properties:19
(Diff revision 1)
>  
>  error_invalid_engine_title=Install Error
>  # LOCALIZATION NOTE (error_invalid_engine_msg): %S = brandShortName
>  error_invalid_engine_msg=This search engine isn’t supported by %S and can’t be installed.
> +# LOCALIZATION NOTE (error_invalid_engine_format_msg): %1$S = brandShortName, %2$S = location (url)
> +error_invalid_engine_format_msg=%1$S could not install the search plugin from "%2$S" due to an invalid file format.

The normal double quotes here will fail the browser/base/content/test/general/browser_misused_characters_in_strings.js test.
Attachment #8818548 - Flags: review?(florian) → review+
(In reply to Mark Banner (:standard8, limited time in Dec) from comment #10)
> You're right, there is some
> overlap with the other message. Maybe it is right to combine those and put
> specifics to the error console?

Could be a good idea. I dislike the existing "This search engine isn’t supported by %S" message, as it seems to imply there's a problem in Firefox, whereas in most cases we really mean the search plugin is broken.

The only case I can think of where that message makes sense is when attempting to install a Sherlock plugin, for which we dropped support in bug 862148.
Comment hidden (mozreview-request)
Comment on attachment 8818548 [details]
Bug 1218324 - Correctly report when a search engine could not be installed due to an invalid format.

This is an updated version that combines the strings. Florian, what do you think to this?
Attachment #8818548 - Flags: review+ → review?(florian)
Comment on attachment 8818548 [details]
Bug 1218324 - Correctly report when a search engine could not be installed due to an invalid format.

https://reviewboard.mozilla.org/r/98602/#review99836

r+ on the code. Still unsure about the wording, we may want to consult mheubusch.

::: toolkit/locales/en-US/chrome/search/search.properties:17
(Diff revision 2)
>  error_loading_engine_msg2=%S could not download the search plugin from:\n%S
>  error_duplicate_engine_msg=%S could not install the search plugin from “%S” because an engine with the same name already exists.
>  
>  error_invalid_engine_title=Install Error
> -# LOCALIZATION NOTE (error_invalid_engine_msg): %S = brandShortName
> -error_invalid_engine_msg=This search engine isn’t supported by %S and can’t be installed.
> +# LOCALIZATION NOTE (error_invalid_engine_msg2): %1$S = brandShortName, %2$S = location (url)
> +error_invalid_engine_msg2=%1$S could not install the search engine from “%2$S” due to an unsupported file format.

Should this say "search plugin" like the 2 error messages above?

"unsupported file format" sounds like it could be Firefox's fault. Should we put the blame on the file or on the site instead?
Attachment #8818548 - Flags: review?(florian) → review+
Comment on attachment 8818548 [details]
Bug 1218324 - Correctly report when a search engine could not be installed due to an invalid format.

mheubusch, please can you comment on the wording of the strings here?
Attachment #8818548 - Flags: ui-review?(mheubusch)
Comment on attachment 8818548 [details]
Bug 1218324 - Correctly report when a search engine could not be installed due to an invalid format.

Maybe Ryan can help with the wording of the string(s) here?
Attachment #8818548 - Flags: ui-review?(rfeeley)
I propose the more generic:

Title: Invalid Format
Body: Firefox could not install the search engine from: {URL}
Button: OK
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment on attachment 8818548 [details]
Bug 1218324 - Correctly report when a search engine could not be installed due to an invalid format.

Florian, can you just check the additional changes please? I've left error_invalid_engine_title as search.xml uses it.

mheubusch/Ryan, I've updated for the strings that Ryan suggested, can I get ui-review now please?
Flags: needinfo?(rfeeley)
Flags: needinfo?(mheubusch)
Attachment #8818548 - Flags: review+ → review?(florian)
Comment on attachment 8818548 [details]
Bug 1218324 - Correctly report when a search engine could not be installed due to an invalid format.

https://reviewboard.mozilla.org/r/98602/#review107040
Attachment #8818548 - Flags: review?(florian) → review+
(In reply to Mark Banner (:standard8) from comment #21)
> Comment on attachment 8818548 [details]
> Bug 1218324 - Correctly report when a search engine could not be installed
> due to an invalid format.
> 
> Florian, can you just check the additional changes please? I've left
> error_invalid_engine_title as search.xml uses it.
> 
> mheubusch/Ryan, I've updated for the strings that Ryan suggested, can I get
> ui-review now please?
Flags: needinfo?(rfeeley) → needinfo?(standard8)
Can I get a try-build or screenshot please?
Created attachment 8831089 [details]
Screen shot of fixed prompt

Here's the screenshot. I've also triggered some try builds in case you want to check those out.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3eef2049525d
Flags: needinfo?(standard8)
Attachment #8831089 - Flags: ui-review?(rfeeley)
Great! Thanks!
Flags: needinfo?(mheubusch)
Comment on attachment 8831089 [details]
Screen shot of fixed prompt

Looks good. My + didn't stick last time oddly.
Attachment #8831089 - Flags: ui-review?(rfeeley) → ui-review+
Attachment #8818548 - Flags: ui-review?(rfeeley)
Attachment #8818548 - Flags: ui-review?(mheubusch)
Comment hidden (mozreview-request)

Comment 29

6 months ago
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e2d388263c3d
Correctly report when a search engine could not be installed due to an invalid format. r=florian

Comment 30

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e2d388263c3d
Status: NEW → RESOLVED
Last Resolved: 6 months ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
I have reproduced this bug on Firefox Nightly according to(2015-10-26)

Fixing bug is verified on Latest Nightly --- Build ID: (20170224030232),User Agent: Mozilla/5.0 (Windows NT 10.0; rv:54.0) Gecko/20100101 Firefox/54.0


Tested OS-- Windows10 32bit
QA Whiteboard: [bugday-20170222]
(Reporter)

Comment 32

5 months ago
I tested on Mac OS X 10.10 with the latest FF Nighlty 54.0a1(2017-02-26) and if I go to http://uk.ask.com/ the + doesn't appear, but if I go on us.ask.com I see the + icon. This is the expected behaviour? Please note that I'm not in US. Thanks
(In reply to ovidiu boca[:Ovidiu] from comment #32)
> I tested on Mac OS X 10.10 with the latest FF Nighlty 54.0a1(2017-02-26) and
> if I go to http://uk.ask.com/ the + doesn't appear, but if I go on
> us.ask.com I see the + icon. This is the expected behaviour? Please note
> that I'm not in US. Thanks

The uk.ask.com page doesn't contain this line that is part of the us page:
<link REL="search" type="application/opensearchdescription+xml" HREF="//www.ask.com/ask-search.xml" title="Ask Search">

So I don't know if this is the behavior ask.com wants, but it's what they implemented on their websites.
(Reporter)

Comment 34

5 months ago
Ok, thanks for clearing this out. I tested on Windows 10 and I can confirm the fix with the latest FF Nighlty 54.0a1(2017-02-26).
Status: RESOLVED → VERIFIED
status-firefox54: fixed → verified
Duplicate of this bug: 1202439
You need to log in before you can comment on or make changes to this bug.