If you form_alter a textarea on to a node form with a weight that puts it before the body of the form, the teaser splitter breaks badly.

This was reported in http://drupal.org/node/192913, but the CCK code is still broken and not a good test of the problem, so I tested it by just form_altering a simple textarea (with no filter form) onto a node form with a weight less than zero.

When you try to use the splitter, it fails to produce the separate teaser box. When you try to re-join the information, the body completely disappears.

If the additional textarea has a weight that puts it below the body, things work correctly.

CommentFileSizeAuthor
#8 teaser-textarea.patch944 bytescatch
#5 teaser-textarea.patch945 byteskkaefer
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

KarenS’s picture

Title: Teaser splitter breaks if another textarea precedes the body on the form » CCK Breaker: Teaser splitter breaks if another textarea precedes the body on the form

Renaming to point out that not fixing this will impair CCK's ability to add textareas to a form. But it is equally true that any other module that wants to add a textarea to a node form will run into this, so it's not just a CCK issue.

cburschka’s picture

I wanted to begin with the real basics to get closer to identifying the problem, so I diffed a page with and without the additional textarea. Just to see if adding the field in the Forms API changed anything in obscure parts of the HTML code.

This may be obvious, but I just wanted to confirm that, barring build IDs etc, this is the only part of the HTML code that changes:

+<div class="form-item" id="edit-testfield-wrapper">
+ <label for="edit-testfield">Test: </label>
+ <textarea cols="60" rows="6" name="testfield" id="edit-testfield"  class="form-textarea resizable"></textarea>
+</div>

Is it possible that jQuery uses a numerical index to access either of the fields, which gets messed up by the addition of a field before that? I find this hard to believe, but hey.

For additional reference, here are the two functions that split and join the teaser field:

    // Join the teaser back to the body.
    function join_teaser() {
      if (teaser.val()) {
        body.val(trim(teaser.val()) +'\r\n\r\n'+ trim(body.val()));
      }
      // Hide and disable teaser
      $(teaser).attr('disabled', 'disabled');
      $(teaser).parent().slideUp('fast');
      // Change label
      $(this).val(Drupal.t('Split summary at cursor'));
      // Show separate teaser checkbox
      $(checkbox).hide();
    }

    // Split the teaser from the body.
    function split_teaser() {
      body[0].focus();
      var selection = Drupal.getSelection(body[0]);
      var split = selection.start;
      var text = body.val();

      // Note: using val() fails sometimes. jQuery bug?
      teaser[0].value = trim(text.slice(0, split));
      body[0].value = trim(text.slice(split));
      // Reveal and enable teaser
      $(teaser).attr('disabled', '');
      $(teaser).parent().slideDown('fast');
      // Change label
      $(this).val(Drupal.t('Join summary'));
      // Show separate teaser checkbox
      $(checkbox).show();
    }

Unless this bug is hiding in another obscure location, the two code blocks above should be enough to identify the problem - unfortunately I'm clueless about JS and jQuery, so I can't be of much help.

jp.stacey’s picture

I think this is a lot more complex than the above suggests, because (a) there are things happening in between HTML page load and teaser.js being called and (b) all the .js files monkey with the HTML DOM, so looking at the source doesn't specifically help.

With an extra textarea weighted above the body, I can remove the .parent() from $(teaser).parent().slideUp('fast') and $(teaser).parent().slideDown('fast'), and it sort of fixes the bug (still no teaser grippie - the drag bar that makes it bigger). However, it breaks the standard behaviour without the extra textarea.

teaser.js is a bit comment-free (and it e.g. gets the ID of edit-teaser-include via two different routes: first from Drupal.settings.teaserCheckbox, and later from this.id.substring(...) + 'include'). Here's a more annotated version:

// $Id: teaser.js,v 1.10 2007/11/13 14:04:08 dries Exp $

/**
 * Auto-attach for teaser behavior.
 *
 * Note: depends on resizable textareas.
 */
Drupal.behaviors.teaser = function(context) {
  // This breaks in Konqueror. Prevent it from running.
  if (/KDE/.test(navigator.vendor)) {
    return;
  }

  // For each textarea with teaser class, that hasn't already been processed
  $('textarea.teaser:not(.teaser-processed)', context).each(function() {
    // jQuery it and add the processed class so it doesn't get touched again
    var teaser = $(this).addClass('teaser-processed');

    // Move teaser textarea before body, and remove its form-item wrapper.
    // Drupal.settings.teaser.edit-teaser-js = edit-body
    var body = $('#'+ Drupal.settings.teaser[this.id]);
    // Drupal.settings.teaser.edit-teaser-js = edit-teaser-include
    var checkbox = $('#'+ Drupal.settings.teaserCheckbox[this.id]).parent();

    // Parent should just contain the teaser textarea, but sometimes contains a grippie too
    var parent = teaser[0].parentNode;
    // Transfer just the teaser textarea over to immediately before the body textarea
    $(body).before(teaser);
    // Remove parent, plus possible redundant grippie
    $(parent).remove();

    function trim(text) {
      return text.replace(/^\s+/g, '').replace(/\s+$/g, '');
    }

    // Join the teaser back to the body.
    function join_teaser() {
      if (teaser.val()) {
        body.val(trim(teaser.val()) +'\r\n\r\n'+ trim(body.val()));
      }
      // Hide and disable teaser
      $(teaser).attr('disabled', 'disabled');
      $(teaser).parent().slideUp('fast');
      // Change label
      $(this).val(Drupal.t('Split summary at cursor'));
      // Show separate teaser checkbox
      $(checkbox).hide();
    }

    // Split the teaser from the body.
    function split_teaser() {
      body[0].focus();
      var selection = Drupal.getSelection(body[0]);
      var split = selection.start;
      var text = body.val();

      // Note: using val() fails sometimes. jQuery bug?
      teaser[0].value = trim(text.slice(0, split));
      body[0].value = trim(text.slice(split));
      // Reveal and enable teaser
      $(teaser).attr('disabled', '');
      $(teaser).parent().slideDown('fast');
      // Change label
      $(this).val(Drupal.t('Join summary'));
      // Show separate teaser checkbox
      $(checkbox).show();
    }

    // Add split/join button.
    var button = $('<div class="teaser-button-wrapper"><input type="button" class="teaser-button" /></div>');
    // Element with ID = edit-teaser-include, but that's in Drupal.settings ?
    var include = $('#'+ this.id.substring(0, this.id.length - 2) +'include');
    // parent() just gets the parentNode
    $(include).parent().parent().before(button);

    // Extract the teaser from the body, if set. Otherwise, stay in joined mode.
    var text = body.val().split('<!--break-->', 2);
    if (text.length == 2) {
      // Put split text into teaser and body
      teaser[0].value = trim(text[0]);
      body[0].value = trim(text[1]);
      // Enable the teaser and show it; add behaviours and explanatory text to the input button
      $(teaser).attr('disabled', '');
      $('input', button).val(Drupal.t('Join summary')).toggle(join_teaser, split_teaser);
      $(teaser).show();
    }
    else {
      // Hide the teaser and the checkbox; add behaviours and explanatory text to the input button
      $(teaser).hide();
      $('input', button).val(Drupal.t('Split summary at cursor')).toggle(split_teaser, join_teaser);
      $(checkbox).hide();
    }

  });
};

Depending on the weighting of the extra textarea, the teaser sometimes magically acquires a grippie before teaser.js gets to it, and I think it ends up in a container element too (so teaser[0].parentNode returns a different node). I'm not sure how the Drupal.behaviors work, but my guess is that there's some sort of crawl through the form's DOM, in document order. That means that the presence of the textarea above the body/teaser fields invokes some behaviour that adds a grippie to everything resizable, before teaser.js is able to come in and rearrange things.

When it works, that's when teaser.js gets invoked before this other behaviour. That means that the teaser gets moved, then gets a grippie and a container element, and hence those .parent() calls are required to get out of that container element.

I haven't much more to add right now unless I can find what's adding the grippies and maybe prevent it from acting on the teaser textarea. I hope this all provides some rather incoherent clues, though.

kkaefer’s picture

For reference, here is a module that adds a textarea:


function textarea_teaser_form_alter(&$form, $form_storage, $form_id) {
  if ($form['#id'] == 'node-form') {
    $form['new_textarea'] = array(
      '#type' => 'textarea',
      '#title' => t('New textarea'),
      '#weight' => -10,
    );
  }
}

kkaefer’s picture

Status: Active » Needs review
FileSize
945 bytes

I found the bug: when adding a textarea before the actual body textarea, the file textarea.js gets added before teaser.js. This doesn’t happen when the node body textarea is the first one, becasue in theme_textarea, teaser.js is added at first. teaser.js only works correctly when it is the first file performing changes to the markup.

This patch solves this problem by adding a call to the teaser function in the textarea when the currently treated textarea is a teaser textarea.

RobLoach’s picture

Status: Needs review » Reviewed & tested by the community

Huge bug, huge patch. Well done, kkaefer. Patch applies cleanly, fixes the problem.

keith.smith’s picture

Status: Reviewed & tested by the community » Needs work

One minor nitpick:

+    // Make sure that teaser.js has done it's magic before converting this textarea.

it's should be its

(There's another patch somewhere in the queue that fixes about twenty similar ones all across core.)

catch’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
944 bytes

it's >> its

don't credit me if this is committed.

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Fixed

Um, hum. This is not exceptionally nice, but as we have no dependency system for behaviors, I cannot think of an elegant solution either. Committed this.

KarenS’s picture

Great detective work everyone, especially kkaefer. Thanks!

Anonymous’s picture

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for two weeks with no activity.