#1762 [PATCH] Possible rendering bug on label_tag vs. radio_button_tag - Ruby on Rails

Type	To find
responsible:me	tickets assigned to you
tagged:"@high"	tickets tagged @high
milestone:next	tickets in the upcoming milestone
state:invalid	tickets with the state invalid
created:"last week"	tickets created last week
sort:number, importance, updated	tickets sorted by #, importance or updated
Combine keywords for powerful searching.
Use advanced searching »

This project is archived and is in readonly mode.

#1762 ✓resolved

[PATCH] Possible rendering bug on label_tag vs. radio_button_tag

Reported by Felipe Coury | January 15th, 2009 @ 04:29 PM | in 3.0.2

The way the id is handled when you use a label_tag with a radio_button_tag is not consistent.

Replicating the problem

I created a GitHub project that isolates the problem: http://github.com/fcoury/labelbu...

And put on a live page with the problem: http://labelbug.felipecoury.com/

Problem summary

If you create a radio button:

radio_button_tag 'ctrlname', 'apache2.2'

along with a label tag to match it:

label_tag 'ctrlname_apache2.2', 'Apache 2.2'

And you try to click the label, it won't select the radio as expected, because the radio id rendering took out the dot, while the label for rendering did not.

HTML for radio:

<input id="ctrlname_apache22" name="ctrlname" type="radio" value="apache2.2" />

HTML for label:

<label for="ctrlname_apache2.2">Apache 2.2</label>

Comments and changes to this ticket

You flagged this item as spam.
Felipe Coury January 16th, 2009 @ 02:09 PM
http://blog.webbynode.com/2009/1...
CancelProfileIsBroken August 4th, 2009 @ 05:46 PM
- Tag changed from “2.1.2, action_view, label_tag, radio_button_tag, view, view_helper” to “2.1.2, action_view, bugmash, label_tag, radio_button_tag, view, view_helper”
Rizwan Reza August 7th, 2009 @ 09:29 PM
- Tag changed from “2.1.2, action_view, bugmash, label_tag, radio_button_tag, view, view_helper” to “2.3.3, action_view, bugmash, label_tag, radio_button_tag, view, view_helper”
verified

+1 Tested under 2.3.3. This only works if the label_tag has dot removed in id, by manually putting 'ctrlname_apache22' in your Rails Views. This is a consistency issue. It's probably good to solve this.
Rizwan Reza August 7th, 2009 @ 10:41 PM
- Tag changed from “2.3.3, action_view, bugmash, label_tag, radio_button_tag, view, view_helper” to “2.3.3, action_view, bugmash, label_tag, patch, radio_button_tag, view, view_helper”
verified this under 2.3.3 and master.

This is the patch with test case for solving this bug.
- label_tag.diff 2.2 KB
Dan Pickett August 7th, 2009 @ 11:45 PM
+1 verified
You flagged this item as spam.
Derander August 8th, 2009 @ 02:07 AM
+1 verified

Rizwan's patch produces correct behavior and tests pass on 2-3-stable
You flagged this item as spam.
Steve St. Martin August 8th, 2009 @ 02:50 AM
+1 verified I've attached a patch that modifies radio_button_tag instead of label_tag because radio_tag_button appears to not consistently use sanitize_to_id as all other tags do.
- patch.diff 2.5 KB
You flagged this item as spam.
Steve St. Martin August 8th, 2009 @ 02:53 AM
- Title changed from “Possible rendering bug on label_tag vs. radio_button_tag” to “[PATCH] Possible rendering bug on label_tag vs. radio_button_tag”
José Valim August 8th, 2009 @ 01:27 PM
@stevestmartin, your patch make a good assumption for pretty_name, but it won't work for pretty_tag_value (for example, it's not going to be downcased anymore).

Do you mind providing a new patch where pretty_tag_value was calculated as previously?
You flagged this item as spam.
Steve St. Martin August 8th, 2009 @ 03:14 PM
pretty_tag_value is actually the culprit as its the non standard, all other tag helpers just make a single call to sanitize_to_id, my recommendation is that if .downcase, etc.. is to be preserved that its moved to sanitize_to_id so is consistent with all form elements.
You flagged this item as spam.
Rebecca Frankel August 9th, 2009 @ 12:14 AM
stevesmartin's patch.diff verified on 2-3-stable

+1 that this is a bug and it is good to fix it I don't have an informed opinion on the pretty tag debate.
You flagged this item as spam.
Elad Meidar August 9th, 2009 @ 01:29 AM
+1 2-3-stable verified (stevestmartin's patch). i agree that it's important to keep behavior consistency between form elements.

patch applies to master too.
You flagged this item as spam.
John Pignata August 9th, 2009 @ 02:57 AM
+1 - verified stevestmartin's patch.
Jeremy Kemper August 9th, 2009 @ 09:36 AM
- State changed from “new” to “open”
Good idea, but it breaks compatibility with existing labels. Not good for stable branch since other things in peoples' apps will reference these DOM ids.

I think the patch should be applied to master. I'm not sure how best to deprecate in 2.3, other than a documentation update showing how the generated DOM id will change. Perhaps a config option to enable the Rails 3 behavior?
Rizwan Reza August 9th, 2009 @ 03:45 PM
I think it should not break the compatibility if only label_tag changes. The main problem is that the label tag isn't 'attaching' properly to the radio tag, and hence the issue.
José Valim August 9th, 2009 @ 03:56 PM
Rizwan,

If you look to sanitize_to_id method, you will see that it has the proper behavior and radio_button_tag is sanitizing wrongly. That said, bringing radio_button_tag behavior to label_tag, will also make label_tag behavior wrongly.

Besides, any changes on both label and radio is backward incompatible. Adding downcase to label_tag make an attribute like "myAttributes" become "myattribute", instead of "myAttribute". I know that this unlikely to happen, but we shouldn't break things, even if small, without a warning.
You flagged this item as spam.
Josh Nichols August 10th, 2009 @ 04:38 AM
+1 for fixing this inconsistency. Verified it applies to master, and tests pass.

The backwards incompatibility this would introduce is fairly subtle. If someone were to target the id of a label for css or javascript, they'll probably end up banging their head on their desks for awhile until they notice the difference of a period.
Jeremy Kemper August 10th, 2009 @ 05:09 AM
- Assigned user set to “Jeremy Kemper”
Josh, problem is that it's more than just a period. In any case, it breaks apps which runs counter to our stable-branch mandate.

I'm fine with pushing to master, but we need a deprecation strategy for 2.3 first.
You flagged this item as spam.
Steve St. Martin August 10th, 2009 @ 05:23 AM
let me know how you would like to proceed and ill update the patch. Maybe a nice regex that looks for characters that would have been stripped / mixed case and display a deprecated behavior message, and also an update in documentation that mentions the change?

but this also leaves the question should sanitize_to_id remove periods (as they are not valid id attributes)? and should sanitize_to_id downcase aswell?
José Valim August 10th, 2009 @ 08:23 AM
@stevestmartin, I think in the comments of sanitize_to_id, we have a link to the specification :)
Rizwan Reza January 20th, 2010 @ 11:31 AM
- State changed from “open” to “stale”
- Tag changed from “2.3.3, action_view, bugmash, label_tag, patch, radio_button_tag, view, view_helper” to “2.3.3, action_view, label_tag, patch, radio_button_tag, view, view_helper”
- Milestone cleared.
José Valim January 20th, 2010 @ 12:25 PM
- State changed from “stale” to “open”
- Tag cleared.
- Assigned user changed from “Jeremy Kemper” to “José Valim”
Prem Sichanugrist (sikachu) January 27th, 2010 @ 04:47 AM
I also think using sanitize_to_id is the way to go on this. I'm providing the patch which use sanitize_to_id

However, I'm disagreed with raise the deprecation warning. If you're using that tag helper then the warning will be appended to your log every time you use it until we take it off.

This patch can be applied on both master and 2-3-stable
- 0001-modify-the-behavior-of-radio_button_tag-to-use-sanit.patch 2.3 KB
José Valim February 2nd, 2010 @ 10:39 AM
- State changed from “open” to “resolved”
Jeremy Kemper October 15th, 2010 @ 11:01 PM
- Milestone set to “3.0.2”
- Importance changed from “” to “”
[bulk edit]