Details

    • Type: Sub-task Sub-task
    • Status: Reopened
    • Priority: Minor Minor
    • Resolution: Unresolved
    • Affects Version/s: 2.5
    • Fix Version/s: 2.6.3
    • Component/s: Themes
    • Labels:
    • Testing Instructions:
      Hide
      1. Select Standard theme
      2. Make a note of which blocks are currently in the side columns (left & right).
      3. Change to a RTL language like Hebrew (he) or Arabic(ar).
      4. TEST that the blocks are now reversed. What was in the left coloumn is now in the right column and visa versa.
      5. With editing enabled move all blocks to the one side. Turn editing off.
      6. TEST that the page displays accordingly.
      7. Change language back to English and TEST that the side blocks swap sides.
      8. Retest in a theme that inherits from a theme that inherits from base (e.g. serenity).
      Show
      Select Standard theme Make a note of which blocks are currently in the side columns (left & right). Change to a RTL language like Hebrew (he) or Arabic(ar). TEST that the blocks are now reversed. What was in the left coloumn is now in the right column and visa versa. With editing enabled move all blocks to the one side. Turn editing off. TEST that the page displays accordingly. Change language back to English and TEST that the side blocks swap sides. Retest in a theme that inherits from a theme that inherits from base (e.g. serenity).
    • Affected Branches:
      MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_26_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-39871_master
    • Rank:
      50616

      Description

      There are hooks in most themes and in some places in core that try to handle switching blocks for rtl languages.
      Really this is something that would be much better achieved in CSS as not all cases are as clear cut as side-pre and side-post.
      Having a CSS solution would certainly be more autonomous and facilitate and easier solution.
      With the introduction of bootstrap new possibilities have arisen and certainly if bootstrap makes it to core having a bootstrap solution would be excellent.
      Damyon pointed out that there has been discussion (https://github.com/twitter/bootstrap/pull/6409#issuecomment-11725361) in the bootstrap community about this and that there is already a pull request for RTL changes: https://github.com/twitter/bootstrap/pull/6423

        Activity

        Hide
        Nadav Kavalerchik added a comment -

        I am currently using a local fix that seems to work fine for some times on our servers.
        Here is what I have done...

        to theme/base/config.php I added the following lines:

        // What block regions should be swapped when in RTL mode
        $THEME->settings->rtlswapblocks = array('side-pre'=>'side-post','side-post'=>'side-pre');
        

        So any theme can override them with it's own set of swapping block regions

        And inside the block switching handling functions I added the following patch:
        lib/blocklib.php

        $newregion = optional_param('bui_newregion', '', PARAM_ALPHANUMEXT);
        if (right_to_left()) {
          $newregion = isset($PAGE->theme->settings->rtlswapblocks[$newregion]) ? $PAGE->theme->settings->rtlswapblocks[$newregion] : $newregion;
        }
        

        And lib/ajax/blocks.php

        switch ($action) {
            case 'move':
                if (right_to_left() and $PAGE->theme->settings->rtlswapblocks) {
                    $bui_newregion = isset($PAGE->theme->settings->rtlswapblocks[$bui_newregion]) ? $PAGE->theme->settings->rtlswapblocks[$bui_newregion] : $bui_newregion;
                }
        

        Which switches blocks only in RTL mode and using the switch array.
        Of course, RTL themes are also switching the list of blocks in each region using the layout/general.php files of each theme. (see theme/base/layout/general.php)

        What do you think?

        Show
        Nadav Kavalerchik added a comment - I am currently using a local fix that seems to work fine for some times on our servers. Here is what I have done... to theme/base/config.php I added the following lines: // What block regions should be swapped when in RTL mode $THEME->settings->rtlswapblocks = array('side-pre'=>'side-post','side-post'=>'side-pre'); So any theme can override them with it's own set of swapping block regions And inside the block switching handling functions I added the following patch: lib/blocklib.php $newregion = optional_param('bui_newregion', '', PARAM_ALPHANUMEXT); if (right_to_left()) { $newregion = isset($PAGE->theme->settings->rtlswapblocks[$newregion]) ? $PAGE->theme->settings->rtlswapblocks[$newregion] : $newregion; } And lib/ajax/blocks.php switch ($action) { case 'move': if (right_to_left() and $PAGE->theme->settings->rtlswapblocks) { $bui_newregion = isset($PAGE->theme->settings->rtlswapblocks[$bui_newregion]) ? $PAGE->theme->settings->rtlswapblocks[$bui_newregion] : $bui_newregion; } Which switches blocks only in RTL mode and using the switch array. Of course, RTL themes are also switching the list of blocks in each region using the layout/general.php files of each theme. (see theme/base/layout/general.php) What do you think?
        Hide
        Mary Evans added a comment -

        Sounds awesome! If it works then I think we should consider implementing it.
        If you could create a branch off Moodle(master), to contain all the changes necessary for this fix, that would be great? You can add it above as normal and we can get someone to peer review it.

        Show
        Mary Evans added a comment - Sounds awesome! If it works then I think we should consider implementing it. If you could create a branch off Moodle(master), to contain all the changes necessary for this fix, that would be great? You can add it above as normal and we can get someone to peer review it.
        Hide
        Mary Evans added a comment -

        Assigning this to you Nadav.

        Show
        Mary Evans added a comment - Assigning this to you Nadav.
        Hide
        Nadav Kavalerchik added a comment -

        Hi Sam Hemelryk,

        I came across this: https://github.com/moodle/moodle/commit/d89fbe0285ffd56bb4632566a92073a3df9f3f5b
        It's seems to solve this issue. but I see this issue open. How can it be?

        Show
        Nadav Kavalerchik added a comment - Hi Sam Hemelryk , I came across this: https://github.com/moodle/moodle/commit/d89fbe0285ffd56bb4632566a92073a3df9f3f5b It's seems to solve this issue. but I see this issue open. How can it be?
        Hide
        Sam Hemelryk added a comment -

        Hi Nadav, this issue is open because it is looking to find a solution in CSS rather than a PHP solution.

        Re-thinking about it now I'm not sure why I thought this was such a great idea at the time.In the mean time I have implemented a PHP solution and as code can never just be removed this issue is now perhaps redundant and can be closed.
        I still like the idea of having a theme that uses CSS to position block regions appropriately for the language but I wouildn't consider it necessary any more.

        Kind regards
        Sam

        Show
        Sam Hemelryk added a comment - Hi Nadav, this issue is open because it is looking to find a solution in CSS rather than a PHP solution. Re-thinking about it now I'm not sure why I thought this was such a great idea at the time.In the mean time I have implemented a PHP solution and as code can never just be removed this issue is now perhaps redundant and can be closed. I still like the idea of having a theme that uses CSS to position block regions appropriately for the language but I wouildn't consider it necessary any more. Kind regards Sam
        Hide
        Nadav Kavalerchik added a comment -

        I also think this should be solved on the PHP level (As you can see from the code snippets above)

        I still think we should implement some code that handles the relocating (drag and drop) of blocks from region to region. And handle the block display on the layout pages better then it is right now. For all the standard themes and also for the bootstrapbase based themes too. (Which uses different layouts)

        Is the above commit the only code you added to help solve this issue?
        Did you fix any of the themes to support this block regions switching?

        Show
        Nadav Kavalerchik added a comment - I also think this should be solved on the PHP level (As you can see from the code snippets above) I still think we should implement some code that handles the relocating (drag and drop) of blocks from region to region. And handle the block display on the layout pages better then it is right now. For all the standard themes and also for the bootstrapbase based themes too. (Which uses different layouts) Is the above commit the only code you added to help solve this issue? Did you fix any of the themes to support this block regions switching?
        Hide
        Sam Hemelryk added a comment -

        That commit is the only commit made against the issue it relates to MDL-39871.
        The bootstrapbase and clean theme are already using this new feature which is pretty much what you are proposing above.
        As for drag and drop I'm not sure of the state of it presently with this patch on those fixed themes and whether it is addressing the issue or whether further changes are required - I've not looked at how dnd is handling block regions.

        Show
        Sam Hemelryk added a comment - That commit is the only commit made against the issue it relates to MDL-39871 . The bootstrapbase and clean theme are already using this new feature which is pretty much what you are proposing above. As for drag and drop I'm not sure of the state of it presently with this patch on those fixed themes and whether it is addressing the issue or whether further changes are required - I've not looked at how dnd is handling block regions.
        Hide
        Nadav Kavalerchik added a comment -

        I see.
        So what do you suggest I can do the help this issue move forward?

        Show
        Nadav Kavalerchik added a comment - I see. So what do you suggest I can do the help this issue move forward?
        Hide
        Mary Evans added a comment -

        You could test your idea Nadav in Base theme. If it works we can go with this idea and drop the other method which we added to Afterburner?

        Show
        Mary Evans added a comment - You could test your idea Nadav in Base theme. If it works we can go with this idea and drop the other method which we added to Afterburner?
        Hide
        Nadav Kavalerchik added a comment -

        Ok (Mary Evans) I will have a go and post and update when it is ready.

        Show
        Nadav Kavalerchik added a comment - Ok ( Mary Evans ) I will have a go and post and update when it is ready.
        Hide
        Mary Evans added a comment -

        When I say 'Base' theme I really meant to say 'Standard' theme as that uses Base/layout files.

        Show
        Mary Evans added a comment - When I say 'Base' theme I really meant to say 'Standard' theme as that uses Base/layout files.
        Hide
        Nadav Kavalerchik added a comment -

        Sam Hemelryk,
        I have updated this issue with an initial code suggestion that solves some parts of the block swapping challenge.

        The suggested patch, seems to handle cases in which themes have 2 regions that needs to be swapped. For example, the common "side-pre" and "side-post" that most core theme use (especially, those derived from theme/base) works fine. Especially in course view. BUT, when viewing a "Block setting" page or an "Admin setting" page, where only one block region is displayed... I did not manage to swap the only region with the appropriate region from the suggested swap array ($THEME->blockrtlmanipulations) so a "single region" page displays in LTR (English) and RTL (Hebrew) modes the same way. No block swapping.

        To fix this single block region scenario, it seems to me, that I need to go deeper and change the blocks manager class and not just the $PAGE or the $THEME APIs.

        I will need your feedback and coding help with that

        This patch includes:

        1. I have removed all the rtl related code in the base/layout/general.php which was responsible for swapping only the "side-pre" and "side-post" regions. (It was not generic, but it worked)
        2. I have added block region swapping array initialization (theme/base/config.php) and the code to override it in derived themes (lib/outputlib.php)
        3. I have refactored blockmanipulations to _blockmanipulations (lib/pagelib.php)

        Nadav

        Show
        Nadav Kavalerchik added a comment - Sam Hemelryk , I have updated this issue with an initial code suggestion that solves some parts of the block swapping challenge. The suggested patch, seems to handle cases in which themes have 2 regions that needs to be swapped. For example, the common "side-pre" and "side-post" that most core theme use (especially, those derived from theme/base) works fine. Especially in course view. BUT, when viewing a "Block setting" page or an "Admin setting" page, where only one block region is displayed... I did not manage to swap the only region with the appropriate region from the suggested swap array ($THEME->blockrtlmanipulations) so a "single region" page displays in LTR (English) and RTL (Hebrew) modes the same way. No block swapping. To fix this single block region scenario, it seems to me, that I need to go deeper and change the blocks manager class and not just the $PAGE or the $THEME APIs. I will need your feedback and coding help with that This patch includes: I have removed all the rtl related code in the base/layout/general.php which was responsible for swapping only the "side-pre" and "side-post" regions. (It was not generic, but it worked) I have added block region swapping array initialization (theme/base/config.php) and the code to override it in derived themes (lib/outputlib.php) I have refactored blockmanipulations to _blockmanipulations (lib/pagelib.php) Nadav
        Hide
        Mary Evans added a comment -

        @Nadav, I think I may have solved the CSS issue.

        Try this for a .dir-rtl.side-pre-only fix for standard theme.

         
        /** Only side PRE **/
        .dir-rtl.side-pre-only #page-content #region-main-box {left:0px;}
        .dir-rtl.side-pre-only #page-content #region-post-box {margin-left:-200px;}
        .dir-rtl.side-pre-only #page-content #region-main {margin-left:200px;}
        .dir-rtl.side-pre-only #page-content #region-post {width:0px;}
        .dir-rtl.side-pre-only #page-content #region-pre {left:50%; width: 200px;}
        
        Show
        Mary Evans added a comment - @Nadav, I think I may have solved the CSS issue. Try this for a .dir-rtl.side-pre-only fix for standard theme.   /** Only side PRE **/ .dir-rtl.side-pre-only #page-content #region-main-box {left:0px;} .dir-rtl.side-pre-only #page-content #region-post-box {margin-left:-200px;} .dir-rtl.side-pre-only #page-content #region-main {margin-left:200px;} .dir-rtl.side-pre-only #page-content #region-post {width:0px;} .dir-rtl.side-pre-only #page-content #region-pre {left:50%; width: 200px;}
        Hide
        Mary Evans added a comment -

        I need to test this all again tomorrow...but looks promising.

        Show
        Mary Evans added a comment - I need to test this all again tomorrow...but looks promising.
        Hide
        Mary Evans added a comment - - edited

        Now try this for .dir-rtl.side-post-only

         
        /**RTL Only side POST **/
        .dir-rtl.side-post-only #page-content {clear:both;float:left;overflow:hidden;position:relative;width:100%;min-width:900px;}
        .dir-rtl.side-post-only #page-content #region-main-box {float:left;left:200px;position:relative;width:200%;}
        .dir-rtl.side-post-only #page-content #region-post-box {margin-left:-200px;}
        .dir-rtl.side-post-only #page-content #region-main {margin-left:200px;}
        .dir-rtl.side-post-only #page-content #region-pre {width: 0;}
        .dir-rtl.side-post-only #page-content #region-post {float:left;left:0px;overflow:hidden;position:relative;width:200px;margin-left:-50%;}
        
        Show
        Mary Evans added a comment - - edited Now try this for .dir-rtl.side-post-only   /**RTL Only side POST **/ .dir-rtl.side-post-only #page-content {clear:both; float :left;overflow:hidden;position:relative;width:100%;min-width:900px;} .dir-rtl.side-post-only #page-content #region-main-box { float :left;left:200px;position:relative;width:200%;} .dir-rtl.side-post-only #page-content #region-post-box {margin-left:-200px;} .dir-rtl.side-post-only #page-content #region-main {margin-left:200px;} .dir-rtl.side-post-only #page-content #region-pre {width: 0;} .dir-rtl.side-post-only #page-content #region-post { float :left;left:0px;overflow:hidden;position:relative;width:200px;margin-left:-50%;}
        Hide
        Mary Evans added a comment -

        @Nadav,

        -1 for this patch as it's not working as expected, probably because 'blockrtlmanipulations' exists already and is in use in Bootstrap(base).

        However, working in a new branch without your patch I found that if the 'blockrtlmanipulations' php is added to base theme as it is here.
         

        $THEME->blockrtlmanipulations = array(
            'side-pre' => 'side-post',
            'side-post' => 'side-pre'
        );
        

        And the providing theme/base/layout files have been amended to remove the original left to right swap, and that the RTL CSS as posted in my previous comments, is added to the pagelayout.css it works perfectly!

        I'll put this all together push it all to my github then you can see what I'm jabbering on about!

        Show
        Mary Evans added a comment - @Nadav, -1 for this patch as it's not working as expected, probably because 'blockrtlmanipulations' exists already and is in use in Bootstrap(base). However, working in a new branch without your patch I found that if the 'blockrtlmanipulations' php is added to base theme as it is here.   $THEME->blockrtlmanipulations = array( 'side-pre' => 'side-post', 'side-post' => 'side-pre' ); And the providing theme/base/layout files have been amended to remove the original left to right swap, and that the RTL CSS as posted in my previous comments, is added to the pagelayout.css it works perfectly! I'll put this all together push it all to my github then you can see what I'm jabbering on about!
        Hide
        Nadav Kavalerchik added a comment - - edited

        I have added your CSS to the patch and it looks great! (In single region layout)
        This patch is meant for all other than bootstrapbase based themes. So I do not see the problem.
        I have also tested theme/clean with this patch and it seems to work fine. It does not break it.

        The change I made to "blockrtlmanipulations" (Defining it and setting it to protected "_blockrtlmanipulations") was on the page class level not on the theme class level. In which it should be used as: "$THEME->blockrtlmanipulations = array(..." on the bootstrapbase config, Or on any derived themes.

        Not sure what you consider a problem.

        Looking forward to see what you are jabbering about. So far it was poetry

        I have updated this patch with your new CSS code. (Which seems to solve the entire issue, as far as I can see)

        Show
        Nadav Kavalerchik added a comment - - edited I have added your CSS to the patch and it looks great! (In single region layout) This patch is meant for all other than bootstrapbase based themes. So I do not see the problem. I have also tested theme/clean with this patch and it seems to work fine. It does not break it. The change I made to "blockrtlmanipulations" (Defining it and setting it to protected "_blockrtlmanipulations") was on the page class level not on the theme class level. In which it should be used as: "$THEME->blockrtlmanipulations = array(..." on the bootstrapbase config, Or on any derived themes. Not sure what you consider a problem. Looking forward to see what you are jabbering about. So far it was poetry I have updated this patch with your new CSS code. (Which seems to solve the entire issue, as far as I can see)
        Hide
        Mary Evans added a comment -

        Hi Nadav,

        You look to have missed off frontpage.php, as the bodyclasses and block-regions need altering too.

        I was testing Standard theme not Bootstrap.

        I'm just fixing my branch which I will upload again to my git hub! Third time lucky! LOL

        Show
        Mary Evans added a comment - Hi Nadav, You look to have missed off frontpage.php, as the bodyclasses and block-regions need altering too. I was testing Standard theme not Bootstrap. I'm just fixing my branch which I will upload again to my git hub! Third time lucky! LOL
        Hide
        Nadav Kavalerchik added a comment -

        Right! forgot about that one. me bad.
        Fixed it too and updated github (third time too)

        Show
        Nadav Kavalerchik added a comment - Right! forgot about that one. me bad. Fixed it too and updated github (third time too)
        Hide
        Mary Evans added a comment - - edited

        OK...here it is...
        https://github.com/lazydaisy/moodle/compare/MDL-39871_master

        This is using...
         

        $THEME->blockrtlmanipulations = array(
            'side-pre' => 'side-post',
            'side-post' => 'side-pre'
        );
        

        Which is available now in Moodle 2.6Alpha(current Moodle(master)).

        This works OK where for some reason I could not get yours to work, even after changing the frontpage.php in your patch while testing Standard theme.

        Show
        Mary Evans added a comment - - edited OK...here it is... https://github.com/lazydaisy/moodle/compare/MDL-39871_master This is using...   $THEME->blockrtlmanipulations = array( 'side-pre' => 'side-post', 'side-post' => 'side-pre' ); Which is available now in Moodle 2.6Alpha(current Moodle(master)). This works OK where for some reason I could not get yours to work, even after changing the frontpage.php in your patch while testing Standard theme.
        Hide
        Nadav Kavalerchik added a comment -

        For my patch to work, you need the entire patch. Including core changes to page class.
        Not even sure how it was working without them. I need Sam Hemelryk input on this. (review my patch)

        Show
        Nadav Kavalerchik added a comment - For my patch to work, you need the entire patch. Including core changes to page class. Not even sure how it was working without them. I need Sam Hemelryk input on this. (review my patch)
        Hide
        Mary Evans added a comment -

        I did use your entire patch, I merged your branch on to one I had made in my localhost server where I do all my testing.

        If you take a look at theme/bootstrapbase/config.php you will see this same RTL block-region manipulation code...CLICK HERE

        This turns region-pre into region-post when RTL is enabled.

        I'll test yours again now that the CSS is in.

        Show
        Mary Evans added a comment - I did use your entire patch, I merged your branch on to one I had made in my localhost server where I do all my testing. If you take a look at theme/bootstrapbase/config.php you will see this same RTL block-region manipulation code... CLICK HERE This turns region-pre into region-post when RTL is enabled. I'll test yours again now that the CSS is in.
        Hide
        Mary Evans added a comment -

        There is more about this in moodle/theme/upgrade.txt READ MORE

        Show
        Mary Evans added a comment - There is more about this in moodle/theme/upgrade.txt READ MORE
        Hide
        Mary Evans added a comment - - edited

        OK...I can get the 3 column to work, and swap side in RTL.
        But as you say although the side-pre-only page is set out correctly there is no sign of the actual block regions, which could well be hidden. Same goes for side-post-only

        Show
        Mary Evans added a comment - - edited OK...I can get the 3 column to work, and swap side in RTL. But as you say although the side-pre-only page is set out correctly there is no sign of the actual block regions, which could well be hidden. Same goes for side-post-only
        Hide
        Nadav Kavalerchik added a comment -

        I think I finally understood how the "magic" works with those magic functions ("magic_get_blockmanipulations()")
        I will upload a new patch. Some of the code in my patch is redundant.

        Show
        Nadav Kavalerchik added a comment - I think I finally understood how the "magic" works with those magic functions ("magic_get_blockmanipulations()") I will upload a new patch. Some of the code in my patch is redundant.
        Hide
        Mary Evans added a comment -

        OK...I found that if I removed the if statement for the blockrtlmanipulations the missing side blocks for side-pre-only and side-post-only appear.
        So YES! your patch works!!!

        Let's get it integrated now before it is too late!

        Show
        Mary Evans added a comment - OK...I found that if I removed the if statement for the blockrtlmanipulations the missing side blocks for side-pre-only and side-post-only appear. So YES! your patch works!!! Let's get it integrated now before it is too late!
        Hide
        Mary Evans added a comment -

        Just posted almost at same time

        Show
        Mary Evans added a comment - Just posted almost at same time
        Hide
        Nadav Kavalerchik added a comment -

        I am working on it, and soon update it.

        Show
        Nadav Kavalerchik added a comment - I am working on it, and soon update it.
        Hide
        Nadav Kavalerchik added a comment -

        Ok. NOW! it is better
        I am more happy with the last update of the patch.

        Ready for core review.

        Show
        Nadav Kavalerchik added a comment - Ok. NOW! it is better I am more happy with the last update of the patch. Ready for core review.
        Hide
        Mary Evans added a comment -

        This get me +1

        Code looks good...and works as expected.

        Show
        Mary Evans added a comment - This get me +1 Code looks good...and works as expected.
        Hide
        Mary Evans added a comment -

        That ends Peer Review.

        Show
        Mary Evans added a comment - That ends Peer Review.
        Hide
        Mary Evans added a comment -

        Submitting for Integration review

        Show
        Mary Evans added a comment - Submitting for Integration review
        Hide
        Mary Evans added a comment -

        Thanks Nadav!

        Show
        Mary Evans added a comment - Thanks Nadav!
        Hide
        Damyon Wiese added a comment -

        Thanks Nadav.

        I have integrated this to master only.

        I added one commit to search for rtlblockmanipulations in any parent, not just the immediate parent. This is for themes like serenity that inherit from a theme which inherits from base.

        I really like how this cleans up the layout files.

        Show
        Damyon Wiese added a comment - Thanks Nadav. I have integrated this to master only. I added one commit to search for rtlblockmanipulations in any parent, not just the immediate parent. This is for themes like serenity that inherit from a theme which inherits from base. I really like how this cleans up the layout files.
        Hide
        Nadav Kavalerchik added a comment -

        Great!, Damyon.

        Since many of us are starting this academic year with Moodle 2.5.x, It would be great to have this backported to Moodle 2.5 stable.

        Please send me a link to the extra commit so I can prepare a backport for this.

        Show
        Nadav Kavalerchik added a comment - Great!, Damyon. Since many of us are starting this academic year with Moodle 2.5.x, It would be great to have this backported to Moodle 2.5 stable. Please send me a link to the extra commit so I can prepare a backport for this.
        Hide
        Mary Evans added a comment -

        Nadav, I think it would be safe to just cherry-pick this commit to a new branch based on MOODLE_25_STABLE as the blockrtlmanipulations is in 2.5 already.

        Show
        Mary Evans added a comment - Nadav, I think it would be safe to just cherry-pick this commit to a new branch based on MOODLE_25_STABLE as the blockrtlmanipulations is in 2.5 already.
        Hide
        Mary Evans added a comment -

        Having said that you still need Damyons commit ... I missed what you actually said! duh! Sorry...

        Show
        Mary Evans added a comment - Having said that you still need Damyons commit ... I missed what you actually said! duh! Sorry...
        Hide
        Nadav Kavalerchik added a comment -

        Actually, I am thinking of creating a separate issue for that.
        Putting everything together, nicely

        Show
        Nadav Kavalerchik added a comment - Actually, I am thinking of creating a separate issue for that. Putting everything together, nicely
        Hide
        Damyon Wiese added a comment -

        Hi Nadav,

        If you want to backport this we need to be really, really sure it will not require any changes to any custom themes - which I'm not sure we can guarantee.

        Show
        Damyon Wiese added a comment - Hi Nadav, If you want to backport this we need to be really, really sure it will not require any changes to any custom themes - which I'm not sure we can guarantee.
        Hide
        Nadav Kavalerchik added a comment -

        Afterburner is the only theme that need to be cleaned out of all the right_to_left() hacks, in the layout pages.

        I am observing every new 3rd party theme that comes out, And I do not know of any custom theme that implemented this kind of support. so I do not think anything can be broken.

        I am very confident that this can be backported.

        Please add the link to the last commit you made over the suggested fix.

        Show
        Nadav Kavalerchik added a comment - Afterburner is the only theme that need to be cleaned out of all the right_to_left() hacks, in the layout pages. I am observing every new 3rd party theme that comes out, And I do not know of any custom theme that implemented this kind of support. so I do not think anything can be broken. I am very confident that this can be backported. Please add the link to the last commit you made over the suggested fix.
        Show
        Damyon Wiese added a comment - Here it is: https://github.com/moodle/moodle/commit/2e68b1254f4613c679bc7338a097d63f09b4089e
        Hide
        Michael de Raadt added a comment -

        I ran into some problems testing this.

        In an RTL language (Arabic) I tried to shift all the blocks from the left (right in an LTR language) to the right (left in LTR). When I started to move the last block from the left to the right, all the blocks on the right somehow shifted immediately to the left (through JavaScript, not a page reload) and I lost the right block area. After a page reload, the blocks had all disappeared. Turning editing off did not restore the blocks. Switching back to an LTR language did restore the blocks, which were now all on the left side.

        Show
        Michael de Raadt added a comment - I ran into some problems testing this. In an RTL language (Arabic) I tried to shift all the blocks from the left (right in an LTR language) to the right (left in LTR). When I started to move the last block from the left to the right, all the blocks on the right somehow shifted immediately to the left (through JavaScript, not a page reload) and I lost the right block area. After a page reload, the blocks had all disappeared. Turning editing off did not restore the blocks. Switching back to an LTR language did restore the blocks, which were now all on the left side.
        Hide
        Damyon Wiese added a comment -

        Thanks Michael,

        Note this issue is just adding support for this existing feature to the base theme - it is already in clean. So if the same behaviour occurs in clean, then IMO we should add a new issue for it and pass this (if there are no other related fails).

        For your consideration...

        Show
        Damyon Wiese added a comment - Thanks Michael, Note this issue is just adding support for this existing feature to the base theme - it is already in clean. So if the same behaviour occurs in clean, then IMO we should add a new issue for it and pass this (if there are no other related fails). For your consideration...
        Hide
        Michael de Raadt added a comment -

        I'll try this with Clean and see if it is the same.

        Show
        Michael de Raadt added a comment - I'll try this with Clean and see if it is the same.
        Hide
        Michael de Raadt added a comment -

        I tried this in Clean, Standard and Serenity. The Clean theme functioned as expected. In themes based on Base the pre and post columns seemed to switch unexpectedly and disappear (as described earlier).

        I also tried this in 2.5. The problems I described didn't happen there.

        Show
        Michael de Raadt added a comment - I tried this in Clean, Standard and Serenity. The Clean theme functioned as expected. In themes based on Base the pre and post columns seemed to switch unexpectedly and disappear (as described earlier). I also tried this in 2.5. The problems I described didn't happen there.
        Hide
        Damyon Wiese added a comment - - edited

        Ok thanks Michael - looks like a fail caused by this patch then - will check in a sec.

        Show
        Damyon Wiese added a comment - - edited Ok thanks Michael - looks like a fail caused by this patch then - will check in a sec.
        Hide
        Damyon Wiese added a comment -

        Thanks Nadav - but I've had to revert this patch as it seems to be causing some problems as noted by Michael above (I was able to reproduce this).

        I'll have a look at the code and see if I can find where the problem is being triggered.

        Once we fix this problem we can send this back to integration again.

        Show
        Damyon Wiese added a comment - Thanks Nadav - but I've had to revert this patch as it seems to be causing some problems as noted by Michael above (I was able to reproduce this). I'll have a look at the code and see if I can find where the problem is being triggered. Once we fix this problem we can send this back to integration again.
        Hide
        CiBoT added a comment -

        Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

        Show
        CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
        Hide
        Mary Evans added a comment -

        @Michael, where you using Drag n Drop or was that disabled? If it was then the problem is in the css for Base where there are setting for Moving blocks. These never got changed.

        Also but not related to this...but might be...there is some code there that I cannot for the life of me understand why it is there. Where is class 'page-middle' for goodness sake?

        Show
        Mary Evans added a comment - @Michael, where you using Drag n Drop or was that disabled? If it was then the problem is in the css for Base where there are setting for Moving blocks. These never got changed. Also but not related to this...but might be...there is some code there that I cannot for the life of me understand why it is there. Where is class ' page-middle ' for goodness sake?
        Hide
        Mary Evans added a comment -

        PS: I just did a grep in git and only found the css in a bunch f themes including Base theme which I linked to about. So I can safely say this is obsolete as it it not styling anything in Moodle.
        I'll set up a tracker for this.

        Show
        Mary Evans added a comment - PS: I just did a grep in git and only found the css in a bunch f themes including Base theme which I linked to about. So I can safely say this is obsolete as it it not styling anything in Moodle. I'll set up a tracker for this.
        Hide
        Mary Evans added a comment -

        I think that if a theme is to use this blocksrtlmanipulation code it should be styled to use it. It cannot be used via a parent as this will break some themes as layouts are all different, and in some cases themes can, and do, exclude base and canvas pagelayout.css, so those themes will need extra css adding.

        Standard theme should work OK as that uses Base theme, but even so it should carry the blocksrtlmanipulation setting in standard/config.php.

        Whilst the commit Damyon added is, on the face of it a good idea, we need time to fix every theme in Moodle. Back-porting this sounds too dangerous to me, as it is just as likely to brake stuff if the CSS is wrong.

        Show
        Mary Evans added a comment - I think that if a theme is to use this blocksrtlmanipulation code it should be styled to use it. It cannot be used via a parent as this will break some themes as layouts are all different, and in some cases themes can, and do, exclude base and canvas pagelayout.css, so those themes will need extra css adding. Standard theme should work OK as that uses Base theme, but even so it should carry the blocksrtlmanipulation setting in standard/config.php. Whilst the commit Damyon added is, on the face of it a good idea, we need time to fix every theme in Moodle. Back-porting this sounds too dangerous to me, as it is just as likely to brake stuff if the CSS is wrong.
        Hide
        Mary Evans added a comment -

        @Michael,

        Sorry for the noise...but further to my last comment. Serenity was a bad choice as no CSS was added to Canvas theme to compensate for RTL in this situation. From what I can gather Damyons commit was made on the spare of the moment, seemingly without understanding the complexity of CSS for this change in block-region direction.

        Just so you know what happens, dir-rtl uses side-pre-only CSS which then become becomes .dir-rtl.side-post-only but with one difference it uses .side-pre-only #page-content #region-pre, and .side-pre-only #page-content #region-post at the same time. so those two values remain the same it's just that the other CSS around them change direction. So what might look like simple CSS is in effect quite complicated.

        Also if you look at the layouts in Magazine, Nonzero, & Fusion, these use a similar layout to Afterburner which use http://matthewjamestaylor.com/blog/perfect-3-column.htm or a derivative of it. Which again is a very complex layout, so restyling these is not easy and take time and lots of consentration.

        So had you chosen one of those themes you would have seen no side blocks.

        @Damyon, I have been told time and time again, recently, to request Peer Review before committing changes, as minor as they appear in some cases, yet you as an Integrator added the commit that adds further changes to Nadav's original code, which in all honesty should have gone in as a separate issue and been peer reviewed prior to integration review.

        Show
        Mary Evans added a comment - @Michael, Sorry for the noise...but further to my last comment. Serenity was a bad choice as no CSS was added to Canvas theme to compensate for RTL in this situation. From what I can gather Damyons commit was made on the spare of the moment, seemingly without understanding the complexity of CSS for this change in block-region direction. Just so you know what happens, dir-rtl uses side-pre-only CSS which then become becomes .dir-rtl.side-post-only but with one difference it uses .side-pre-only #page-content #region-pre, and .side-pre-only #page-content #region-post at the same time. so those two values remain the same it's just that the other CSS around them change direction. So what might look like simple CSS is in effect quite complicated. Also if you look at the layouts in Magazine, Nonzero, & Fusion, these use a similar layout to Afterburner which use http://matthewjamestaylor.com/blog/perfect-3-column.htm or a derivative of it. Which again is a very complex layout, so restyling these is not easy and take time and lots of consentration. So had you chosen one of those themes you would have seen no side blocks. @Damyon, I have been told time and time again, recently, to request Peer Review before committing changes, as minor as they appear in some cases, yet you as an Integrator added the commit that adds further changes to Nadav's original code, which in all honesty should have gone in as a separate issue and been peer reviewed prior to integration review.
        Hide
        Michael de Raadt added a comment -

        Hi, Mary.

        I was using drag-and-drop to move the blocks. I used Serenity because it was suggested in the testing instructions.

        Show
        Michael de Raadt added a comment - Hi, Mary. I was using drag-and-drop to move the blocks. I used Serenity because it was suggested in the testing instructions.
        Hide
        Mary Evans added a comment -

        I realise that what I was really meaning is that prior to the EXTRA commit Damyon added (at the last minute) there was no provision to style those theme that inherit from Canvas.

        Prior to Damyon's intervention here this patch worked for those themes that use the exact same layout as base theme.

        To get Damyons changes added we need to add compensating CSS in every theme that excludes page-layout from both canvas and base themes, and make sure that the rest all work by adding extra css in canvas and/or those remaining themes.

        Show
        Mary Evans added a comment - I realise that what I was really meaning is that prior to the EXTRA commit Damyon added (at the last minute) there was no provision to style those theme that inherit from Canvas. Prior to Damyon's intervention here this patch worked for those themes that use the exact same layout as base theme. To get Damyons changes added we need to add compensating CSS in every theme that excludes page-layout from both canvas and base themes, and make sure that the rest all work by adding extra css in canvas and/or those remaining themes.
        Hide
        Nadav Kavalerchik added a comment -

        Hi guys/girls

        Where is this standing?
        (Anything I can do to move this forward?)

        Show
        Nadav Kavalerchik added a comment - Hi guys/girls Where is this standing? (Anything I can do to move this forward?)
        Hide
        Mary Evans added a comment -

        As far as I am aware this was working OK prior to Damyon's extra commit. So if you could resubmit your original patch for whatever you were fixing then that would be a start...as you were only fixing base theme.

        You could change the test instructions and take Serenity theme off. Just test Standard theme. I think that would be in order.

        Any new theme using Base as a parent would nee to add styling to adjust the layout accordingly, so perhaps this needs documenting in theme/upgrade.txt

        Show
        Mary Evans added a comment - As far as I am aware this was working OK prior to Damyon's extra commit. So if you could resubmit your original patch for whatever you were fixing then that would be a start...as you were only fixing base theme. You could change the test instructions and take Serenity theme off. Just test Standard theme. I think that would be in order. Any new theme using Base as a parent would nee to add styling to adjust the layout accordingly, so perhaps this needs documenting in theme/upgrade.txt
        Hide
        Mary Evans added a comment -

        Nadav,

        If you add this to Canvas theme it works...

        
        $THEME->blockrtlmanipulations = array(
            'side-pre' => 'side-post',
            'side-post' => 'side-pre'
        );
        

        So I am guessing that if this is added to all themes config.php the RTL swap side will work for every theme without making amendments to CSS.

        I dare say if the same code was added to Standard theme then it too would work correctly without CSS changes as we had to do for this!

        I'm going to see if this is correct because if it is we can get this in for 2.6.

        Show
        Mary Evans added a comment - Nadav, If you add this to Canvas theme it works... $THEME->blockrtlmanipulations = array( 'side-pre' => 'side-post', 'side-post' => 'side-pre' ); So I am guessing that if this is added to all themes config.php the RTL swap side will work for every theme without making amendments to CSS. I dare say if the same code was added to Standard theme then it too would work correctly without CSS changes as we had to do for this! I'm going to see if this is correct because if it is we can get this in for 2.6.
        Hide
        Mary Evans added a comment - - edited

        We do not need your patch Nadav, all we need to do is add this to all themes config.php

        $THEME->blockrtlmanipulations = array(
            'side-pre' => 'side-post',
            'side-post' => 'side-pre'
        );
        
        Show
        Mary Evans added a comment - - edited We do not need your patch Nadav, all we need to do is add this to all themes config.php $THEME->blockrtlmanipulations = array( 'side-pre' => 'side-post', 'side-post' => 'side-pre' );
        Hide
        Mary Evans added a comment -

        I'll fix Afterburner as this is having a big overhaul at the moment and will conflict badly.

        Show
        Mary Evans added a comment - I'll fix Afterburner as this is having a big overhaul at the moment and will conflict badly.
        Hide
        Nadav Kavalerchik added a comment -

        This patch changes the layout of base (Reverts the layout to its original state, before all the RTL changes), which seems to me necessary for the proper layout of blocks in RTL/LTR.

        Also, it looks for the $THEME->blockrtlmanipulations setting in the theme and its parent, which is missing from the original patch Sam made. Important.

        And so we do not need to put $THEME->blockrtlmanipulations in all themes, since it is put in base.

        Am I right?

        Show
        Nadav Kavalerchik added a comment - This patch changes the layout of base (Reverts the layout to its original state, before all the RTL changes), which seems to me necessary for the proper layout of blocks in RTL/LTR. Also, it looks for the $THEME->blockrtlmanipulations setting in the theme and its parent, which is missing from the original patch Sam made. Important. And so we do not need to put $THEME->blockrtlmanipulations in all themes, since it is put in base. Am I right?
        Hide
        Sam Hemelryk added a comment -

        Nadav - you are a legend!

        I've just [quikcly] looked over your changes here and they look spot on.

        Show
        Sam Hemelryk added a comment - Nadav - you are a legend! I've just [quikcly] looked over your changes here and they look spot on.
        Hide
        Nadav Kavalerchik added a comment -

        Sam, officially you made my day and I am also blushing (which we do not have an emoticon for so...)
        (After hours of developing SQL reports using the beautiful blocks/configurable_reports)

        Please do your magic and make this issue dissolve into the integration cycle

        Show
        Nadav Kavalerchik added a comment - Sam, officially you made my day and I am also blushing (which we do not have an emoticon for so...) (After hours of developing SQL reports using the beautiful blocks/configurable_reports) Please do your magic and make this issue dissolve into the integration cycle
        Hide
        Sam Hemelryk added a comment -

        Thanks Nadav - this has been integrated now

        Show
        Sam Hemelryk added a comment - Thanks Nadav - this has been integrated now
        Hide
        Mary Evans added a comment -

        Apologies Nadav...I'm not sure what I was talking about yesterday...I'm must be going crazy. I'm getting too old for this.

        Show
        Mary Evans added a comment - Apologies Nadav...I'm not sure what I was talking about yesterday...I'm must be going crazy. I'm getting too old for this.
        Hide
        Nadav Kavalerchik added a comment -

        Hi Mary, You are not going crazy. Do not worry Your input is ALWAYS valuable! It helped me check myself again, which is always a good thing before we integrate something that important into core.

        If anything... Thank you for all your feedback, across all issues!

        Show
        Nadav Kavalerchik added a comment - Hi Mary, You are not going crazy. Do not worry Your input is ALWAYS valuable! It helped me check myself again, which is always a good thing before we integrate something that important into core. If anything... Thank you for all your feedback, across all issues!
        Hide
        Barbara Ramiro added a comment - - edited

        Hi Nadav,

        Thanks for your work. Please find attached screenshots for my visual comments (" ,)

        English Hebrew

        Serenity theme

        English Hebrew
        Show
        Barbara Ramiro added a comment - - edited Hi Nadav, Thanks for your work. Please find attached screenshots for my visual comments (" ,) English Hebrew Serenity theme English Hebrew
        Hide
        Barbara Ramiro added a comment -

        Sorry Nadav, I have to fail this on two major reasons:

        1. Blocks are gone on all activity/resource pages
        2. Editing icon context menu overlaps on other icons and activity/resource titles

        Cheers (" ,)

        Show
        Barbara Ramiro added a comment - Sorry Nadav, I have to fail this on two major reasons: Blocks are gone on all activity/resource pages Editing icon context menu overlaps on other icons and activity/resource titles Cheers (" ,)
        Hide
        Mary Evans added a comment -

        @Nadav,

        The test instructions still have Serenity theme added. This patch does not account for grandparents we discovered that that was wrong last this this only inherits from parent.

        @Barbara,
        Please test again this time add the following to Canvas theme config.php...

        $THEME->blockrtlmanipulations = array(
            'side-pre' => 'side-post',
            'side-post' => 'side-pre'
        );
        
        Show
        Mary Evans added a comment - @Nadav, The test instructions still have Serenity theme added. This patch does not account for grandparents we discovered that that was wrong last this this only inherits from parent. @Barbara, Please test again this time add the following to Canvas theme config.php... $THEME->blockrtlmanipulations = array( 'side-pre' => 'side-post', 'side-post' => 'side-pre' );
        Hide
        Sam Hemelryk added a comment -

        Hmm looking at this now

        Show
        Sam Hemelryk added a comment - Hmm looking at this now
        Hide
        Nadav Kavalerchik added a comment -

        Me too

        Show
        Nadav Kavalerchik added a comment - Me too
        Hide
        Sam Hemelryk added a comment -

        Ok block manipulations are only applied when using $OUTPUT and not the blocks API.
        Because the base theme still uses the blocks API we must apply the manipulations manually.
        The following patch should do it:

        https://github.com/samhemelryk/moodle/commit/39871-26

        You can pull test this by running the following command on an integration repository with the master branch checked out.
        Make sure you update your integration repository first.

        git fetch git://github.com/samhemelryk/moodle.git 39871-26 && git merge FETCH_HEAD --no-edit
        

        Nadav or Mary could you please have a look at this and let me know if you are both happy with it and if it solves the problem for you?
        This patch both applies the manipulations and then removes the old .dir-rtl CSS from pagelayout.css as it is no longer needed and only causes bugs.

        Many thanks
        Sam

        Show
        Sam Hemelryk added a comment - Ok block manipulations are only applied when using $OUTPUT and not the blocks API. Because the base theme still uses the blocks API we must apply the manipulations manually. The following patch should do it: https://github.com/samhemelryk/moodle/commit/39871-26 You can pull test this by running the following command on an integration repository with the master branch checked out. Make sure you update your integration repository first. git fetch git: //github.com/samhemelryk/moodle.git 39871-26 && git merge FETCH_HEAD --no-edit Nadav or Mary could you please have a look at this and let me know if you are both happy with it and if it solves the problem for you? This patch both applies the manipulations and then removes the old .dir-rtl CSS from pagelayout.css as it is no longer needed and only causes bugs. Many thanks Sam
        Hide
        Sam Hemelryk added a comment -

        Ah great to see you're online and looking at it Nadav - would you mind looking at that solution?
        Not sure if you're got dev chat set up but if you do you could always pop in and chat with me about it if you like.

        Show
        Sam Hemelryk added a comment - Ah great to see you're online and looking at it Nadav - would you mind looking at that solution? Not sure if you're got dev chat set up but if you do you could always pop in and chat with me about it if you like.
        Hide
        Nadav Kavalerchik added a comment -

        Me bad. I was not testing pages with single region.

        We have a problem with single region "pages" (layouts) like "admin":

        // Server administration scripts.
            'admin' => array(
                'file' => 'general.php',
                'regions' => array('side-pre'),
                'defaultregion' => 'side-pre',
            ),
        

        The following code, switches regions from "side-pre" to "side-post" where there is not "side-post"

        if ($this->blocks->is_known_region($regionwas) && $this->blocks->is_known_region($regionnow)) {
         // Both the before and after regions are known so we can swap them over.
         return $regionnow;
        }
        
        Show
        Nadav Kavalerchik added a comment - Me bad. I was not testing pages with single region. We have a problem with single region "pages" (layouts) like "admin": // Server administration scripts. 'admin' => array( 'file' => 'general.php', 'regions' => array('side-pre'), 'defaultregion' => 'side-pre', ), The following code, switches regions from "side-pre" to "side-post" where there is not "side-post" if ($ this ->blocks->is_known_region($regionwas) && $ this ->blocks->is_known_region($regionnow)) { // Both the before and after regions are known so we can swap them over. return $regionnow; }
        Hide
        Nadav Kavalerchik added a comment -

        How do I setup the dev chat?

        Show
        Nadav Kavalerchik added a comment - How do I setup the dev chat?
        Hide
        Sam Hemelryk added a comment -

        Just adding the reason we don't apply theme manipulations within the blocks API is that its used internally as well as externally.
        For bootstrapbase we don't use the blocks API at all, all block regions are rendered all the time. Empty regions are handled with CSS.
        I think a good improvement (for after this issue) would be to expose some of those block API methods through $OUTPUT and convert base + friends to use those $OUTPUT methods instead.
        This will both simplify the layouts (one less global to be aware of) and allow us to apply manipulations automatically.
        This however would need to be done as a separate issue (which I am happy to handle)

        Show
        Sam Hemelryk added a comment - Just adding the reason we don't apply theme manipulations within the blocks API is that its used internally as well as externally. For bootstrapbase we don't use the blocks API at all, all block regions are rendered all the time. Empty regions are handled with CSS. I think a good improvement (for after this issue) would be to expose some of those block API methods through $OUTPUT and convert base + friends to use those $OUTPUT methods instead. This will both simplify the layouts (one less global to be aware of) and allow us to apply manipulations automatically. This however would need to be done as a separate issue (which I am happy to handle)
        Hide
        Nadav Kavalerchik added a comment -

        BTW, I have added Damyon's excellent patch on top of this fix, since it looks back into each of the parent themes for any block manipulations. (I was only looking one level back)
        And so I have changed the fix branch from WIP-MDL-39871_master to MDL-39871_master, to avoid confusion.

        Show
        Nadav Kavalerchik added a comment - BTW, I have added Damyon's excellent patch on top of this fix, since it looks back into each of the parent themes for any block manipulations. (I was only looking one level back) And so I have changed the fix branch from WIP- MDL-39871 _master to MDL-39871 _master, to avoid confusion.
        Hide
        Sam Hemelryk added a comment -

        I've messaged you via moodle.org with details

        Show
        Sam Hemelryk added a comment - I've messaged you via moodle.org with details
        Hide
        Nadav Kavalerchik added a comment -

        Sam, I agree with your above comment.
        (OK, diving back into the code... looking for a proper solution )

        Show
        Nadav Kavalerchik added a comment - Sam, I agree with your above comment. (OK, diving back into the code... looking for a proper solution )
        Hide
        Mary Evans added a comment -

        OK...prior to any changes here it is possible for any theme to apply the block manipulation code to the themes config.php to make it work in that theme.

        Why is it that now we have to apply all kinds of changes to allow inheritance and only have the code in one place...base theme?

        OK..it makes sense to have the code in only one place...but why is it necessary to have so many changes to make it work?

        Perhaps I am missing something vital here like a brain! LOL

        Show
        Mary Evans added a comment - OK...prior to any changes here it is possible for any theme to apply the block manipulation code to the themes config.php to make it work in that theme. Why is it that now we have to apply all kinds of changes to allow inheritance and only have the code in one place...base theme? OK..it makes sense to have the code in only one place...but why is it necessary to have so many changes to make it work? Perhaps I am missing something vital here like a brain! LOL
        Hide
        Sam Hemelryk added a comment -

        After much deliberation between Nadav and myself we've decided it would be best to revert it again this week and try again next week.

        Many thanks
        Sam

        Show
        Sam Hemelryk added a comment - After much deliberation between Nadav and myself we've decided it would be best to revert it again this week and try again next week. Many thanks Sam
        Hide
        CiBoT added a comment -

        Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

        Show
        CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
        Hide
        Nadav Kavalerchik added a comment -

        Ok. I think I finally fixed it

        Sam, on top of your $this->blocks->is_known_region() checks I had to add two more checks $this->blocks->get_blocks_for_region() which helped to prevent switching block regions in case the "about to be displayed region" is valid but has no blocks. (This is what we saw yesterday, while testing it together).

        I tested this fix on many system pages, course pages (deep ones too) and on several major themes (with single block column style and two columns styles)... All worked fine, Except Afterburner which was the only one implementing the old RTL switching technique (Thanks to Mary!) So, I have also removed them from Afterburner, like I did in Base.

        Now, this fix seems to solve all RTL switching for all themes.

        Also, Barbara Ramiro, First, Thank you for helping test this issue.
        Second, I saw you added several valid RTL UI issues to this one which are not directly related to this "project". Am I correct? If so, can you please open a new tracker(s) for those issues.

        Show
        Nadav Kavalerchik added a comment - Ok. I think I finally fixed it Sam, on top of your $this->blocks->is_known_region() checks I had to add two more checks $this->blocks->get_blocks_for_region() which helped to prevent switching block regions in case the "about to be displayed region" is valid but has no blocks. (This is what we saw yesterday, while testing it together). I tested this fix on many system pages, course pages (deep ones too) and on several major themes (with single block column style and two columns styles)... All worked fine, Except Afterburner which was the only one implementing the old RTL switching technique (Thanks to Mary!) So, I have also removed them from Afterburner, like I did in Base. Now, this fix seems to solve all RTL switching for all themes. Also, Barbara Ramiro , First, Thank you for helping test this issue. Second, I saw you added several valid RTL UI issues to this one which are not directly related to this "project". Am I correct? If so, can you please open a new tracker(s) for those issues.
        Hide
        Barbara Ramiro added a comment -

        Hi Nadav,

        You're welcome. Yes I added those issues as I encounter them along the way because I don't know exactly which ones were affected by your code change as I don't understand code (fully) and as a designer I only rely on visual differences. I must admit I am not really fit to be a tester (" ,)

        Show
        Barbara Ramiro added a comment - Hi Nadav, You're welcome. Yes I added those issues as I encounter them along the way because I don't know exactly which ones were affected by your code change as I don't understand code (fully) and as a designer I only rely on visual differences. I must admit I am not really fit to be a tester (" ,)
        Hide
        Nadav Kavalerchik added a comment -

        Thank you Barbara!
        Actually you are more than fit to be a tester, since you point out issues that my eye seldom sees. I am sooooo used to the UI that some issues are "hidden" to me. And it is very helpful to have someone with a different point of view looking at the UI.

        I encourage you to open as many issues as you find and label them RTL or just assign them to me.

        Show
        Nadav Kavalerchik added a comment - Thank you Barbara! Actually you are more than fit to be a tester, since you point out issues that my eye seldom sees. I am sooooo used to the UI that some issues are "hidden" to me. And it is very helpful to have someone with a different point of view looking at the UI. I encourage you to open as many issues as you find and label them RTL or just assign them to me.
        Hide
        Nadav Kavalerchik added a comment -

        Sam Hemelryk, Please see if you can spare time to check this fix. I think I found a good solution to this issue.
        I'd love to see if it can be integrated before the freez.

        Show
        Nadav Kavalerchik added a comment - Sam Hemelryk , Please see if you can spare time to check this fix. I think I found a good solution to this issue. I'd love to see if it can be integrated before the freez.
        Hide
        Mary Evans added a comment -

        Hi Nadav, I think the code freeze is on now. It started today, if I am not mistaken?

        Also your patch needs re-basing as Moodle(master) was updated on the 4th Oct 2013.
        Afterburner has been through some big changes too, so you will get some conflicts in the code.

        I will be happy to test it for you.

        Cheers
        Mary

        Show
        Mary Evans added a comment - Hi Nadav, I think the code freeze is on now. It started today, if I am not mistaken? Also your patch needs re-basing as Moodle(master) was updated on the 4th Oct 2013. Afterburner has been through some big changes too, so you will get some conflicts in the code. I will be happy to test it for you. Cheers Mary
        Hide
        Nadav Kavalerchik added a comment -

        Ok. I see.
        Please let me know when I can continue to push this forward.

        Show
        Nadav Kavalerchik added a comment - Ok. I see. Please let me know when I can continue to push this forward.
        Hide
        Marina Glancy added a comment -

        I can see this is the most recent discussion on blocks in RTL.
        FYI I just found and created MDL-42852

        Show
        Marina Glancy added a comment - I can see this is the most recent discussion on blocks in RTL. FYI I just found and created MDL-42852
        Hide
        Nadav Kavalerchik added a comment -

        Hi Marina,

        This fix solves the RTL block switching issue.
        We are using this fix on our servers for some time.
        Please see if you or Sam Hemelryk can review it and merge it if it look ok to you.
        And please close MDL-42852 as duplicate.

        Please let me know if you are reviewing it so I will rebase it.

        Show
        Nadav Kavalerchik added a comment - Hi Marina, This fix solves the RTL block switching issue. We are using this fix on our servers for some time. Please see if you or Sam Hemelryk can review it and merge it if it look ok to you. And please close MDL-42852 as duplicate. Please let me know if you are reviewing it so I will rebase it.
        Hide
        Marina Glancy added a comment -

        Nadav, this issue is an improvement and will be applied to master only. We need a fix for 2.5 as well, and actually 2.6 too because improvements will go to 2.7 now

        Show
        Marina Glancy added a comment - Nadav, this issue is an improvement and will be applied to master only. We need a fix for 2.5 as well, and actually 2.6 too because improvements will go to 2.7 now
        Hide
        Nadav Kavalerchik added a comment -

        Marina, this is semantics, but... this is not an Improvement but a loooong standing issue for the RTL community and it is only my fault that I was not pressing hard enough each release to implement it. Still it is needed in Moodle 2.5 versions. Not just Master or future Moodle 2.7 . Hope you will consider.

        Indeed, after Sam's core improvement for Moodle 2.5 it was easier to implement it. But if you search the tracker, you will see different patches we (Mary and I) tried to push into the themes to solve this.

        Anyways, please if if you can review it and push it first into master.

        Show
        Nadav Kavalerchik added a comment - Marina, this is semantics, but... this is not an Improvement but a loooong standing issue for the RTL community and it is only my fault that I was not pressing hard enough each release to implement it. Still it is needed in Moodle 2.5 versions. Not just Master or future Moodle 2.7 . Hope you will consider. Indeed, after Sam's core improvement for Moodle 2.5 it was easier to implement it. But if you search the tracker, you will see different patches we (Mary and I) tried to push into the themes to solve this. Anyways, please if if you can review it and push it first into master.
        Hide
        Mary Evans added a comment - - edited

        Can I say something here.
        I personally think that if you allow the setting which is in Moodle core already to be added to the config.php of every theme it should work without major changes to CSS. However if HQ insist it should be inherited from the Parent theme, as it is in Bootstrap, then this will need lots of css changes in EVERY theme.

        By the way it was some time ago that I tested this, and it all worked just fine.
        However Marina looks to have found some bugs in Clean theme, which wasn't the problems originally. At least not in my version of Moodle, but time has passed and so Moodle is a lot different than it was.

        So in retrospect I am not sure where we are at this moment in time.

        Show
        Mary Evans added a comment - - edited Can I say something here. I personally think that if you allow the setting which is in Moodle core already to be added to the config.php of every theme it should work without major changes to CSS. However if HQ insist it should be inherited from the Parent theme, as it is in Bootstrap, then this will need lots of css changes in EVERY theme. By the way it was some time ago that I tested this, and it all worked just fine. However Marina looks to have found some bugs in Clean theme, which wasn't the problems originally. At least not in my version of Moodle, but time has passed and so Moodle is a lot different than it was. So in retrospect I am not sure where we are at this moment in time.
        Hide
        Nadav Kavalerchik added a comment -

        I am a "little" busy this week, so let's wait one more week with this until I have time to rebase it and see where it stands, regarding the latest master.

        Show
        Nadav Kavalerchik added a comment - I am a "little" busy this week, so let's wait one more week with this until I have time to rebase it and see where it stands, regarding the latest master.
        Hide
        Mary Evans added a comment - - edited

        I think the idea of swapping sides blocks is getting confused here, especially with side-pre-only and side-post-only, as these do NOT need to swap content only their location. Whereas in a three column page the content of the blocks need to swap sides. Therefore the logic is all wrong in the original blockrtlmanipulations code.

        Gareth got this right in bootstrapbase, after much persuasion from me, to just have span9 float one way or the other, since the contents of the block region remains constant when in RTL mode in a side-pre-only or side-post-only page its just the main content that shifts sides and NOT the blocks, if you see what I mean? So this can be easily achieved in CSS for standard themes too.

        Whereas in a 3 column page, be it Bootstrapbase or standard Moodle themes, the layout is fixed so all that needs to happen in this case, is that the content of the blocks needs to swap sides, which is what the blockrtlmanipulations does, but seems to do it globally whereas in practice it needs to be layout specific.

        Show
        Mary Evans added a comment - - edited I think the idea of swapping sides blocks is getting confused here, especially with side-pre-only and side-post-only, as these do NOT need to swap content only their location. Whereas in a three column page the content of the blocks need to swap sides. Therefore the logic is all wrong in the original blockrtlmanipulations code. Gareth got this right in bootstrapbase, after much persuasion from me, to just have span9 float one way or the other, since the contents of the block region remains constant when in RTL mode in a side-pre-only or side-post-only page its just the main content that shifts sides and NOT the blocks, if you see what I mean? So this can be easily achieved in CSS for standard themes too. Whereas in a 3 column page, be it Bootstrapbase or standard Moodle themes, the layout is fixed so all that needs to happen in this case, is that the content of the blocks needs to swap sides, which is what the blockrtlmanipulations does, but seems to do it globally whereas in practice it needs to be layout specific.
        Hide
        CiBoT added a comment -

        Results for MDL-39871

        • Remote repository: git://github.com/nadavkav/moodle.git
        Show
        CiBoT added a comment - Results for MDL-39871 Remote repository: git://github.com/nadavkav/moodle.git Remote branch MDL-39871 _master to be integrated into upstream master Executed job http://integration.moodle.org/job/Precheck%20remote%20branch/353 Error: The MDL-39871 _master branch at git://github.com/nadavkav/moodle.git is very old. Please rebase against current master.
        Hide
        Nadav Kavalerchik added a comment -

        Sam Hemelryk,

        Once again, I have some free time I can spare to get this issue integrated.
        Should I rebase it? Will you have time to review it once again?

        Show
        Nadav Kavalerchik added a comment - Sam Hemelryk , Once again, I have some free time I can spare to get this issue integrated. Should I rebase it? Will you have time to review it once again?
        Hide
        Sam Hemelryk added a comment -

        Hi Nadav,

        I've just had a quick look at this issue again now. Both the concept and the code.
        There are a couple of things that strike me about this issue.
        The first is the concept. This change could only land to 2.7 and at present I believe the plan for 2.7 is to make bootstrapbase/clean the default themes.
        I suspect base, standard etc assumably will be deprecated.
        Second there are a couple of things I noted in the code that could cause potential issue:

        1. The changes to outputlib.php - inheriting block manipulations from the parent theme may not be a great idea. We'd need to ensure that there was a way to not apply block manipulations within a theme that extends a parent with them (perhaps the design necessitates it)
        2. The changes to pagelib.php would need to be closely looked at. Calling $this->blocks->get_blocks_for_region can lead to an exception, and already calls is_known_region internally. Really we'd just need to verify the need for this change.

        At present I'm not sure what the best path for this issue is, perhaps given it is 2.7 only and base won't be default in 2.7 we don't worry about it. But then again, there will still be lost of themes based on base out there and they won't disappear for a long time in which case perhaps the focus of this issue should be on converting the existing base + standard to use the new blockrtlmanipulations and not worry about implementing inheriting from parent themes as part of this issue. (we could consider that separately later may be better).

        What are your thoughts?

        Show
        Sam Hemelryk added a comment - Hi Nadav, I've just had a quick look at this issue again now. Both the concept and the code. There are a couple of things that strike me about this issue. The first is the concept. This change could only land to 2.7 and at present I believe the plan for 2.7 is to make bootstrapbase/clean the default themes. I suspect base, standard etc assumably will be deprecated. Second there are a couple of things I noted in the code that could cause potential issue: The changes to outputlib.php - inheriting block manipulations from the parent theme may not be a great idea. We'd need to ensure that there was a way to not apply block manipulations within a theme that extends a parent with them (perhaps the design necessitates it) The changes to pagelib.php would need to be closely looked at. Calling $this->blocks->get_blocks_for_region can lead to an exception, and already calls is_known_region internally. Really we'd just need to verify the need for this change. At present I'm not sure what the best path for this issue is, perhaps given it is 2.7 only and base won't be default in 2.7 we don't worry about it. But then again, there will still be lost of themes based on base out there and they won't disappear for a long time in which case perhaps the focus of this issue should be on converting the existing base + standard to use the new blockrtlmanipulations and not worry about implementing inheriting from parent themes as part of this issue. (we could consider that separately later may be better). What are your thoughts?
        Hide
        Nadav Kavalerchik added a comment -

        Hi Sam,

        Since 2.7 is, probably, going to be the next version everybody in the Israeli (RTL) Education sector is going to upgrade to. And Since it will be using the default Clean theme. And also everybody (here) is talking about developing new bootstrap based theme for the next academic year... I guess we missed the window of opportunity with integrating this patch into core. It seems not so relevant anymore. Especially with theme Elegance which is based on bootstrap 3, that will get RTL support at some point. So I feel there are good options to work with for Moodle 2.7 .

        The positive side in all this is that I have 30+ system using this patch the entire last semester just fine.

        Let's not waste your precious time, Let's close this issue

        How about it?

        Show
        Nadav Kavalerchik added a comment - Hi Sam, Since 2.7 is, probably, going to be the next version everybody in the Israeli (RTL) Education sector is going to upgrade to. And Since it will be using the default Clean theme. And also everybody (here) is talking about developing new bootstrap based theme for the next academic year... I guess we missed the window of opportunity with integrating this patch into core. It seems not so relevant anymore. Especially with theme Elegance which is based on bootstrap 3, that will get RTL support at some point. So I feel there are good options to work with for Moodle 2.7 . The positive side in all this is that I have 30+ system using this patch the entire last semester just fine. Let's not waste your precious time, Let's close this issue How about it?
        Hide
        Michael de Raadt added a comment -

        Hi, Nadav.

        The window for getting fixes in for 2.7 is still wide open at this stage.

        I wouldn't bank on Bootstrap 3 at this stage.

        Show
        Michael de Raadt added a comment - Hi, Nadav. The window for getting fixes in for 2.7 is still wide open at this stage. I wouldn't bank on Bootstrap 3 at this stage.
        Hide
        Mary Evans added a comment -

        @Michael, it is possible to build a Bootstrap3 theme and still have bootstrapbase as a parent theme. I've done this and it works great. So as Nadav say might as well close this issue it is no-longer relevant.

        If hard pushed there is still Afterburner that is fully responsive and caters fro RTL better than any other theme in Moodle at this time.

        Show
        Mary Evans added a comment - @Michael, it is possible to build a Bootstrap3 theme and still have bootstrapbase as a parent theme. I've done this and it works great. So as Nadav say might as well close this issue it is no-longer relevant. If hard pushed there is still Afterburner that is fully responsive and caters fro RTL better than any other theme in Moodle at this time.
        Hide
        Dan Poltawski added a comment -

        Sending all 'waiting for peer review' issues for integration review. The integration team are doing this to ensure any 'integratable issues' will not got missed for freeze.

        Note: We will prioritise peer reviewed issues and may not spend as much time examining non-integratable, non peer-reviewed issues.

        This is a present from the iTeam - it means that peer review is not working well enough! We really do not want to do this again! Lets improve our peer review process!

        Show
        Dan Poltawski added a comment - Sending all 'waiting for peer review' issues for integration review. The integration team are doing this to ensure any 'integratable issues' will not got missed for freeze. Note: We will prioritise peer reviewed issues and may not spend as much time examining non-integratable, non peer-reviewed issues. This is a present from the iTeam - it means that peer review is not working well enough! We really do not want to do this again! Lets improve our peer review process!
        Hide
        Sam Hemelryk added a comment -

        Hmmm

        I am very sorry, really I want to help with the situation here and despite bootstrapbase being default for 2.7 I don't want to give up on base.
        I will endeavour to look at this in the next day or two and see what I can make of it.
        At present this is reopened and I will try to get something done and plead my case with the powers that be.

        Show
        Sam Hemelryk added a comment - Hmmm I am very sorry, really I want to help with the situation here and despite bootstrapbase being default for 2.7 I don't want to give up on base. I will endeavour to look at this in the next day or two and see what I can make of it. At present this is reopened and I will try to get something done and plead my case with the powers that be.
        Hide
        CiBoT added a comment -

        Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

        Show
        CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
        Hide
        CiBoT added a comment -

        Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

        Show
        CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

          People

          • Votes:
            2 Vote for this issue
            Watchers:
            5 Start watching this issue

            Dates

            • Created:
              Updated: