Field UI JavaScript is broken after validation fails due to an error in toolbar.js [#841410]

Steps:
1. Go to admin/structure/types/manage/page/fields.
2. Try to add a new field, enter label and field name, but leave field and widget type as is.
3. Validation fails as expected, but the dynamic widget select list is broken. See: http://img.skitch.com/20100630-pfa2s8rypnbjfces3d47kjxtjm.png

Also, a JS error is shown in console: http://img.skitch.com/20100630-rjeyyx2f3i31pu3inj4jj1kbuf.png

Uncaught TypeError: Cannot call method 'match' of undefined

I've tested it in Chrome 5 and Firefox 3.5, occurred in both. Seems like it doesn't matter how the validation fails.

Comment	File	Size	Author
#9	overlay-remove-extra-js-css-841410-9.patch	1.77 KB	David_Rothstein
#9
#5	overlay-remove-extra-js-css-841410-5.patch	1.14 KB	David_Rothstein
#5

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

zserno CreditAttribution: zserno commented 30 June 2010 at 10:59

Title:

Field UI JavaScript is broken after validation fails due to en error in toolbar.js

» Field UI JavaScript is broken after validation fails due to an error in toolbar.js

Relevant code has been added in #535066: Use CSS3 / IE filter to render toolbar shadow.
Can somebody test it in IE? This is an IE specific bugfix, so if it works in IE, there could be a check in the code for browser type, so it wouldn't break in other browsers either. E.g.

if ($('#toolbar').css('filter')) {

Comment #2

zserno CreditAttribution: zserno commented 30 June 2010 at 11:32

Tested in IE, same applies: http://img.skitch.com/20100630-trpsyt685xwne3qb5jbsrasqbx.png

Comment #3

asrob

he/him

Hungarian

Hungary 🇭🇺 🇪🇺

CreditAttribution: asrob commented 30 June 2010 at 13:49

I have tested it in Chrome 5 and Firefox 3.6.6 on Arch Linux and I have not got this error.

Comment #4

casey CreditAttribution: casey commented 30 June 2010 at 15:54

Component:

toolbar.module

» overlay.module

This is actually an overlay issue; it removes the page_top region (which includes the toolbar), but the attached javascript in there is still being added to the page.

Comment #5

David_Rothstein CreditAttribution: David_Rothstein commented 1 July 2010 at 03:59

Status:

Active

» Needs review

File	Size
overlay-remove-extra-js-css-841410-5.patch	1.14 KB

This patch is kind of ugly, but I believe it's the correct solution.

Comment #6

zserno CreditAttribution: zserno commented 1 July 2010 at 10:30

Patch works great, I tested it in chrome 5, FF 3.6 and IE 7.
One edgecase: there is a drupal_render_page call before restoring the original css+js files. In theory, a module could use hook_page_build (which is called by drupal_render_page) to add it's own css or js, but those won't be included.
Is it something we should care?

Comment #7

casey CreditAttribution: casey commented 15 July 2010 at 10:39

Status:

Needs review

» Reviewed & tested by the community

overlay_render_region() is used to check for changes in content. Added css/js (through hook_page_build or any other way) aren't added tot the content of the region (but to <head>) so it doesn't really matter.

I don't think we can fix this in a cleaner way. Anyways, it does the job.

Comment #8

casey CreditAttribution: casey commented 15 July 2010 at 10:46

Toolbar bug mentioned in #1 does still exists too: #854956: Error: $("#toolbar").css("filter") is undefined.

Comment #9

David_Rothstein CreditAttribution: David_Rothstein commented 16 July 2010 at 02:53

File	Size
overlay-remove-extra-js-css-841410-9.patch	1.77 KB

Yes, I meant to respond here sooner, but that explanation sounds about right.

We already hint at that in the PHPDoc of overlay_render_region() but could perhaps explain it a bit more, so I added some additional PHPDoc to that effect in the attached patch. Shouldn't change the RTBC status unless someone disagrees with the new PHPDoc :)