This project is archived and is in readonly mode.

#848 ✓resolved
Gabriel

Assets.Image syntax error when onLoad

Reported by Gabriel | February 16th, 2010 @ 09:56 PM

I was currently working on a project where i had to synchronously load images, when assigning a function to the onLoad event of Assets.Image an Uncaught SyntaxError: Unexpected token is fired. In my specific project this is stopping the load of the remaining content.

Here i leave a simple code example: http://jsfiddle.net/rxvMx/

In chrome developer tools the error is shown as: Uncaught SyntaxError: Unexpected token (
In firebug as syntax error : error source line: [Break on this error] function () {\n

Comments and changes to this ticket

  • fakedarren

    fakedarren February 17th, 2010 @ 12:49 PM

    • State changed from “new” to “hold”
    • Assigned user set to “fakedarren”
    • Milestone cleared.

    We have Asset.Images for collections of images.

  • Gabriel

    Gabriel February 17th, 2010 @ 02:43 PM

    Images fires all requests at once. The server i am working with is not fast enough, it doesn't allow multiple connection, and it has a maximum queues of requests it can handle, so as it is right now, it loads only the first 4-5 images and the rest are timeout or failed events. My purpose of using Image and onLoad was loading images only when the previous one has finished.

    Thanks for your prompt attention
    Mootools FTW :D

  • gonchuki

    gonchuki February 18th, 2010 @ 04:32 PM

    the mentioned bug seems to happen when using dromedaryCase "onLoad", if you use lowercase "onload" it works perfectly. http://jsfiddle.net/wrJNF/
    don't know anyways what's the problem here, as documentation for Asset.image explicits the usage of "onLoad" but source of Asset.images uses "onload" internally.
    protip: notice I replaced your "v" var for the "this" keyword inside the onload function. That is the expected usage in this scenario.

    Also, solution for your Asset.images issue is to manually build the queue to ensure you only have those 4-5 requests max at all times.

  • gonchuki

    gonchuki February 19th, 2010 @ 06:59 PM

    attached a proposed fix for this bug, after close inspection I found out the onLoad property wasn't being deleted and the code failed when Element.set was called.

  • Fábio M. Costa

    Fábio M. Costa February 20th, 2010 @ 01:51 AM

    The idea of the patch looks fine to me. This onload (without camelCase) causes too much confusion.

  • fakedarren

    fakedarren February 21st, 2010 @ 10:44 AM

    • Assigned user changed from “fakedarren” to “Aaron Newton”

    we actually changed this to 'onLoad' recently to make it more 'moo-ish'. But I deliberately retained the lowercase version for backwards compatibility. My opinion is that this lowercase event would not be removed for any 1.2.4.x release but would be a 'breaking change' in -more 1.3.

    Aaron, thoughts?

  • Gabriel

    Gabriel March 5th, 2010 @ 05:01 PM

    The patch works great, but I realized that if the server is non-responsive when the client is trying to load the image, then the onError event doesn't fire. I believe the onError should also be fired in this case. I did a quick fix so it works on my side. I propose an extra timeout option for Assets.image. If the option is set and the image hasnt been loaded in the given time the event onError is fired. Here is my patch:

    image: function(source, properties){

    properties = $merge({
        onload: $empty,
        onabort: $empty,
        onerror: $empty,
    }, properties);
    var image = new Image();
    
    • var check; var element = document.id(image) || new Element('img'); ['load', 'abort', 'error'].each(function(name){
      var type = 'on' + name;
      var cap = name.capitalize();
      if (properties['on' + cap]) {
          properties[type] = properties['on' + cap];
      delete properties['on' + cap];
      }
      var event = properties[type];
      delete properties[type];
      image[type] = function(){
          if (!image) return;
          if (!element.parentNode){
              element.width = image.width;
              element.height = image.height;
          }
          image = image.onload = image.onabort = image.onerror = null;
          event.delay(1, element, element);
          element.fireEvent(name, element, 1);
      
    •     if(name == 'load') $clear(check);
      };
      
      }); image.src = element.src = source; if (image && image.complete) image.onload.delay(1);
    • else if(properties['timeout']) check = image.onerror.delay(properties['timeout']); return element.set(properties); },
  • Gabriel

    Gabriel March 5th, 2010 @ 05:11 PM

    oops, don't know what happen to the formatting... deeply sorry!. If the moderator can erase the previous post it will be great!...

    The patch works great, but I realized that if the server is non-responsive when the client is trying to load the image, then the onError event doesn't fire. I believe the onError should also be fired in this case. I did a quick fix so it works on my side. I propose an extra timeout option for Assets.image. If the option is set and the image hasnt been loaded in the given time the event onError is fired. Here is my patch:

    image: function(source, properties){
            properties = $merge({
                onload: $empty,
                onabort: $empty,
                onerror: $empty,
            }, properties);
            var image = new Image();
            var check;
            var element = document.id(image) || new Element('img');
            ['load', 'abort', 'error'].each(function(name){
                var type = 'on' + name;
                var cap = name.capitalize();
                if (properties['on' + cap]) {
                    properties[type] = properties['on' + cap];
                    delete properties['on' + cap];
                }
                var event = properties[type];
                delete properties[type];
                image[type] = function(){
                    if (!image) return;
                    if (!element.parentNode){
                        element.width = image.width;
                        element.height = image.height;
                    }
                    image = image.onload = image.onabort = image.onerror = null;
                    event.delay(1, element, element);
                    element.fireEvent(name, element, 1);
                    if(name == 'load') $clear(check);
                };
            });
            image.src = element.src = source;
            if (image && image.complete) image.onload.delay(1);
            else if(properties['timeout']) var check = image.onerror.delay(properties['timeout']);
            return element.set(properties);
        }
    
  • Aaron Newton

    Aaron Newton March 5th, 2010 @ 10:13 PM

    • State changed from “hold” to “resolved”

    I've fixed this in the develop-848-assetsimage-syntax-error-when-onload branch as well as develop-edge branch. It'll be in the next release.

    I'm not going to add the timeout behavior; I feel like that's outside the scope of this plugin. Timeout behavior is something you should handle on your own (IMHO).

  • Gabriel

    Gabriel March 5th, 2010 @ 10:30 PM

    But what happens when you request the image and you don't get a response from the server??? is the patch going to fire the onError event???

  • Aaron Newton

    Aaron Newton March 5th, 2010 @ 10:40 PM

    I already think this plugin, which has been around for a long time (since 1.1) is of dubious value. We have a perfectly good element constructor. If you want to manage a timeout from the server you don't need this in Assets.image. You can just as easily do:

    myImg = new Asset.image(...);
    (function(){
      if (!myImg.complete) //do something
    }).delay(1000);
    

    I'm just not sure what the assets code needs is more functionality. This bug that opened this ticket exists because we added more functionality, making this simple plugin more complex and, in a way, not very much more useful. I think people would be better off just using the Element constructor, IMHO.

    ... But open a ticket for the onError timeout stuff and the rest of the -more team can weigh in on the topic. Please open it in the -more lighthouse though: http://mootools.lighthouseapp.com/projects/24057-mootoolsmore/tickets/

  • gonchuki

    gonchuki March 6th, 2010 @ 01:20 PM

    Knowing this is not a discussion board, I think the usefulness of this plugin relies on the specific properties of the element where you can't use addEventListener to capture the related onload/onerror events thus using the Element constructor doesn't give the same effect (unless the code from Asset.Image is moved to the -core Element, or is built as a sub-module like Element.Image in -more).

    I'm open to further discussion outside lighthouse if you think this is a productive and debatable topic (I've done tons of work piggybacking on this plugin).

  • Aaron Newton

    Aaron Newton March 7th, 2010 @ 01:06 AM

    Like I said, open a ticket for that issue (in the -more lighthouse please) and our dev team will discuss and vote on it. This timeout stuff has nothing to do with the error this ticket was opened for.

  • gonchuki

    gonchuki March 8th, 2010 @ 02:30 PM

    maybe you got me wrong but I was not talking about the timeout addition, only about the plugin itself as a whole (replying to your questioning of the usefulness of this plugin, a topic that you brought in the first place).

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

Shared Ticket Bins

Attachments

Pages