This project is archived and is in readonly mode.
[PATCH] Possible rendering bug on label_tag vs. radio_button_tag
-
-
CancelProfileIsBroken- 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- 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
- 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.22 KB
-
Dan Pickett+1 verified
-
Derander
+1 verified
Rizwan's patch produces correct behavior and tests pass on 2-3-stable
-
Steve St. Martin+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.45 KB
-
-
Steve St. Martin
- 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@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?
-
Steve St. Martin
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.
-
Rebecca Frankel
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.
-
Elad Meidar+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.
-
John Pignata
+1 - verified stevestmartin's patch.
-
Jeremy Kemper- 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
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
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.
-
Josh Nichols+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
- 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.
-
Steve St. Martin
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
@stevestmartin, I think in the comments of sanitize_to_id, we have a link to the specification :)
-
Rizwan Reza
- 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
- State changed from stale to open
- Tag cleared.
- Assigned user changed from Jeremy Kemper to José Valim
-
Prem Sichanugrist (sikachu)I also think using
sanitize_to_idis the way to go on this. I'm providing the patch which usesanitize_to_idHowever, 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
masterand2-3-stable -
José Valim
- State changed from open to resolved
-