Closed Bug 1218324 Opened 9 years ago Closed 7 years ago

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

Categories

(Firefox :: Search, defect, P3)

Product:
Firefox
Component:
Search
Version:
44 Branch
Platform:
Other
Windows 10
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 54
Tracking Flags:
Tracking Status
firefox44 --- affected
firefox54 --- verified

People

(Reporter: Ovidiu, Assigned: standard8)

References

Details

(Whiteboard: [fxsearch])

Attachments

(3 files)

Ask.com Error
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
Screen shot of fixed prompt
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.
Attached image Ask.com Error — 

    
    

    
  
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

    
    

    
  
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 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

    
    

    
  
It looks as the idea of the proposed patch can work for bug 1313856 too.

    
  
Blocks: 1313856

    
    

    
  
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 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 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?

    
    

    
  


  
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)

    
    

    
      
  

    
    

    
  
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

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/e2d388263c3d
Status: NEW → RESOLVED
Closed: 7 years 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]

    
    

    
  
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.

    
    

    
  
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

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: