Bug in teaser.js prevents other JS from working
Wim Leers - November 19, 2008 - 19:52
| Project: | Drupal |
| Version: | 6.x-dev |
| Component: | javascript |
| Category: | bug report |
| Priority: | critical |
| Assigned: | Wim Leers |
| Status: | needs work |
| Issue tags: | undefined |
Description
Teaser.js has a minor bug with big consequences:
Undefined value
http://localhost/d6/misc/teaser.js?O (line 74)(This happened both in Safari 3 and Firefox 3.)
And for some unknown reason (probably somebody with thorough JS/browser knowledge can explain this), this prevents other things from working, amongst which AHAH callbacks. Simply fixing teaser.js does the trick for me. The change is very trivial.
| Attachment | Size |
|---|---|
| teaser_undefined_value.patch | 1.58 KB |
| Testbed results | ||
|---|---|---|
| teaser_undefined_value.patch | passed | Passed: 10426 passes, 0 fails, 0 exceptions Detailed results |
#1
Forgot to check initially, but this also exists in HEAD! The same patch applies since teaser.js was unchanged.
#2
I'd be glad to test this and provide review, but I'm not sure how to repeat your problem.
Could you give me a few steps to repeat? I have D7 installed without the patch and see plenty of javascript sworking. The split summary button works, the menu form slides up and down, it finds tags that have already been used, etc.
Once I see the problem, I'll repeat with the patch and post the affirmative. Thanks!
#3
Just open a node/add page that uses teaser.js and you'll see this error in your JS console.
#4
Sorry if I'm wasting your time, but I tried Firefox 3.0.5 and Safari 3.2.1 on Mac. I confirmed that teaser.js was loading and played with the "Split Summary at Cursor". I have yet to receive an error on the most current D7 version (Dec. 26th).
Am I missing something?
#5
Hrm weird… I'll do some more tests then, with D6 *and* D7 in Firefox 3, Safari 3 and IE7 on both Windows and Mac OS X.
#6
Drupal 6.9 gives the same error with malfunctioning JS on node create pages, in every different browsers. If you preview the node JS functions again. In our case, on node create page body textarea has id of "edit-body-1", and on preview page it has id of "edit-body". Not sure if this is related but reported anyway.
Fortunately this patch fixes everything back.
Tx.
#7
I think this patch should be included in a next 6.x release, as this bug has a big impact. I can confirm this problem with a fresh 6.9 drupal and any other javascript feature. This patch fixed the problem.
#8
The last submitted patch failed testing.
#9
What it means that this patch failed only once on user language settings and passed 54 times? Does it mean that this patch fails or maybe it just means that the test it self failed? Forgive me for my ignorance, but is it possible passing a single test 54 times and fail only once and be that due to a wrong patch?
#10
Weird :S
#11
yes it seems that have an impact on fckeditor js ...
http://drupal.org/node/336753
when I apply the patch, fckeditor runs correctly, whitout patch, no fckeditor on my edit pages.
#12
Lets see if it passes all tests now. I haven't changed anything, just trying to retest as I don't understand why it fails.
mmm I just realized there was a re-test link. Just clicked on it.
#13
I did apply the patch to latest 7.x-dev and looks fine for me. no problems found. could some else review it?
#14
I can't reproduce this bug at all, FF3 on Windows. Tried loading the node add/edit page in D6 (latest stable), and D7 (HEAD). Tried pushing the 'split/join' button a few times. Also tried adding another textarea field (in D7, using core fields), and giving it a lower weight than the body field (thus trying to force the body to have an id of 'field-body-1', although it didn't work, id remained 'field-body'). No bug for me.
However, this patch does nothing except wrap a
(text !== undefined)check around the JS, so I see nothing wrong with it, even though it can't be reliably reproduced at the moment. It would be very unlikely for it to cause regression bugs, and it will certainly fix the bug for those users that are experiencing it.It seems that the problem occurs if the body field has a CSS id other than the one expected (i.e.
#edit-body). Ideally, we should work out and document the steps necessary to reproduce this scenario.#15
The last submitted patch failed testing.
#16
It need to be said that this bug exists also in d6.x, where it is not solved yet, and very easy to reproduce by installing fckeditor or gmaps. It might be better to create a new issue for d6.x so this could be commited for a next release of d6 and leave this issue for d7, as this testing failures (by the way, why does it fail?) keeps holding back this patch to get applied. So this is the issue for d6: http://drupal.org/node/395828
BTW, as far as I know, this bug shows up when enabling other js modules (fckeditor, gmaps) so it might not be reproduced if no other js modules are present in the node editing form page.
#17
yes you are right, i 'm working with drupal 6 and fckeditor installed. This bug is in d6 branch and it's a good idea to resolve the problem in d6 before d7 :-)
#18
Wrong. If the bug is in HEAD too, you must get it into HEAD first. Doing it the other way around is an invitation for regressions.
#19
right. got it. sorry about that.
#20
I was having issues with this (I am using gmap module on 6.10) and the patch fixed the issue for me.
+1 for commit.
#21
This is a seriously real 6.x bug as well; I'm commenting here in the hopes that others will not spend hours trying to figure this out on 6.x. If your FCK editor is vanishing -- and reappearing when you click Preview, or if your Firebug is giving you body.val errors, this is the patch to install. (Assume all other version numbers are the same, and you're running current 6.9> and FCKEditor 6.x-1.3-rc7.
+1 for commit
#22
Patches go into HEAD first. As explained in #18 by bdragon. Please stop changing the version. What we need for this patch to get committed, is a set of clear steps to reproduce this, and explain why it only happens on certain systems. That's why it's marked as CNW.
Since it's affecting so many people, I'm bumping it to critical.
#23
Hi, I also had problems with the "teaser.js" file, I'm not using FCKEditor at all, but the teaser was never correctly joined when I clicked on "edit". So I started debuging. With the use of JSLint and Google I came to the following solution (see attached file). This works absolutely perfect for me and validates perfectly in JSLint. I don't know how to apply a patch, maybe someone else can make a patch out of the attached file.
Kindest regards
Richard
PS: Would be great to see this change in Drupal 6.11 to, otherwise I'll have to apply the patch changes again and that sucks. ;-)
PPS: Nearly forgot to mention, works finde with jQuery 1.3.2
#24
I'm not sure what you're doing differently, Fleshgrinder? And it's definitely not against Drupal 6.10. Please provide a proper patch: http://drupal.org/patch/create.
#25
I'm really sorry, but that's way too much work at the moment for me. The patch is nearlly the same - you're right - but I had to made to slight changes to get the teaser.js working on my development server. I try to create it here:
- var text = body.val().split('<!--break-->', 2);- if (text.length == 2) {
- teaser[0].value = trim(text[0]);
- body[0].value = trim(text[1]);
- $(teaser).attr('disabled', '');
- $('input', button).val(Drupal.t('Join summary')).toggle(join_teaser, split_teaser);
- }
- else {
- $('input', button).val(Drupal.t('Split summary at cursor')).toggle(split_teaser, join_teaser);
- $(checkbox).hide().children('input').attr('checked', true);
+ var text = body.val();
+ if (text !== undefined) {
+ text.split('<!--break-->', 2);
+ if (text.length == 2) {
+ teaser[0].value = trim(text[0]);
+ body[0].value = trim(text[1]);
+ $(teaser).attr('disabled', '');
+ $('input', button).val(Drupal.t('Join summary')).toggle(join_teaser, split_teaser);
+ }
+ else {
+ $('input', button).val(Drupal.t('Split summary at cursor')).toggle(split_teaser, join_teaser);
+ $(checkbox).hide().children('input').attr('checked', true);
+ }
That's the patch from above, that's okay so far, the problem for me started with the last } which I haven't posted in the above code-block, the file goes further:
- // Make sure that textarea.js has done its magic to ensure proper visibility state.- if (Drupal.behaviors.textarea && teaser.is(('.form-textarea:not(.textarea-processed)'))) {
- Drupal.behaviors.textarea(teaser.parentNode);
- }
- // Set initial visibility
- if ($(teaser).is('[@disabled]')) {
- $(teaser).parent().hide();
- }
-
- });
-};
+ // Make sure that textarea.js has done its magic to ensure proper visibility state.
+ if (Drupal.behaviors.textarea && teaser.is(('.form-textarea:not(.textarea-processed)'))) {
+ Drupal.behaviors.textarea(teaser.parentNode);
+ }
+ // Set initial visibility
+ if ($(teaser).is('[disabled]')) { // Deleted the @ because this expression is deprecated within jQuery since (I think) version 1.3 and won't work upon that version of jQuery
+ $(teaser).parent().hide();
+ }
+ } // Here's the closing } from the above if-statement
+ });
+};
As I sad, the code now work's perfectly for me. Sorry for not providing a patch, but I have lot's to do at the moment and only wanted to share it, maybe it helps someone. :)
Kindest regards
Richard
PS: What do you mean whit "And it's definitely not against Drupal 6.10."? I'm not a native English speaker, sorry for the dump question.
#26
Hm. This issue is confusing. Could someone please:
a) Post clear, reproduceable steps on how to see this bug (go to page X, open Firebug console Y, look at line Z)
b) Verify that this bug *does* in fact exist in Drupal 7 (we're using jQuery 1.3.2 now, so it's possible it does not)
c) If it does not, state this and then bump the issue down to 6.x instead
d) If it does, re-roll the patch in a way that makes testing bot happy
Thanks.
#27
a) That's the problem indeed. It seems to occur only in certain circumstances. We haven't been able to nail down those circumstances yet.
b) It's in code that doesn't *use* jQuery, so yes, it's definitely still relevant in D7.
d) My initial patch applies: http://testing.drupal.org/pifr/file/1/teaser_undefined_value.patch. (All it does is wrap the code in a
if (text !== undefined) { … }check.)Fleshgrinder's changes can be ignored as they're not related to this issue. They're about jQuery 1.3 compatibility.
When I find the time, I'll try to narrow down reproducible steps.
#28
Thanks for the summary, Wim. :)
#29
I can try and verify these steps but:
1) Drupal 6.10
2) Install Location, Node Location, GMap, and GMap Location.
3) Go to node/add/page
4) ...
5) Fail.
You'll see you cannot collapse/expand fieldsets, etc.
#30
This problem definitely exists in 6.10 in FF3 and IE7 (it seems to only occur once you install a few contrib module)
The js error the shows up in firebug is:
body.val() is undefinedvar text = body.val().split('<!--break-->', 2);
The original patch on this issue is a simplest fix to just getting the collapsible fields back again and also allowing the reset of the javascript to execute on the page, but there is still a problem as the "Split summary at cursor"/"Join summary" button has disappeared with this fix since its now ignoring that section of code between the
if (text !== undefined) {since body is undefined??One obvious difference that I noticed was:
In a default install of 6.10 the Drupal.settings variable contained:
"teaserCheckbox": { "edit-teaser-js": "edit-teaser-include" }, "teaser": { "edit-teaser-js": "edit-body" }While in a 6.10 install with a number of other modules installed with this error appearing there is an extra '-1' in there:
"teaserCheckbox": { "edit-teaser-js-1": "edit-teaser-include" }. "teaser": { "edit-teaser-js-1": "edit-body" }So that got me wonder where the '-1' came from, so I checked what was the id of the form elements on the actually page, they also included the '-1', ie: "edit-teaser-include-1" and "edit-body-1".
In misc/teaser.js file:
$('textarea.teaser:not(.teaser-processed)', context).each(function() {
var teaser = $(this).addClass('teaser-processed');
// Move teaser textarea before body, and remove its form-item wrapper.
var body = $('#'+ Drupal.settings.teaser[this.id]);
var checkbox = $('#'+ Drupal.settings.teaserCheckbox[this.id]).parent();
That means that the body variable will be trying to query the form element "edit-body" which doesn't exist as it is "edit-body-1"
So that is the reason for the error being seen in firebug!
This got me curious where was the '-1' be coming from?
Digging around further I ended up at this function:
<?php
function theme_textarea($element) {
$class = array('form-textarea');
// Add teaser behavior (must come before resizable)
if (!empty($element['#teaser'])) {
drupal_add_js('misc/teaser.js');
// Note: arrays are merged in drupal_get_js().
drupal_add_js(array('teaserCheckbox' => array($element['#id'] => $element['#teaser_checkbox'])), 'setting');
drupal_add_js(array('teaser' => array($element['#id'] => $element['#teaser'])), 'setting');
$class[] = 'teaser';
}
// Add resizable behavior
if ($element['#resizable'] !== FALSE) {
drupal_add_js('misc/textarea.js');
$class[] = 'resizable';
}
_form_set_class($element, $class);
return theme('form_element', $element, '<textarea cols="'. $element['#cols'] .'" rows="'. $element['#rows'] .'" name="'. $element['#name'] .'" id="'. $element['#id'] .'" '. drupal_attributes($element['#attributes']) .'>'. check_plain($element['#value']) .'</textarea>');
}
?>
And wondered what was being passed in $element, what I got was:
Array
(
[#type] => textarea
[#rows] => 10
[#teaser] => edit-body
[#teaser_checkbox] => edit-teaser-include
[#disabled] => 1
[#post] => Array
(
)
[#programmed] =>
[#tree] =>
[#parents] => Array
(
[0] => teaser_js
)
[#array_parents] => Array
(
[0] => body_field
[1] => teaser_js
)
[#weight] => 0
[#processed] => 1
[#description] =>
[#attributes] => Array
(
[disabled] => disabled
[class] => img_assist
)
[#required] =>
[#input] => 1
[#cols] => 60
[#resizable] => 1
[#process] => Array
(
[0] => form_expand_ahah
[1] => img_assist_textarea
)
[#name] => teaser_js
[#id] => edit-teaser-js-1
[#value] =>
[#defaults_loaded] => 1
[#sorted] => 1
[#title] =>
)
The thing I noticed here is that
#id = edit-teaser-js-1
#teaser => edit-body
#teaser_checkbox => edit-teaser-include
But that still didn't answer why there was a '-1', so after more searching I came across the function form_clean_id which I realised added a number to the end of elements if there is already another form element with that id to avoid duplication. Did a little bit of debugging and released that every form element on my page has '-1' and taking a look at the values passed to the form_clean_id i realsed each item was being passed twice as if my entire form on the page was being generated twice and only the second one rendered?
Tried changing the theme to garland just to make sure it isn't something in my custom theme (error still occuring)
So this is as far as I have got... hopefully someone may be able to jump in from here and work out what is happen or fix up the #teaser and #teaser_checkbox variables so they match the id's on the site.
#31
Just to summaries my post above, there are two errors here:
1) the Drupal.settings variables for teaserCheckbox & teaser are hard coded to specific ID's when its possible if you have another form element on the page with edit-body for example that the actually form element will be something like edit-body-1 or edit-body-2
2) the edit node form is being generated twice
#32
At last, we are getting somewhere with this.
So the bug is both that the '#teaser' and '#teaser-checkbox' parameters are supposed to be element ids, which makes no sense because at this step of the form building we don't know the ID of elements, we only know their #name.
A plan to fix this:
* make #teaser and #teaser-checkbox specify element #names instead of #ids
* add a form_process_teaser() #process function to the 'textarea' element, and make that convert those #names into #ids
* don't touch anything neither in theme_textarea() nor in the javascript
#33
One thing that does actually fix this problem is if you hit preview after you go to node/%/add or node/%/edit and then hit preview - the page goes back to normal with the preview at the top - but all js works.
But that obviously is not a usable solution as I can't ask users to hit preview before adding or editing a node.
#34
same issue on drupal 6.12., so i subscribe.
Greetz from germany
#35
Subscribing. Would like to see this in the next version of 6. I'm using 6.12 right now.
-Tim
#36
I tried the steps in #29 and I still can't replicate the problem.
In form_clean_id there is a comment that tells us the javascript can be broken if it uses id selectors.
// Ensure IDs are unique. The first occurrence is held but left alone.// Subsequent occurrences get a number appended to them. This incrementing
// will almost certainly break code that relies on explicit HTML IDs in
// forms that appear more than once on the page, but the alternative is
// outputting duplicate IDs, which would break JS code and XHTML
// validity anyways. For now, it's an acceptable stopgap solution.
#37
The real question is why is it trying to generate the form twice? Thats the only way you end up with a form with '-1' appended to their ID's.
#38
Same problem in 6.12, subscribing.
#39
teaser.js no longer exists in HEAD
#40
Suddenly started getting this in 6.14 after disabling 90% of my modules in an attempt to debug something else.
#41
getting same problem with 6.14 - subscribing
#42
Getting issue too with 6.14 and FCKeditor. Subscribing...
#43
I tried to patch teaser.js with http://drupal.org/files/issues/teaser_undefined_value.patch but it doesn't seem to help. My issue occurs with one particular user (at least, but not my test user).