Posted by Darren Oh on September 2, 2008 at 8:01pm
24 followers
| Project: | Drupal core |
| Version: | 6.x-dev |
| Component: | javascript |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs review |
| Issue tags: | needs backport to D6 |
Issue Summary
Problem/Motivation
Drupal.tableDrag.prototype.initColumns hides the tabledrag ordering column, but skips checking elements that are already hidden. This means that the ordering column is not properly hidden in collapsed fieldsets, vertical tabs, or in elements initially hidden using #states.
Proposed resolution
#303189-52: Tabledrag doesn't hide columns when the whole table is initially hidden resolves the issue. It has been confirmed to work properly in major browsers, both when the table is initially hidden and when it is not.
- Firefox: confirmed
- Safari: confirmed
- Chrome: confirmed
- IE: confirmed
Remaining tasks
None.
User interface changes
None.
API changes
None.
Original report by @Darren Oh
Tested in Firefox 3. Elements in a collapsed fieldset inherit the display: none setting, and tabledrag.js skips elements with display set to none. Skipping does not seem to be necessary.
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| tabledrag.js_DRUPAL-6.patch | 744 bytes | Idle | Failed: Failed to apply patch. | View details | Re-test |
Comments
#1
Patch failed to apply. More information can be found at http://testing.drupal.org/node/14016. If you need help with creating patches please look at http://drupal.org/patch/create
#2
Bot needs to learn not to run check_plain() on issue titles.
#3
oh, so THAT's why I'm having problems trying to add tabledrag compatibility to location.
Subscribing.
#4
The header wasn't hiding either.
#5
I ran into this with a collapsed fieldset containing a draggable table. Manually applying the patch solves the problem for me at least in Safari4beta and Opera9.6.
#6
Any updates on this? WOuld really need this for Location module
#7
This is ready.
#8
New stuff and bugfixes go first to Drupal 7 to avoid regressions popping up. Needs review on D7.
#9
The last submitted patch failed testing.
#10
Patch for Drupal 7.
#11
The last submitted patch failed testing.
#12
#13
The last submitted patch failed testing.
#14
#15
#16
The last submitted patch failed testing.
#17
Marked #339105: Tabledrag not hiding columns inside collapsed fieldsets as a duplicate of this issue.
#18
This definitely needs work, as there's lots of cruft in tabledrag from getting it to work in older browsers. Please see this comment.
#19
#20
#425148: Form elements within a collapsed field group are not visible in Internet Explorer was marked as duplicate for this issue. Is it?
The symptoms mentioned here seems to be different then those mentioned there.
In addition can the above patch be applied for D6 as well or only D7?
thanks.
#21
Use the patch in #4 for Drupal 6.
#22
Keeping up to date with HEAD. Still needs review!
#23
Tagging this with 'Usability'
#24
Re-test of from comment #2242176 was requested by @user.
#26
This problem appeared together with cck multiple value fields, fieldgroup and vertical_tabs. To not patch the core itself I took the critical function (patched it) and put it in a separate js-file in my theme/js folder and I overwrote the function theme_content_multiple_values in the themes template.php like this:
<?php
function my_them_content_multiple_values($element) {
$output = theme_content_multiple_values($element);
drupal_add_js($GLOBALS['theme_path'].'/js/tabledrag-workaround.js', 'theme');
return $output;
}
?>
I attached the js-file (remove the txt-extension to use it).
#27
TS:
This is no longer the case in D7 as collapse only hides the wrapper with class .fieldset-wrapper. Does this mean this issue is fixed for D7?
#28
display:none is added by jQuery when the fieldset is hidden. So this is still waiting to be fixed.
#29
If it needs to be fixed why RTBC?
#30
Should be needs review, as #22 still hasn't had anyone review it. :)
#31
aspilicious: Drupal is waiting to be fixed. I reviewed #22, and it was ready to be committed. I'm attaching a simplified version with no major changes, so this is still reviewed and tested.
#32
Looks good to me. Needs testing across major browsers.
#33
The last submitted patch, tabledrag.js-303189-31.patch, failed testing.
#34
#31: tabledrag.js-303189-31.patch queued for re-testing.
#35
The index variable needs to be reset every time that we loop, in case there was a colspan and the index got adjusted.
#36
In the meantime, there is now a working patch that leaves the Safari workaround in place:
#769520: tabledrag does not work on hidden tables
#37
Marked #769520: tabledrag does not work on hidden tables as a duplicate of this issue.
#38
#35: drupal7-tabledrag.js_303189.patch queued for re-testing.
#39
The last submitted patch, drupal7-tabledrag.js_303189.patch, failed testing.
#40
The tabledrag JavaScript code in Drupal 7 seems to have changed a lot since the last patch was submitted, is this still an issue in D7?
#41
Bump. Definitely still an issue in Drupal 6.
#42
+1 subscribing, it needs to be fixed in D6 too.
#43
New patch against HEAD, removed unused variable
parentTag.#44
#45
I hit this bug too. My drag table is in a vertical tab that is not the first one so it's not selected in the initial loading of the page.
#46
subscribing
#47
subscribe
#48
Patch adapted a bit to clean up the var columnIndex. Patches correctly on both D7.2 and D8-dev. Works fine on D7.2 in firefox 4. Can anyone confirm and RTBC?
#49
adding tag
#50
The last submitted patch, drupal7-tabledrag.js_303189-48.patch, failed testing.
#51
#52
I did some further testing myself, and on tables with a colspan in the header it went wrong :(. Over optimised by removing the var index. So I reintroduced this var but did optimise the jquery object creation in the remainder (use a var cells instead of a var row) and that does work fine.
Can anyone confirm and RTBC?
#53
This fix also applies to the same bug when a table is initially hidden with #states.
It solves both. And the resulting code is easier to understand!
#54
The patch in #53 works for Drupal 7 aswell.
#55
Subscribing, ran into the same issue described in #45
#56
#57
Added an issue summary. It would probably be a good idea to state explicitly which browsers this has been tested in, and also confirm that the table behaves properly when the whole thing is not initially hidden.
#58
Re-RTBC, the issue summary is updated.
#59
Well this looks like a great code simplification, and a bug fix. Nice!
Committed and pushed to 8.x and 7.x. Marking back to 6.x.
#60
tested in FF 3.16 & Chrome 12.0.742
#61
Looks good, same code. Maybe we want one more person to test the patch functionally on D6.
#62
I just found this -- I had written a similar patch in a different, duplicate issue. I closed the other issue, http://drupal.org/node/842498#comment-5199120, for reference.
I have not tested vctoroc's patch -- it seems to make more dramatic alterations than I did, but the adjustment is very similar.
Something to note, this also affects JQuery Update module, since that module rewrites and replaces tabledrag. If this patch is accepted, would you re-file with JQuery update?
see: http://drupal.org/node/1298618
#63
Note that you can type the issue number like so:
[#1298618]To render a nice issue link like so: #1298618: tabledrag.js does not work properly with CCK "collapsed" fieldset
I'd just leave the jQ update issue you've already filed open. When this issue is fixed, that module can be updated as needed.
If you have the chance, could you test this patch and report the results? If you can confirm that it works, then we can mark this RTBC for D6.
#64
cool i always wondered how people got those nicely formatted links...
I will test when i get a chance, unfortunately, however, i'm pretty buried at the moment and actually ditched the tabledrag field i was using so i won't be able to get into to it right away.
#65
#60 worked for me in FF Mac 16.0.2.