Search code examples
javascriptgoogle-chromesafariwebkit

Javascript object callback not being called on webkit


I have defined a js "class" like this:

var TelephoneFormGroup = function(targetId, removePhoneCallback) {

    this.phone = '';
    this.phone_type_options = [];
    this.counter = 0;
    this.targetId = targetId;
    this.form_group = {};
    this.removePhone = removePhoneCallback;

    this.buildFormGroup = function() {
        var outerDiv = document.createElement('div');
        outerDiv.className = 'form-group';

        var label = document.createElement('label');
        label.className = 'label-control col-sm-3';

        var innerDiv = document.createElement('div');
        innerDiv.className = 'col-sm-8';

        var inputId = document.createElement('input');
        inputId.type = 'hidden';
        inputId.name = 'data[Telephone][' + this.counter + '][id]';

        var inputPhoneTypeId = document.createElement('select');
        inputPhoneTypeId.name = 'data[Telephone][' + this.counter + '][phone_type_id]';
        inputPhoneTypeId.className = 'form-control col-sm-3';

        var removePhoneIcon = document.createElement('i');
        removePhoneIcon.className = "glyphicon glyphicon-remove";

        var removePhoneLink = document.createElement('a');
        removePhoneLink.appendChild(removePhoneIcon);
        removePhoneLink.title = "Remover Telefone";
        removePhoneLink.href = "#";
        removePhoneLink.dataset.toggle = "tooltip";

        removePhoneLink.onclick = this.removePhone;

        label.appendChild(removePhoneLink);
        label.innerHTML += 'Telefone ' + this.counter;

        var that = this;
        Object.keys(this.phone_type_options).forEach( function(key) {
            inputPhoneTypeId.options[inputPhoneTypeId.options.length] = new Option(that.phone_type_options[key], key);
        });

        var inputPhone = document.createElement('input');
        inputPhone.type = 'text';
        inputPhone.name = 'data[Telephone][' + this.counter + '][phone]';
        inputPhone.className = 'form-control phone col-sm-9';


        outerDiv.appendChild(label);
        outerDiv.appendChild(innerDiv);
        innerDiv.appendChild(inputId);
        innerDiv.appendChild(inputPhoneTypeId);
        innerDiv.appendChild(inputPhone);

        this.form_group = outerDiv;
    },

    this.render = function() {
        if (this.targetId == null || this.targetId == '') throw 'Empty target id';

        if (typeof(this.targetId) === 'string') {
            document.getElementById(this.targetId).appendChild(this.form_group);    
        } else if (typeof(this.targetId === 'object')) {
            this.targetId.appendChild(this.form_group);
        };

    }

};

My issue is that removePhoneLink.onclick = this.removePhone; is not firing on webkit browsers. Works correctly on Firefox (mac), but its not working on safari/chrome (mac), safari/chrome (ipad).

What am I missing?

Thanks!


Solution

  • tl;dr attach the element to the DOM before attaching the event handler

    Thanks for the interesting case. After a few tries, I figured WebKit needs the element attached to the DOM before registering the event handler (stumbled accross Todd Motto's example, he does get the element with jQuery before attaching the event handler!)

    Here's a working fiddle of your code, annotated. Mainly, I added a bindEventHandlers method called after the render method, made the whole object into a fluent interface with chained method (a la jQuery), added a static counter on the "class". Short excerpts :

    • keep track of how many objects were instantiated

      TelephoneFormGroup.counter = ++TelephoneFormGroup.counter || 0;
      this.counter = TelephoneFormGroup.counter;
      
    • encapsulate the callback in a method and call it with apply (ensures the method is called on the right object, had some problem with this on another project)

      this.removePhone = function(){
          removePhoneCallback.apply(this);
      };
      
    • bound this to your Object.keys's forEach (removed var that=this; trick)

      Object.keys(this.phone_type_options).forEach( function(key) {
          inputPhoneTypeId.options[inputPhoneTypeId.options.length] = new Option(this.phone_type_options[key], key);
      }.bind(this));
      // Here, we tell forEach we want 'this' to be bound to the object, not the current item being iterated over (i.e. that=this trick)
      
    • since the removePhoneLink construction is now built in two phase (build markup, then attach event handlers), we couple it using a dynamically created id

      ...
      // in buildFormGroup()
      removePhoneLink.id = "btnRemove"+this.targetId;
      ...
      
      // called after attaching the form group to the dom
      this.bindEventHandlers = function() {
          document.getElementById("btnRemove"+this.targetId).onclick = function(e) {
              this.removePhone();
          }.bind(this);
      }
      

    I hope it works for your use case. I tried it on Chrome and Safari (Mac OS X Yosemite).

    edit : I found your code interesting, simple, yet complicated enough for a small test I had in mind for a while. All your methods are bound to the instantiated object. With JavaScript and the prototype pattern, you should add methods to the object's prototype (as in this fiddle for your code).
    I did a small benchmark to compare the two techniques, checking memory usage and time to complete the method (machine spec).
    I would use Chrome Developer tools (heap size runs, refresh between runs, then render time runs, refresh between runs). Find the results there (fiddle.md).
    As you can see, it doesn't yield much difference until 50 objects are instantiated. Then you'd render until 18% faster with prototypes, and gain up to almost 30% in memory usage (for the 50'000 objects run). So I guess for a production use, go for prototypes?