when we print in page.tpl.php "render($page['content'])" in the table
script in "tableselect.js" doing multiple execution
bug

in archive theme "tableselect_bug_theme" for demonstrating this bug
1) install theme
2) and go to "admin/content" or "admin/people" and you can see this bug

OR use this code:


  $header = array(
    array('class' => array('select-all')),
    '1',
  );
  $rows = array(
    array('<input type="checkbox" />', 2),
    array('<input type="checkbox" />', 3),
  );
  print '<table><tr><td>'.theme('table', array('header' => $header, 'rows' => $rows)).'</td></tr></table>';
CommentFileSizeAuthor
#13 tableselect-1319420-13.patch502 bytesnod_
#13 tableselect-1319420-13-D7.patch482 bytesnod_
#6 table.png25.01 KBdroplet
#5 tableselect-1319420-5.patch440 bytesnod_
#5 tableselect-1319420-5-D7.patch420 bytesnod_
tableselect_bug_theme.zip699 bytesmaxrys
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

maxrys’s picture

maxrys CreditAttribution: maxrys commented
Issue summary: View changes

correct text

maxrys’s picture

maxrys CreditAttribution: maxrys commented

+code

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented
Title: multiple execution in tableselect.js » multiple execution of tableselect.js with nested tables
Version: 7.8 » 7.x-dev

issue in Drupal.behaviors.tableSelect selector. Might want to select th.select-all and climb up the tree to the first table element. The following seems to work well.

$('th.select-all', context).closest('table').once('table-select', Drupal.tableSelect);

Ends up being faster than using :has() in the selector.

The obvious question is why you'd want to have nested tables, but I guess it's still a proper bug.

oriol_e9g’s picture

oriol_e9g
Primary language Catalan
Location Barcelona
CreditAttribution: oriol_e9g commented
Status: Active » Needs work

Go with a patch to test it

droplet’s picture

droplet CreditAttribution: droplet commented
Version: 7.x-dev » 8.x-dev

basically 2 problem

ONE: above report
TWO: .select-all checkbox would checked all page's tables

suggest in #2 solve half of the problem. but still need to patch to get .sticky-header works (check table's .select-all, .sticky-header should also checked automatically.)

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented
Assigned: maxrys » Unassigned
Status: Needs work » Needs review
FileSize
tableselect-1319420-5-D7.patch420 bytes
tableselect-1319420-5.patch440 bytes

That should not be the case, all selectors are scoped with the table element, now that it's the right table there shouldn't be any issue.

Do you have a example for #2 ?

droplet’s picture

droplet CreditAttribution: droplet commented
FileSize
table.png25.01 KB

latest problem is .sticky-header needs to checked.

maxrys’s picture

maxrys CreditAttribution: maxrys commented
Version: 8.x-dev » 7.10
Assigned: Unassigned » maxrys
aspilicious’s picture

aspilicious CreditAttribution: aspilicious commented
Version: 7.10 » 8.x-dev

Needs to be fixed in D8 first

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented

What's pointed out in #4 is the original Drupal behavior, not a bug introduced by a change of selectors. #4 is an unrelated feature request.

Please test #5 and confirm.

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented
Issue tags: +JavaScript

tagging

droplet’s picture

droplet CreditAttribution: droplet commented
Assigned: maxrys » Unassigned
Status: Needs review » Reviewed & tested by the community

Tested again. no problem as I mentioned. Sorry

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Reviewed & tested by the community » Needs work

I did not understand why this selector plus traversing is needed by merely looking at the patch.

We should add a short inline comment right above the line that explains why this is done this way. Primarily to prevent someone changing it back in the future.

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented
Status: Needs work » Needs review
FileSize
tableselect-1319420-13-D7.patch482 bytes
tableselect-1319420-13.patch502 bytes

I'm just reversing the traversal, :has is a proprietary jquery selector that can't be optimized by querySelectorAll (on top of being buggy in this case), so the code went from basically:

$('table', context).filter(function () { return $(this).find('th.select-all').length !== 0; });
Which select the all tables having a th.select-all child somewhere, to:

$('th.select-all', context).closest('table');
Which select the innermost table having a th.select-all and it's much faster, an element having only one immediate parent possible.

That's what it should have looked like from the beginning. I'd be more interested in a comment explaining why it's been done this way instead of the other. I'm not sure why would someone revert working code to broken code but here is the comment.

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented
Status: Needs review » Reviewed & tested by the community

Just added a comment and cleared that up, reverting to rtbc.

Dries’s picture

Dries CreditAttribution: Dries commented
Version: 8.x-dev » 7.x-dev

The extra comment should have addressed sun's concerns. Committed to 8.x. Moving to 7.x for Angie to consider.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Fixed

Looks fairly harmless, so committed and pushed to 7.x. Thanks!

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

Anonymous’s picture

(not verified) CreditAttribution: commented
Issue summary: View changes

+ code