search

leonidas / transparency

Transparency is a semantic template engine for the browser. It maps JSON objects to DOM elements by id, class and data-bind attributes.
http://leonidas.github.com/transparency/
MIT License
969 stars 112 forks source link

List rendering fails on IE8 when multiple list items have the same content. #38

Closed mtkopone closed 12 years ago

mtkopone commented 12 years ago

Hi, and sorry about moaning about legacy browsers, but:

The following page outputs 'Same Same' on all browsers expect IE8, where it outputs 'Same'.

<!DOCTYPE html>
<html>
<head>
  <script src="http://ajax.googleapis.com/ajax/libs/jquery/1.7.1/jquery.min.js"></script>
  <script src="https://raw.github.com/leonidas/transparency/master/lib/transparency.js"></script>  
  <script>
    $(function() {
      $('.content').render({ items: [ { name: 'Same' }, { name: 'Same' } ] });
    })
  </script>
</head>
<body>
  <div class="content">
    <div class="items">
      <div class="name"></div>
    </div>
  </div>
</body>
</html>
pyykkis commented 12 years ago

Sorry for the late reply, I was travelling for a month without access to the Internet.

Thanks for reporting the bug. Definitely, IE8 needs to be supported. Will fix.

pyykkis commented 12 years ago

Uah, I located the bug. It's kind of nasty to fix, IE8 DOM implementation is seriously broken (surprise..).

Transparency extends matching DOM elements with element.transparency object, which contains some internal info, e.g., the text rendered to the node. Apparently, in IE8, extending DOM elements with objects doesn't work.

jQuery extends DOM elements only with cache id, storing the data elsewhere. Similar approach should fix the problem, but it might take a day or two, before I've enough time to do the implementation.

Background info: http://perfectionkills.com/whats-wrong-with-extending-the-dom/

pyykkis commented 12 years ago

Even Element.cloneNode() is broken in IE8 and below. https://github.com/jquery/jquery/blob/master/src/manipulation.js#L585

I'm seriously reconsidering introducing jQuery dependency in order to ensure interoperability and legacy support..

mtkopone commented 12 years ago

Dealing with IE8 can be a real pain...

We're currently using transparency.js in the following way:

var $template = $('#templateId').detach().removeAttr('id')
...
$output.append($template.clone().render(data, directives))

So I figured it'd be ok to comment out line 220, the one that says: "if (!e || !(content != null) || e.transparency.content === content) return;"

Seems to do the trick, but I didn't go deep enough to check what side-effects this could have...

pyykkis commented 12 years ago

Finally fixed: https://github.com/leonidas/transparency/commit/bee5a9b701e0358588e8f42e88f780947e35f8dc

The solution you suggested should work in the scenario above. However, element.transparency.model would still point to first model in the list, instead of the model that was actually rendered. This would break event handlers, which are using the model data, e.g. https://github.com/leonidas/transparency/blob/master/examples/todo-app/index.html#L41

The fix in the commit above ensures that the Transparency data is not shared between template instances.

mtkopone commented 12 years ago

Cool, thanks!