Duplicate IDs in Form Radios

[Jon Elofson]

Howdy,
Sorry for the length of this...

There is an existing bug (http://solarphp.com/trac/core/ticket/37)
indicating that the radio form helper creates radio buttons with
duplicate IDs.

For example:
<label><input type="radio" name="foo" value="1" id="foo"
class="input-radio foo" />Yes</label>
<label><input type="radio" name="foo" value="0" id="foo"
class="input-radio foo" checked="checked" />No</label>

Additionally, if you will get a parent label for the radios

<label for="foo">Foo</label>

This, won't validate as XHTML since each radio element needs a unique
id. Additionally, each radio should have a label for the element
itself.

In other words:

<label>Foo</label>

<label for="foo-1"><input type="radio" name="foo" value="1" id="foo-1"
class="input-radio foo require" />Yes</label>
<label for="foo-2"><input type="radio" name="foo" value="0" id="foo-2"
class="input-radio foo require" checked="checked" />No</label>

Radios are sort of a special case because the group of radios share a
common name, but each radio needs to be unique.

So how can you handle this in Solar?
I took a stab at this and put some details/suggestions in the comments
of the ticket listed above.
My solution requires a change to 2 files: Solar_View_Helper_Form and
Solar_View_Helper_FormRadio

First, in the _Helper_Form there is a change in addElement() to check
for type radio, then begins a group, adds the radios, then ends the
group
    public function addElement($info)
    {
        // make sure we have all the info keys we need
        $info = array_merge($this->_default_info, $info);

        // fix up certain pieces
        $this->_fixElementType($info);
        $this->_fixElementName($info);
        $this->_fixElementId($info);
        $this->_fixElementClass($info);

        // place in the normal stack, or as hidden?
        if (strtolower($info['type']) == 'hidden') {
            // hidden elements are a special case
            $this->_hidden[] = $info;
        } else if (strtolower($info['type']) == 'radio') {
            // Radios should be in a group
            $this->beginGroup($info['label']);
            $this->_stack[] = array('element', $info);
            $this->endGroup();

        } else {
            // non-hidden element, non-radio
            $this->_stack[] = array('element', $info);
        }

        return $this;
    }

This seems to work well and solves the parent label problem. Now you
get a parent label without for="foo" .

Next, I altered _Helper_FormRadio to create a unique id for each radio
element. Basically, I take the current id of the radio group and
concatenate the word '-radio-' and an incrementing number at the end,
starting with 1.
I use the word radio so as not to conflict with Solar's id duplication
fix (_fixElementId). This method only looks at the radio group id, not
each radio element id. Without the word radio, it would still be
possible to get a duplicate ID.
Here is the FormRadio helper.

    public function formRadio($info)
    {
        $this->_prepare($info);
        $radios = array();

        // default value if none are checked.
        $radios[] = $this->_view->formHidden(array('name' =>
$this->_name, 'value' => null));

        // add radio buttons.
        // start with radio button numbered 1
    $count = 1;
   
    // get the id from the attribs array. This is set for each form element in
                // Solar_View_Helper_Form::_fixElementId()
                $id = $this->_attribs['id'];


        foreach ($this->_options as $opt_value => $opt_label) {

            // is it checked?
            if ($opt_value == $this->_value) {
                $this->_attribs['checked'] = 'checked';
            } else {
                unset($this->_attribs['checked']);
            }

            // Create a new id attribute based on the original.
                    // Add the word 'radio' so as not to conflict with renumbered
ids of other
                    // elements resulting from Solar_View_Helper_Form::_fixElementId()
                    $this->_attribs['id'] = $id.'-radio-'.$count;


            // build the radio button
            $radios[] = '<label for="'.$this->_attribs['id'].'"><input
type="radio"'
                      . ' name="' . $this->_view->escape($this->_name) . '"'
                      . ' value="' . $this->_view->escape($opt_value) . '"'
                      . $this->_view->attribs($this->_attribs) . ' />'
                      . $this->_view->escape($opt_label) . '</label>';
            $count++;

        }

        return implode("\n", $radios);
    }

Ultimately, it results in something like this (I took out the dt and dd tags):
<label>Foo</label>
<input type="hidden" name="foo" value="" />
<label for="foo-radio-1"><input type="radio" name="foo" value="0"
id="foo-radio-1" class="input-radio foo" checked="checked"
/>cool</label>
<label for="foo-radio-2"><input type="radio" name="foo" value="1"
id="foo-radio-2" class="input-radio foo" />stuff</label>

Does this seem like a viable solution?

Jon
_______________________________________________
Solar-talk mailing list
Solar-talk@...
http://mailman-mail5.webfaction.com/listinfo/solar-talk

[Jeff Moore-4]

On Sep 15, 2009, at 7:55 PM, Jon Elofson wrote:

> There is an existing bug (http://solarphp.com/trac/core/ticket/37)
> indicating that the radio form helper creates radio buttons with
> duplicate IDs.


We've had the same problem.  Clay, the original filer of the bug (3  
years ago!), solved the by doing this:

         // prep to make sure that ids are not identical
         if (isset($this->_attribs['id'])) {
             $e = 1;
             $id = $this->_attribs['id'];
         }

Ahead of the for loop in Solar_View_Helper_FormRadio and the following  
inside the loop:

             // Make sure ids are not identical for each radio button
             if (isset($this->_attribs['id'])) {
                 $this->_attribs['id'] = "{$id}-{$e}";
                 $e++;
             }

This converts id's into idname-1, idname-2, etc.

This has more than just implications for validation, there are also  
implications for targeting javascript.  We ran afoul of this when we  
removed our custom helper and went to the default one.  We had some  
form augmentation based on javascript that stopped working.

I think alternatively, I'd be tempted to use $opt_value as the id  
extension rather than a count.

so instead of

<input type="radio" value="1" id="myidname-1"
<input type="radio" value="0" id="myidname-2"

Which is taken from an actual page we have, you'd have:

<input type="radio" value="1" id="myidname-1"
<input type="radio" value="0" id="myidname-0"

Jeff

_______________________________________________
Solar-talk mailing list
Solar-talk@...
http://mailman-mail5.webfaction.com/listinfo/solar-talk

[Jon Elofson]

Thanks, Jeff.
That is how I am dealing with it too, although I was tacking on the
word '-radio-' because it is still theoretically possible to get a
duplicate id via the _fixElementId method. However, I suppose if that
happens you have other issues that need tending. I also like your idea
of using the opt_val over an incrementing integer. That would make it
easier to identify the element using jquery or similar js.

What do you think about forcing radios into a group? Or should you
always rely on beginGroup() and endGroup() in the form helper?

Jon


On 9/16/09, Jeff Moore <jeff@...> wrote:

>
> On Sep 15, 2009, at 7:55 PM, Jon Elofson wrote:
>
>> There is an existing bug (http://solarphp.com/trac/core/ticket/37)
>> indicating that the radio form helper creates radio buttons with
>> duplicate IDs.
>
>
> We've had the same problem.  Clay, the original filer of the bug (3
> years ago!), solved the by doing this:
>
>          // prep to make sure that ids are not identical
>          if (isset($this->_attribs['id'])) {
>              $e = 1;
>              $id = $this->_attribs['id'];
>          }
>
> Ahead of the for loop in Solar_View_Helper_FormRadio and the following
> inside the loop:
>
>              // Make sure ids are not identical for each radio button
>              if (isset($this->_attribs['id'])) {
>                  $this->_attribs['id'] = "{$id}-{$e}";
>                  $e++;
>              }
>
> This converts id's into idname-1, idname-2, etc.
>
> This has more than just implications for validation, there are also
> implications for targeting javascript.  We ran afoul of this when we
> removed our custom helper and went to the default one.  We had some
> form augmentation based on javascript that stopped working.
>
> I think alternatively, I'd be tempted to use $opt_value as the id
> extension rather than a count.
>
> so instead of
>
> <input type="radio" value="1" id="myidname-1"
> <input type="radio" value="0" id="myidname-2"
>
> Which is taken from an actual page we have, you'd have:
>
> <input type="radio" value="1" id="myidname-1"
> <input type="radio" value="0" id="myidname-0"
>
> Jeff
>
> _______________________________________________
> Solar-talk mailing list
> Solar-talk@...
> http://mailman-mail5.webfaction.com/listinfo/solar-talk
>

_______________________________________________
Solar-talk mailing list
Solar-talk@...
http://mailman-mail5.webfaction.com/listinfo/solar-talk