Moodle
  1. Moodle
  2. MDL-34293

Table headings inside the tbody rather than the thead section

    Details

    • Testing Instructions:
      Hide

      Prerequisite:
      You need firebug or able to see page source.

      Test:
      visit tables on these pages:
      table#modules /admin/modules.php
      table#partipants /user/index.php?id=1
      table.compatibleblockstable /admin/blocks.php

      Make sure first row in the table containing headers should be wrapped in <thead> tags and rest in <tbody>

      Show
      Prerequisite: You need firebug or able to see page source. Test: visit tables on these pages: table#modules /admin/modules.php table#partipants /user/index.php?id=1 table.compatibleblockstable /admin/blocks.php Make sure first row in the table containing headers should be wrapped in <thead> tags and rest in <tbody>
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull Master Branch:
      wip-mdl-34293
    • Rank:
      42659

      Description

      Some tables in Moodle put the table heading inside the tbody section rather than the thead. As well as being incorrect, this has at least two pragmatic effects. 1) It makes Moodle harder to theme, 2) it affects accessability.

      Some examples are:

      table#modules /admin/modules.php
      table#partipants /user/index.php?id=1
      table.compatibleblockstable /admin/blocks.php
      table.minicalendar /calendar/view.php?view=month&cal_d=1&cal_m=7&cal_y=2012&course=1

      There might be more, adding the following lines to a theme should highlight the issue as you browse the site, the second line hides a common false positive for vertical tables with headers as the left column:

      table tbody tr th

      {border: thick solid green !important; }

      table tbody tr th:first-child

      {border: none !important;}

        Issue Links

          Activity

          Hide
          Mary Evans added a comment -

          Thanks for reporting this David, it is certainly something which needs addressing. However this is not a theme issue, but an administrative one, so I am going to have to assign it to someone else.

          Cheers
          Mary

          Show
          Mary Evans added a comment - Thanks for reporting this David, it is certainly something which needs addressing. However this is not a theme issue, but an administrative one, so I am going to have to assign it to someone else. Cheers Mary
          Hide
          David Scotson added a comment -

          Thanks Mary. I've got a few other bugs of a similar nature, i.e. about core Moodle HTML that tripped me up when creating themes, in future I'll try and assign them directly to the relevant components.

          Show
          David Scotson added a comment - Thanks Mary. I've got a few other bugs of a similar nature, i.e. about core Moodle HTML that tripped me up when creating themes, in future I'll try and assign them directly to the relevant components.
          Hide
          David Scotson added a comment -

          So it seems like the common thread is the use of the "flexible table" library function, which forgets to add the <thead>:

          — a/lib/tablelib.php
          +++ b/lib/tablelib.php
          @@ -1050,6 +1050,7 @@ class flexible_table {
          function print_headers() {
          global $CFG, $OUTPUT;

          + echo html_writer::start_tag('thead');
          echo html_writer::start_tag('tr');
          foreach ($this->columns as $column => $index) {

          @@ -1120,6 +1121,7 @@ class flexible_table {
          }

          echo html_writer::end_tag('tr');
          + echo html_writer::end_tag('thead');
          }

          /**

          This seems to fix the modules, participatns, blocks, an internal report tool we've got but not the minicalendar (which is perhaps a special case anyway).

          Show
          David Scotson added a comment - So it seems like the common thread is the use of the "flexible table" library function, which forgets to add the <thead>: — a/lib/tablelib.php +++ b/lib/tablelib.php @@ -1050,6 +1050,7 @@ class flexible_table { function print_headers() { global $CFG, $OUTPUT; + echo html_writer::start_tag('thead'); echo html_writer::start_tag('tr'); foreach ($this->columns as $column => $index) { @@ -1120,6 +1121,7 @@ class flexible_table { } echo html_writer::end_tag('tr'); + echo html_writer::end_tag('thead'); } /** This seems to fix the modules, participatns, blocks, an internal report tool we've got but not the minicalendar (which is perhaps a special case anyway).
          Hide
          David Scotson added a comment -

          Let me know if I'm doing these github things right, this is my first go at it.

          Show
          David Scotson added a comment - Let me know if I'm doing these github things right, this is my first go at it.
          Hide
          David Scotson added a comment -

          To be honest, I'm really not sure where the code that writes out the <tbody> tags for flexible tables is. Before my change, it wraps them around all the <tr>s, afterwards it correctly opens the tag after the thead closes. It works, but I don't really understand why.

          Show
          David Scotson added a comment - To be honest, I'm really not sure where the code that writes out the <tbody> tags for flexible tables is. Before my change, it wraps them around all the <tr>s, afterwards it correctly opens the tag after the thead closes. It works, but I don't really understand why.
          Hide
          David Scotson added a comment -

          Okay, the explanation for the mystery in the last comment is that Moodle isn't adding <tbody> tags at all, but in both the Firefox and Chrome developer tools it will automatically add them to the DOM. However, if you look at the source they are not there.

          Some technical details here:
          http://stackoverflow.com/questions/1678494/why-does-firebug-add-tbody-to-table

          So I'm guessing a proper fix would add tbody tags too.

          Show
          David Scotson added a comment - Okay, the explanation for the mystery in the last comment is that Moodle isn't adding <tbody> tags at all, but in both the Firefox and Chrome developer tools it will automatically add them to the DOM. However, if you look at the source they are not there. Some technical details here: http://stackoverflow.com/questions/1678494/why-does-firebug-add-tbody-to-table So I'm guessing a proper fix would add tbody tags too.
          Hide
          David Scotson added a comment -

          Updated the github links to a version that adds tbody tags too.

          It only adds an opening tbody tag if there's a thead, I believe the tags are optional if you've got a single tbody and no thead or tfoot, but for simplicity it always adds a tbody closing tag before the table close tag.

          Show
          David Scotson added a comment - Updated the github links to a version that adds tbody tags too. It only adds an opening tbody tag if there's a thead, I believe the tags are optional if you've got a single tbody and no thead or tfoot, but for simplicity it always adds a tbody closing tag before the table close tag.
          Hide
          David Scotson added a comment -

          So reading up on when tbody is required in HTML5 suggests that long-term, leaving out the tbody is probably fine, as the browser will add it back in again anyway. If it was my call I'd probably just add the thead tags and leave out the tbody tags entirely today as it's a bit cleaner and the less PHP/HTML code the better.

          I've reverted the github link to the version that only adds thead (and updated the testing instructions to not worry about tbody), but I'll leave the other version below for comparison purposes:

          https://github.com/ds125v/moodle-bootstrap/tree/flexible_thead_tbody

          https://github.com/ds125v/moodle-bootstrap/compare/MOODLE_23_STABLE...flexible_thead_tbody

          Show
          David Scotson added a comment - So reading up on when tbody is required in HTML5 suggests that long-term, leaving out the tbody is probably fine, as the browser will add it back in again anyway. If it was my call I'd probably just add the thead tags and leave out the tbody tags entirely today as it's a bit cleaner and the less PHP/HTML code the better. I've reverted the github link to the version that only adds thead (and updated the testing instructions to not worry about tbody), but I'll leave the other version below for comparison purposes: https://github.com/ds125v/moodle-bootstrap/tree/flexible_thead_tbody https://github.com/ds125v/moodle-bootstrap/compare/MOODLE_23_STABLE...flexible_thead_tbody
          Hide
          Rajesh Taneja added a comment -

          Thanks for the patch David,

          I have combined your commits. I agree, thead and tbody should be added to table.

          Show
          Rajesh Taneja added a comment - Thanks for the patch David, I have combined your commits. I agree, thead and tbody should be added to table.
          Hide
          Frédéric Massart added a comment -

          Thanks David and Raj. Patch looks nice, please feel free to submit for integration whenever you like. (btw, typo in your commit message )

          Show
          Frédéric Massart added a comment - Thanks David and Raj. Patch looks nice, please feel free to submit for integration whenever you like. (btw, typo in your commit message )
          Hide
          Rajesh Taneja added a comment -

          Thanks Fred,

          Pushing for integration.

          FYI:
          Not back-porting as this might break some existing themes.

          Show
          Rajesh Taneja added a comment - Thanks Fred, Pushing for integration. FYI: Not back-porting as this might break some existing themes.
          Hide
          Dan Poltawski added a comment -

          Hi Guys,

          Thanks for this.

          What I don't understand why we need to do all the work in start_table_body() and end_table_body() and track whether the body has been sent?

          We already have start_output() which will print the headers, and then we can print the table body start tag directly after that, rather than checking in add_data()?

          Similarly can't we do the footer in finish_html()?

          It seems to me like we already have all the information in the class to decide whether or not to print the body tag.

          Show
          Dan Poltawski added a comment - Hi Guys, Thanks for this. What I don't understand why we need to do all the work in start_table_body() and end_table_body() and track whether the body has been sent? We already have start_output() which will print the headers, and then we can print the table body start tag directly after that, rather than checking in add_data()? Similarly can't we do the footer in finish_html()? It seems to me like we already have all the information in the class to decide whether or not to print the body tag.
          Hide
          Rajesh Taneja added a comment -

          Yes Dan, we can do that and that was our initial approach. But the cleaner approach is to track tbody start. Also, tbody should only be added if there is some row, so rather then adding to start_output this has been added when we add a row.

          Hope this make it clear.

          Show
          Rajesh Taneja added a comment - Yes Dan, we can do that and that was our initial approach. But the cleaner approach is to track tbody start. Also, tbody should only be added if there is some row, so rather then adding to start_output this has been added when we add a row. Hope this make it clear.
          Hide
          Dan Poltawski added a comment -

          Hi Raj,

          I don't see why we need to track tbody start. Tablelib will not output the table when there are no rows.

          See print_nothing_to_display() and this->started_output in add_data().

          Show
          Dan Poltawski added a comment - Hi Raj, I don't see why we need to track tbody start. Tablelib will not output the table when there are no rows. See print_nothing_to_display() and this->started_output in add_data().
          Hide
          Rajesh Taneja added a comment -

          Hello Dan,

          I have modified the patch and integrated your inputs.

          Show
          Rajesh Taneja added a comment - Hello Dan, I have modified the patch and integrated your inputs.
          Hide
          Dan Poltawski added a comment -

          Thanks Raj, i've integrated that now.

          Nice argument about semantics

          Show
          Dan Poltawski added a comment - Thanks Raj, i've integrated that now. Nice argument about semantics
          Hide
          Rajesh Taneja added a comment -

          Thanks Dan

          Show
          Rajesh Taneja added a comment - Thanks Dan
          Hide
          Ankit Agarwal added a comment -

          Works as described.
          Passing
          Thanks!

          Show
          Ankit Agarwal added a comment - Works as described. Passing Thanks!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          YEAR!*

          CAF*, TOT!*

          • Your effort amazingly resulted. (unbelievable :-P)
          • Closing as fixed.
          • Tons of thanks.
          Show
          Eloy Lafuente (stronk7) added a comment - YEAR!* CAF*, TOT!* Your effort amazingly resulted. (unbelievable :-P) Closing as fixed. Tons of thanks.

            People

            • Votes:
              1 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: