#753 ✓wont_fix
Matt Haggard

Element() constructor doesn't use Event.observe for events

Reported by Matt Haggard | July 31st, 2009 @ 11:28 PM

When creating a DOM Element with an event handler you can't use a function as the handler like this:


function handler(ev) {
  alert('event handled: '+ev);
}


var e = Element('div', { style: 'color: #000000;', onclick: handler });


document.body.appendChild(e);

I've attached the function I use to create Elements. Maybe that helps.

Comments and changes to this ticket

  • Tobie Langel

    Tobie Langel August 1st, 2009 @ 05:19 PM

    • State changed from “new” to “wont_fix”

    Thanks for the ticket.

    That's by design. You should be using the Event API / Element#observe for the adding events.

    Closing as wont_fix

  • Tobie Langel

    Tobie Langel August 3rd, 2009 @ 04:41 PM

    • Tag changed from enhancements, needs:discussion, section:dom to needs:discussion, section:dom
  • Matt Haggard

    Matt Haggard August 3rd, 2009 @ 05:39 PM

    Rats, I thought it might be by design. Oh well :)

    For what it's worth, my implementation does use Element#observe. It also uses Element#addClassName and Element#setStyle instead of setAttribute (if the arguments are appropriate).

    ...
    switch (key.toLowerCase()) {
          
          case 'tag':
            break;
               
          case 'innerhtml':
            node.innerHTML = val;
            break;
          
          case 'children':
            var nodes = Element.create(val);
            for (var i = 0; i < nodes.length; i++) {
              node.appendChild(nodes[i]);
            }
            break;
          
          case 'class':
            node.addClassName(val);
            break;
          
          case 'style':
            if (Object.isString(val)) {
              node.setAttribute(key, val);
            } else {
              node.setStyle(val);
            }
            break;
          
          case 'value':
            node.value = val;
            break;
          
          default:
            if (key.substr(0,2) == 'on') {
              // Events
              var event_name = key.substr(2).toLowerCase();
              if (Object.isFunction(val)) {
                Event.observe(node, event_name, val);
              } else if (Object.isString(val)) {
                node.setAttribute(key, val);
              }
            } else {
              // Every other attribute
              node.setAttribute(key, val);
            }
            break;
        }
    ...
    
  • Tobie Langel

    Tobie Langel August 3rd, 2009 @ 06:54 PM

    There are some interesting ideas in here. Maybe for Prototype 2.0 ?

  • Juriy Zaytsev

    Juriy Zaytsev August 3rd, 2009 @ 08:06 PM

    ...
    switch (key.toLowerCase()) {
          
          case 'tag':
            break;
    

    What is this tag for?

          case 'innerhtml':
            node.innerHTML = val;
            break;
          
          case 'children':
            var nodes = Element.create(val);
            for (var i = 0; i < nodes.length; i++) {
              node.appendChild(nodes[i]);
            }
            break;
    

    These look useful and could abstract an all-so-common need to populate an element with content right after creation; something that is currently accomplished with - new Element(...).update(...). On the other hand, there's an ambiguity with whatever property name will be chosen for content - "content, innerHTML, children, update, insert". Whatever it is, I would probably be in favor of delegating it to update and/or insert which can already handle both - string and node -based - content.

          case 'class':
            node.addClassName(val);
            break;
          
          case 'style':
            if (Object.isString(val)) {
              node.setAttribute(key, val);
            } else {
              node.setStyle(val);
            }
            break;
    

    class and style are already handled by Prototype. I'm not sure if style should be set via setAttribute when passed as a string. Perhaps assigning to cssText is a better choice (which Prototype might actually already abstract through its setStyle, I'm not sure)

          case 'value':
            node.value = val;
            break;
    

    IIRC, this is also supported by Element constructor (and is routed through more robust setValue)

          default:
            if (key.substr(0,2) == 'on') {
              // Events
              var event_name = key.substr(2).toLowerCase();
              if (Object.isFunction(val)) {
                Event.observe(node, event_name, val);
              } else if (Object.isString(val)) {
                node.setAttribute(key, val);
              }
            } else {
              // Every other attribute
              node.setAttribute(key, val);
            }
            break;
        }
    ...
    

    Attaching event handlers through intrinsic event attributes is not really standardized and something like setAttribute('onclick', fnRef); (which is what else if branch seems to be doing) does not work reliably across current browsers. Fully simulating the way browsers handle intrinsic event handlers would also require scope augmentation as well as normalization of things like event argument (which MSHTML DOM gets gets "wrong").

    For these reasons, it might be a good idea to disallow string-based event handler bodies.

    I'm also not sure if treating any "on"-prefixed property as event handler is a good idea. Maybe it would be less ambiguous and clearer to put them under events property (as an object):

    new Element('a', {
      href: '#',
      events: {
        'click': function(){ ... },
        'mouseover': function(){ ... }
      }
    });
    
  • Radoslav Stankov

    Radoslav Stankov August 3rd, 2009 @ 09:53 PM

    Hm, one thing I like about Prototype is extendability. One example is Element.insertionTranslations, from where I have added ability to write $(element).insert({into: ''});.
    My point here is that something like the current Element.
    attributeTranslations could be used (and even documented) so these special attributes could be managed.

  • Matt Haggard

    Matt Haggard August 6th, 2009 @ 06:11 PM

    @Juriy

    The tag is the tagName usually passed to the Element constructor. Because this Element.create() function I've made supports creating an array of elements, I couldn't keep the tagName argument, so I folded it into the dict describing the element.

    I've incorporated many of your recommendations in this latest file that I've uploaded. And how about using childNodes for the children property?

    With these changes, you could do the following:

    
    function click_handler(ev) {
      // do stuff
    }
    
    var rows = Element.create([
      {
        tag: 'tr',
        id: 'row1',
        childNodes: [
          {
            tag: 'td',
            innerHTML: 'the innerHTML',
            style: {
              backgroundColor: '#990000',
              fontSize: '12px'
            },
            events: {
              'click': click_handler,
            }
          },
          {
            tag: 'td',
            style: 'background: #990000; font-size: 12px;',
            childNodes: 'A text node'
          }
        ]
      },
      {
        tag: 'tr',
        id: 'row2',
        class: 'class2',
        childNodes: {
          tag: 'td',
          colspan: '2',
          innerHTML: 'more text',
          events: {
            'mouseover': function(ev) {
              // handle mouseover
            }
          }
        }
      }
    ]);
    

    which would translate to the equivalent of:

    
    <tr id="row1">
      <td style="background: #990000; font-size: 12px;" onclick="...">the innerHTML</td>
      <td style="background: #990000; font-size: 12px;">A text node</td>
    </tr>
    <tr id="row2" class="class2">
      <td colspan="2" onmouseover="...">more text</td>
    </tr>
    

    One remaining problem I see with doing event observing this way is that there's not an easy way to reference the newly created element to bind it in the event handler. Meaning this is hard to do:

    
    function handler(obj) {
      // do stuff to obj
    }
    
    var e = Element('div');
    e.observe('click', handler.bind(e));
    
  • Juriy Zaytsev

    Juriy Zaytsev August 6th, 2009 @ 07:12 PM

    Handler should already be bound to an element. Or am I missing something?

  • Matt Haggard

    Matt Haggard August 6th, 2009 @ 07:35 PM

    Huh... my bad :) It does bind to the element by default. Sweet!

  • John-David Dalton

    John-David Dalton August 6th, 2009 @ 08:06 PM

    Keep in mind your events are not inline as your example above suggests.

    <td colspan="2" onmouseover="...">more text</td>
    

    I like these changes but the end result makes me think a simple update() would be easier to follow and faster to boot.

    var td = 'td style="background: #990000; font-size: 12px;"';
    
    $('my_tbody').update('\
      <tr id="row1">\
        <' + td + '>the innerHTML</td>\
        <' + td + '>A text node</td>\
      </tr>\
      <tr id="row2" class="class2">\
        <td colspan="2">more text</td>\
      </tr>');
    
    $('row1').down().observe('click', clickHandler);
    $('row2').down().observe('mouseover', mouseoverHandler);
    

Please Sign in or create a free account to add a new ticket.

With your very own profile, you can contribute to projects, track your activity, watch tickets, receive and update tickets through your email and much more.

New-ticket Create new ticket

Create your profile

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

The Prototype JavaScript library.

Shared Ticket Bins

Attachments

Pages