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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

casey’s picture

Subscribing. I'll give this a shot later today.

nod_’s picture

Version: 7.x-dev » 8.x-dev
Category: bug » task

+1

@casey, must have been a big day :D

xjm’s picture

Title: drupal_add_tabledrag() still using durpal_add_js(), should return #attached array » drupal_add_tabledrag() still using drupal_add_js(), should return #attached array
Priority: Normal » Critical
Issue tags: +API change, +D8 cacheability, +Approved API change

This 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.

kscheirer’s picture

Title: drupal_add_tabledrag() still using drupal_add_js(), should return #attached array » drupal_add_tabledrag() still using drupal_add_js(), should return array for #attached
Assigned: Unassigned » kscheirer
Status: Active » Needs review
FileSize
425 bytes

Just coming to this issue, I used the setting example from https://api.drupal.org/api/drupal/developer%21topics%21forms_api_referen.... Is that sufficient?

Wim Leers’s picture

Title: drupal_add_tabledrag() still using drupal_add_js(), should return array for #attached » drupal_add_tabledrag() still using drupal_add_(js|library)(), should return array for #attached
Status: Needs review » Needs work

#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.

quicksketch’s picture

Coming 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:

  drupal_add_tabledrag($form['#attached'], 'webform-components', 'order', 'sibling', 'webform-weight');
  drupal_add_tabledrag($form['#attached'], 'webform-components', 'match', 'parent', 'webform-pid', 'webform-pid', 'webform-cid');

Then developers don't need to worry about merging the JS calls in manually.

Wim Leers’s picture

#6: indeed, that's much better :)

quicksketch’s picture

One 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.

Wim Leers’s picture

The "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?

catch’s picture

Renaming it to drupal_attach_tabledrag() doesn't seem like a bad idea in itself though.

Wim Leers’s picture

Indeed!

yurtboy’s picture

I still see drupal_add_js in that function so I assume this ticket still needs work?

nod_’s picture

Component: javascript » base system
Assigned: kscheirer » Unassigned
Status: Needs work » Needs review
Issue tags: +JavaScript
FileSize
10.39 KB

lol that's ugly.

New function signature.
drupal_attach_tabledrag($element, $options)

$options is an array of parameters, same ones as drupal_add_tabledrag.

Status: Needs review » Needs work

The last submitted patch, core-tabledrag-732022-13.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
501 bytes
10.39 KB

Just fixing the test exceptions.

quicksketch’s picture

New function signature.

drupal_attach_tabledrag($element, $options)

+  $table_id = $options[0];
+  $action = $options[1];
+  $relationship = $options[2];
+  $group = $options[3];
+  $subgroup = !empty($options[4]) ? $options[4] : NULL;
+  $source = !empty($options[5]) ? $options[5] : NULL;
+  $hidden = !empty($options[6]) ? $options[6] : TRUE;
+  $limit = !empty($options[7]) ? $options[7] : 0;

Why 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.

nod_’s picture

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.

nod_’s picture

vijaycs85’s picture

Issue summary: View changes
FileSize
13.92 KB
12.95 KB

Updating the patch (try) to address #16 and #17

The last submitted patch, 19: 732022-table-attach-19.patch, failed testing.

nod_’s picture

Need to add: group, table_id, relationship, action to the defaults.

vijaycs85’s picture

Issue tags: +Needs tests
FileSize
10.15 KB
21.06 KB

Found 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).

Status: Needs review » Needs work

The last submitted patch, 22: 732022-table-attach-22.patch, failed testing.

vijaycs85’s picture

Status: Needs work » Needs review

22: 732022-table-attach-22.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 22: 732022-table-attach-22.patch, failed testing.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
3.79 KB
21.23 KB

More fix for views test fails...

Status: Needs review » Needs work

The last submitted patch, 26: 732022-table-attach-26.patch, failed testing.

vijaycs85’s picture

Status: Needs work » Needs review

26: 732022-table-attach-26.patch queued for re-testing.

nod_’s picture

Looks great to me :)

catch’s picture

Status: Needs review » Reviewed & tested by the community

Same here.

Wim Leers’s picture

Status: Reviewed & tested by the community » Needs work

I see a lot of remaining minor problems. Rerolling.

Wim Leers’s picture

Status: Needs work » Needs review
Issue tags: +Spark, +Performance, +sprint
FileSize
33.85 KB
25.25 KB
  1. The RTBC'd patch did not yet remove drupal_add_tabledrag(), even though it was no longer being used! Neither were the docs migrated from there.
  2. Zero references to drupal_add_tabledrag() now remain.
  3. In several places, where #tabledrag could be used, it wasn't yet. Simply changing from #theme => table to #type => table fixes that.
  4. In fact, I think we should aim to never have to call drupal_attach_tabledrag(), unless you need custom tables, which means only 1% would need to call drupal_attach_tabledrag() explicitly. Doing otherwise would be bad DX.(field_ui has a custom table, hence they need to call it manually).
  5. This broke tabledrag for menus! And why? Because it didn't use #tabledrag but was calling drupal_attach_tabledrag() manually, which allowed an incorrect table_id to creep in, hence breaking things.
  6. There are now only 3 calls to drupal_attach_tabledrag():
    1. in drupal_pre_render_table() (duh)
    2. in \Drupal\field_ui\OverviewBase::tablePreRender() (because it's an alternative to #theme/#type => table)
    3. in theme_views_ui_rearrange_filter_form() because that one is rather tricky to grok and I don't want to break it

    Down from a few dozen or so.

  7. Manually tested everything to ensure it's still working as expected.
  • Patch in #26: 18 files changed, 233 insertions(+), 47 deletions(-)
  • This patch: 21 files changed, 283 insertions(+), 134 deletions(-)
Wim Leers’s picture

FileSize
33.78 KB
928 bytes

Removed two obsolete lines.

The last submitted patch, 32: 732022-table-attach-32.patch, failed testing.

vijaycs85’s picture

Updating 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().

Wim Leers’s picture

#35

  1. HAHAH! Epicfail! Wim--. Thanks :)
  2. Good catch :)
benjy’s picture

  1. +++ b/core/modules/views_ui/lib/Drupal/views_ui/Form/Ajax/Rearrange.php
    @@ -77,7 +77,11 @@ public function buildForm(array $form, array &$form_state) {
    +        array (
    

    Additional space.

  2. +++ b/core/modules/views_ui/views_ui.theme.inc
    @@ -296,12 +308,18 @@ function theme_views_ui_rearrange_filter_form(&$variables) {
    +        array (
    

    Space again.

  3. +++ b/core/modules/views_ui/views_ui.theme.inc
    @@ -309,6 +327,13 @@ function theme_views_ui_rearrange_filter_form(&$variables) {
    +      array (
    

    Again

Other than a few un-needed spaces above I think this is good for RTBC. Tested and all works well.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
32.27 KB
1.93 KB

Fixed 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.

webchick’s picture

Assigned: Unassigned » catch

This feels catch-ish.

catch’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/includes/common.inc
@@ -2867,69 +2877,71 @@ function drupal_get_library($module, $name = NULL) {
   // If a subgroup or source isn't set, assume it is the same as the group.

If the default is FALSE, then isset is always going to return TRUE no?

vijaycs85’s picture

@catch, the default is NULL and yes, if it NULL, we always get TRUE for isset and set group as subgroup and/or source.

catch’s picture

That was a dreditor issue, I meant this:

+  $tabledrag_id = &drupal_static(__FUNCTION__, FALSE);
+  $tabledrag_id = (!isset($tabledrag_id)) ? 0 : $tabledrag_id + 1;
vijaycs85’s picture

Status: Needs work » Needs review
FileSize
534 bytes
32.27 KB

right, FALSE + 1 = 1 which is not what we want here. here is the fix.

catch’s picture

NULL is already the default, so doesn't need to be passed explicitly.

vijaycs85’s picture

oh yes, updated...

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

I 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!

catch’s picture

Title: drupal_add_tabledrag() still using drupal_add_(js|library)(), should return array for #attached » Change notice: drupal_add_tabledrag() still using drupal_add_(js|library)(), should return array for #attached
Priority: Critical » Major
Status: Reviewed & tested by the community » Active

Well 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!

Wim Leers’s picture

Assigned: catch » vijaycs85

vijaycs85 created a placeholder change notice already ([#2160571]) and is going to write the actual text next week. Assigning to him for clarity.

claudiu.cristea’s picture

We got a regression with draggable table on image style form. #2171737: Regression: Draggable table broken on image style form may be related to this?

Wim Leers’s picture

Title: Change notice: drupal_add_tabledrag() still using drupal_add_(js|library)(), should return array for #attached » drupal_add_tabledrag() still using drupal_add_(js|library)(), should return array for #attached
Status: Active » Fixed
Issue tags: -Needs tests

vijaycs85 finished the change notice.

catch’s picture

Priority: Major » Critical
Wim Leers’s picture

#51: thanks, and sorry for not thinking of that.

Status: Fixed » Closed (fixed)

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

Wim Leers’s picture

Issue tags: -sprint