When theming a form that contains a drag and drop table, usually some CSS and JS will be added, along with the tabledrag.js and necessary settings for that table. If this structure were cached however, the drag and drop functionality would not work because drupal_add_tabledrag() adds JavaScript directly to the page through drupal_add_js(), instead of using #attached properties.
For example we currently use something like this (taken from Webform):
$form['#attached'] = array(
'css' => array($path . '/webform.css' => array('preprocess' => FALSE, 'weight' => CSS_DEFAULT + 1)),
'js' => array($path . '/webform.js' => array('preprocess' => FALSE)),
);
drupal_add_tabledrag('webform-components', 'order', 'sibling', 'webform-weight');
drupal_add_tabledrag('webform-components', 'match', 'parent', 'webform-pid', 'webform-pid', 'webform-cid');
When we should be able to do something like this:
$form['#attached'] = array(
'css' => array($path . '/webform.css' => array('preprocess' => FALSE, 'weight' => CSS_DEFAULT + 1)),
'js' => array($path . '/webform.js' => array('preprocess' => FALSE)),
);
$drag_js_sibling = drupal_add_tabledrag('webform-components', 'order', 'sibling', 'webform-weight');
$drag_js_parent = drupal_add_tabledrag('webform-components', 'match', 'parent', 'webform-pid', 'webform-pid', 'webform-cid');
$form['#attached']['js'] = array_merge($form['#attached']['js'], $drag_js_sibling, $drag_js_parent);
I'm not certain this particular syntax would work or if there is a more elegant syntax, but either way we'd need make drupal_add_tabledrag() return a JS array that could be put into #attached, rather than just calling drupal_add_js() inside of it.
Comment | File | Size | Author |
---|---|---|---|
#45 | 732022-table-attach-45.patch | 32.26 KB | vijaycs85 |
#45 | 732022-diff-43-45.txt | 527 bytes | vijaycs85 |
#43 | 732022-table-attach-43.patch | 32.27 KB | vijaycs85 |
#43 | 732022-diff-38-43.txt | 534 bytes | vijaycs85 |
#38 | interdiff.txt | 1.93 KB | Wim Leers |
Comments
Comment #1
casey CreditAttribution: casey commentedSubscribing. I'll give this a shot later today.
Comment #2
nod_+1
@casey, must have been a big day :D
Comment #3
xjmThis is a hard blocker for removing
drupal_add_js()
for #1839338: [meta] Remove drupal_set_*() drupal_add_*() in favour of #attached/response for out-of-band stuff, so bumping to critical.There is also a minor API change here around how
drupal_add_tabledrag()
would be called and used, so tagging accordingly. I spoke to @webchick about this issue and she confirmed that this change was approved as a part of the parent issue.Comment #4
kscheirerJust coming to this issue, I used the setting example from https://api.drupal.org/api/drupal/developer%21topics%21forms_api_referen.... Is that sufficient?
Comment #5
Wim Leers#4: sadly, no, it's slightly more involved than that. There's e.g. still the
drupal_add_library()
call and the$js_added
static. The end result must look like the description in the issue summary.Comment #6
quicksketchComing back to this a few years later, I think it would be easier if you just passed the entire $form['#attached'] array by reference into the call. i.e:
Then developers don't need to worry about merging the JS calls in manually.
Comment #7
Wim Leers#6: indeed, that's much better :)
Comment #8
quicksketchOne more thought: I think we should leave drupal_add_tabledrag() as a global "add to page" function, just like drupal_add_js/css/library, but make a second function for drupal_attach_tabledrag() that has the signature mentioned above.
Comment #9
Wim LeersThe "global"
drupal_add_(css|js|library)()
are going away, see #1839338: [meta] Remove drupal_set_*() drupal_add_*() in favour of #attached/response for out-of-band stuff. So I don't see why we'd want to do that?Comment #10
catchRenaming it to drupal_attach_tabledrag() doesn't seem like a bad idea in itself though.
Comment #11
Wim LeersIndeed!
Comment #12
yurtboy CreditAttribution: yurtboy commentedI still see drupal_add_js in that function so I assume this ticket still needs work?
Comment #13
nod_lol that's ugly.
New function signature.
drupal_attach_tabledrag($element, $options)
$options is an array of parameters, same ones as drupal_add_tabledrag.
Comment #15
dawehnerJust fixing the test exceptions.
Comment #16
quicksketchWhy in the world would you change a real set of parameters that can be documented and autocompleted and change it to a numerically indexed array? "Ugly" function parameters are better than artificially shortening it into a single $options parameter.
Comment #17
nod_Because I'm a horrible person.
Like I said, it's ugly. I don't remember exactly why I did it but it was getting in the way somewhere. Also a function with 9 parameters feels like a failure somehow.
Comment #18
nod_maybe this can benefit from #2113015: Add drupal_merge_attached() function, to merge #attached assets too
Comment #19
vijaycs85Updating the patch (try) to address #16 and #17
Comment #21
nod_Need to add: group, table_id, relationship, action to the defaults.
Comment #22
vijaycs85Found that the update needs to go on all getting called by drupal_pre_render_table() and controllers and confirmed locally, working as expected.
#15 - green shows that we don't have enough test to cover that the drag-and-drop works as expected. Since the changes in #15 attach only first event, block region select works, but not block weight change...
Reg #21 - didn't add group, table_id, relationship, action to default, as it changes the current behaviour (If anyone of mandatory field miss, should throw warning).
Comment #24
vijaycs8522: 732022-table-attach-22.patch queued for re-testing.
Comment #26
vijaycs85More fix for views test fails...
Comment #28
vijaycs8526: 732022-table-attach-26.patch queued for re-testing.
Comment #29
nod_Looks great to me :)
Comment #30
catchSame here.
Comment #31
Wim LeersI see a lot of remaining minor problems. Rerolling.
Comment #32
Wim Leersdrupal_add_tabledrag()
, even though it was no longer being used! Neither were the docs migrated from there.drupal_add_tabledrag()
now remain.#tabledrag
could be used, it wasn't yet. Simply changing from#theme => table
to#type => table
fixes that.drupal_attach_tabledrag()
, unless you need custom tables, which means only 1% would need to calldrupal_attach_tabledrag()
explicitly. Doing otherwise would be bad DX.(field_ui
has a custom table, hence they need to call it manually).#tabledrag
but was callingdrupal_attach_tabledrag()
manually, which allowed an incorrecttable_id
to creep in, hence breaking things.drupal_attach_tabledrag()
:drupal_pre_render_table()
(duh)\Drupal\field_ui\OverviewBase::tablePreRender()
(because it's an alternative to#theme/#type => table
)theme_views_ui_rearrange_filter_form()
because that one is rather tricky to grok and I don't want to break itDown from a few dozen or so.
18 files changed, 233 insertions(+), 47 deletions(-)
21 files changed, 283 insertions(+), 134 deletions(-)
Comment #33
Wim LeersRemoved two obsolete lines.
Comment #35
vijaycs85Updating patch with below changes:
1. Removing info.php
2. updating
$tabledrag_id = &drupal_static(__FUNCTION__ . '_setting', FALSE);
to$tabledrag_id = &drupal_static(__FUNCTION__, FALSE);
as it makes sense on drupal_add_tabledrag() had two static keys, but not in drupal_attach_tabledrag().Comment #36
Wim Leers#35
Comment #37
benjy CreditAttribution: benjy commentedAdditional space.
Space again.
Again
Other than a few un-needed spaces above I think this is good for RTBC. Tested and all works well.
Comment #38
Wim LeersFixed the 3 extraneous spaces reported in #37, plus one more.
Since this is a simple patch that has effectively been RTBC'd in #37 barring some stupid spaces, I'm marking this RTBC right now, to expedite things.
Comment #39
webchickThis feels catch-ish.
Comment #40
catchIf the default is FALSE, then isset is always going to return TRUE no?
Comment #41
vijaycs85@catch, the default is NULL and yes, if it NULL, we always get TRUE for isset and set group as subgroup and/or source.
Comment #42
catchThat was a dreditor issue, I meant this:
Comment #43
vijaycs85right, FALSE + 1 = 1 which is not what we want here. here is the fix.
Comment #44
catchNULL is already the default, so doesn't need to be passed explicitly.
Comment #45
vijaycs85oh yes, updated...
Comment #46
Wim LeersI don't get why we'd hold up this issue on such a tiny detail — it's the same as it was before!
In any case: back to RTBC!
Comment #47
catchWell I could have asked for a follow-up, but opening the issue would've taken as long as the re-roll in this case.
Committed/pushed to 8.x, thanks!
Comment #48
Wim Leersvijaycs85 created a placeholder change notice already ([#2160571]) and is going to write the actual text next week. Assigning to him for clarity.
Comment #49
claudiu.cristeaWe got a regression with draggable table on image style form. #2171737: Regression: Draggable table broken on image style form may be related to this?
Comment #50
Wim Leersvijaycs85 finished the change notice.
Comment #51
catchComment #52
Wim Leers#51: thanks, and sorry for not thinking of that.
Comment #54
Wim Leers