Details

    • Type: Sub-task
    • Status: Closed
    • Priority: 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

      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.

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            lazydaisy Mary Evans added a comment - - edited

            David, this is more libraries issue and as such needs to be looked at by Petr Skoda. 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
            lazydaisy Mary Evans added a comment - - edited David, this is more libraries issue and as such needs to be looked at by Petr Skoda . 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
            dougiamas Martin Dougiamas added a comment -

            Adding Marina Glancy in case she can help too.

            Show
            dougiamas Martin Dougiamas added a comment - Adding Marina Glancy in case she can help too.
            Hide
            skodak Petr Skoda 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
            skodak Petr Skoda 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
            dougiamas 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
            dougiamas 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
            bawjaws 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
            bawjaws 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
            skodak Petr Skoda added a comment -

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

            Show
            skodak Petr Skoda added a comment - Oh, one level tabs seems like a good idea to me. Martin?
            Hide
            dougiamas 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
            dougiamas 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 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 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
            bawjaws 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
            bawjaws 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 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 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 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 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 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 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
            lazydaisy 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
            lazydaisy 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 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 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 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 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
            poltawski 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
            poltawski 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
            bawjaws 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
            bawjaws 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 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 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
            abgreeve 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
            abgreeve 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
            poltawski 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
            poltawski 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:
                  Fix Release Date:
                  14/May/13