#2864 [PATCH] fix rendering of boolean attributes in tag helper methods - Ruby on Rails

You do not have access to this ticket

This project is archived and is in readonly mode.

#2864 ✓resolved

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

Reported by Emilio Tagua | July 2nd, 2009 @ 09:17 PM | in 3.0.2

After adding video_tag in ticket #2843, we definitely should have the audio tag.

The patch:
- Adds the audio_tag and audio_path, with test and docs. - Fixes video_path typo and error in docs. - Changes audio and video attributes values so they are either true or false and not attribute's name value. This is specified in [1] and [2].

[1] http://www.w3schools.com/tags/html5_audio.asp [2] http://www.w3schools.com/tags/html5_video.asp

Comments and changes to this ticket

Emilio Tagua July 2nd, 2009 @ 10:04 PM
- Assigned user set to “Yehuda Katz (wycats)”
Yehuda Katz (wycats) July 3rd, 2009 @ 05:46 AM
- State changed from “new” to “incomplete”
git claims this patch is corrupted. Can you try again?
Emilio Tagua July 3rd, 2009 @ 01:29 PM
- no changes were found...
- add_audio_tag.diff 9 KB
Emilio Tagua July 3rd, 2009 @ 01:30 PM
The new patch should be working :)
Repository July 7th, 2009 @ 11:57 PM
- State changed from “incomplete” to “resolved”
(from [1e2d7229602f467cfdc0ef606b5ef8a5566a1501]) Adds a audio_tag helper for the HTML5 audio tag. Fixed video_path docs. HTML attributes values should be true or false not attribute's name. [#2864 state:resolved]

Signed-off-by: Yehuda Katz wycats@yehuda-katzs-macbookpro41.local
http://github.com/rails/rails/commit/1e2d7229602f467cfdc0ef606b5ef8...
You flagged this item as spam.
tiegz July 16th, 2009 @ 07:53 PM
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...
You flagged this item as spam.
Marc Love July 26th, 2009 @ 09:28 PM
+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?
Pratik July 26th, 2009 @ 10:01 PM
- State changed from “resolved” to “new”
Pratik July 26th, 2009 @ 10:02 PM
Marc : Better to just create a new patch fixing the issue.
You flagged this item as spam.
Lukas Westermann (lwe) July 26th, 2009 @ 10:20 PM
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".
You flagged this item as spam.
Lukas Westermann (lwe) July 27th, 2009 @ 12:02 AM
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.
- lwe-adapt_tag_options_method.diff 12.2 KB
Yehuda Katz (wycats) July 27th, 2009 @ 01:41 AM
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
You flagged this item as spam.
Marc Love July 29th, 2009 @ 11:17 PM
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.
You flagged this item as spam.
Marc Love July 30th, 2009 @ 12:08 PM
- Title changed from “[PATCH] add audio_tag to ActionView” to “[PATCH] fix rendering of boolean attributes in tag helper methods”
Attached is the new patch as requested, Yehuda, which fixes boolean attributes to follow the XHTML/HTML5 standards.
- fix-boolean-attributes.diff 5.2 KB
Repository July 30th, 2009 @ 05:48 PM
- State changed from “new” to “resolved”
(from [d860c8917023e62d24c239bbc9aa6c629704f416]) Fix tag helpers so that all HTML element boolean attributes render according to the specs. Added all boolean attributes listed in the XHTML 1.0 specs (http://www.w3.org/TR/xhtml1/guidelines.html) and HTML 5 specs (http://www.whatwg.org/specs/web-apps/current-work). HTML 5 boolean attribute rendering was broken in commit 1e2d7229602f467cfdc0ef606b5ef8a5566a1501 / [#2864 state:resolved].

Signed-off-by: Yehuda Katz wycats@gmail.com
http://github.com/rails/rails/commit/d860c8917023e62d24c239bbc9aa6c...
Yehuda Katz (wycats) July 30th, 2009 @ 05:49 PM
- Milestone cleared.
@marc the reason this is adequate is that it's only an HTML spec problem and we've included all HTML5 attributes? Seems reasonable to me.
You flagged this item as spam.
Marc Love July 30th, 2009 @ 08:50 PM
@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. :)
Yehuda Katz (wycats) July 30th, 2009 @ 09:22 PM
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?
You flagged this item as spam.
Marc Love August 1st, 2009 @ 08:20 AM
Yes, that's correct. All HTML5 boolean attributes are now covered according to the current draft of the HTML5 specs.
Ryan Bigg October 9th, 2010 @ 09:53 PM
- Tag cleared.
Automatic cleanup of spam.
Ryan Bigg October 9th, 2010 @ 10:00 PM
Automatic cleanup of spam.
Jeremy Kemper October 15th, 2010 @ 11:01 PM
- Milestone set to “3.0.2”
[bulk edit]
Ryan Bigg November 8th, 2010 @ 01:51 AM
Automatic cleanup of spam.