Moodle
  1. Moodle
  2. MDL-34496

issue(s) with custom block regions

    Details

    • Affected Branches:
      MOODLE_23_STABLE, MOODLE_24_STABLE, MOODLE_25_STABLE
    • Rank:
      42916

      Description

      There are some issues with custom block regions which have been identified. The problems encountered so far are when using Aardvark-Postit Version: 2012071202(DRAG-N-DROP), Aerie and Rocket themes.

      After blocks have been successfully created and then moved to the new block-region areas, using AJAX Drag-n-Drop, the page layout no longer appears to work. This behaviour seems to be connected to body classes generated by the dock.js.

      The only way to be sure of all the problems is to create a theme that is as simple as possible only introducing 2 custom block regions (4 block regions in total) and enabling the dock.

        Issue Links

          Activity

          Hide
          Mary Evans added a comment - - edited

          @Sam

          I have Assigned this to you as you suggested. I'm going to create a theme as soon as possible, so that I, and others, can test and log the results of the various combinations of events, which can trigger the behaviour as explained in the description of this issue.

          @Miriam, Julian & Dietmar please feel free to add a comment and share your experiences of these themes which are trying to push the boundaries of Moodle theme design, and being stopped at ever turn since Moodle 2.3 was released!

          Show
          Mary Evans added a comment - - edited @Sam I have Assigned this to you as you suggested. I'm going to create a theme as soon as possible, so that I, and others, can test and log the results of the various combinations of events, which can trigger the behaviour as explained in the description of this issue. @Miriam, Julian & Dietmar please feel free to add a comment and share your experiences of these themes which are trying to push the boundaries of Moodle theme design, and being stopped at ever turn since Moodle 2.3 was released!
          Hide
          Julian Ridden added a comment -

          Thanks for logging this Mary,

          MD said in the beginning his vision for the new themes engine was to push the boundaries of design, and sadly we are currently frustrated that the moment we go outside the three column design we get these issues. Would be nice to see this fixed asap.

          No more to add in relation to the bug itself that Mary has not already added.

          Show
          Julian Ridden added a comment - Thanks for logging this Mary, MD said in the beginning his vision for the new themes engine was to push the boundaries of design, and sadly we are currently frustrated that the moment we go outside the three column design we get these issues. Would be nice to see this fixed asap. No more to add in relation to the bug itself that Mary has not already added.
          Hide
          Dietmar Wagner added a comment -

          On the road again. Will be back in a few days. I too think we are going backwards at the moment.

          Show
          Dietmar Wagner added a comment - On the road again. Will be back in a few days. I too think we are going backwards at the moment.
          Hide
          Miriam Laidlaw added a comment -

          As Mary said, this issue is affecting my theme, Aerie, which worked perfectly in Moodle 2.2 and under. The theme is non-functional in Moodle 2.3 now, and there doesn't appear to be anything I can do at my end to fix it. I have had several people message me begging for it to be fixed.

          We also have several other themes where I have implemented extra block areas (http://www.hrdnz.com and http://themes.moodlebites.com ), but thankfully haven't updated those sites to 2.3 yet. We won't be able to until this is fixed, or I have to shelve those themes and make new ones.

          Show
          Miriam Laidlaw added a comment - As Mary said, this issue is affecting my theme, Aerie, which worked perfectly in Moodle 2.2 and under. The theme is non-functional in Moodle 2.3 now, and there doesn't appear to be anything I can do at my end to fix it. I have had several people message me begging for it to be fixed. We also have several other themes where I have implemented extra block areas ( http://www.hrdnz.com and http://themes.moodlebites.com ), but thankfully haven't updated those sites to 2.3 yet. We won't be able to until this is fixed, or I have to shelve those themes and make new ones.
          Hide
          Sam Hemelryk added a comment -

          Hi Michael,

          I've added you as a watcher to this issue. I would greatly appreciate it if you could get this issue into an upcoming stable sprint.
          Pretty much it is going to require creating and testing a theme with unconventional block regions in as many situations as we can imagine.
          There will no doubt be several areas of code (JS + PHP) that will require rectification and I would I suspect we may need to look at how we can routinely asses a page for block regions or whether we need to pass that information around.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Michael, I've added you as a watcher to this issue. I would greatly appreciate it if you could get this issue into an upcoming stable sprint. Pretty much it is going to require creating and testing a theme with unconventional block regions in as many situations as we can imagine. There will no doubt be several areas of code (JS + PHP) that will require rectification and I would I suspect we may need to look at how we can routinely asses a page for block regions or whether we need to pass that information around. Cheers Sam
          Hide
          Mary Evans added a comment -

          @Tim
          I have just added you as a watcher here Tim

          Show
          Mary Evans added a comment - @Tim I have just added you as a watcher here Tim
          Hide
          Tim Hunt added a comment -

          So much of the necessary infrastructure is in place to handle arbitrary block regions that I really think it is worth finding and fixing those parts of the code that currently make the unwarranted assumption that there just just two block regions, side-pre and side-post.

          As Sam says, what we really need is a crazy theme with a set of block regions that break all possible assumptions

          Show
          Tim Hunt added a comment - So much of the necessary infrastructure is in place to handle arbitrary block regions that I really think it is worth finding and fixing those parts of the code that currently make the unwarranted assumption that there just just two block regions, side-pre and side-post. As Sam says, what we really need is a crazy theme with a set of block regions that break all possible assumptions
          Hide
          Mary Evans added a comment - - edited

          Add two new block regions to a clone of standard theme, put one in the header and the other in the footer. Then using ADD Block create a HTML block, add a small image and some text. Save it and then MOVE it to the header. Add another block and move it into the footer. Then try docking some/all of the blocks other than the ones in the header and the footer.

          I suppose one could always change the default region to the footer block region, thus adding the ADD BLOCK function to the footer, which would be interesting to see how Moodle responds.

          Show
          Mary Evans added a comment - - edited Add two new block regions to a clone of standard theme, put one in the header and the other in the footer. Then using ADD Block create a HTML block, add a small image and some text. Save it and then MOVE it to the header. Add another block and move it into the footer. Then try docking some/all of the blocks other than the ones in the header and the footer. I suppose one could always change the default region to the footer block region, thus adding the ADD BLOCK function to the footer, which would be interesting to see how Moodle responds.
          Hide
          Mary Evans added a comment - - edited

          @Tim

          This is where I think it is all wrong...moodle/lib/yui/blocks/blocks.js

          // See if we are missing either of block regions,
          // if yes we need to add an empty one to use as target
          if (blockregionlist.size() != this.get('regions').length) {
              var blockregion = Y.Node.create('<div></div>')
                  .addClass(CSS.BLOCKREGION);
              var regioncontent = Y.Node.create('<div></div>')
                  .addClass(CSS.REGIONCONTENT);
              blockregion.appendChild(regioncontent);
          
              var regionid = this.get_region_id(blockregionlist.item(0));
              if (regionid === 'post') {
                  // pre block is missing, instert it before post
                  blockregion.setAttrs({id : 'region-pre'});
                  blockregionlist.item(0).insert(blockregion, 'before');
                  blockregionlist.unshift(blockregion);
              } else {
                  // post block is missing, instert it after pre
                  blockregion.setAttrs({id : 'region-post'});
                  blockregionlist.item(0).insert(blockregion, 'after');
                  blockregionlist.push(blockregion);
              }
          }
          
          
          Show
          Mary Evans added a comment - - edited @Tim This is where I think it is all wrong...moodle/lib/yui/blocks/blocks.js // See if we are missing either of block regions, // if yes we need to add an empty one to use as target if (blockregionlist.size() != this .get('regions').length) { var blockregion = Y.Node.create('<div></div>') .addClass(CSS.BLOCKREGION); var regioncontent = Y.Node.create('<div></div>') .addClass(CSS.REGIONCONTENT); blockregion.appendChild(regioncontent); var regionid = this .get_region_id(blockregionlist.item(0)); if (regionid === 'post') { // pre block is missing, instert it before post blockregion.setAttrs({id : 'region-pre'}); blockregionlist.item(0).insert(blockregion, 'before'); blockregionlist.unshift(blockregion); } else { // post block is missing, instert it after pre blockregion.setAttrs({id : 'region-post'}); blockregionlist.item(0).insert(blockregion, 'after'); blockregionlist.push(blockregion); } }
          Hide
          Sam Hemelryk added a comment -

          Hi guys,

          I've spent an hour this morning familiarising myself with this issue, how things are done and what I think needs to happen in order to allow us to work around this issue.

          The biggest hurdle here is that presently if a block region contains no blocks, the user has both turned editing on and JavaScript enabled then although the user is able to move blocks to the empty region the region has not been rendered at all and the JavaScript must guess where the section should exist.
          The current JS works on the principle that the there are two block regions, pre and post, and that those two regions exist next to each other and with identical structure other than classes+id's specific to the region.
          If thats not the case (alternative HTML layout structure or custom regions) then things will break.
          The issue could be summarised to there being a lack of information about missing regions, where they should exist or what they should look like leading to taking a punt being the only option available to the JavaScript.

          Pretty much in order to make this work we are going to need to ensure that a region is either always rendered and that we implement a better system of marking regions as visible with content, visible without content, and not visible.
          The regions always existing in HTML and their visibility controlled via CSS is what will allow us to write JavaScript to control the state of things, required in order to get this working.
          The biggest hurdle in this plan presently is that every theme contains logic that decides whether or not to display a region.
          Pretty much we can't fix this issue in a way that is going to work with existing themes with custom regions without requiring them be modified.
          The upside is that we will likely strip out all of that logic from theme layout files and instead will introduce better structure through the blocks region and add some better CSS classes to represent region states.
          The following needs to happen to start with:

          1. Change core theme's to always render block regions.
          2. Add HTML structure to core_renderer::blocks_for_region to contain/wrap the block region contents and embed in it the region's name, and any state information we need.
          3. Add to the base themes CSS to handle the state of all block regions (required to maintain bc)
          4. Update lib/yui/blocks/blocks.js. It has lots of logic to replicate structure, identify blocks within structure, and move them that is going to need to be rewritten.
          5. Update docs/tutorials.

          Worth pointing out such a change would not make it into 2.4 at this point I imagine, I doubt I'll have time to get around to it myself but would be more than happy to help someone to a final solution if someone has time and needs a hand.

          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - Hi guys, I've spent an hour this morning familiarising myself with this issue, how things are done and what I think needs to happen in order to allow us to work around this issue. The biggest hurdle here is that presently if a block region contains no blocks, the user has both turned editing on and JavaScript enabled then although the user is able to move blocks to the empty region the region has not been rendered at all and the JavaScript must guess where the section should exist. The current JS works on the principle that the there are two block regions, pre and post, and that those two regions exist next to each other and with identical structure other than classes+id's specific to the region. If thats not the case (alternative HTML layout structure or custom regions) then things will break. The issue could be summarised to there being a lack of information about missing regions, where they should exist or what they should look like leading to taking a punt being the only option available to the JavaScript. Pretty much in order to make this work we are going to need to ensure that a region is either always rendered and that we implement a better system of marking regions as visible with content, visible without content, and not visible. The regions always existing in HTML and their visibility controlled via CSS is what will allow us to write JavaScript to control the state of things, required in order to get this working. The biggest hurdle in this plan presently is that every theme contains logic that decides whether or not to display a region. Pretty much we can't fix this issue in a way that is going to work with existing themes with custom regions without requiring them be modified. The upside is that we will likely strip out all of that logic from theme layout files and instead will introduce better structure through the blocks region and add some better CSS classes to represent region states. The following needs to happen to start with: Change core theme's to always render block regions. Add HTML structure to core_renderer::blocks_for_region to contain/wrap the block region contents and embed in it the region's name, and any state information we need. Add to the base themes CSS to handle the state of all block regions (required to maintain bc) Update lib/yui/blocks/blocks.js. It has lots of logic to replicate structure, identify blocks within structure, and move them that is going to need to be rewritten. Update docs/tutorials. Worth pointing out such a change would not make it into 2.4 at this point I imagine, I doubt I'll have time to get around to it myself but would be more than happy to help someone to a final solution if someone has time and needs a hand. Many thanks Sam
          Hide
          Mary Evans added a comment - - edited

          Thanks Sam for pointing out where the major problems are, and for your suggestions on where the changes need to be made.

          Strangely enough, the Aardvark Post-IT theme, which has two custom block regions in a hidden part of the header which can be accessed with a show/hide toggle switch, works great in Moodle 2.2.x. It was not until 2.3 that the problems started. This is when changes were made to use Drag-n-Drop. The fact that moving blocks into a new custom block region worked OK in Moodle 2.2, providing one did not add class="block-region" to the custom block ID container, suggests that the solution may be simpler than you think.

          It strikes me that somewhere, in the current logic which is in use, that each block carries some 'relationship' that shows its parent is either region-pre or region-post. It is this relationship we need to remove.

          Blocks should be just 'movable objects' which have no home only the place they are at a given time, and should not be restricted to region-pre and/or region-post, they should be able to go anywhere. It is only the blocks that are in region-pre and region-post that govern the page-layout 'side-pre-only', 'side-post-only', 'content-only', therefore all the other blocks, scattered about the page, do not need to be included in the algorithm other than a value of blocks which are NOT_SIDE_PRE and NOT_SIDE_POST.

          Show
          Mary Evans added a comment - - edited Thanks Sam for pointing out where the major problems are, and for your suggestions on where the changes need to be made. Strangely enough, the Aardvark Post-IT theme, which has two custom block regions in a hidden part of the header which can be accessed with a show/hide toggle switch, works great in Moodle 2.2.x. It was not until 2.3 that the problems started. This is when changes were made to use Drag-n-Drop. The fact that moving blocks into a new custom block region worked OK in Moodle 2.2, providing one did not add class="block-region" to the custom block ID container, suggests that the solution may be simpler than you think. It strikes me that somewhere, in the current logic which is in use, that each block carries some 'relationship' that shows its parent is either region-pre or region-post. It is this relationship we need to remove. Blocks should be just 'movable objects' which have no home only the place they are at a given time, and should not be restricted to region-pre and/or region-post, they should be able to go anywhere. It is only the blocks that are in region-pre and region-post that govern the page-layout 'side-pre-only', 'side-post-only', 'content-only', therefore all the other blocks, scattered about the page, do not need to be included in the algorithm other than a value of blocks which are NOT_SIDE_PRE and NOT_SIDE_POST.
          Hide
          Ruslan Kabalin added a comment -

          @Sam

          Agree with your list of changes. This is partly addressed in MDL-32608.

          Show
          Ruslan Kabalin added a comment - @Sam Agree with your list of changes. This is partly addressed in MDL-32608 .
          Hide
          Julian Ridden added a comment -

          Thanks for adding your thoughts Sam. Good to know at least the problems have identifiable solutions.

          Am hoping we can get this proritised as it is a blocker item. Anyone able to lean on MD to make this part of 2.4.1 perhaps?

          Show
          Julian Ridden added a comment - Thanks for adding your thoughts Sam. Good to know at least the problems have identifiable solutions. Am hoping we can get this proritised as it is a blocker item. Anyone able to lean on MD to make this part of 2.4.1 perhaps?
          Hide
          Ruslan Kabalin added a comment - - edited

          We have time allocated to do that, I think I will be able to properly focus on this at some point in December/January.
          @Sam thanks for the readiness to help, I will definitely have some questions about blocks rendering changes.

          Show
          Ruslan Kabalin added a comment - - edited We have time allocated to do that, I think I will be able to properly focus on this at some point in December/January. @Sam thanks for the readiness to help, I will definitely have some questions about blocks rendering changes.
          Hide
          Sam Hemelryk added a comment -

          Hi guys, first up thanks for the replies.

          Mary, I don't think there is any easier solution that will allow us to do both custom block regions and JS. Custom block regions still work fine if you disable JavaScript, its the JS that is killing things. I'm pretty sure that for the JS to truly work no matter what regions and layout are being used we will need to improve our block region implementations.
          While the pre and post regions are what we've used for our core theme's things have (in the past) being designed to support absolutely any regions. That drag and drop stuff unfortunately broke this design, bu hopefully we can get back to it. The end solution should ideally result in both sets of functionality (dnd + custom blocks) being supported.
          That being said I would be very happy to being proven wrong

          Ruslan, great to hear that you are able to help with this one! Do ping me if you need a hand or if there is any way I can help.

          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - Hi guys, first up thanks for the replies. Mary, I don't think there is any easier solution that will allow us to do both custom block regions and JS. Custom block regions still work fine if you disable JavaScript, its the JS that is killing things. I'm pretty sure that for the JS to truly work no matter what regions and layout are being used we will need to improve our block region implementations. While the pre and post regions are what we've used for our core theme's things have (in the past) being designed to support absolutely any regions. That drag and drop stuff unfortunately broke this design, bu hopefully we can get back to it. The end solution should ideally result in both sets of functionality (dnd + custom blocks) being supported. That being said I would be very happy to being proven wrong Ruslan, great to hear that you are able to help with this one! Do ping me if you need a hand or if there is any way I can help. Many thanks Sam
          Hide
          Mary Evans added a comment -

          I've been working on this for that last 4 hours (how time flies) I checked the ../lib/yui/blocks/blocks.js and that seems to be working OK.

          What is not working, for my Aardvark Post-IT theme at least, are the body classes, even though I managed to get a complete new set to show up, none of them worked because the ../blocks/dock.js was blocking them (no pun intended). So this is where I think it's all wrong.

          /**
           * Resizes the space that contained blocks if there were no blocks left in
           * it. e.g. if all blocks have been moved to the dock
           * @param {Y.Node} node
           */
          M.core_dock.resizeBlockSpace = function(node) {
          
              if (this.Y.all('.block.dock_on_load').size()>0) {
                  // Do not resize during initial load
                  return;
              }
              var blockregions = [];
              var populatedblockregions = 0;
              this.Y.all('.block-region').each(function(region){
                  var hasblocks = (region.all('.block').size() > 0);
                  if (hasblocks) {
                      populatedblockregions++;
                  }
                  blockregions[region.get('id')] = {hasblocks: hasblocks, bodyclass: region.get('id').replace(/^region\-/, 'side-')+'-only'};
              });
              var bodynode = M.core_dock.nodes.body;
              var showregions = false;
              if (bodynode.hasClass('blocks-moving')) {
                  // open up blocks during blocks positioning
                  showregions = true;
              }
          
              var noblocksbodyclass = 'content-only';
              var i = null;
              if (populatedblockregions==0 && showregions==false) {
                  bodynode.addClass(noblocksbodyclass);
                  for (i in blockregions) {
                      bodynode.removeClass(blockregions[i].bodyclass);
                  }
              } else if (populatedblockregions==1 && showregions==false) {
                  bodynode.removeClass(noblocksbodyclass);
                  for (i in blockregions) {
                      if (!blockregions[i].hasblocks) {
                          bodynode.removeClass(blockregions[i].bodyclass);
                      } else {
                          bodynode.addClass(blockregions[i].bodyclass);
                      }
                  }
              } else {
                  bodynode.removeClass(noblocksbodyclass);
                  for (i in blockregions) {
                      bodynode.removeClass(blockregions[i].bodyclass);
                  }
              }
          };
          
          Show
          Mary Evans added a comment - I've been working on this for that last 4 hours (how time flies) I checked the ../lib/yui/blocks/blocks.js and that seems to be working OK. What is not working, for my Aardvark Post-IT theme at least, are the body classes, even though I managed to get a complete new set to show up, none of them worked because the ../blocks/dock.js was blocking them (no pun intended). So this is where I think it's all wrong. /** * Resizes the space that contained blocks if there were no blocks left in * it. e.g. if all blocks have been moved to the dock * @param {Y.Node} node */ M.core_dock.resizeBlockSpace = function(node) { if ( this .Y.all('.block.dock_on_load').size()>0) { // Do not resize during initial load return ; } var blockregions = []; var populatedblockregions = 0; this .Y.all('.block-region').each(function(region){ var hasblocks = (region.all('.block').size() > 0); if (hasblocks) { populatedblockregions++; } blockregions[region.get('id')] = {hasblocks: hasblocks, bodyclass: region.get('id').replace(/^region\-/, 'side-')+'-only'}; }); var bodynode = M.core_dock.nodes.body; var showregions = false ; if (bodynode.hasClass('blocks-moving')) { // open up blocks during blocks positioning showregions = true ; } var noblocksbodyclass = 'content-only'; var i = null ; if (populatedblockregions==0 && showregions== false ) { bodynode.addClass(noblocksbodyclass); for (i in blockregions) { bodynode.removeClass(blockregions[i].bodyclass); } } else if (populatedblockregions==1 && showregions== false ) { bodynode.removeClass(noblocksbodyclass); for (i in blockregions) { if (!blockregions[i].hasblocks) { bodynode.removeClass(blockregions[i].bodyclass); } else { bodynode.addClass(blockregions[i].bodyclass); } } } else { bodynode.removeClass(noblocksbodyclass); for (i in blockregions) { bodynode.removeClass(blockregions[i].bodyclass); } } };
          Hide
          Martin Dougiamas added a comment -

          Ah, I assumed this had made it in 2.4. Any new news?

          Show
          Martin Dougiamas added a comment - Ah, I assumed this had made it in 2.4. Any new news?
          Hide
          Mary Evans added a comment -

          Nope!

          Show
          Mary Evans added a comment - Nope!
          Hide
          Julian Ridden added a comment -

          Any more updates on this? Any idea when we will see a fix hit a release?

          Show
          Julian Ridden added a comment - Any more updates on this? Any idea when we will see a fix hit a release?
          Hide
          Robert Boloc added a comment -

          I tracked down the issue to course/lib.php file:

              // Include blocks dragdrop
              $params = array(
                  'courseid' => $course->id,
                  'pagetype' => $PAGE->pagetype,
                  'pagelayout' => $PAGE->pagelayout,
                  'subpage' => $PAGE->subpage,
                  'regions' => get_blocks_regions(),
              );
              $PAGE->requires->yui_module('moodle-core-blocks', 'M.core_blocks.init_dragdrop', array($params), null, true);
          

          the regions key is used later on in lib/yui/blocks/blocks.js

                      if (blockregionlist.size() != this.get('regions').length) {
                          var blockregion = Y.Node.create('<div></div>')
                              .addClass(CSS.BLOCKREGION);
                          var regioncontent = Y.Node.create('<div></div>')
                              .addClass(CSS.REGIONCONTENT);
                          blockregion.appendChild(regioncontent);
          
                          var regionid = this.get_region_id(blockregionlist.item(0));
                          if (regionid === 'post') {
                              // pre block is missing, instert it before post
                              blockregion.setAttrs({id : 'region-pre'});
                              blockregionlist.item(0).insert(blockregion, 'before');
                              blockregionlist.unshift(blockregion);
                          } else {
                              // post block is missing, instert it after pre
                              blockregion.setAttrs({id : 'region-post'});
                              blockregionlist.item(0).insert(blockregion, 'after');
                              blockregionlist.push(blockregion);
                          }
                      }
          

          On the line

          if (blockregionlist.size() != this.get('regions').length)

          the length return in wrong if custom regions are defined.
          So the fix is to filter the custom regions when passing the regions to this piece of code.

          Fix for 2.3.x branch: https://github.com/robertboloc/moodle/tree/%23MDL-34496-2.3
          Diff: https://github.com/robertboloc/moodle/compare/MOODLE_23_STABLE...%23MDL-34496-2.3

          Fix for 2.4.x branch: https://github.com/robertboloc/moodle/tree/%23MDL-34496-2.4
          Diff: https://github.com/robertboloc/moodle/compare/MOODLE_24_STABLE...%23MDL-34496-2.4

          Show
          Robert Boloc added a comment - I tracked down the issue to course/lib.php file: // Include blocks dragdrop $params = array( 'courseid' => $course->id, 'pagetype' => $PAGE->pagetype, 'pagelayout' => $PAGE->pagelayout, 'subpage' => $PAGE->subpage, 'regions' => get_blocks_regions(), ); $PAGE->requires->yui_module('moodle-core-blocks', 'M.core_blocks.init_dragdrop', array($params), null, true); the regions key is used later on in lib/yui/blocks/blocks.js if (blockregionlist.size() != this.get('regions').length) { var blockregion = Y.Node.create('<div></div>') .addClass(CSS.BLOCKREGION); var regioncontent = Y.Node.create('<div></div>') .addClass(CSS.REGIONCONTENT); blockregion.appendChild(regioncontent); var regionid = this.get_region_id(blockregionlist.item(0)); if (regionid === 'post') { // pre block is missing, instert it before post blockregion.setAttrs({id : 'region-pre'}); blockregionlist.item(0).insert(blockregion, 'before'); blockregionlist.unshift(blockregion); } else { // post block is missing, instert it after pre blockregion.setAttrs({id : 'region-post'}); blockregionlist.item(0).insert(blockregion, 'after'); blockregionlist.push(blockregion); } } On the line if (blockregionlist.size() != this.get('regions').length) the length return in wrong if custom regions are defined. So the fix is to filter the custom regions when passing the regions to this piece of code. Fix for 2.3.x branch: https://github.com/robertboloc/moodle/tree/%23MDL-34496-2.3 Diff: https://github.com/robertboloc/moodle/compare/MOODLE_23_STABLE...%23MDL-34496-2.3 Fix for 2.4.x branch: https://github.com/robertboloc/moodle/tree/%23MDL-34496-2.4 Diff: https://github.com/robertboloc/moodle/compare/MOODLE_24_STABLE...%23MDL-34496-2.4
          Hide
          Paul Nicholls added a comment -

          Can somebody familiar with this issue please test this on current master? If this is related to the sections of code identified by Robert, I think that the changes made in MDL-32652 in order to support the central "content" block region (on My Moodle pages, profile pages, etc) might have resolved this issue - or at least gone part way to a resolution. There may still be some issues (depending on how the theme is handling the custom block regions, you may not get a placeholder to drop blocks onto for custom regions if they're currently empty), but it's hopefully at least a step in the right direction (i.e. away from brokenness).

          Show
          Paul Nicholls added a comment - Can somebody familiar with this issue please test this on current master? If this is related to the sections of code identified by Robert, I think that the changes made in MDL-32652 in order to support the central "content" block region (on My Moodle pages, profile pages, etc) might have resolved this issue - or at least gone part way to a resolution. There may still be some issues (depending on how the theme is handling the custom block regions, you may not get a placeholder to drop blocks onto for custom regions if they're currently empty), but it's hopefully at least a step in the right direction (i.e. away from brokenness).
          Hide
          Mary Evans added a comment -

          Yes I will...

          Show
          Mary Evans added a comment - Yes I will...
          Hide
          Mary Evans added a comment - - edited

          I've just discovered a problem with my Aardvark Post-IT theme which I have fixed in lib/yui/blocks/blocks.js

          Because my theme's custom regions are hidden in the drop-down section within the header, where two regions are defined and styled. I've called one block region side-left and the other side-right.

          When editing is off the drop-down section looks normal with two empty regions waiting to add blocks and two other static sections with images.

          When editing is on the drop-down area mysteriously acquires a third block region which sits between the two pre-defined block regions. The third region is side-post.

          This problem appears to be caused by this code in lib/yui/blocks/blocks.js

                          var regionid = this.get_region_id(blockregionlist.item(0));
                          if (regionid === 'post') {
                              // pre block is missing, instert it before post
                              blockregion.setAttrs({id : 'region-pre'});
                              blockregionlist.item(0).insert(blockregion, 'before');
                              blockregionlist.unshift(blockregion);
                          } else {
                              // post block is missing, instert it after pre
                              blockregion.setAttrs({id : 'region-post'});
                              blockregionlist.item(0).insert(blockregion, 'after');
                              blockregionlist.push(blockregion);
                          }
                      }
          

          which I changed to this...

                          var regionid = this.get_region_id(blockregionlist.item(0));
                          if (regionid === 'post') {
                              // pre block is missing, instert it before post
                              blockregion.setAttrs({id : 'region-pre'});
                              blockregionlist.item(0).insert(blockregion, 'before');
                              blockregionlist.unshift(blockregion);
                          } else if (regionid === 'pre') {
                              // post block is missing, instert it after pre
                              blockregion.setAttrs({id : 'region-post'});
                              blockregionlist.item(0).insert(blockregion, 'after');
                              blockregionlist.push(blockregion);
                          }
                      }
          

          And everything seems to work OK until you dock the blocks. It is here that the layout starts to misbehave, which was what was happening before.

          Almost there, but not quite.

          Show
          Mary Evans added a comment - - edited I've just discovered a problem with my Aardvark Post-IT theme which I have fixed in lib/yui/blocks/blocks.js Because my theme's custom regions are hidden in the drop-down section within the header, where two regions are defined and styled. I've called one block region side-left and the other side-right. When editing is off the drop-down section looks normal with two empty regions waiting to add blocks and two other static sections with images. When editing is on the drop-down area mysteriously acquires a third block region which sits between the two pre-defined block regions. The third region is side-post. This problem appears to be caused by this code in lib/yui/blocks/blocks.js var regionid = this .get_region_id(blockregionlist.item(0)); if (regionid === 'post') { // pre block is missing, instert it before post blockregion.setAttrs({id : 'region-pre'}); blockregionlist.item(0).insert(blockregion, 'before'); blockregionlist.unshift(blockregion); } else { // post block is missing, instert it after pre blockregion.setAttrs({id : 'region-post'}); blockregionlist.item(0).insert(blockregion, 'after'); blockregionlist.push(blockregion); } } which I changed to this... var regionid = this .get_region_id(blockregionlist.item(0)); if (regionid === 'post') { // pre block is missing, instert it before post blockregion.setAttrs({id : 'region-pre'}); blockregionlist.item(0).insert(blockregion, 'before'); blockregionlist.unshift(blockregion); } else if (regionid === 'pre') { // post block is missing, instert it after pre blockregion.setAttrs({id : 'region-post'}); blockregionlist.item(0).insert(blockregion, 'after'); blockregionlist.push(blockregion); } } And everything seems to work OK until you dock the blocks. It is here that the layout starts to misbehave, which was what was happening before. Almost there, but not quite.
          Hide
          Mary Evans added a comment -

          Attaching Aardvark Post-IT theme zip file.

          Show
          Mary Evans added a comment - Attaching Aardvark Post-IT theme zip file.
          Hide
          Paul Nicholls added a comment -

          Mary, did you check this on current master? As part of the changes I mentioned earlier, I made those checks much tighter - the "before" code that you posted seems to be the old code (which is still current on 2.4, as MDL-32652 isn't being backported).

          Show
          Paul Nicholls added a comment - Mary, did you check this on current master? As part of the changes I mentioned earlier, I made those checks much tighter - the "before" code that you posted seems to be the old code (which is still current on 2.4, as MDL-32652 isn't being backported).
          Hide
          Mary Evans added a comment - - edited

          I tested Robert's code in M24 not Master, as it did not apply to master for the reasons you just mentioned.

          I've been testing DnD with custom block regions in Master and the blocks work OK, it's just the layout caused by blocks/dock.js. All the css body classes are screwed up.

          I added a new block region into region-main by making that a block region. That worked perfectly when I dragged a block from the side to the center. But when I docked all blocks other than the one in the center, the layout was wrong. It was as though it was displaying a three column page, with two empty side columns and a narrow center column.

          Show
          Mary Evans added a comment - - edited I tested Robert's code in M24 not Master, as it did not apply to master for the reasons you just mentioned. I've been testing DnD with custom block regions in Master and the blocks work OK, it's just the layout caused by blocks/dock.js. All the css body classes are screwed up. I added a new block region into region-main by making that a block region. That worked perfectly when I dragged a block from the side to the center. But when I docked all blocks other than the one in the center, the layout was wrong. It was as though it was displaying a three column page, with two empty side columns and a narrow center column.
          Hide
          Paul Nicholls added a comment -

          Mary, there's definitely still work that needs to be done to make it work 100% properly - but I'd suggest working on master rather than 2.4, both because there are some changes on master that aren't being backported and because the necessary changes are more likely to be integrated into master than a stable branch.

          Show
          Paul Nicholls added a comment - Mary, there's definitely still work that needs to be done to make it work 100% properly - but I'd suggest working on master rather than 2.4, both because there are some changes on master that aren't being backported and because the necessary changes are more likely to be integrated into master than a stable branch.
          Hide
          Mary Evans added a comment - - edited

          I am working on Master, I was only stating that the problem exists in 2.3 & 2.4 as well as master. I appreciate only master will get fix, even if it's only in 2.6 as I can't see it getting fixed before 2.5 is rolled out.

          What I have found is that if I add blocks to moodle/my the dock problem doesn't exist.
          This because the central region is defined in CORE and not as a custom block region.

          Show
          Mary Evans added a comment - - edited I am working on Master, I was only stating that the problem exists in 2.3 & 2.4 as well as master. I appreciate only master will get fix, even if it's only in 2.6 as I can't see it getting fixed before 2.5 is rolled out. What I have found is that if I add blocks to moodle/my the dock problem doesn't exist. This because the central region is defined in CORE and not as a custom block region.
          Hide
          Richard Oelmann added a comment - - edited

          Is there any news on this issue being fixed? as far as I can see the latest situation remains that we can get drag and drop working for blocks in custom regions, but only if dock is disabled - is that correct?
          As Miriam says in one of the posts in the forum https://moodle.org/mod/forum/discuss.php?d=211633#p922336, I don't think that is a viable trade off for me and should not be necessary.
          This was a regression in 2.3 and I am hoping it will be fixed for 2.5 (Please!) and as much as I am enjoying following the bootstrap theme discussion I would see this as far more crucial to get sorted properly!
          Richard

          Show
          Richard Oelmann added a comment - - edited Is there any news on this issue being fixed? as far as I can see the latest situation remains that we can get drag and drop working for blocks in custom regions, but only if dock is disabled - is that correct? As Miriam says in one of the posts in the forum https://moodle.org/mod/forum/discuss.php?d=211633#p922336 , I don't think that is a viable trade off for me and should not be necessary. This was a regression in 2.3 and I am hoping it will be fixed for 2.5 (Please!) and as much as I am enjoying following the bootstrap theme discussion I would see this as far more crucial to get sorted properly! Richard
          Hide
          Mary Evans added a comment -

          In a nutshell...NO!

          Show
          Mary Evans added a comment - In a nutshell...NO!
          Hide
          Richard Oelmann added a comment -

          That's what I thought Mary

          Show
          Richard Oelmann added a comment - That's what I thought Mary
          Hide
          Richard Oelmann added a comment -

          Please can I ask why this issue, identified as a Blocker back in 2.3, is still open and not resolved? Has it been downgraded - is it not believed to be a blocker - it obviously is not being treated as such, despite Martin's comment that he believed it had been fixed for 2.4.
          It is not fixed, it remains identified as a blocker, yet development continues in other directions apparently sidestepping this issue.

          Show
          Richard Oelmann added a comment - Please can I ask why this issue, identified as a Blocker back in 2.3, is still open and not resolved? Has it been downgraded - is it not believed to be a blocker - it obviously is not being treated as such, despite Martin's comment that he believed it had been fixed for 2.4. It is not fixed, it remains identified as a blocker, yet development continues in other directions apparently sidestepping this issue.

            People

            • Votes:
              25 Vote for this issue
              Watchers:
              31 Start watching this issue

              Dates

              • Created:
                Updated: