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

      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;}

        Gliffy Diagrams

          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: