$Element1.set($Element2) results in empty tag

[nateroling]

Example:

var $a = DOM.find("#parent");
var $b = DOM.create("<b class='bar'>Foo</b>");
$a.set($b); // $a now contains an empty <b> tag

[chemerisuk]

@nateroling this is a correct behavior at present, because I use String function for objects. It triggers method $Element#toString that returns <${tagName}>:

$a.toString(); // => "<a>"
$b.toString(); // => "<b>"

that's why you see empty tag <b>. On the other hand I agree, this is not an intuitive behavior. Do you think that it should produce result like $a.set("").append($b)?

[nateroling]

That might be a little more intuitive, yeah.

On the other hand, I think using `set with one argument to set innerHTML is unnecessarily clever in the first place. It took me quite a while to even realize that was an option.

I was looking for a something like $a.content($b), or even $a.empty().append($b). Using set never occurred to me, since I wrongly assumed that it was only for attributes.

Perhaps adding a content and/or empty function would be better?

[chemerisuk]

Saw your PR, I think we need to discuss it. I actually had an idea of $Element#empty in the past (https://github.com/chemerisuk/better-dom/commit/34963a141a3df987c2994508531094ecfa2d3af8), but decided to drop the method in the release version. The main concern is that in general it promotes a poor performance pattern:

var ul = DOM.create("ul>li*10");

ul.empty().set("innerHTML", DOM.emmet("li>`test`")); // 11 reflows!!!

Reflows are slow. They happen because every time you add/remove an element from/to the DOM it forces browser to do a reflow. You can improve it via using textContent with empty string, but IE8 has issues with it - that's why jQuery 1 uses while loop instead. Though not sure why they can't use innerHTML (may be some edge cases exist). Anyway jQuery 2 uses textContent:

ul.set("textContent", "").set("innerHTML", DOM.emmet("li>`test`")); // 2 reflows!

Now, how to improve the example? Just use single set instead:

ul.set("innerHTML", DOM.emmet("li>`test`")); // 1 reflow!
// or
ul.set(DOM.emmet("li>`test`"));

This is was the main reason why I decided to drop this method. But there are others:

1) empty elements (input, textarea etc.) have completely different approach for cleaning up inner content 2) You have to trigger$Element#watch manually in $Element#empty to catch such cases too

I agree that empty() looks cleaner than set("") but performance and code size matter. So before adding a new method we should keep it in mind.

Let's continue investigating $Element#content after addressing the issues above, because it's relies on $Element#empty by sense. I see the point, but we need to think 10 times before adding a new method.

Also, if you provide the real life problem that you faced with, it will help too.

[nateroling]

Absolutely. I'm writing a small library for loading SVG icons. It takes markup like this:

<a href="http://facebook.com" data-icon="images/facebook.svg">Facebook</a>

And replaces the text with the SVG:

<a href="http://facebook.com" data-icon="images/facebook.svg"><!-- inline SVG here --></a>

If the browser doesn't support SVG, or it fails to load, or even if the library fails to load entirely, the text remains.

So back to better-dom. Why not just alias empty() to set("")? Or am I being thick?

If set($Element) worked for my use case I wouldn't need empty() at all, though it does still feel like a method that should exist.

[chemerisuk]

@nateroling I agree to have $Element#empty as an alias for set(""). It looks reasonable and clear.

Now about $Element#content. I like an idea to introduce a new method that will remove "value shortcut" from $Element#get/set which are quite complex now. Also this new method will add capability to set $Element objects that you requested. We can include it into a 2.1.* version and deprecate value shortcuts starting from 2.2.0.

However I want to suggest a different name $Element#value, because word "content" doesn't make sense for form elements that can't contain inner HTML (e.g. content). Name "value" is inspired by Node#nodeValue that exists in vanilla DOM but (for a some reason) works for non-elements only. Let's fix it in better-dom APIs :)

Another difference from you proposal is ability to read inner value when no argument exists:

var input = DOM.create("input[value=foo]"),
      div = DOM.create("div>a+b");

input.value(); // => "foo"
div.value(); // => "<a></a><b></b>"

Also, let's create a separate file for each new method (and spec as well), you don't need any logic from manipulation.js. Just use _.register to extend $Element prototype (see set.js for example). For now, use $Element#get/set/append, later I'll revisit the implementation.

[nateroling]

Ok, I updated the PR.

What's the use case for returning the innerHTML string from value()? Would it make more sense for value() to return an Array of $Elements?

I'm also wondering if it makes more sense for the value tests to be inside manipulation.js, since it should be put through the same tests as append/prepend/after/before...

[chemerisuk]

The main reason of why this method should return innerHTML is type consistency. Obviously for empty form elements like <input> type of returned value is String, therefore it should be String for regular elements as well. It's a poor practice if method can have different types of returned value (get/set method like $Element#css is an exception).

Another feature is that innerHTML in general is more completed than Array<$Element>. For example <a>foo<i>bar</i></a> has foo<i>bar</i> as innerHTML, but only one child <i> (should we add a test for that?). So I think having both methods make sense (at present you can retrieve Array<$Element> using $Element#children) for different use cases.

Most of logic from manipulation.js is about constructing a single fragment that allows to insert multiple elements using a single call. It doesn't make sense to implement the same for $Element#value I think.

[nateroling]

Ok, that makes sense.

I found a wrinkle, though. With a text input input, calling input.set(DOM.create("div")) should probably set the input's value to "<div></div>", correct?

The current implementation uses input.set("").append(val), so we're clearing the input's value, then appending a child.

To do it properly we'd have to check the type of the element we are calling set on and decide whether set(val) should deal with child elements or the node's value. Is there anything in the library that is doing that check already?

I updated my branch with the suggestions you added a failing test case for this.

[chemerisuk]

@nateroling don't worry about this edge case, you can call appendChild on empty elements in vanilla DOM without an error, so it's a valid operation.

[chemerisuk]

So new method $Element#value will be introduced in 2.1.1.