Favorite Services JS typo

Not working on Safari/Leopard

This error comes up on Mac with firebug (service links was breaking things on the Ubercart checkout page on a Mac in Safari, not FF, and turning it off fixed it)

service_links/js/favorite_services.js --> on line 4 TypeError: Result of expression 'window.external' [undefined] is not an object.

I found the problem. The code on line 4 is testing for window.external.AddFavourite, but on Safari window.external is not an object, so you can't test for window.external.AddFavourite. You first need to test for window.external all by its lonesome. I've changed my code to the following and it works fine.

    if (window.sidebar || window.external) {                                  // this line is new
      if (window.sidebar || window.external.AddFavourite) {
      $("a.service_links_favorite").show();
      $("a.service_links_favorite").click(function(event){
        event.preventDefault();
        var url = unescape(this.href.replace(/\+/g,' '));
        var url = url.replace(/^[^\?]*\?/g, "");
        var title = url.replace(/^[^#]*#/g,"");
        url = url.replace(/#.*$/g, "");

        if (window.sidebar) {
          window.sidebar.addPanel(title, url,"");
        } else if ( window.external ) {
          window.external.AddFavorite( url, title);
        }
      });
      } else {                                                                              // this line is new
      $("a.service_links_favorite").hide();                                         // this line is new
      }                                                                                      // this line is new
    } else {
      $("a.service_links_favorite").hide();
    }

Here's the above code in a patch. In case people don't know how to patch, just cd to the js directory and run patch -p0 < name_of_patch_here

This fixed my Javascript and made it work on my site in Safari 4

Added the support for Opera and Chrome, and Safari shouldn't have problems with it.

About Chrome it show just a message who ask to press CTRL + D, if someone has a better idea i'll be happy to integrate it.

No problem to support more browser but not alls allow the automatic bookmarking through javascript (this is the case of Chrome).

I had a syntax error with the patch.
This resolves the problem:

Replace:
if (window.sidebar || window.external.AddFavourite) {

By:
if (window.sidebar || window.external || window.external.AddFavourite) {

Thanks Moove, reworked and pushed!

I'm reopening this ticket because this is still broken in IE7 and 8. Firstly, the commit in 51a0f92c18df3eb6583d5c3cc6954c0cfaca19cd doesn't fix the "favourite" typo in the conditional (only in the actual call), which means that the expression (window.sidebar || (window.external && window.external.AddFavourite)) returns false (as the AddFavourite property does not exist). This effectively causes the link to show up, but without Javascript functionality behind it (instead, it links to the same page again), because the outermost conditional succeeds but none of its sub-conditionals do. This is of course extremely confusing for the user.

Furthermore, in IE7 and IE8 the bare expression window.external.AddFavorite raises an exception "object does not support this property or method", but actually calling the method does work nonetheless!

I'm afraid the best check is just for the window.external property, and if it exists you'd have to assume that the method on it exists too.

The attached patch (against 7.x-2.x) fixes this, and also simplifies the conditional tree, so that it will always finish hiding the link when no supported method is found.

I'm not sure I get the logic of popping up an alert for some browsers, but hiding the link for others. IMHO it would be nicer to just use an alert() whenever there are problems doing things automatically. Or have I missed something?! I've attached a patch based on #10.

Thanks

Yeah I agree that's confusing. It's probably better to always hide the link on browsers where it's not supported.

Just closed #1903234: Favorites service link doesn't work in Internet explorer as a duplicate of this one.

Could someone re-roll the patch? It doesn't apply on current -dev.

This have to be rewrite after the last patch provided by TimeRider, let me try!

patch updated i used #11 but credits go to Peter Bex and AndyF

[Error to send] go below

forgot 2 ;

Committed for both branches!