Details

    • Testing Instructions:
      Hide
      1. Using the Clean theme with a course which 3 columns and blocks on both sides.
      2. Turn editing on.
      3. Move all the blocks to the left.
      4. Turn editing off.
      5. Switch to RTL (by perhaps appending '&lang=he' to the URL).
      6. Confirm that no blocks are displayed and there is a blank area on the right with the content filling all the way to the left.
      7. Apply the patch, 'Purge all caches' and refresh the page.
      8. Confirm that the blocks appear on the right and the content filling all the way to the left.
      9. Switch to LTR (by perhaps appending '&lang=en' to the URL).
      10. Confirm that the blocks appear on the left and the content fills all the space to the right.
      11. Switch to the Standard theme (by perhaps appending '&theme=standard' to the URL if 'Allow theme changes in the URL' is set - docs.moodle.org/25/en/Theme_settings#Allow_theme_changes_in_the_URL).
      12. Confirm that the blocks appear on the left and the content fills all the space to the right.
      13. Switch to RTL (by perhaps appending '&lang=he' to the URL).
      14. Confirm that the blocks appear on the right and the content fills all the space to the left.
      15. Switch back to the Clean theme and to LTR (by perhaps appending '&lang=en' to the URL).
      16. Go back to the course and turn editing on.
      17. Move all bar the navigation and administration blocks to the right.
      18. Turn editing off.
      19. Confirm that the navigation and administration blocks are on the left and the others are on the right.
      20. Switch to RTL (by perhaps appending '&lang=he' to the URL).
      21. Confirm that the navigation and administration blocks are on the right and the others are on the left.
      22. Switch to LTR (by perhaps appending '&lang=en' to the URL).
      23. Turn editing on.
      24. Move all blocks to the right.
      25. Turn editing off.
      26. Confirm that all blocks are on the right.
      27. Switch to RTL (by perhaps appending '&lang=he' to the URL).
      28. Confirm that all blocks are on the left.
      29. Switch to LTR (by perhaps appending '&lang=en' to the URL).
      30. Go to the two column 'Purge all caches' page being an 'admin' layout it will be 'columns2.php' in the 'config.php' file.
      31. Confirm that all blocks are on the left.
      32. Switch to RTL (by perhaps appending '&lang=he' to the URL).
      33. Confirm that all blocks are on the right.
      Show
      Using the Clean theme with a course which 3 columns and blocks on both sides. Turn editing on. Move all the blocks to the left. Turn editing off. Switch to RTL (by perhaps appending '&lang=he' to the URL). Confirm that no blocks are displayed and there is a blank area on the right with the content filling all the way to the left. Apply the patch, 'Purge all caches' and refresh the page. Confirm that the blocks appear on the right and the content filling all the way to the left. Switch to LTR (by perhaps appending '&lang=en' to the URL). Confirm that the blocks appear on the left and the content fills all the space to the right. Switch to the Standard theme (by perhaps appending '&theme=standard' to the URL if 'Allow theme changes in the URL' is set - docs.moodle.org/25/en/Theme_settings#Allow_theme_changes_in_the_URL). Confirm that the blocks appear on the left and the content fills all the space to the right. Switch to RTL (by perhaps appending '&lang=he' to the URL). Confirm that the blocks appear on the right and the content fills all the space to the left. Switch back to the Clean theme and to LTR (by perhaps appending '&lang=en' to the URL). Go back to the course and turn editing on. Move all bar the navigation and administration blocks to the right. Turn editing off. Confirm that the navigation and administration blocks are on the left and the others are on the right. Switch to RTL (by perhaps appending '&lang=he' to the URL). Confirm that the navigation and administration blocks are on the right and the others are on the left. Switch to LTR (by perhaps appending '&lang=en' to the URL). Turn editing on. Move all blocks to the right. Turn editing off. Confirm that all blocks are on the right. Switch to RTL (by perhaps appending '&lang=he' to the URL). Confirm that all blocks are on the left. Switch to LTR (by perhaps appending '&lang=en' to the URL). Go to the two column 'Purge all caches' page being an 'admin' layout it will be 'columns2.php' in the 'config.php' file. Confirm that all blocks are on the left. Switch to RTL (by perhaps appending '&lang=he' to the URL). Confirm that all blocks are on the right.
    • Affected Branches:
      MOODLE_25_STABLE, MOODLE_26_STABLE
    • Fixed Branches:
      MOODLE_25_STABLE, MOODLE_26_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      wip-MDL-42852_master

      Description

      Found lots of related bugs/improvement requests but nothing as major as this:

      If my course has blocks only on one side, when switching to Clean theme and RTL language the blocks disappear.

      Reproduced on 2.5

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Mary Evans added a comment -

            Well they didn't used to...they worked really well at one time.
            Looks like something has changed. Has someone messed with layouts again?

            Show
            Mary Evans added a comment - Well they didn't used to...they worked really well at one time. Looks like something has changed. Has someone messed with layouts again?
            Hide
            Mary Evans added a comment -

            I'm going to try and fix this whatever is causing the problem.

            Show
            Mary Evans added a comment - I'm going to try and fix this whatever is causing the problem.
            Hide
            Mary Evans added a comment -

            Well all I can say about LESS is that is a bloody mess!
            I can't find a darn thing.

            Show
            Mary Evans added a comment - Well all I can say about LESS is that is a bloody mess! I can't find a darn thing.
            Hide
            Marina Glancy added a comment -

            I don't think this is a good reason to close issue Mary. Were you able to reproduce it?

            Show
            Marina Glancy added a comment - I don't think this is a good reason to close issue Mary. Were you able to reproduce it?
            Hide
            Mary Evans added a comment -

            I'm sorry, but I was so frustrated with this last night, if it were a glass vase I would have smashed it to pieces!

            Yes I did replicate this, and cannot for the life of me find a way round it, which was, in itself, the most frustrating thing I have ever had to deal with.

            It would in actual fact make this easier if the layout was as a 3 column page should be, left | centre | right rather that (left | centre) + right, because all that happens here is that, yes the left become right, but does not work as Moodle wants it to work, it just sits at the bottom of the page below the main content, and no matter what I did to it is would not budge. Hence my closing this in a fit of rage, which, at the end of the day, I have to put down to my Latin temperament, me being half Italian! LOL

            Show
            Mary Evans added a comment - I'm sorry, but I was so frustrated with this last night, if it were a glass vase I would have smashed it to pieces! Yes I did replicate this, and cannot for the life of me find a way round it, which was, in itself, the most frustrating thing I have ever had to deal with. It would in actual fact make this easier if the layout was as a 3 column page should be, left | centre | right rather that (left | centre) + right, because all that happens here is that, yes the left become right, but does not work as Moodle wants it to work, it just sits at the bottom of the page below the main content, and no matter what I did to it is would not budge. Hence my closing this in a fit of rage, which, at the end of the day, I have to put down to my Latin temperament, me being half Italian! LOL
            Hide
            Mary Evans added a comment -

            OK...re-opening to have another bash.

            Show
            Mary Evans added a comment - OK...re-opening to have another bash.
            Hide
            Mary Evans added a comment -

            Just added Gareth as a watcher.

            Show
            Mary Evans added a comment - Just added Gareth as a watcher.
            Hide
            Mary Evans added a comment -

            Gareth, this is a throw back from the work you did with collumns2.php where you maintained that Moodle should have a side post only setting too. If you have only 2 columns and the layout is using collumns3.php which is how this problem came to light, you end up with a blank column when in RTL mode, although it may be present when editing it vanishes after editing is disabled.

            Test in Clean which 3 columns blocks on both left and right. Switch to RTL and move blocks to one side. Turn editing off and you will see the blocks at the bottom of the page.

            I can't make my mind up if it the 'rtlblockmanipulation' feature that is causing this or the fact that the section in the page span8/span4 is not working. Also where and at what stage does span9/span3 come into this equation?

            Show
            Mary Evans added a comment - Gareth, this is a throw back from the work you did with collumns2.php where you maintained that Moodle should have a side post only setting too. If you have only 2 columns and the layout is using collumns3.php which is how this problem came to light, you end up with a blank column when in RTL mode, although it may be present when editing it vanishes after editing is disabled. Test in Clean which 3 columns blocks on both left and right. Switch to RTL and move blocks to one side. Turn editing off and you will see the blocks at the bottom of the page. I can't make my mind up if it the 'rtlblockmanipulation' feature that is causing this or the fact that the section in the page span8/span4 is not working. Also where and at what stage does span9/span3 come into this equation?
            Hide
            Gareth J Barnard added a comment - - edited

            Hi Mary,

            To be honest, off the top of my head before I test, it's the absolutely horrible JS controlled 'this block area is empty but needs a drag and drop area when editing and not when umm not editing so fiddle with the 'spanX' classes such that when there are no blocks a span8 is in fact a span9 etc. even if the markup says span8'. It's a mess. I'll look into it.

            Cheers,

            Gareth

            P.S. I once spent a whole day trying a new strategy of having a 24 instead of 12 column layout so that the span3 / span9 -> span8 / span4 nesting could be removed to have a pure span5 / span14 / span5 type layout but gave up as the JS was a pain in the XXXX. The code is on https://github.com/gjb2048/moodle-theme_shoelace/tree/MOODLE_25_FOOTER_BLOCK_GRID_24.

            Show
            Gareth J Barnard added a comment - - edited Hi Mary, To be honest, off the top of my head before I test, it's the absolutely horrible JS controlled 'this block area is empty but needs a drag and drop area when editing and not when umm not editing so fiddle with the 'spanX' classes such that when there are no blocks a span8 is in fact a span9 etc. even if the markup says span8'. It's a mess. I'll look into it. Cheers, Gareth P.S. I once spent a whole day trying a new strategy of having a 24 instead of 12 column layout so that the span3 / span9 -> span8 / span4 nesting could be removed to have a pure span5 / span14 / span5 type layout but gave up as the JS was a pain in the XXXX. The code is on https://github.com/gjb2048/moodle-theme_shoelace/tree/MOODLE_25_FOOTER_BLOCK_GRID_24 .
            Hide
            Gareth J Barnard added a comment - - edited

            Ok, this is interesting. This must now be a logic flaw in the layout files as the mark-up for RTL is not rendering any mark-up for the PRE blocks.

            Show
            Gareth J Barnard added a comment - - edited Ok, this is interesting. This must now be a logic flaw in the layout files as the mark-up for RTL is not rendering any mark-up for the PRE blocks.
            Hide
            Gareth J Barnard added a comment - - edited

            Ok, it looks like 'blocks()' in '/lib/outputrenderers.php' is returning empty content, but '$this->page->blocks->region_has_content($region, $this)' is true, so '$this->blocks_for_region($region)' is not returning content - the only thing I can think of would be the side effect of MDL-41516 - but not certain.

            In 'blocks_for_region($region)' - '$this->page->blocks->get_blocks_for_region($region)' returns no blocks in RTL.

            Show
            Gareth J Barnard added a comment - - edited Ok, it looks like 'blocks()' in '/lib/outputrenderers.php' is returning empty content, but '$this->page->blocks->region_has_content($region, $this)' is true, so '$this->blocks_for_region($region)' is not returning content - the only thing I can think of would be the side effect of MDL-41516 - but not certain. In 'blocks_for_region($region)' - '$this->page->blocks->get_blocks_for_region($region)' returns no blocks in RTL.
            Hide
            Gareth J Barnard added a comment -

            Ok, commented out MDL-41516 and it's not it.

            Show
            Gareth J Barnard added a comment - Ok, commented out MDL-41516 and it's not it.
            Hide
            Mary Evans added a comment -

            Well this is a whole kettle of smelly fish is it not!
            Will get back to this later and see if I can get rid of the JS and just go with CSS as it should have been done in the first place.

            If we need JS to run Bootstrap themes, then this goes against how Bootstrap was intended to work, and in some respects goes against Martin's original model for Moodle, in that Moodle should work even when JS is disabled.

            Show
            Mary Evans added a comment - Well this is a whole kettle of smelly fish is it not! Will get back to this later and see if I can get rid of the JS and just go with CSS as it should have been done in the first place. If we need JS to run Bootstrap themes, then this goes against how Bootstrap was intended to work, and in some respects goes against Martin's original model for Moodle, in that Moodle should work even when JS is disabled.
            Hide
            Mary Evans added a comment -

            Just added Martin as a watcher although he probably is by default!

            Show
            Mary Evans added a comment - Just added Martin as a watcher although he probably is by default!
            Hide
            Gareth J Barnard added a comment -

            Hi Mary Evans,

            Bingo!! Got it after many hours investigation it's a logic flaw in the block rendering code:

            diff --git "a/C:\\Users\\Gareth\\AppData\\Local\\Temp\\TortoiseGit\\outA0C5.tmp\\outputrenderers-89e6c5c-left.php" "b/F:\\moodledev\\moodle25\\lib\\outputrenderers.php"
            index e3e3a78..de28c8b 100644
            --- "a/C:\\Users\\Gareth\\AppData\\Local\\Temp\\TortoiseGit\\outA0C5.tmp\\outputrenderers-89e6c5c-left.php"
            +++ "b/F:\\moodledev\\moodle25\\lib\\outputrenderers.php"
            @@ -1241,7 +1241,6 @@ class core_renderer extends renderer_base {
                  * @return string the HTML to be output.
                  */
                 public function blocks_for_region($region) {
            -        $region = $this->page->apply_theme_region_manipulations($region);
                     $blockcontents = $this->page->blocks->get_content_for_region($region, $this);
                     $blocks = $this->page->blocks->get_blocks_for_region($region);
                     $lastblock = null;
            @@ -3063,8 +3062,8 @@ EOD;
                         'data-blockregion' => $displayregion,
                         'data-droptarget' => '1'
                     );
            -        if ($this->page->blocks->region_has_content($region, $this)) {
            -            $content = $this->blocks_for_region($region);
            +        if ($this->page->blocks->region_has_content($displayregion, $this)) {
            +            $content = $this->blocks_for_region($displayregion);
                     } else {
                         $content = '';
                     }
            

            So in effect in '/lib/outputrenderers.php' I've changed the methods:

            'blocks' from:

            ....
                    if ($this->page->blocks->region_has_content($region, $this)) {
                        $content = $this->blocks_for_region($region);
                    } else {
                        $content = '';
                    } 
            ....
            

            to

            ....
                    if ($this->page->blocks->region_has_content($displayregion, $this)) {
                        $content = $this->blocks_for_region($displayregion);
                    } else {
                        $content = '';
                    }
            ....
            

            and in 'blocks_for_region' removed the line:

            $region = $this->page->apply_theme_region_manipulations($region);
            

            Therefore, would you be so kind as to relinquish the issue and assign to me so I get credit for the commit please.

            Cheers,

            Gareth

            Show
            Gareth J Barnard added a comment - Hi Mary Evans , Bingo!! Got it after many hours investigation it's a logic flaw in the block rendering code: diff --git "a/C:\\Users\\Gareth\\AppData\\Local\\Temp\\TortoiseGit\\outA0C5.tmp\\outputrenderers-89e6c5c-left.php" "b/F:\\moodledev\\moodle25\\lib\\outputrenderers.php" index e3e3a78..de28c8b 100644 --- "a/C:\\Users\\Gareth\\AppData\\Local\\Temp\\TortoiseGit\\outA0C5.tmp\\outputrenderers-89e6c5c-left.php" +++ "b/F:\\moodledev\\moodle25\\lib\\outputrenderers.php" @@ -1241,7 +1241,6 @@ class core_renderer extends renderer_base { * @return string the HTML to be output. */ public function blocks_for_region($region) { - $region = $this->page->apply_theme_region_manipulations($region); $blockcontents = $this->page->blocks->get_content_for_region($region, $this); $blocks = $this->page->blocks->get_blocks_for_region($region); $lastblock = null; @@ -3063,8 +3062,8 @@ EOD; 'data-blockregion' => $displayregion, 'data-droptarget' => '1' ); - if ($this->page->blocks->region_has_content($region, $this)) { - $content = $this->blocks_for_region($region); + if ($this->page->blocks->region_has_content($displayregion, $this)) { + $content = $this->blocks_for_region($displayregion); } else { $content = ''; } So in effect in '/lib/outputrenderers.php' I've changed the methods: 'blocks' from: .... if ($this->page->blocks->region_has_content($region, $this)) { $content = $this->blocks_for_region($region); } else { $content = ''; } .... to .... if ($this->page->blocks->region_has_content($displayregion, $this)) { $content = $this->blocks_for_region($displayregion); } else { $content = ''; } .... and in 'blocks_for_region' removed the line: $region = $this->page->apply_theme_region_manipulations($region); Therefore, would you be so kind as to relinquish the issue and assign to me so I get credit for the commit please. Cheers, Gareth
            Hide
            Mary Evans added a comment -

            Over to you Gareth

            Sorry I only just got to read my emails...been a bit busy with other stuff. I am happy for you to take over...it's a big load off my mind that I can do without.
            Cheers and many thanks for going the extra mile with this.
            Much appreciated
            Mary

            Show
            Mary Evans added a comment - Over to you Gareth Sorry I only just got to read my emails...been a bit busy with other stuff. I am happy for you to take over...it's a big load off my mind that I can do without. Cheers and many thanks for going the extra mile with this. Much appreciated Mary
            Hide
            Marina Glancy added a comment - - edited

            looking at the Gareths patch I remember that we have just introduced this piece of code because of another error with the pagelayout not set - MDL-42648
            I knew I was not the right person to suggest solutions for themes problems

            Show
            Marina Glancy added a comment - - edited looking at the Gareths patch I remember that we have just introduced this piece of code because of another error with the pagelayout not set - MDL-42648 I knew I was not the right person to suggest solutions for themes problems
            Hide
            Gareth J Barnard added a comment -

            Thanks Mary and Marina,

            I'll crack on with this. But just need to check that the changes to 'blocks_for_region' would not cause a regression in non-bootstrapbased themes.

            Cheers,

            Gareth

            Show
            Gareth J Barnard added a comment - Thanks Mary and Marina, I'll crack on with this. But just need to check that the changes to 'blocks_for_region' would not cause a regression in non-bootstrapbased themes. Cheers, Gareth
            Hide
            Gareth J Barnard added a comment -

            Looking at the calls to 'blocks_for_region' it looks like only contributed themes will be affected, such as my own Shoelace and Mutant Banjo themes and Julian Ridden's Essential theme. The patch will mean that existing 'base' themes will no longer call 'apply_theme_region_manipulations' which finds that there are none in the 'config.php' file and actually do nothing - so for them will be an optimisation to have it removed.

            Show
            Gareth J Barnard added a comment - Looking at the calls to 'blocks_for_region' it looks like only contributed themes will be affected, such as my own Shoelace and Mutant Banjo themes and Julian Ridden's Essential theme. The patch will mean that existing 'base' themes will no longer call 'apply_theme_region_manipulations' which finds that there are none in the 'config.php' file and actually do nothing - so for them will be an optimisation to have it removed.
            Hide
            Gareth J Barnard added a comment -

            Sorry Mary, but it looks like MDL-42648 caused a regression.

            Show
            Gareth J Barnard added a comment - Sorry Mary, but it looks like MDL-42648 caused a regression.
            Hide
            Gareth J Barnard added a comment -

            I've added the label 'qa_test_required' because I saw a post on Moodle News that all the tests for M2.6 had passed and yet this is happening, so therefore as an 'important' issue thought that a QA test would be needed. Thoughts?

            Show
            Gareth J Barnard added a comment - I've added the label 'qa_test_required' because I saw a post on Moodle News that all the tests for M2.6 had passed and yet this is happening, so therefore as an 'important' issue thought that a QA test would be needed. Thoughts?
            Hide
            Mary Evans added a comment -

            Good thinking! This make one wonder if the QA tests were carried out correctly, or at least in RTL mode does it not? Indeed do they test in RTL mode if it is not asked for?

            Show
            Mary Evans added a comment - Good thinking! This make one wonder if the QA tests were carried out correctly, or at least in RTL mode does it not? Indeed do they test in RTL mode if it is not asked for?
            Hide
            Mary Evans added a comment - - edited

            As for the renderer...well that is not surprising as I didn't write it, it was a suggestion to fix something else. Swings and roundabouts! It's just the nuts and bolts of Moodle. You fix something, you loosen something else! ROFL

            Show
            Mary Evans added a comment - - edited As for the renderer...well that is not surprising as I didn't write it, it was a suggestion to fix something else. Swings and roundabouts! It's just the nuts and bolts of Moodle. You fix something, you loosen something else! ROFL
            Hide
            Mary Evans added a comment - - edited

            OK...here are my findings:

            The patch works as expected in base/standard/afterburner theme in RTL or LTR mode where the page has blocks in a side-pre-only or side-post-only state.

            The patch works OK in bootstrapbase/clean in a side-pre-only state when in RTL or LTR mode and layout uses column3.php, however, it FAILS when in a side-post-only state where blocks have been shifted to left-hand side when in RTL mode.

            Show
            Mary Evans added a comment - - edited OK...here are my findings: The patch works as expected in base/standard/afterburner theme in RTL or LTR mode where the page has blocks in a side-pre-only or side-post-only state. The patch works OK in bootstrapbase/clean in a side-pre-only state when in RTL or LTR mode and layout uses column3.php, however, it FAILS when in a side-post-only state where blocks have been shifted to left-hand side when in RTL mode.
            Hide
            Mary Evans added a comment -

            End Peer Review

            Show
            Mary Evans added a comment - End Peer Review
            Hide
            Mary Evans added a comment - - edited

            Base theme does not use blockrtlmanipulation code as the switch for RTL is in the layout files in base theme (which standard theme uses) and also afterburner which is the only theme where the layout is different anyway but set up to cater for RTL layout.

            Show
            Mary Evans added a comment - - edited Base theme does not use blockrtlmanipulation code as the switch for RTL is in the layout files in base theme (which standard theme uses) and also afterburner which is the only theme where the layout is different anyway but set up to cater for RTL layout.
            Hide
            Gareth J Barnard added a comment -

            Hi Mary,

            Thanks for peer reviewing the patch.

            For the fail, do you mean two column 'side-pre' only LTR fails when RTL?

            Cheers,

            Gareth

            Show
            Gareth J Barnard added a comment - Hi Mary, Thanks for peer reviewing the patch. For the fail, do you mean two column 'side-pre' only LTR fails when RTL? Cheers, Gareth
            Hide
            Gareth J Barnard added a comment -

            Ok, looking at columns2.php I can see why. Need to think of a solution as columns2.php (no region manipulation flip flop required) operates differently to columns3.php (region manipulation flip flop required).

            Show
            Gareth J Barnard added a comment - Ok, looking at columns2.php I can see why. Need to think of a solution as columns2.php (no region manipulation flip flop required) operates differently to columns3.php (region manipulation flip flop required).
            Hide
            Mary Evans added a comment - - edited

            It only happens in layouts that allow you to move blocks from left to right or visa versa, these layout use column3.php so if you check in Clean/config.php its course, category, frontpage, standard, mydashboard layouts...to name a few.

            I think it's the span9/span3 that is causing the problem because it's set to float right in LTR and float-left in RTL, where as if it is a side-post-only layout in RTL need act like a side-pre-only and visa-versa.

            Perhaps we need a specific class selector that just pushes it one way or t'other!

            Perhaps the layout is all wrong and should be span6/span3/span3 and work like so...
            normal = span6(pull-right)/span3(pull-right)/span3(pull-left first-column)
            side-pre-only = span(6+3)/span3(pull-left first-column)
            side-post-only = span(6+3 first-column)/span3(pull-right)

            Show
            Mary Evans added a comment - - edited It only happens in layouts that allow you to move blocks from left to right or visa versa, these layout use column3.php so if you check in Clean/config.php its course, category, frontpage, standard, mydashboard layouts...to name a few. I think it's the span9/span3 that is causing the problem because it's set to float right in LTR and float-left in RTL, where as if it is a side-post-only layout in RTL need act like a side-pre-only and visa-versa. Perhaps we need a specific class selector that just pushes it one way or t'other! Perhaps the layout is all wrong and should be span6/span3/span3 and work like so... normal = span6(pull-right)/span3(pull-right)/span3(pull-left first-column) side-pre-only = span(6+3)/span3(pull-left first-column) side-post-only = span(6+3 first-column)/span3(pull-right)
            Hide
            Gareth J Barnard added a comment - - edited

            Hi Mary Evans,

            It appears to be all to do with the CSS in 'bootstrapbase/less/moodle/core.less' and how it makes 'adjustments' for empty regions. I've made a few changes and optimised where multiple selectors were applying only 'width: 100%'. As you can see from the comprehensive test instructions I've tested quite a bit. Both branches updated and squashed into a single commit.

            I've also tested the 'columns2.php' scenario and looked at the 'apply_theme_region_manipulations' method in '/lib/pagelib.php' to confirm that it does not perform a swap if there is no alternative region. Therefore in effect when:

            $OUTPUT->blocks('side-pre' ....
            

            Gets called in 'columns2.php' no swap is made for RTL but in 'columns3.php' a swap is made.

            Cheers,

            Gareth

            P.S.
            I once looked into the span3 / span6 / span3 layout and the block areas were too big. With a span2 / span8 / span2 layout the block areas were too small. So hence trying a grid 24 layout once to improve the granularity. But as I said before this was tricky to implement because of all the other CSS etc.

            Show
            Gareth J Barnard added a comment - - edited Hi Mary Evans , It appears to be all to do with the CSS in 'bootstrapbase/less/moodle/core.less' and how it makes 'adjustments' for empty regions. I've made a few changes and optimised where multiple selectors were applying only 'width: 100%'. As you can see from the comprehensive test instructions I've tested quite a bit. Both branches updated and squashed into a single commit. I've also tested the 'columns2.php' scenario and looked at the 'apply_theme_region_manipulations' method in '/lib/pagelib.php' to confirm that it does not perform a swap if there is no alternative region. Therefore in effect when: $OUTPUT->blocks('side-pre' .... Gets called in 'columns2.php' no swap is made for RTL but in 'columns3.php' a swap is made. Cheers, Gareth P.S. I once looked into the span3 / span6 / span3 layout and the block areas were too big. With a span2 / span8 / span2 layout the block areas were too small. So hence trying a grid 24 layout once to improve the granularity. But as I said before this was tricky to implement because of all the other CSS etc.
            Hide
            Mary Evans added a comment -

            Starting Peer Review...watch this space

            Show
            Mary Evans added a comment - Starting Peer Review...watch this space
            Hide
            Gareth J Barnard added a comment -

            Like - also updated test instructions with a 'Purge all caches' at step 7.

            Show
            Gareth J Barnard added a comment - Like - also updated test instructions with a 'Purge all caches' at step 7.
            Hide
            Mary Evans added a comment -

            I did not find any need to do a Purge all caches but will do just to see if anything arrises just the same.

            Show
            Mary Evans added a comment - I did not find any need to do a Purge all caches but will do just to see if anything arrises just the same.
            Hide
            Mary Evans added a comment -

            Passed! So pushing for Integration Review.

            It works wonderfully well now.
            Thanks Gareth.

            Show
            Mary Evans added a comment - Passed! So pushing for Integration Review. It works wonderfully well now. Thanks Gareth.
            Hide
            Mary Evans added a comment -

            Over to the Integrators!

            Show
            Mary Evans added a comment - Over to the Integrators!
            Hide
            Gareth J Barnard added a comment -

            No worries Mary Evans, thanks for peer reviewing .

            Show
            Gareth J Barnard added a comment - No worries Mary Evans , thanks for peer reviewing .
            Hide
            Mary Evans added a comment -

            Just added Nadav as a watcher as this may affect MDL-39871

            Show
            Mary Evans added a comment - Just added Nadav as a watcher as this may affect MDL-39871
            Hide
            Sam Hemelryk added a comment -

            Preemptively moving this into this weeks integration.

            Show
            Sam Hemelryk added a comment - Preemptively moving this into this weeks integration.
            Hide
            Sam Hemelryk added a comment -

            Thanks guys - after really working through this I think this is the best fix available.

            Really we need to start looking to convert the base themes to this new blocks method and then look to deprecate and tidy up some of the other functions.

            Just noting for anyone else investigating this, the issues comes because the only place applying theme manipulations is the blocks method, really we need to keep it that way and look to convert the base based themes to use the manipulation system (they currently all have independent logic to switch etc).

            Many thanks
            Sam

            Show
            Sam Hemelryk added a comment - Thanks guys - after really working through this I think this is the best fix available. Really we need to start looking to convert the base themes to this new blocks method and then look to deprecate and tidy up some of the other functions. Just noting for anyone else investigating this, the issues comes because the only place applying theme manipulations is the blocks method, really we need to keep it that way and look to convert the base based themes to use the manipulation system (they currently all have independent logic to switch etc). Many thanks Sam
            Hide
            Mary Evans added a comment - - edited

            Not ALL Moodle theme have the ability to swap sides. Only Base/Standard & Afterburner do this. We never ever got round to adding the swap to all themes.

            Up to recently I think the blockrtlmanipulation worked in each theme without any changes to CSS. However with new changes in MDL-39871 which is to allow inheritance for this setting has made it somewhat complicated, and so each theme will need restyling to accommodate the changes.

            Show
            Mary Evans added a comment - - edited Not ALL Moodle theme have the ability to swap sides. Only Base/Standard & Afterburner do this. We never ever got round to adding the swap to all themes. Up to recently I think the blockrtlmanipulation worked in each theme without any changes to CSS. However with new changes in MDL-39871 which is to allow inheritance for this setting has made it somewhat complicated, and so each theme will need restyling to accommodate the changes.
            Hide
            Adrian Greeve added a comment -

            Tested on the 2.5 and master integration branches.
            Everything worked as described.
            Step 30 had me confused and I had to go and find an integrator to explain what it meant.
            All good.

            Show
            Adrian Greeve added a comment - Tested on the 2.5 and master integration branches. Everything worked as described. Step 30 had me confused and I had to go and find an integrator to explain what it meant. All good.
            Hide
            Eloy Lafuente (stronk7) added a comment -

            ...
            But still, I thank you, for you made me stronger…

            Stronger as the beast that roar in the wild
            Stronger as the storm across the ocean
            Stronger as the diamond that won’t break
            Stronger enough to take all heart aches….

            I thank you my friend, for you made me stronger…

            ---- Juneah Landicho

            Closing as fixed. Ciao

            Show
            Eloy Lafuente (stronk7) added a comment - ... But still, I thank you, for you made me stronger… Stronger as the beast that roar in the wild Stronger as the storm across the ocean Stronger as the diamond that won’t break Stronger enough to take all heart aches…. I thank you my friend, for you made me stronger… ---- Juneah Landicho Closing as fixed. Ciao

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved: