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 2.5 Branch:
      wip-MDL-42852_M25
    • Pull Master Branch:
      wip-MDL-42852_master
    • Rank:
      54722

      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

        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: