Erector allows nil for text content of tag/text methods

[leafo]

Valid in erector:

div nil
text nil
rawtext nil

Erector just uses to_s on nil, converting it to empty string. Fortitude errors.

Also noticed this difference:

div class: nil

In Erector: <div></div>
In Fortitude <div class></div>

[ageweke]

Hmmm. I'm having trouble reproducing this, which I'd imagine should be easy. Can you post a stack trace? I just added specs (

fortitude/spec/system/tag_rendering_system_spec.rb

Lines 14 to 32 in 4441327

	it "should allow rendering nil to a tag" do
	should_render_to("<p></p>") { p(nil) }
	end
	it "should allow rendering nil via text" do
	should_render_to("") { text(nil) }
	end
	it "should allow rendering nil via rawtext" do
	should_render_to("") { rawtext(nil) }
	end
	it "should render an attribute with no value if it's mapped to nil" do
	should_render_to("<p class></p>") { p(:class => nil) }
	end
	it "should render an attribute with no value if it's mapped to nil, even with a prefix" do
	should_render_to("<p foo-bar></p>") { p(:foo => { :bar => nil } )}
	end

) that make sure this works. rawtext nil did indeed fail, although I fixed it in my latest commit (4441327). But the others seemed to be working fine previously…

Also, I was seeing p :class => nil rendering <p class=""></p>, which I actually thought was kind of weird — I changed it so that it just renders <p class></p>, since there are cases in HTML where you want an attribute with no value (like <input type="checkbox" checked> — gross IMHO, but it's how it works IIRC).

However, my biggest concern is that we seem to be seeing different behavior from Fortitude. If you’re still seeing this, could you post stack traces for the errors, and full sample code for any rendering differences? I'm happy to separate out base classes or whatever…just concerned that we see different behavior (and different from the specs, which Travis runs on a pretty broad matrix).

[leafo]

Oh, I was mistaken. It was only for rawtext. Sorry about that. Thanks for the patch.

Also regarding the empty string attribute, that's also what happens for me, but chrome inspector stripped it out so I was also mistaken there.

I'm not sure I agree with converting nil into having a valueless attribute, mainly for cases where you might want the result of a function to control whether the attribute is there or not. Maybe re-purposing false and true might be useful. Erector seems to remove the attribute if false or nil is the value, does a to_s on true though.

[ajb]

I'm not sure I agree with converting nil into having a valueless attribute, mainly for cases where you might want the result of a function to control whether the attribute is there or not. Maybe re-purposing false and true might be useful. Erector seems to remove the attribute if false or nil is the value, does a to_s on true though.

@leafo is correct here, although I think it depends on the doctype. Here's how I updated my Erector fork to handle this: https://github.com/ajb/erector-rails4/blob/d2fb36cd2f15d075b1f7c371b0c0978775b52908/lib/erector/attributes.rb

IMO, the behavior (for HTML5, at least) should be:

div(class: nil) => <div>
div(class: false) => <div> (differs from behavior in my Erector fork)
div(class: true) => <div class>
div(class: 'true') => <div class='true'>

[ageweke]

Hmm. So here’s my thinking on this — I kind of want to type it out as I think through it, no matter where it leads. My goals for Fortitude around issues like these are, in order:

1. Do not emit invalid markup under any circumstances, at least on the levels that Fortitude is concerned with (i.e., it's not going to stop you doing <input type="foobar">, but it should avoid emitting <input>>>);
2. Be as straightforward and predictable as possible for someone new learning the language;
3. Where reasonable and not conflicting with 1 or 2, be compatible with Erector.

In other words, I would rather sacrifice some Erector compatibility at times if it makes Fortitude more internally consistent or easier to learn.

For the first item: Valueless attributes are allowed in both HTML4 (all variants) and HTML5 (see this article for more details, for example), but not allowed in XHTML doctypes since well-formed XML requires all attributes to have a value (even if it’s ""). Although it’s not like anybody uses XHTML any more (or ever did), I still think it’s valuable as a forcing function to keep Fortitude able to produce XML as well as HTML. So I should make sure that, no matter what you do, attributes always have a value in XHTML.

For the second item, it seems to get less clean. The HTML5 spec says that “boolean attributes” that need a false value must simply be omitted; if they need a true value, they must be present, with either no value at all (<tag attribute>), an empty string (<tag attribute="">), or the name of the attribute as the value (<tag attribute=attribute> or <tag attribute="attribute">).

The part I don’t like about this is that I suspect people will find it counterintuitive, particularly when using it on HTML5 data-* attributes. To me, it sure is a lot cleaner and more comprehensible to see <user data-disabled="false" data-active="true"> than <user data-active> (and does anybody know how JavaScript/jQuery/etc. return those various values using their support for data attributes?).

But, in the end, the spec always wins. So I think you’re right; it should be:

div # => '<div>'
div(:class => false) # => '<div>'
div(:class => true) # => '<div class>'
div(:class => 'true') # => '<div class="true">'

Then there’s the question of nil. In Ruby that’s falsey, but I don’t think we need to interpret it that way, because if you’re using the result of a Ruby expression in a Fortitude tag as a boolean attribute, you’ll need to force it to a boolean (probably just using !!) anyway lest you get an arbitrary Ruby object’s #to_s rendered as the value of your attribute. At the same time, having it be empty-string would make it "more truthy" than false, which would be weird, and if you really want an attribute with an empty string, you can do that trivially by just passing that explicitly.

So, I think I land at this (which I think agrees with both of you above):

div # => '<div>'
div(:class => nil) # => '<div>'
div(:class => "") # => '<div class="">'
div(:class => false) # => '<div>'
div(:class => true) # => '<div class>'
div(:class => 'true') # => '<div class="true">'

[ageweke]

Oh, and for XHTML, I think we end up with:

div # => '<div>'
div(:class => nil) # => '<div>'
div(:class => "") # => '<div class="">'
div(:class => false) # => '<div>'
div(:class => true) # => '<div class="class">'
div(:class => 'true') # => '<div class="true">'

I’ll make these changes this evening, when I have time.

[ageweke]

OK, I pushed this last night, and it passed all tests. I think we can call this one done. :)