Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.9, 2.0
    • Fix Version/s: 1.9, 2.0
    • Component/s: Themes
    • Labels:
      None
    • Affected Branches:
      MOODLE_19_STABLE, MOODLE_20_STABLE
    • Fixed Branches:
      MOODLE_19_STABLE, MOODLE_20_STABLE
    • Rank:
      29899

      Description

      Add custom corner divs to boxes on Moodle pages.

      Create wrapper functions to add the necessary divs within boxes.

        Issue Links

          Activity

          Hide
          Urs Hunkler added a comment -

          The wrapper functions are:

          /**

          • Function adds custom_corners to boxes
            *
          • @param string $message, the content of the box
          • @param string $classes, space-separated class names.
          • @param string $ids, space-separated id names.
          • @param boolean $return, return as string or just print it
            */
            function print_custom_corners_box($message, $classes='generalbox', $ids='', $return=false) {

          $output = print_custom_corners_box_start($classes, $ids, true);
          $output .= stripslashes_safe($message);
          $output .= print_custom_corners_box_end(true);

          if ($return)

          { return $output; } else { echo $output; }
          }


          /**
          * Function adds custom_corners to boxes
          * Calls print_box_start
          *
          * @param string $classes, space-separated class names.
          * @param string $ids, space-separated id names.
          * @param boolean $return, return as string or just print it
          */
          function print_custom_corners_box_start($classes='generalbox', $ids='', $return=false) {
          global $CFG, $THEME;

          $output = print_box_start('ccbox '.$classes, $ids, true);

          if (!empty($THEME->customcorners)) { require_once($CFG->dirroot.'/lib/custom_corners_lib.php'); $output .= print_custom_corners_start(true, true); }

          if ($return) { return $output; }

          else

          { echo $output; }
          }


          /**
          * Function adds custom_corners to boxes
          * Calls print_box_end
          *
          * @param boolean $return, return as string or just print it
          */
          function print_custom_corners_box_end($return=false) {
          global $CFG, $THEME;

          if (!empty($THEME->customcorners)) { require_once($CFG->dirroot.'/lib/custom_corners_lib.php'); $output .= print_custom_corners_end(true); }

          $output .= print_box_end(true);;

          if ($return) { return $output; } else { echo $output; }

          }

          Show
          Urs Hunkler added a comment - The wrapper functions are: /** Function adds custom_corners to boxes * @param string $message, the content of the box @param string $classes, space-separated class names. @param string $ids, space-separated id names. @param boolean $return, return as string or just print it */ function print_custom_corners_box($message, $classes='generalbox', $ids='', $return=false) { $output = print_custom_corners_box_start($classes, $ids, true); $output .= stripslashes_safe($message); $output .= print_custom_corners_box_end(true); if ($return) { return $output; } else { echo $output; } } /** * Function adds custom_corners to boxes * Calls print_box_start * * @param string $classes, space-separated class names. * @param string $ids, space-separated id names. * @param boolean $return, return as string or just print it */ function print_custom_corners_box_start($classes='generalbox', $ids='', $return=false) { global $CFG, $THEME; $output = print_box_start('ccbox '.$classes, $ids, true); if (!empty($THEME->customcorners)) { require_once($CFG->dirroot.'/lib/custom_corners_lib.php'); $output .= print_custom_corners_start(true, true); } if ($return) { return $output; } else { echo $output; } } /** * Function adds custom_corners to boxes * Calls print_box_end * * @param boolean $return, return as string or just print it */ function print_custom_corners_box_end($return=false) { global $CFG, $THEME; if (!empty($THEME->customcorners)) { require_once($CFG->dirroot.'/lib/custom_corners_lib.php'); $output .= print_custom_corners_end(true); } $output .= print_box_end(true);; if ($return) { return $output; } else { echo $output; } }
          Hide
          Petr Škoda added a comment -

          Please watch your trailing whitespace and initialize all variables. Committing patch into cvs...

          Show
          Petr Škoda added a comment - Please watch your trailing whitespace and initialize all variables. Committing patch into cvs...
          Hide
          Martin Dougiamas added a comment -

          OK, we really need to revert this ASAP.

          We can not start asking all the modules to identify particular boxes for custom_corners support. It's too hard on the API.

          Two other solutions could be (in order of preference):

          1) Leave things alone (were they really that bad?)

          2) Fix print_box functions so that they do this when $THEME->customcorners is on, fix all the calls to them that might not provide good IDs to work with, and then fix the custom_corners theme so that it excludes any extra custom_corner boxes that should really not have corners displayed.

          And if this is really too much overhead then,

          3) come up with a more generic function like print_text_box or print_content_box ... it would work something like the patch in this bug but would at least be a bit more generic and describe the structure of the data rather than the appearance.

          Show
          Martin Dougiamas added a comment - OK, we really need to revert this ASAP. We can not start asking all the modules to identify particular boxes for custom_corners support. It's too hard on the API. Two other solutions could be (in order of preference): 1) Leave things alone (were they really that bad?) 2) Fix print_box functions so that they do this when $THEME->customcorners is on, fix all the calls to them that might not provide good IDs to work with, and then fix the custom_corners theme so that it excludes any extra custom_corner boxes that should really not have corners displayed. And if this is really too much overhead then, 3) come up with a more generic function like print_text_box or print_content_box ... it would work something like the patch in this bug but would at least be a bit more generic and describe the structure of the data rather than the appearance.
          Hide
          Martin Dougiamas added a comment -

          Petr, can you please revert this (solution 1) while we work/test/think about solution 2 and 3?

          Show
          Martin Dougiamas added a comment - Petr, can you please revert this (solution 1) while we work/test/think about solution 2 and 3?
          Hide
          Petr Škoda added a comment -

          yes

          Show
          Petr Škoda added a comment - yes
          Hide
          Petr Škoda added a comment -

          work in progress attached, pleasereview...

          Show
          Petr Škoda added a comment - work in progress attached, pleasereview...
          Hide
          Petr Škoda added a comment -

          Should be fixed in cvs now, head was a bit broken already I guess.
          Tested in FF and IE6 under wine.

          Show
          Petr Škoda added a comment - Should be fixed in cvs now, head was a bit broken already I guess. Tested in FF and IE6 under wine.
          Hide
          Petr Škoda added a comment - - edited

          Some more info:

          • implemented method 3/ above
          • We have now print_container_xxx() and print_box_xxx() functions, container is more general than box with no classes by default. All the code needed for themes using custom corners is only in weblib.php.
          • all themes were patched to use print_container_start() and _end() while keeping it backwards compatible
          • we are tracking open boxes and containers (we need this for ids in custom corners), the bonus is that error(), notice() and redirect() close open boxes and containers

          how to use:

          • print_box() - the same as before, internally uses containers with box css
          • print_container() - general div container for related elements in rectangular area, custom corners themes may draw round corners; in normal themes only one div used
          Show
          Petr Škoda added a comment - - edited Some more info: implemented method 3/ above We have now print_container_xxx() and print_box_xxx() functions, container is more general than box with no classes by default. All the code needed for themes using custom corners is only in weblib.php. all themes were patched to use print_container_start() and _end() while keeping it backwards compatible we are tracking open boxes and containers (we need this for ids in custom corners), the bonus is that error(), notice() and redirect() close open boxes and containers how to use: print_box() - the same as before, internally uses containers with box css print_container() - general div container for related elements in rectangular area, custom corners themes may draw round corners; in normal themes only one div used
          Hide
          Petr Škoda added a comment -

          if anybody objects I can revert/rework it easily

          Show
          Petr Škoda added a comment - if anybody objects I can revert/rework it easily
          Hide
          Dan Poltawski added a comment - - edited

          I've just upgraded a site, and i'm getting "Incorrect closing of custom corners - no more open containers" everywhere.. Not looked into it any further and watching the debug messages..

          Problem goes when switching to custom corners theme..

          Like the rework though.

          Show
          Dan Poltawski added a comment - - edited I've just upgraded a site, and i'm getting "Incorrect closing of custom corners - no more open containers" everywhere.. Not looked into it any further and watching the debug messages.. Problem goes when switching to custom corners theme.. Like the rework though.
          Hide
          Petr Škoda added a comment -

          eh, reopening

          Show
          Petr Škoda added a comment - eh, reopening
          Hide
          Petr Škoda added a comment -

          On which page are you getting the warning?
          Could you please copy/paste it here?

          Show
          Petr Škoda added a comment - On which page are you getting the warning? Could you please copy/paste it here?
          Hide
          Petr Škoda added a comment - - edited

          The previous code was ignoring double/missing closing of boxes/containers. There might be some places where this problem appears, I am going to add more debug info into print_footer to catch all these...

          Show
          Petr Škoda added a comment - - edited The previous code was ignoring double/missing closing of boxes/containers. There might be some places where this problem appears, I am going to add more debug info into print_footer to catch all these...
          Hide
          Petr Škoda added a comment -

          ... and thanks a lot for the testing!

          Show
          Petr Škoda added a comment - ... and thanks a lot for the testing!
          Hide
          Petr Škoda added a comment -

          found and fixed

          Show
          Petr Škoda added a comment - found and fixed
          Show
          Martin Dougiamas added a comment - http://moodle.cvs.sourceforge.net/moodle/moodle/theme/standard/header.html?r1=1.39&r2=1.40 should there be a clearfix there?
          Hide
          Petr Škoda added a comment -

          The clearfix is the first parameter in print_container(), right?

          Show
          Petr Škoda added a comment - The clearfix is the first parameter in print_container(), right?
          Hide
          Martin Dougiamas added a comment -

          Regarding the change to standard/header.html, do we really need them at all?

          Custom_corners can continue to use it of course, but I think 'standard' themes should NOT introduce this function here as it breaks compatibility with 1.8.

          http://moodle.org/mod/forum/discuss.php?d=85763

          Show
          Martin Dougiamas added a comment - Regarding the change to standard/header.html, do we really need them at all? Custom_corners can continue to use it of course, but I think 'standard' themes should NOT introduce this function here as it breaks compatibility with 1.8. http://moodle.org/mod/forum/discuss.php?d=85763
          Hide
          Petr Škoda added a comment -

          yes, the thems from 1.9 would not be compatible with 1.8, this could be partially fixed by backporting simplified print_container into 1.8.x
          if we remove it from standard themes the footer hacks will throw developer warnings in several places - though it might be better to fix them anyway

          Show
          Petr Škoda added a comment - yes, the thems from 1.9 would not be compatible with 1.8, this could be partially fixed by backporting simplified print_container into 1.8.x if we remove it from standard themes the footer hacks will throw developer warnings in several places - though it might be better to fix them anyway
          Hide
          Urs Hunkler added a comment -

          Several aspects are involved

          _1 backwards compatible themes:
          1.9 themes can't ever be really compatible to 1.8 because several CLASS names have changed from 1.8 to 1.9.
          +1 to implement the enhancement (compatibility arguments not really count).

          _2 consistency:
          Petr has done a great job to make the Moodle container handling more consistent. Now several hacks emerge which have been sleeping in the dark and affected advanced themes only. Backporting a simple print_container function to 1.8 would be marvelous.
          +1 to fix this hacks and to add the enhancement to the Moodle 1.9 standard themes.

          _3 In the standard theme we can show the enhancements created under the hood.
          +1 to fix this hacks and to add the enhancement to the Moodle 1.9 standard themes.

          _4 Martins argument to change the standard theme structure only when absolute necessary. This position is very wise to keep the Moodle changes as smooth as possible.
          -2 to implement the enhancement

          After calculating the numbers 3 - 2 I vote +1 to implement the enhancements. If Martin decides to not implement them now consistency needs to be reestablished in 2.0 with all themes using the enhancement.

          Show
          Urs Hunkler added a comment - Several aspects are involved _1 backwards compatible themes: 1.9 themes can't ever be really compatible to 1.8 because several CLASS names have changed from 1.8 to 1.9. +1 to implement the enhancement (compatibility arguments not really count). _2 consistency: Petr has done a great job to make the Moodle container handling more consistent. Now several hacks emerge which have been sleeping in the dark and affected advanced themes only. Backporting a simple print_container function to 1.8 would be marvelous. +1 to fix this hacks and to add the enhancement to the Moodle 1.9 standard themes. _3 In the standard theme we can show the enhancements created under the hood. +1 to fix this hacks and to add the enhancement to the Moodle 1.9 standard themes. _4 Martins argument to change the standard theme structure only when absolute necessary. This position is very wise to keep the Moodle changes as smooth as possible. -2 to implement the enhancement After calculating the numbers 3 - 2 I vote +1 to implement the enhancements. If Martin decides to not implement them now consistency needs to be reestablished in 2.0 with all themes using the enhancement.
          Hide
          Urs Hunkler added a comment -

          It would be marvelous if Petr may find a solution that header and footer work without the print_container functions without errors. Then theme designers who do not want to implement this enhancement are not forced to do so.

          And again I think in the standard themes we should use the print_container functions.

          Show
          Urs Hunkler added a comment - It would be marvelous if Petr may find a solution that header and footer work without the print_container functions without errors. Then theme designers who do not want to implement this enhancement are not forced to do so. And again I think in the standard themes we should use the print_container functions.
          Hide
          Urs Hunkler added a comment -

          The admin/lang.php?mode=missing shows an interesting page structure. Please check the screenshot http://screencast.com/t/AhxuZTfCULp.

          I am not sure any more if it is wise to replace all boxes with the same custom_corners supporting function. Developers use boxes not only for content containers. Content containers should have custom_corners support. Any other boxes (to collect some elements or whatever) should be built without custom_corners support.

          A bit more semantic code would help out IMO. May be we need a semantic option to specify the content type of the box: "layout", "wrapper", "text" for example. And wrapper boxes don't write the custom_corners divs. We may need to analyze the box usage a bit more.

          Custom_corners looks like a never ending story

          Show
          Urs Hunkler added a comment - The admin/lang.php?mode=missing shows an interesting page structure. Please check the screenshot http://screencast.com/t/AhxuZTfCULp . I am not sure any more if it is wise to replace all boxes with the same custom_corners supporting function. Developers use boxes not only for content containers. Content containers should have custom_corners support. Any other boxes (to collect some elements or whatever) should be built without custom_corners support. A bit more semantic code would help out IMO. May be we need a semantic option to specify the content type of the box: "layout", "wrapper", "text" for example. And wrapper boxes don't write the custom_corners divs. We may need to analyze the box usage a bit more. Custom_corners looks like a never ending story
          Hide
          Petr Škoda added a comment -

          I have committed rewritten autoclosing for error() and notice() - should be more flexible and compatible with old themes - please note that is is impossible to automatically close containers and tables, the problem is that each page may have different structure, error() and now also notice() should be used before the print_header() - this will have to be solved properly in 2.0

          going to backport print_container into 18 now...

          Show
          Petr Škoda added a comment - I have committed rewritten autoclosing for error() and notice() - should be more flexible and compatible with old themes - please note that is is impossible to automatically close containers and tables, the problem is that each page may have different structure, error() and now also notice() should be used before the print_header() - this will have to be solved properly in 2.0 going to backport print_container into 18 now...
          Hide
          Petr Škoda added a comment -

          basic containers backported into 1.8.x, themes from 1.9 work again in latest 1.8.x

          Show
          Petr Škoda added a comment - basic containers backported into 1.8.x, themes from 1.9 work again in latest 1.8.x
          Hide
          Petr Škoda added a comment -

          reclosing, please reopen or file a new issue if needed

          Show
          Petr Škoda added a comment - reclosing, please reopen or file a new issue if needed
          Hide
          Urs Hunkler added a comment -

          I add changes to weblib and the forum and glossary view pages to get those pages working.

          Show
          Urs Hunkler added a comment - I add changes to weblib and the forum and glossary view pages to get those pages working.
          Hide
          Urs Hunkler added a comment -

          I added changes to weblib and the forum and glossary view pages, the custom_corners and standard theme to get those pages working.

          In the view pages I changed the container function call to a simple "echo 'div ...'" to avoid the custom_corners container overhead and page oddity.

          In weblib I added "clearfix" handling to the function "print_box_start($classes ...". "clearfix" is handed over to the containing divs. This is a hack, but I didn't know how to handle it without rewriting too much areas."

          Petr, please review and close the issue when you think the changes are ok.

          Show
          Urs Hunkler added a comment - I added changes to weblib and the forum and glossary view pages, the custom_corners and standard theme to get those pages working. In the view pages I changed the container function call to a simple "echo 'div ...'" to avoid the custom_corners container overhead and page oddity. In weblib I added "clearfix" handling to the function "print_box_start($classes ...". "clearfix" is handed over to the containing divs. This is a hack, but I didn't know how to handle it without rewriting too much areas." Petr, please review and close the issue when you think the changes are ok.
          Hide
          Martin Dougiamas added a comment -

          I continue to really dislike all the inappropriate PHP, includes etc that has crept into the custom_corners header.html and footer.html. We must get rid of it.

          It is impractical to make a theme that is uses custom_corners as a parent (eg I use one on partners.moodle.com) because all this fragile PHP code breaks every time I upgrade.

          Show
          Martin Dougiamas added a comment - I continue to really dislike all the inappropriate PHP, includes etc that has crept into the custom_corners header.html and footer.html. We must get rid of it. It is impractical to make a theme that is uses custom_corners as a parent (eg I use one on partners.moodle.com) because all this fragile PHP code breaks every time I upgrade.
          Hide
          Urs Hunkler added a comment -

          Martin, I could not agree more.

          As a first step I removed the "nocoursepage" hack, which was an attempt to find out if a page has a layout-table. I developed this hack because you said, that you don't not see any chance to implement a layout-table check. This hack followed several exception hacks because Moodle pages are inconsistently created. All those exceptions I could delete too.

          Now I added a JavaScript layout-table check which works great. Without the layout-table info custom_corners does not work. There must be a body class "haslayouttabel" or "nolayouttable" no matter how it's added to the page.

          Show
          Urs Hunkler added a comment - Martin, I could not agree more. As a first step I removed the "nocoursepage" hack, which was an attempt to find out if a page has a layout-table. I developed this hack because you said, that you don't not see any chance to implement a layout-table check. This hack followed several exception hacks because Moodle pages are inconsistently created. All those exceptions I could delete too. Now I added a JavaScript layout-table check which works great. Without the layout-table info custom_corners does not work. There must be a body class "haslayouttabel" or "nolayouttable" no matter how it's added to the page.
          Hide
          Urs Hunkler added a comment - - edited

          When the issue with the "inpopup" handling (MDL-12093) will be better implemented I can take out some more "hacks" in header.html and may be in footer.html. Then header and footer are nearly back to normal.

          Show
          Urs Hunkler added a comment - - edited When the issue with the "inpopup" handling ( MDL-12093 ) will be better implemented I can take out some more "hacks" in header.html and may be in footer.html. Then header and footer are nearly back to normal.
          Hide
          Urs Hunkler added a comment -

          Discussion seams to be closed.

          Show
          Urs Hunkler added a comment - Discussion seams to be closed.

            People

            • Votes:
              0 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: