Blocks disappear in Clean in RTL

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

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

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

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

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

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

Just added Gareth as a watcher.

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?

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.

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.

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.

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

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.

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

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

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

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

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

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.

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

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?

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?

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

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.

End Peer Review

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.

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

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).

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)

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.

Starting Peer Review...watch this space

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

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

Passed! So pushing for Integration Review.

It works wonderfully well now.
Thanks Gareth.

Over to the Integrators!

No worries Mary Evans, thanks for peer reviewing .

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

Preemptively moving this into this weeks integration.

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

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.

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.

...
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