Make WordPress Core

Opened 8 years ago

Closed 8 years ago

#39739 closed defect (bug) (fixed)

Select All Checkbox Not Working

Reported by: reldev's profile reldev Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 4.7.3 Priority: normal
Severity: normal Version: 4.7
Component: Administration Keywords: has-patch commit fixed-major
Focuses: javascript Cc:

Description (last modified by SergeyBiryukov)

I'm seeing a problem that appears to be related to the change introduced by ticket #37973 in 4.7. I did not see this issue on 4.6.2, but have verified it also exists on 4.7.1 and 4.7.2.

In short, I have a "select all" checkbox in a table on an admin page that is not functioning properly if the checkbox is within a nested table. If I click the checkbox, it is never "checked" and the checkboxes below in the table are never checked either. I've simplified my html quite a bit in order to isolate the problem and here is an HTML snippet that can be used to reproduce the problem:

<table class="form-table">
  <tbody>
    <tr>
      <td>
        <table class="wp-list-table fixed widefat striped">
          <thead>
            <tr>
              <th scope="col" class="manage-column check-column">
                <label><input type="checkbox" class="check-all"></label>
              </th>
              <th scope="col">
                Name
              </th>
            </tr>
          </thead>
          <tbody id="the-list">
            <tr>
              <th scope="row" class=" check-column">
                <input type="checkbox" id="cb-select-1" name="nslb_import_rows" class="nslb-input" value="1">
              </th>
              <td>
                Steven<input type="hidden" name="s_1_1" class="nslb-input" value="Steven">
              </td>
            </tr>
            <tr>
              <th scope="row" class=" check-column">
                <input type="checkbox" id="cb-select-2" name="nslb_import_rows" class="nslb-input" value="2">
              </th>
              <td>
                David<input type="hidden" name="s_2_1" class="nslb-input" value="David">
              </td>
            </tr>
          </tbody>
        </table>
      </td>
    </tr>
  </tbody>
</table>

What's happening specifically is that the code in common.js on line 421 at the 4.7+ level ($body.on('click'...) is firing when I click the "manage-column check-column" checkbox because it's in a <tbody> element (nested as in my example above). The code on line 430 in common.js at the 4.6.2 level that 37972 replaced does not fire when I select the same checkbox and it functions properly. The code on line 443 and following in common.js at the 4.7+ level is attempting to determine how to toggle any "select all" checkboxes in a table header or footer but since we're incorrectly in this routine, "false" is being returned for the most part meaning the "manage-column check-column" checkbox will never be checked and the other checkboxes in the following <tbody> will not be checked either. It seems the code at the 4.7+ level is assuming checkboxes within a <tbody> would not also be a "select all" checkbox, but bounded by a <thead> and <tfoot> where the select all boxes would typically be. Because I've nested a table within another <tbody>, this code incorrectly fires.

Attachments (1)

39739.diff (571 bytes) - added by swissspidy 8 years ago.

Download all attachments as: .zip

Change History (14)

#1 @reldev
8 years ago

Sorry, I inadvertently referenced the ticket that introduced the bug as 37972 and it should 37973. The link to the ticket is correct however.

Last edited 8 years ago by reldev (previous) (diff)

#2 @afercia
8 years ago

Changing
$body.on( 'click', 'tbody .check-column :checkbox', function( event ) {
in
$body.on( 'click', 'tbody > .check-column :checkbox', function( event ) {
should fix it. @reldev could you please check if that addresses your specific use case?
Even if I'm not sure nested tables are a good thing, if the fix is so simple I'd be in favor of fixing it ;)

#3 @SergeyBiryukov
8 years ago

  • Component changed from General to Administration
  • Description modified (diff)

#4 @SergeyBiryukov
8 years ago

  • Milestone changed from Awaiting Review to 4.7.3

#5 @reldev
8 years ago

@afercia I can verify the fix you provided takes care of this problem. Thanks so much for the quick response!

@swissspidy
8 years ago

#6 @swissspidy
8 years ago

  • Keywords has-patch added

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


8 years ago

#8 @dd32
8 years ago

  • Keywords commit added

Although nested tables like this is an edge case, I see no reason not to make the selector more specific to ensure it works. 39739.diff looks good to me.

This ticket was mentioned in Slack in #core by jnylen. View the logs.


8 years ago

#10 @jnylen0
8 years ago

  • Owner set to SergeyBiryukov
  • Status changed from new to assigned

Handing this off to @SergeyBiryukov for commit per above Slack discussion.

#11 @SergeyBiryukov
8 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 40118:

Plugins: After [38703], adjust the selector for checkbox selection to account for nested tables.

Props afercia, swissspidy, reldev.
Fixes #39739.

#12 @SergeyBiryukov
8 years ago

  • Keywords fixed-major added
  • Resolution fixed deleted
  • Status changed from closed to reopened

#13 @SergeyBiryukov
8 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 40119:

Plugins: After [38703], adjust the selector for checkbox selection to account for nested tables.

Props afercia, swissspidy, reldev.
Merges [40118] to the 4.7 branch.
Fixes #39739.

Note: See TracTickets for help on using tickets.