[PATCH] fix rendering of boolean attributes in tag helper methods

The new patch should be working :)

Hey Emilio,

I saw your references to W3Schools to the boolean attributes, but I think they might be out-of-date on the HTML5 specs. I left some further comments on github and wanted to get some feedback from you guys? Of course it might raise the question of Rails following the HTML5 spec totally (or just leaving things as they work in all browsers):

http://github.com/rails/rails/commit/1e2d7229602f467cfdc0ef606b5ef8...

+1 For tiegz comment

This brings the produced HTML out of compliance with the HTML5 draft specs. All prior HTML specs have treated boolean attributes in the same manner, so there's no reason to believe they'd suddenly change direction with "true"/"false" values.

What's the preferred method for fixing something like this Yehuda? Doing a revert on the commit and creating a new patch that does not include the incorrect changes? or a new patch that will correct the errors created by this patch?

Marc : Better to just create a new patch fixing the issue.

Left a comment on github:

How about changing ActionView::Helpers::TagHelper#tag_options to handle boolean arguments differently? So that tag_options(:disabled => true) would render to disabled="disabled" and tag_options(:disabled => false) to "", just as in the HTML docs. However, I'm not sure what the implications would be for existing applications, that depend on the fact that currently tag_options(:foo => true) returns foo="true".

If anyone is interested, I've written a patch which addresses the issues described above and gets rid of the TagHelper#BOOLEAN_ATTRIBUTES array as well. Though, if there are any projects out there which rely on the fact that tag_options(:foo => false) returned " foo=\"false\"", they will break... On the other hand the method proposed by the patch is (in my humble opinion) much more logical and can be used to do some stuff like: audio_tag 'some.mp3', :autoplay => @user_preferences.autoplay?, which will either omit or define the attribute "autoplay", based on a boolean value.

PS: added some more tests as well, to check for the escape option and adopted some more tests (forms etc.) to use boolean values.
PPS: As described above, I've no idea if this patch breaks anything else, the tests run fine though :)

Btw - if somebody used to do text_field_tag 'foo', :disabled => 'yes' it will return invalid HTML though... honestly, I think it's ok like that, boolean attributes should be used with boolean values. FYI - in fact some tag helpers depend on a boolean value being passed (i.e. select_tag for :multiple), otherwise they might not function correctly anyway.

The HTML5 spec specifically disallows true/false and allows the lowercased attribute value or empty string only. I'd say write a patch against current master, rather than reverting

Lukas, I don't understand what the need is for eliminating the #BOOLEAN_ATTRIBUTES array. Your patch introduces new, unexpected functionality for HTML boolean attributes and I don't see where you've delineated the need to do so.

I'm going to add a patch here in a second, against the master as suggested by Yehuda, that will simply return the expected functionality of boolean attributes to what they were before this patch was applied. If you really think its important to change it to the way it is in your patch, I'd recommend creating a new ticket explaining the need, attach your patch and get community feedback/review to see if such a change is deemed necessary.

@wycats was that a question? The phrasing is kind of throwing me off. If you're asking if those are all the HTML5 boolean attributes, yes they are (at least according to the current draft). If you're asking why I included all of them, I just figured I might as well add them all while I had the HTML5 spec in front of me. If you weren't asking a question, then nevermind. :)

I was asking about forward compatibility. This isn't an issue because the problem only affected HTML5 attributes and we have them all covered, right?

Yes, that's correct. All HTML5 boolean attributes are now covered according to the current draft of the HTML5 specs.

Automatic cleanup of spam.

Automatic cleanup of spam.