After an AHAH request, the wrapper <div> is rendered again. This seems unnecessary and results in the document having two <div>s with the same ID.

Steps to reproduce:

  1. Enable the ahah_helper_demo module.
  2. Go to '/ahah_helper_demo'.
  3. Change the dropdown from 'Private' to 'Company'.

The document now looks like this:

<form id="ahah-helper-demo-form" method="post" accept-charset="UTF-8" action="/ahah_helper_demo">
  <div>
    <div id="billing-info-wrapper">
      <div>
        <div class="messages error">Address field is required.</div>
        <div id="billing-info-wrapper">
          <fieldset>
            ...
          </fieldset>
        </div>
      </div>
    </div>
    ...
  </div>
</form>

Notice the two <div>s with the ID 'billing-info-wrapper'.

The attached patch removes the #prefix and #suffix from the AHAH form element before it is rendered. The result of the above steps with the patch applied is:

<form id="ahah-helper-demo-form" method="post" accept-charset="UTF-8" action="/ahah_helper_demo">
  <div>
    <div id="billing-info-wrapper">
      <div>
        <div class="messages error">Address field is required.</div>
        <fieldset>
          ...
        </fieldset>
      </div>
    </div>
    ...
  </div>
</form>
CommentFileSizeAuthor
#17 ahah_wrapper_elelemnt.patch1.3 KBjohn franklin
#8 ahah_helper_wrapperreplace.patch554 bytesdabblela
#3 ahah_helper.module.patch778 bytesantgiant
ahah_helper.module.patch781 byteswulff

Comments

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Version: 6.x-1.0 » 6.x-2.0

Thanks, I'll review this soon.

zeropaper’s picture

zeropaper
Primary language French
commented

It helped here +1 ;)

antgiant’s picture

antgiant commented
StatusFileSize
new ahah_helper.module.patch778 bytes
antgiant’s picture

antgiant commented

Just a warning. If you use the #prefix or #suffix to display content this patch will cause that content to disappear on ahah update.

Rok Žlender’s picture

Rok Žlender commented
Status: Needs review » Needs work

I don't think blindly removing prefix and suffix is the way to go. People use different ways to create divs which ahah then replaces. One thing that jumps out to me is if you don't use replace as a ahah method but rather append or after then removing suffix and prefix doesn't make sense. Unfortunately I don't have a solution how to solve this to please everyone.

jefkin’s picture

jefkin commented

I agree that a one size fits all solution is rarely correct.

I think replace though is the most common usage. At least it has been for me.

My thought is that the form definition could? / should? tell ahah_helper what to do. So, for example, if we say strip #prefix and #suffix is the 'default' action then:

if (!$form['#keep_wrapper'])
{
unset($form_item['#prefix'], $form_item['#suffix']);
}

If we say keeping #prefix and #suffix is the 'default' action then:

if ($form['#strip_wrapper'])
{
unset($form_item['#prefix'], $form_item['#suffix']);
}

perhaps a configuration for the module is the right way to handle choosing from the above?

sbandyopadhyay’s picture

sbandyopadhyay commented

+1

The patch in #3 works very well.

dabblela’s picture

dabblela commented
StatusFileSize
new ahah_helper_wrapperreplace.patch554 bytes

I have taken a slightly different approach to the issue with this patch. On the form item that specifies the wrapper and sets #tree, you would set an additional property ('#ahah_wrapper') to replace the repeated div:

$form['billing_info'] = array(
    '#type'   => 'fieldset',
    '#title'  => t('Billing information'),
    '#prefix' => '<div id="billing-info-wrapper">', // This is our wrapper div.
    '#suffix' => '</div>',
    '#tree'   => TRUE, // Don't forget to set #tree!
    '#ahah_wrapper' => 'replace',
  );
dabblela’s picture

dabblela commented
Version: 6.x-2.0 » 6.x-2.x-dev
Status: Needs work » Needs review
sbandyopadhyay’s picture

sbandyopadhyay commented

The patch in #8 works great. As it's just an optional setting, it won't affect anyone who decides against setting the flag -- and for those who don't want the repeated

, it works perfectly with just one extra step.

The only question is: is there anyone who WOULD want the wrapped div repeated?? If not, then the patch in #3 might just be better after all.

Anonymous’s picture

Anonymous (not verified) commented

I cannot see that anyone should want the wrapper div repeated. The page would not be valid html as you are not supposed to have multiple elements on the page with the same id anyway.

Even though the patches above work for the outer wrapper which is defined in the form, It doesn't work for the nested ahah on the vat text box because #prefix & #suffix were not defined in the form.

I am suggesting a fix in the js code that will see if the wrapper exists in the new content and if so replace the original wrapper completely rather than just replacing the contents.

I have also fixed the extra div that is introduced when the contents are replaced normally. This is due to a previous fix for a safari crash.

old code:

  // Manually insert HTML into the jQuery object, using $() directly crashes
  // Safari with long string lengths. http://dev.jquery.com/ticket/1152
  var new_content = $('<div></div>').html(response.data);
  .
  .
  .
  if (this.method == 'replace') {
   wrapper.empty().append(new_content);
  }

suggested fix:

  // Manually insert HTML into the jQuery object, using $() directly crashes
  // Safari with long string lengths. http://dev.jquery.com/ticket/1152
  var new_content = $('<div></div>').html(response.data);
  // Will remove this temp <div> tag later if replacing wrapper contents.
  .
  .
  .
  if (this.method == 'replace') {
      //Check if the wrapper was removed server side if not, find the wrapper in new_content and replace original wrapper.
      if(new_content.find(this.wrapper).size() > 0) {
          new_content = new_content.find(this.wrapper);
          wrapper.replaceWith(new_content);
      } else {
          //else, set new_content to the contents of the temp <div> and replace wrapper contents.
          new_content = new_content.contents();
          wrapper.empty().append(new_content);
      }
  }
d.sibaud’s picture

d.sibaud commented

#11 work perfectly.... until the form is validated with errors, then it returns to the form and if an ahah event is triggered the div duplication is there again .-(

neilnz’s picture

neilnz commented

Yeah, and I had a problem with a manual solution similar to #6:

if (arg(0) == 'ahah_helper') {
  // remove the wrapper divs to prevent them nesting
  unset($form['wrapper']['#prefix']);
  unset($form['wrapper']['#suffix']);
}

It worked perfectly until there was a validation error upon submitting the form. When re-rendering with the error, it obviously pulled out the stashed form that was missing the wrapper div, so this was missing from the form with the error. This meant my ahah events replaced nothing anymore :(

I'm going to try another route - adding the wrapper div using a form theme function rather than a prefix/suffix.

neilnz’s picture

neilnz commented

Ok gave up on that tack. It doesn't theme the elements normally.

Applied #8 and it works for me.

adamdicarlo’s picture

adamdicarlo commented

Patch at #3 works great for me.

john franklin’s picture

john franklin commented

The problem is actually in the ahah_helper.js file starting around line 17

  var new_content = $('<div></div>').html(response.data);

  // [... snip a dozen lines ....]  

  // Add the new content to the page.
  Drupal.freezeHeight();
  if (this.method == 'replace') {
    wrapper.empty().append(new_content);
  }
  else {
    wrapper[this.method](new_content);
  }

The new_content is a pair of DIVs with the replacement HTML. When the new HTML gets stitched into the document, the old wrapper DIV is emptied and the new version is stuffed inside the original.

What SHOULD happen is the old element is removed and the new one put in its place MINUS the extra DIV added on line 17. If you're using a TABLE instead of a DIV as your wrapper, then you end up with a second set of TABLE tags, and that really screws up the page.

john franklin’s picture

john franklin commented
StatusFileSize
new ahah_wrapper_elelemnt.patch1.3 KB

What might help is to create an AHAH Helper custom form element. Here's a patch that does that. The ahah_helper_wrapper form element includes two special attributes '#inner_prefix' and '#inner_suffix'. They're like '#prefix' and '#suffix', except they go inside the DIV, not outside. This is a great place to put TABLE tags so the AHAH helper DIV isn't tucked inside the table.

bendiy’s picture

bendiy commented

#8 worked for me.