I've had a hard time debugging a news ticker - which I wrote from scratch using JavaScript.

It works fine on most browsers apart from IE9 (and some mobile browsers - Opera Mobile) where it is moving very slowly.

Using Developer Tools > Profiler enabled me to find the root cause of the problem.

It's a call to offsetLeft to determine whether to rotate the ticker i.e. 1st element bees the last element.

function NeedsRotating() {
    var ul = GetList();
    if (!ul) {
        return false;
    }
    var li = GetListItem(ul, 1);
    if (!li) {
        return false;
    }
    if (li.offsetLeft > ul.offsetLeft) {
        return false;
    }
    return true;
}

function MoveLeft(px) {
    var ul = GetList();
    if (!ul) {
        return false;
    }
    var li = GetListItem(ul, 0);
    if (!li) {
        return false;
    }
    var m = li.style.marginLeft;
    var n = 0;
    if (m.length != 0) {
        n = parseInt(m);
    }
    n -= px;
    li.style.marginLeft = n + "px";
    li.style.zoom = "1";
    return true;
}

It seems to be taking over 300ms to return the value, whereas the ticker is suppose to be moving left 1 pixel every 10ms.

Is there a known fix for this?

Thanks

I've had a hard time debugging a news ticker - which I wrote from scratch using JavaScript.

It works fine on most browsers apart from IE9 (and some mobile browsers - Opera Mobile) where it is moving very slowly.

Using Developer Tools > Profiler enabled me to find the root cause of the problem.

It's a call to offsetLeft to determine whether to rotate the ticker i.e. 1st element bees the last element.

function NeedsRotating() {
    var ul = GetList();
    if (!ul) {
        return false;
    }
    var li = GetListItem(ul, 1);
    if (!li) {
        return false;
    }
    if (li.offsetLeft > ul.offsetLeft) {
        return false;
    }
    return true;
}

function MoveLeft(px) {
    var ul = GetList();
    if (!ul) {
        return false;
    }
    var li = GetListItem(ul, 0);
    if (!li) {
        return false;
    }
    var m = li.style.marginLeft;
    var n = 0;
    if (m.length != 0) {
        n = parseInt(m);
    }
    n -= px;
    li.style.marginLeft = n + "px";
    li.style.zoom = "1";
    return true;
}

It seems to be taking over 300ms to return the value, whereas the ticker is suppose to be moving left 1 pixel every 10ms.

Is there a known fix for this?

Thanks

• javascript
• internet-explorer
• internet-explorer-9
• unobtrusive-javascript

• What property are you using to animate? left? margin-left? Show us a bit more. – Diodeus - James MacFarlane Commented Mar 29, 2012 at 15:04
• Done - I think I may have gone overboard with my functions. – Chris Cannon Commented Mar 29, 2012 at 15:11
• jquery's .animate() handles a lot of this stuff internally, so you don't have to worry about it. Worth looking into. – Diodeus - James MacFarlane Commented Mar 29, 2012 at 15:19

3 Answers 3

DOM operations

I agree with @samccone that if GetList() and GetListItem() are performing DOM operations each time, you should try to save references to the elements retrieved by those calls as much as possible and reduce the DOM operations.

then I can just manipulate that variable and hopefully it won't go out of sync with the "real" value by calling offsetLeft.

You'll just be storing a reference to the DOM element in a variable. Since it's a reference, it is the real value. It is the same exact object. E.g.:

var li = ul.getElementsByTagName( "li" )[ index ];

That stores a reference to the DOM object. You can read offsetLeft from that object anytime, without performing another DOM operation (like getElementsByTagName) to retrieve the object.

On the other hand, the following would just store the value and would not stay in sync:

var offsetLeft = ul.getElementsByTagName( "li" )[ index ].offsetLeft;

offsetLeft

If offsetLeft really is a bottleneck, is it possible you could rework this to just read it a lot less? In this case, each time you rotate out the first item could you read offsetLeft once for the new first item, then just decrement that value in each call to MoveLeft() until it reaches 0 (or whatever)? E.g.

function MoveLeft( px ) {

  current_offset -= px;

If you want to get even more aggressive about avoiding offsetLeft, maybe you could do something where you read the width of each list item once, and the offsetLeft of the first item once, then just use those values to determine when to rotate, without ever calling offsetLeft again.

Global Variables

I think I get it... so elms["foo"] would have to be a global variable?

I think really I just need to use global variables instead of calling offsetLeft every 10 ms.

You don't need to use global variables, and in fact you should avoid it -- it's bad design. There are at least a couple of good approaches you could take without using global variables:

1. You can wrap your whole program in a closure:

   ( function () {
   
     var elements = {};
   
   
     function NeedsRotating() {
   
       ...
   
     }  
   
   
     function example() {
   
       // The follow var declaration will block access
       // to the outer `elements`
   
       var elements;
   
     }
   
   
     // Rest of your code here
   
   } )();
   

   There elements is scoped to the anonymous function that contains it. It's not a global variable and won't be visible outside the anonymous function. It will be visible to any code, including functions (such as NeedsRotating() in this case), within the anonymous function, as long as you don't declare a variable of the same name in your inner functions.

2. You can encapsulate everything in an object:

   ( function () {
   
     var ticker = {};
   
     ticker.elements = {};
   
   
     // Assign a method to a property of `ticker`
   
     ticker.NeedsRotating = function () {
   
       // All methods called on `ticker` can access its
       // props (e.g. `elements`) via `this`
   
       var ul = this.elements.list;
   
       var li = this.elements.list_item;
   
   
       // Example of calling another method on `ticker`
   
       this.do_something();
   
     }  ;
   
   
     // Rest of your code here
   
   
     // Something like this maybe
   
     ticker.start();
   
   } )();
   

   Here I've wrapped everything in an anonymous function again so that even ticker is not a global variable.

Response to Comments

First of all, regarding setTimeout, you're better off doing this:

t = setTimeout( TickerLoop, i );

rather than:

t = setTimeout("TickerLoop();", i);

In JS, functions are first-class objects, so you can pass the actual function object as an argument to setTimeout, instead of passing a string, which is like using eval.

You may want to consider setInterval instead of setTimeout.

Because surely any code executed in setTimeout would be out of scope of the closure?

That's actually not the case. The closure is formed when the function is defined. So calling the function via setTimeout does not interfere with the function's access to the closed variables. Here is a simple demo snippet:

( function () {

  var offset = 100;


  var whatever = function () {

    console.log( offset );

  };


  setTimeout( whatever, 10 );

} )();

setTimeout will, however, interfere with the binding of this in your methods, which will be an issue if you encapsulate everything in an object. The following will not work:

( function () {

  var ticker = {};

  ticker.offset = 100;


  ticker.whatever = function () {

    console.log( this.offset );

  };


  setTimeout( ticker.whatever, 10 );

} )();

Inside ticker.whatever, this would not refer to ticker. However, here you can use an anonymous function to form a closure to solve the problem:

setTimeout( function () { ticker.whatever(); }, 10 );

Should I store it in a class variable i.e. var ticker.SecondLiOffsetLeft = GetListItem(ul, 1).offsetLeft then I would only have to call offsetLeft again when I rotate the list.

I think that's the best alternative to a global variable?

The key things are:

1. Access offsetLeft once each time you rotate the list.

2. If you store the list items in a variable, you can access their offsetLeft property without having to repeatedly perform DOM operations like getElementsByTagName() to get the list objects.

The variable in #2 can either be an object property, if you wrap everything up in an object, or just a variable that is accessible to your functions via their closure scope. I'd probably wrap this up in an object.

I updated the "DOM operations" section to clarify that if you store the reference to the DOM object, it will be the exact same object. You don't want to store offsetLeft directly, as that would just be storing the value and it wouldn't stay in sync.

However you decide to store them (e.g. object property or variable), you should probably retrieve all of the li objects once and store them in an array-like structure. E.g.

this.li = ul.getElementsByTagName( "li" );

Each time you rotate, indicate the current item somehow, e.g.:

this.current_item = ###;

// or

this.li.current = this.li[ ### ];


// Then

this.li[ this.current_item ].offsetLeft

// or

this.li.current.offsetLeft

Or if you want you could store the li objects in an array and do this for each rotation:

this.li.push( this.li.shift() );

// then

this.li[0].offsetLeft

if you dont cache your selectors in var li = GetListItem(ul, 1);

then performance will suffer greatly.. and that is what you are seeing because you are firing up a new selector every 10ms

you should cache the selector in a hash like

elms["foo"] = elms["foo"] || selectElm(foo);

elms["foo"].actionHere(...)

your code is slow because reading offsetLeft will force the browser to do a reflow. the reflow is the part that is slowing you down. the browser is typically smart enough to queue changes to reduce the number of reflows. however, given that you want the most up to date value when access offsetLeft, you're forcing the browser to flush that queue and do a reflow in order to calculate the correct value for you.

without knowing all the details of what you're trying to do, it's hard to know what to remend to improve performance. http://www.phpied./rendering-repaint-reflowrelayout-restyle/ explains this problem in more detail and offers some advice about minimizing reflows.