Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.4.1, 2.5
    • Fix Version/s: 2.5
    • Component/s: Libraries
    • Labels:
    • Testing Instructions:
      Hide

      Check that tabs are displayed and can be navigated on pages:

      Pages where usage of print_tabs() was replaced with $OUTPUT->tabtree():

      1. /admin/roles/manage.php
      2. Badges
      3. Course administration->Groups
      4. Database module
      5. Wiki module

      Also search code for non substituted usage of print_tabs(), pick any page (or two) and check that print_tabs() still works

      Show
      Check that tabs are displayed and can be navigated on pages: Pages where usage of print_tabs() was replaced with $OUTPUT->tabtree(): /admin/roles/manage.php Badges Course administration->Groups Database module Wiki module Also search code for non substituted usage of print_tabs(), pick any page (or two) and check that print_tabs() still works
    • Affected Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE
    • Pull Master Branch:
      wip-MDL-38309-m25
    • Rank:
      48174

      Description

      The tab function in lib/weblib.php has a few issues with its API and the HTML that it outputs.

      It would be a good first step to move this code to the core renderer class.

        Issue Links

          Activity

          Hide
          Mary Evans added a comment - - edited

          David, this is more libraries issue and as such needs to be looked at by Petr Škoda. In fact Petr would be the best person to tell you what can and cannot be done with regards converting core code to renderers.

          Making this Libraries will assign this to Petr who can re-assign it to you or someone who can help you.

          Show
          Mary Evans added a comment - - edited David, this is more libraries issue and as such needs to be looked at by Petr Škoda . In fact Petr would be the best person to tell you what can and cannot be done with regards converting core code to renderers. Making this Libraries will assign this to Petr who can re-assign it to you or someone who can help you.
          Hide
          Martin Dougiamas added a comment -

          Adding Marina Glancy in case she can help too.

          Show
          Martin Dougiamas added a comment - Adding Marina Glancy in case she can help too.
          Hide
          Petr Škoda added a comment -

          Hello, I thought that we are going to remove all tabs and instead use navigation block. So either we deprecate the tabs or move the code to renderers, maybe we could simplify it a bit at the same if there are still some IE6 hacks in there...

          Show
          Petr Škoda added a comment - Hello, I thought that we are going to remove all tabs and instead use navigation block. So either we deprecate the tabs or move the code to renderers, maybe we could simplify it a bit at the same if there are still some IE6 hacks in there...
          Hide
          Martin Dougiamas added a comment -

          Well I don't know if this is such a hard policy. In some cases tabs do make more sense than navigation so we still need to create renderers for tabs.

          Show
          Martin Dougiamas added a comment - Well I don't know if this is such a hard policy. In some cases tabs do make more sense than navigation so we still need to create renderers for tabs.
          Hide
          David Scotson added a comment -

          Maybe we can get rid of two-level tabs? There seems to have been some movement in that direction already. I always feel like I'm looking at the preference pane of a Windows 95 video driver when I'm using those. And it seems they're currently not marked up any differently in the HTML so the stylesheets leave enough space beneath single layers of tabs for another one, just in case.

          If that change was made then the HTML and API could be massively simplified. The HTML would just be an unordered list of links.

          Show
          David Scotson added a comment - Maybe we can get rid of two-level tabs? There seems to have been some movement in that direction already. I always feel like I'm looking at the preference pane of a Windows 95 video driver when I'm using those. And it seems they're currently not marked up any differently in the HTML so the stylesheets leave enough space beneath single layers of tabs for another one, just in case. If that change was made then the HTML and API could be massively simplified. The HTML would just be an unordered list of links.
          Hide
          Petr Škoda added a comment -

          Oh, one level tabs seems like a good idea to me. Martin?

          Show
          Petr Škoda added a comment - Oh, one level tabs seems like a good idea to me. Martin?
          Hide
          Martin Dougiamas added a comment -

          Well, sounds good, BUT there are heaps of places that have two-level tabs (in addons too).

          So there are two issues:

          1) Policy for the future of Moodle. One-level tabs sounds good to me too and they work OK on touch devices so my +1 towards the count, hope most people agree.

          2) Support for plugins and current uses. Must support same API.

          We could have both and clearly mark the BC-one as deprecated.

          Show
          Martin Dougiamas added a comment - Well, sounds good, BUT there are heaps of places that have two-level tabs (in addons too). So there are two issues: 1) Policy for the future of Moodle. One-level tabs sounds good to me too and they work OK on touch devices so my +1 towards the count, hope most people agree. 2) Support for plugins and current uses. Must support same API. We could have both and clearly mark the BC-one as deprecated.
          Hide
          Marina Glancy added a comment -

          Submitting for integration.
          New interface is convenient for one-line tabs but can also be used for multiple row tabs.

          to print list of tabs
          instead of

          print_tabs(array($tabs), $selected, $inactive, $activated);
          

          one can use:

          $OUTPUT->tabtree($tabs, $selected, $inactive);
          

          Note that $tabs is not enclosed in another array and $activated is calculated automatically (parents of $selected)

          Show
          Marina Glancy added a comment - Submitting for integration. New interface is convenient for one-line tabs but can also be used for multiple row tabs. to print list of tabs instead of print_tabs(array($tabs), $selected, $inactive, $activated); one can use: $OUTPUT->tabtree($tabs, $selected, $inactive); Note that $tabs is not enclosed in another array and $activated is calculated automatically (parents of $selected)
          Hide
          David Scotson added a comment -

          Glossary tabs don't seem to have been converted over to the new version.

          I did a Bootstrap renderer and pushed it to a branch here:

          https://github.com/ds125v/theme_bootstrap/blob/99b6e1b5a2276b9d36d429b9d1af29c575b20598/renderers/core.php#L159-L213

          For nested tabtrees it just outputs the second one belowe the first, since in Bootstrap they don't support nesting on this component.

          However, if I'm understanding things right then 3rd party code will still be calling the old code, so I can't remove any workaround styles until that code has been fixed up?

          Show
          David Scotson added a comment - Glossary tabs don't seem to have been converted over to the new version. I did a Bootstrap renderer and pushed it to a branch here: https://github.com/ds125v/theme_bootstrap/blob/99b6e1b5a2276b9d36d429b9d1af29c575b20598/renderers/core.php#L159-L213 For nested tabtrees it just outputs the second one belowe the first, since in Bootstrap they don't support nesting on this component. However, if I'm understanding things right then 3rd party code will still be calling the old code, so I can't remove any workaround styles until that code has been fixed up?
          Hide
          Marina Glancy added a comment -

          Hi David,

          my comment on how to use new renderer function was quite compact. Let me write it in more details.
          The main function that display tabs is

            core_renderer::render_tabtree()
          

          It is defined as protected because it is not called directly. In order to use it developer writes:

            $tabtree = new tabtree(...);
            echo $OUTPUT->render($tabtree);
          

          but there is also a shortcut function tabtree() that will be used in the most cases because it saves a line of code:

            echo $OUTPUT->tabtree(...);
          

          This is exactly the same as it is with other output components such as action_link, pix_icon or single_button.
          Actually after looking at your code I thought that it might me a common mistake and we should define final functions such as core_renderer::tabtree() so theme developers override the render_tabtree() instead.

          For example if you search code for 'action_link', you will see that in the most cases it is used as $OUTPUT->action_link(...) but in some cases some backend API returns the instances of class action_link and frontend (i.e. renderer) calls $OUTPUT->render($actionlink);

          And in conclusion. Function print_tabs() create an instance of class tabtree and calls $OUTPUT->render($tabtree), so it does call the function core_renderer::render_tabtree() that you can override in your theme.

          So please in your code override functions core_renderer::render_tabtree() and core_renderer::render_tabobject() and they will be used by all tabs in core, regardless if they use function print_tabs() or call $OUTPUT->tabtree() or create instance of tabtree and call $OUTPUT->render($tabtree);

          Hope this explanation is better now

          Show
          Marina Glancy added a comment - Hi David, my comment on how to use new renderer function was quite compact. Let me write it in more details. The main function that display tabs is core_renderer::render_tabtree() It is defined as protected because it is not called directly. In order to use it developer writes: $tabtree = new tabtree(...); echo $OUTPUT->render($tabtree); but there is also a shortcut function tabtree() that will be used in the most cases because it saves a line of code: echo $OUTPUT->tabtree(...); This is exactly the same as it is with other output components such as action_link, pix_icon or single_button. Actually after looking at your code I thought that it might me a common mistake and we should define final functions such as core_renderer::tabtree() so theme developers override the render_tabtree() instead. For example if you search code for 'action_link', you will see that in the most cases it is used as $OUTPUT->action_link(...) but in some cases some backend API returns the instances of class action_link and frontend (i.e. renderer) calls $OUTPUT->render($actionlink); And in conclusion. Function print_tabs() create an instance of class tabtree and calls $OUTPUT->render($tabtree), so it does call the function core_renderer::render_tabtree() that you can override in your theme. So please in your code override functions core_renderer::render_tabtree() and core_renderer::render_tabobject() and they will be used by all tabs in core, regardless if they use function print_tabs() or call $OUTPUT->tabtree() or create instance of tabtree and call $OUTPUT->render($tabtree); Hope this explanation is better now
          Hide
          Marina Glancy added a comment -

          grrr, I wanted to add a link to the doc here but found no documentation about using/overriding render_xxx() methods.

          Show
          Marina Glancy added a comment - grrr, I wanted to add a link to the doc here but found no documentation about using/overriding render_xxx() methods.
          Hide
          Marina Glancy added a comment -

          To integrators: I added commit making function core_renderer::tabtree() final. Also added phpdocs to all similar existing core_renderer functions, can not make them final now because themes may already use them.

          Show
          Marina Glancy added a comment - To integrators: I added commit making function core_renderer::tabtree() final. Also added phpdocs to all similar existing core_renderer functions, can not make them final now because themes may already use them.
          Hide
          Mary Evans added a comment -

          Marina the only reference I know f that documents overriding a renderer is in this Moodle Doc written by Sam Helelryk.
          http://docs.moodle.org/dev/Themes_2.0_overriding_a_renderer

          Cheers
          Mary

          Show
          Mary Evans added a comment - Marina the only reference I know f that documents overriding a renderer is in this Moodle Doc written by Sam Helelryk. http://docs.moodle.org/dev/Themes_2.0_overriding_a_renderer Cheers Mary
          Hide
          Marina Glancy added a comment -

          Mary, unfortunately it does not explain the how method render() works:

          There is a method renderer_base::render() in the base class for every renderer in the system. And there is an interface renderable (without any function).
          One can define

          class mysuperobject implements renderable {
          }
          

          and in some renderer define function:

          class myplugin_renderer extends plugin_renderer_base {
            protected function render_mysuperobject(mysuperobject $obj) {
               return 'here is my super object: '.print_r($obj, true);
            }
          }
          

          And after that it is enough to call:

          $renderer = $PAGE->get_renderer('myplugin');
          echo $renderer->render(new mysuperobject(...));
          

          Sounds a little complicated until you get used to it.
          Advantage of this is that you don't need to know the function name for displaying the object. The typical example is the backend function that returns an array of objects to display edit icons: some icons have links, some not, so some objects are instances of action_link and some are pix_icon. Frontend just does not care, it calls render() for each element in array.

          Another example is the object action_link itself. Instead of text the property action_link::$text can contain any renderable object (usually pix_icon). Function core_renderer::render_action_link() has code:

              protected function render_action_link(action_link $link) {
                  if ($link->text instanceof renderable) {
                      $text = $this->render($link->text);
                  } else {
                      $text = $link->text;
                  }
                  ...
              }
          
          Show
          Marina Glancy added a comment - Mary, unfortunately it does not explain the how method render() works: There is a method renderer_base::render() in the base class for every renderer in the system. And there is an interface renderable (without any function). One can define class mysuperobject implements renderable { } and in some renderer define function: class myplugin_renderer extends plugin_renderer_base { protected function render_mysuperobject(mysuperobject $obj) { return 'here is my super object: '.print_r($obj, true ); } } And after that it is enough to call: $renderer = $PAGE->get_renderer('myplugin'); echo $renderer->render( new mysuperobject(...)); Sounds a little complicated until you get used to it. Advantage of this is that you don't need to know the function name for displaying the object. The typical example is the backend function that returns an array of objects to display edit icons: some icons have links, some not, so some objects are instances of action_link and some are pix_icon. Frontend just does not care, it calls render() for each element in array. Another example is the object action_link itself. Instead of text the property action_link::$text can contain any renderable object (usually pix_icon). Function core_renderer::render_action_link() has code: protected function render_action_link(action_link $link) { if ($link->text instanceof renderable) { $text = $ this ->render($link->text); } else { $text = $link->text; } ... }
          Hide
          Marina Glancy added a comment -

          There is documentation! http://docs.moodle.org/dev/Output_renderers#method_render.28.29_and_interface_renderable
          I separated text about renderable objects into separate section and made couple of corrections

          Show
          Marina Glancy added a comment - There is documentation! http://docs.moodle.org/dev/Output_renderers#method_render.28.29_and_interface_renderable I separated text about renderable objects into separate section and made couple of corrections
          Hide
          Dan Poltawski added a comment -

          This has landed pretty late for 2.5, but I suppose its aligned with the major release features.

          I've integrated this now (I also added a commit to fix some whitespace issues)

          Show
          Dan Poltawski added a comment - This has landed pretty late for 2.5, but I suppose its aligned with the major release features. I've integrated this now (I also added a commit to fix some whitespace issues)
          Hide
          David Scotson added a comment -

          I had another go at a Bootstrap renderer based on this after Marina's pointers above. It's actually a bit simpler in the renderer since the tabobject/tabtree classes are doing some of the work I was doing in the renderer (though I think overall complexity is higher)

          https://github.com/bmbrands/theme_bootstrap/blob/moodle_25/renderers/core.php#L160-L192

          And this now handles both new and old API calls so if this renderer is acceptable then we can delete less/moodle/tabs.less and some of the stuff in less/moodle/responsive.less and save about 70 lines of CSS.

          Show
          David Scotson added a comment - I had another go at a Bootstrap renderer based on this after Marina's pointers above. It's actually a bit simpler in the renderer since the tabobject/tabtree classes are doing some of the work I was doing in the renderer (though I think overall complexity is higher) https://github.com/bmbrands/theme_bootstrap/blob/moodle_25/renderers/core.php#L160-L192 And this now handles both new and old API calls so if this renderer is acceptable then we can delete less/moodle/tabs.less and some of the stuff in less/moodle/responsive.less and save about 70 lines of CSS.
          Hide
          Marina Glancy added a comment -

          Hi David, I looked at your patch. I would recommend you to make all it more object-based and also using html_writer, especially for printing a link:

              protected function render_tabtree(tabtree $tabtree) {
                  if (empty($tabtree->subtree)) {
                      return '';
                  }
                  $firstrow = $secondrow = '';
                  foreach ($tabtree->subtree as $tab) {
                      $firstrow .= $this->render($tab);
                      if (($tab->selected || $tab->activated) && !empty($tab->subtree)) {
                          $secondrow = $this->tabtree($tab->subtree);
                      }
                  }
                  return html_writer::tag('ul', $firstrow, array('class' => 'nav nav-tabs')). $secondrow;
              }
          
              protected function render_tabobject(tabobject $tabobject) {
                  if ($tabobject->selected or $tabobject->activated) {
                      return html_writer::tag('li', html_writer::tag('a', $tabobject->text), array('class' => 'active'));
                  } else if ($tabobject->inactive) {
                      return html_writer::tag('li', html_writer::tag('a', $tabobject->text), array('class' => 'disabled'));
                  } else {
                      return html_writer::tag('li', html_writer::tag('a', $tabobject->text, array('href' => $tabobject->link)));
                  }
              }
          
          Show
          Marina Glancy added a comment - Hi David, I looked at your patch. I would recommend you to make all it more object-based and also using html_writer, especially for printing a link: protected function render_tabtree(tabtree $tabtree) { if (empty($tabtree->subtree)) { return ''; } $firstrow = $secondrow = ''; foreach ($tabtree->subtree as $tab) { $firstrow .= $ this ->render($tab); if (($tab->selected || $tab->activated) && !empty($tab->subtree)) { $secondrow = $ this ->tabtree($tab->subtree); } } return html_writer::tag('ul', $firstrow, array('class' => 'nav nav-tabs')). $secondrow; } protected function render_tabobject(tabobject $tabobject) { if ($tabobject->selected or $tabobject->activated) { return html_writer::tag('li', html_writer::tag('a', $tabobject->text), array('class' => 'active')); } else if ($tabobject->inactive) { return html_writer::tag('li', html_writer::tag('a', $tabobject->text), array('class' => 'disabled')); } else { return html_writer::tag('li', html_writer::tag('a', $tabobject->text, array('href' => $tabobject->link))); } }
          Hide
          Adrian Greeve added a comment -

          Pages that use the renderer all navigate properly
          Pages using the old function still work as well.
          Test passed.

          Show
          Adrian Greeve added a comment - Pages that use the renderer all navigate properly Pages using the old function still work as well. Test passed.
          Hide
          Dan Poltawski added a comment -

          Blooming Marvelous! It's time for a knees up - your changes are upstream!

          Thanks for making Moodle better!

          Toodle pip

          Show
          Dan Poltawski added a comment - Blooming Marvelous! It's time for a knees up - your changes are upstream! Thanks for making Moodle better! Toodle pip

            People

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

              Dates

              • Created:
                Updated:
                Resolved: