Closed
Bug 1218324
Opened 9 years ago
Closed 8 years ago
Search engine "ask.com" couldn't be added to the existing list of search engines.
Categories
(Firefox :: Search, defect, P3)
Tracking
()
VERIFIED
FIXED
Firefox 54
People
(Reporter: Ovidiu, Assigned: standard8)
References
Details
(Whiteboard: [fxsearch])
Attachments
(3 files)
354.44 KB,
image/jpeg
|
Details | |
Bug 1218324 - Correctly report when a search engine could not be installed due to an invalid format.
58 bytes,
text/x-review-board-request
|
florian
:
review+
|
Details |
105.72 KB,
image/png
|
rfeeley
:
ui-review+
|
Details |
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•9 years ago
|
||
Reporter | ||
Comment 2•9 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.
Comment 3•9 years ago
|
||
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}&qsrc=997&o=34851&l=dir&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]
Assignee | ||
Comment 4•8 years ago
|
||
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) |
Assignee | ||
Comment 6•8 years ago
|
||
Any idea who I ping to check the new string is ok?
Flags: needinfo?(florian)
Comment 7•8 years ago
|
||
(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 years 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•8 years ago
|
||
It looks as the idea of the proposed patch can work for bug 1313856 too.
Assignee | ||
Comment 10•8 years 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 11•8 years 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/#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+
Comment 12•8 years ago
|
||
(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) |
Assignee | ||
Comment 14•8 years ago
|
||
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 15•8 years 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/#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+
Assignee | ||
Comment 16•8 years ago
|
||
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)
Assignee | ||
Comment 17•8 years ago
|
||
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)
Comment 18•8 years ago
|
||
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) |
Assignee | ||
Comment 21•8 years ago
|
||
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 22•8 years 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/#review107040
Attachment #8818548 -
Flags: review?(florian) → review+
Comment 23•8 years ago
|
||
(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)
Comment 24•8 years ago
|
||
Can I get a try-build or screenshot please?
Assignee | ||
Comment 25•8 years ago
|
||
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)
Comment 27•8 years ago
|
||
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+
Assignee | ||
Updated•8 years ago
|
Attachment #8818548 -
Flags: ui-review?(rfeeley)
Attachment #8818548 -
Flags: ui-review?(mheubusch)
Comment hidden (mozreview-request) |
Comment 29•8 years 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•8 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
Comment 31•8 years ago
|
||
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•8 years 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
Comment 33•8 years ago
|
||
(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•8 years 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
You need to log in
before you can comment on or make changes to this bug.
Description
•