This project is archived and is in readonly mode.

#1762 ✓resolved
Felipe Coury

[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

  • Felipe Coury
  • CancelProfileIsBroken

    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

    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

    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.

  • Dan Pickett
  • Derander

    Derander August 8th, 2009 @ 02:07 AM

    +1 verified

    Rizwan's patch produces correct behavior and tests pass on 2-3-stable

  • Steve St. Martin

    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.

  • Steve St. Martin

    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

    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?

  • Steve St. Martin

    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.

  • Rebecca Frankel

    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.

  • Elad Meidar

    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.

  • John Pignata

    John Pignata August 9th, 2009 @ 02:57 AM

    +1 - verified stevestmartin's patch.

  • Jeremy Kemper

    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

    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

    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.

  • Josh Nichols

    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

    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.

  • Steve St. Martin

    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

    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

    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

    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)

    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

  • José Valim

    José Valim February 2nd, 2010 @ 10:39 AM

    • State changed from “open” to “resolved”
  • Jeremy Kemper

    Jeremy Kemper October 15th, 2010 @ 11:01 PM

    • Milestone set to 3.0.2
    • Importance changed from “” to “”

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

<h2 style="font-size: 14px">Tickets have moved to Github</h2>

The new ticket tracker is available at <a href="https://github.com/rails/rails/issues">https://github.com/rails/rails/issues</a>

Attachments

Pages