can.view.txt does not handle binding hookup on self-closing elements correctly #2113

Closed
rjgotten opened this Issue Dec 2, 2015 · 11 comments

Comments

Projects
None yet
5 participants
@rjgotten

rjgotten commented Dec 2, 2015

This issue surfaces when a hookup ends up inside a colgroup element. Via the elements.tagMap a colgroup element supports only a col child-element, however col is self-closing and may not hold content.

If you have a live-binding expression inside a colgroup (e.g. to render a variable amount of columns in a table), then the hookup in view/render.js;207-222 will emit HTML for a tag with content, e.g. <col data-view-id="1234">@@!!@@</col>.

HTML5 compliant browsers will coerce this to a self-closing col element and will push the @@!!@@ marker outward: placing it before the containing table element, where it will no longer be replaced when hookups are resolved and will end up being exposed to the user.

The solution is to retain some special knowledge of self-closing tags and update the hookup to emit the correct self-closing HTML that way.

@pYr0x

This comment has been minimized.

Show comment
Hide comment
@pYr0x

pYr0x Dec 2, 2015

mustache or stache?

pYr0x commented Dec 2, 2015

mustache or stache?

@rjgotten

This comment has been minimized.

Show comment
Hide comment
@rjgotten

rjgotten Dec 2, 2015

Reproduced in EJS, actually. (Yeah, this bug has been present since forever...)
Not sure if stache or mustache hit the same code paths in can.view.txt.

rjgotten commented Dec 2, 2015

Reproduced in EJS, actually. (Yeah, this bug has been present since forever...)
Not sure if stache or mustache hit the same code paths in can.view.txt.

@pYr0x

This comment has been minimized.

Show comment
Hide comment
@pYr0x

pYr0x Dec 2, 2015

unfortunately ejs is deprecated and this is a "wont fix"
if you can reproduced this issue with can.stache let us know

pYr0x commented Dec 2, 2015

unfortunately ejs is deprecated and this is a "wont fix"
if you can reproduced this issue with can.stache let us know

@justinbmeyer

This comment has been minimized.

Show comment
Hide comment
@justinbmeyer

justinbmeyer Dec 2, 2015

Contributor

@rjgotten can you share an example (or make a pull request with some breaking code). Thanks!

Contributor

justinbmeyer commented Dec 2, 2015

@rjgotten can you share an example (or make a pull request with some breaking code). Thanks!

@rjgotten

This comment has been minimized.

Show comment
Hide comment

rjgotten commented Dec 2, 2015

@justinbmeyer

http://jsfiddle.net/07zks0q1/2/

Fails in both EJS and Mustache

@pYr0x

This comment has been minimized.

Show comment
Hide comment
@rjgotten

This comment has been minimized.

Show comment
Hide comment
@rjgotten

rjgotten Dec 2, 2015

@pYr0x

unfortunately ejs is deprecated and this is a "wont fix"

Your own documentation seems to disagree with that. http://canjs.com/docs/can.ejs.html clearly states that EJS "will still be maintained up to 3.0 and potentially after." That implies bugfixes will still be made.

Deprecation means you stop further development (of new features) and recommend people to switch over to a future-proof alternative, pending removal of the old system. It doesn't mean a free pass to ignore bugs that leave the old system in a broken state in lieu of the newest most shiny toy in the toybox.

rjgotten commented Dec 2, 2015

@pYr0x

unfortunately ejs is deprecated and this is a "wont fix"

Your own documentation seems to disagree with that. http://canjs.com/docs/can.ejs.html clearly states that EJS "will still be maintained up to 3.0 and potentially after." That implies bugfixes will still be made.

Deprecation means you stop further development (of new features) and recommend people to switch over to a future-proof alternative, pending removal of the old system. It doesn't mean a free pass to ignore bugs that leave the old system in a broken state in lieu of the newest most shiny toy in the toybox.

@matthewp

This comment has been minimized.

Show comment
Hide comment
@matthewp

matthewp Dec 2, 2015

Contributor

That's right, ejs is still supported, this isn't a "won't fix".

However bugs only get fixed if someone has time and energy to fix them. This is true of all features, new and old. Pull requests fixing this bug will of course be accepted.

It's also possible to sponsor a bug to ensure it gets fixed if you can't do it yourself, @justinbmeyer can speak more about how that can work in this case.

Contributor

matthewp commented Dec 2, 2015

That's right, ejs is still supported, this isn't a "won't fix".

However bugs only get fixed if someone has time and energy to fix them. This is true of all features, new and old. Pull requests fixing this bug will of course be accepted.

It's also possible to sponsor a bug to ensure it gets fixed if you can't do it yourself, @justinbmeyer can speak more about how that can work in this case.

@rjgotten

This comment has been minimized.

Show comment
Hide comment
@rjgotten

rjgotten Dec 2, 2015

If you're willing to accept pull requests for bug fixes, I'll look into setting one up as I've I've managed to patch this issue locally already. (Wasn't that hard once I figured out where to look.)

Is it still a hassle to check out CanJS and all its dependencies via Git on Windows? I tried this once before in the v1.1 - v2.0 timeframe and it was really annoying having to deal with subrepositories back then.

rjgotten commented Dec 2, 2015

If you're willing to accept pull requests for bug fixes, I'll look into setting one up as I've I've managed to patch this issue locally already. (Wasn't that hard once I figured out where to look.)

Is it still a hassle to check out CanJS and all its dependencies via Git on Windows? I tried this once before in the v1.1 - v2.0 timeframe and it was really annoying having to deal with subrepositories back then.

@matthewp

This comment has been minimized.

Show comment
Hide comment
@matthewp

matthewp Dec 2, 2015

Contributor

@rjgotten we don't use submodules any more, so it's much easier. Just git clone and do an npm install.

Contributor

matthewp commented Dec 2, 2015

@rjgotten we don't use submodules any more, so it's much easier. Just git clone and do an npm install.

@rjgotten

This comment has been minimized.

Show comment
Hide comment
@rjgotten

rjgotten Dec 3, 2015

@matthewp
Done and done.

While the removal of subrepositories greatly simplfied things, you're indirectly using several native modules as dependencies. Those were an absolute pain in the rear to build (node-gyp and Windows is hell) but I eventually managed to skirt that minefield. Oh well. ^_^

rjgotten commented Dec 3, 2015

@matthewp
Done and done.

While the removal of subrepositories greatly simplfied things, you're indirectly using several native modules as dependencies. Those were an absolute pain in the rear to build (node-gyp and Windows is hell) but I eventually managed to skirt that minefield. Oh well. ^_^

@daffl daffl added this to the 2.3.6 milestone Dec 11, 2015

@daffl daffl closed this in #2119 Dec 12, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment