I have an issue on fieldgroup that occurs on the fields-overview screen. When dragging the fields there, onDrop gives an js-error on the rowHandler.
rowHandlers.field is not a constructor
[Break on this error] var rowHandler = eval('new rowHa... + data.rowHandler + '(row, data);');
Now there are a couple of ways to look at this.
1/ A very easy fix:
Drupal.fieldUIOverview = {
// ... So here check if the function exists.
if (typeof(eval('rowHandlers.' + data.rowHandler)) == 'function') {
// Create the row handler, make it accessible from the DOM row element.
var rowHandler = eval('new rowHandlers.' + data.rowHandler + '(row, data);');
$(row).data('fieldUIRowHandler', rowHandler);
}
Eval will not give the actual error that occured, so by checking the type of the var will tell us if it is a function. That's the only way i know in js to really have the expected result.
2/ Field_group's extra attach goes like this on the fields overview:
Drupal.behaviors.fieldUIFieldsOverview = {
attach: function (context, settings) {
$('table#field-overview', context).once('field-field-overview', function() {
Drupal.fieldUIOverview.attach(this, settings.fieldUIRowsData, Drupal.fieldUIFieldOverview);
});
}
};So you could say, ok to alter the onChange or onDrop, or maybe not even hooking the format trigger for fields in that form, i could create my own object as well.
Drupal.fieldUIOverview.attach would be Drupal.fieldGroupUIOverview.attach or something.
What do you think? (i provided a patch for the js check if you find option 1 the best for all other contribs that want to do exotic stuff.)
| Comment | File | Size | Author |
|---|---|---|---|
| #8 | field_ui_js_settings.patch | 4.38 KB | yched |
| #7 | field_ui_js_settings.patch | 1.92 KB | yched |
| #6 | field_ui_js_settings.patch | 1.95 KB | yched |
| #4 | 964092.patch | 854 bytes | Stalski |
| patch.txt | 869 bytes | Stalski |
Comments
Comment #1
yched commentedYou need to upload files with the *.patch extension if you want the test bot and http://drupal.org/project/dreditor to recognize them.
Couldn't we just do
instead ?
Also, beware of trailing whitescpaces.
Comment #2
Stalski commentedThe problem is still there as "extra_field" and "field" are defined, they just can not be called since the methods are not needed. Unless you implement extra_fields and fields with empty methods.
The real question is to know if we can call it (evaluate it), without javascript braking. As soon as i try to use the js api object, then it will target all .draggables, so field and extra_fields as well.
Comment #3
Stalski commentedIssue http://drupal.org/node/964062 is dependent on this.
Comment #4
Stalski commentedNew patch
Comment #5
Stalski commentedUpdate:
- we could change the fact that on manageFields js_settings are not set in php for the fields, so field_group can do it only for its own rows
- maybe a faster approach is possible on the check if the rowHander exists, if it is needed what so ever!
Comment #6
yched commentedCleaner fix, after IRC discussion.
- Drupal.fieldUIOverview.attach needs to run only on rows that have js settings.
- field_ui_table_pre_render() needs to stop adding #js_settings on rows that don't have it.
Comment #7
yched commentedOops, same patch without the console.log().
Comment #8
yched commentedThis requires another check in onDrop : some rows don't have a rowHandler, because we're not watching them. onDrop only needs to act on watched rows.
There's another, different bug when dragging a group row on "Manage fields": onDrop relies on the presence of a hidden "region" row to detect which region we are in (
var regionRow = $(row).prevAll('tr.region-message').get(0);)"Manage fields", because it has only one region, did not add that region row, and the above fails.
If we want rows to be able to have JS behaviors in "Manage fields" (and field group is a use case), that region row must be here.
Attached patch should fix that.
Comment #9
yched commentedbetter title
Comment #10
Stalski commentedIt works very nice. Good work.
Comment #11
lsolesen commentedWhen will this be implemented in the released development version?
Comment #12
Stalski commentedbumping , field_group really needs this, maybe others too.
Comment #13
Stalski commentedComment #14
Stalski commentedComment #15
cgoffin commentedI applied the patch to a Drupal 7.0-beta2 version and it still gives the javascript error.
Comment #16
cgoffin commentedIt works against de latest drupal 7 dev version.
Comment #17
Stalski commentedYes, needs to be dev
Comment #18
donquixote commentedwe applied this patch on a site with beta3, and it fixed the problem. thx everybody.
Comment #19
webchickI don't totally grok everything going on in this patch, but looks like it fixes a pretty nasty bug in contrib.
Committed to HEAD. Thanks!
Comment #20
Stalski commentedthx a lot!