Details

    • Type: Sub-task Sub-task
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.5
    • Fix Version/s: 2.5
    • Component/s: Themes
    • Labels:
    • Rank:
      48994

      Description

      Base theme supports drag and drop on blocks.

      Bootstrap theme does not, and it really must.

        Issue Links

          Activity

          Hide
          Bas Brands added a comment -

          This is possibly related to the use of HTML5 tags <aside> for block areas instead of using <div>

          Show
          Bas Brands added a comment - This is possibly related to the use of HTML5 tags <aside> for block areas instead of using <div>
          Hide
          Martin Dougiamas added a comment -

          Apparently this is probably related to MDL-32608

          Show
          Martin Dougiamas added a comment - Apparently this is probably related to MDL-32608
          Hide
          Andrew Nicols added a comment -

          I think that this is a manifestation of MDL-32608.

          Ruslan, is there a workaround for the Bootstrap theme do you think?

          Show
          Andrew Nicols added a comment - I think that this is a manifestation of MDL-32608 . Ruslan, is there a workaround for the Bootstrap theme do you think?
          Hide
          David Scotson added a comment -

          Just to confirm that it's the use of "aside" tag instead of "div" that broke this, because that's what the javascript in lib/yui/blocks/blocks.js is looking for.

          But there's some complex interaction with other issues here. I'd been having issues with drag'n'drop on mobile devices (though there was some debate in the bug about whether I was imagining it or not, see MDL-38371 and if you give it a try yourself please report back on what you experience) so I'd actually been looking for ways to disable the drag'n'drop javascript and make the non-javascript controls come back, as I though they were a) more suitable and b) less broken on touch devices. The only one I succeeded on bringing back was the block movements, which was an accidental success which occurred as a result of this bug. So "fixing" this by making the javascript not relay on div might actually be a step back for use on touch devices, which might be an initial focus of some users of this theme. So it's tricky.

          Show
          David Scotson added a comment - Just to confirm that it's the use of "aside" tag instead of "div" that broke this, because that's what the javascript in lib/yui/blocks/blocks.js is looking for. But there's some complex interaction with other issues here. I'd been having issues with drag'n'drop on mobile devices (though there was some debate in the bug about whether I was imagining it or not, see MDL-38371 and if you give it a try yourself please report back on what you experience) so I'd actually been looking for ways to disable the drag'n'drop javascript and make the non-javascript controls come back, as I though they were a) more suitable and b) less broken on touch devices. The only one I succeeded on bringing back was the block movements, which was an accidental success which occurred as a result of this bug. So "fixing" this by making the javascript not relay on div might actually be a step back for use on touch devices, which might be an initial focus of some users of this theme. So it's tricky.
          Hide
          Mary Evans added a comment -

          There is no reason why you can't use the the following code inside the aside container...

          
          <aside class="span3">
              <div id="region-pre" class="block-region">
              <div class="region-content">
              <?php
          if (!right_to_left()) {
              echo $OUTPUT->blocks_for_region('side-pre');
          } else if ($hassidepost) {
              echo $OUTPUT->blocks_for_region('side-post');
          } ?>
              </div>
              </div>
          </aside>
          
          
          
          Show
          Mary Evans added a comment - There is no reason why you can't use the the following code inside the aside container... <aside class= "span3" > <div id= "region-pre" class= "block-region" > <div class= "region-content" > <?php if (!right_to_left()) { echo $OUTPUT->blocks_for_region('side-pre'); } else if ($hassidepost) { echo $OUTPUT->blocks_for_region('side-post'); } ?> </div> </div> </aside>
          Hide
          Stuart Lamour added a comment - - edited

          blocks.js
          line 178
          regionname = this.get_region_id(drop.ancestor('div.'+CSS.BLOCKREGION));

          just take out the 'div.' and it works - except, as david states on touch screens....

          should this issue be in javascript rather than themes?

          Show
          Stuart Lamour added a comment - - edited blocks.js line 178 regionname = this.get_region_id(drop.ancestor('div.'+CSS.BLOCKREGION)); just take out the 'div.' and it works - except, as david states on touch screens.... should this issue be in javascript rather than themes?
          Hide
          Damyon Wiese added a comment -

          Hi Mary,

          This change works for the bootstrap theme - but also needs to be implemented for the simple theme (which is the one which will be selectable). We really need to keep these two in sync now. I'll add a commit for this now to save a cycle.

          Re: fixing the JS selector - that sounds like a good improvement, but does not seem like a blocker for this issue thanks to Marys workaround.

          Integrated to master (with support for simple theme).

          Thanks, Damyon

          Show
          Damyon Wiese added a comment - Hi Mary, This change works for the bootstrap theme - but also needs to be implemented for the simple theme (which is the one which will be selectable). We really need to keep these two in sync now. I'll add a commit for this now to save a cycle. Re: fixing the JS selector - that sounds like a good improvement, but does not seem like a blocker for this issue thanks to Marys workaround. Integrated to master (with support for simple theme). Thanks, Damyon
          Hide
          Damyon Wiese added a comment -

          Re: extra commit for simple theme - sorry I didn't see MDL-39250 in the queue - I've closed that one as a duplicate now because I fixed it here.

          Show
          Damyon Wiese added a comment - Re: extra commit for simple theme - sorry I didn't see MDL-39250 in the queue - I've closed that one as a duplicate now because I fixed it here.
          Hide
          Andrew Davis added a comment -

          Tested successfully using Simple. Bootstrap is no longer selectable in the theme selector. After asking in dev chat that is apparently ok.

          Show
          Andrew Davis added a comment - Tested successfully using Simple. Bootstrap is no longer selectable in the theme selector. After asking in dev chat that is apparently ok.
          Hide
          Mary Evans added a comment -

          @Damyon: You need to go to Specsavers then!

             _,aad888P""""""""""Y888888888888888888888888888888P"""""""""""Y888baa,_
            aP'                     `""Ybaa,         ,aadP""'                     `Ya
           dP                             `"b,     ,d"'                             Yb
           8l                               8l_____8l                                8l
          [8l                               8l"""""8l                                8l]
           8l                              d8       8b                               8l
           8l                             dP/       \Yb                              8l
           8l                           ,dP/         \Yb,                            8l
           8l                        ,adP'/           \`Yba,                         8l
           Yb                     ,adP'                   `Yba,                     dP
            Yb                ,aadP'                         `Ybaa,                dP
             `Yb          ,aadP'                                 `Ybaa,          dP'
               `Ybaaaaad8P"'                                         `"Y8baaaaadP'
          
          Show
          Mary Evans added a comment - @Damyon: You need to go to Specsavers then! _,aad888P """"""""""Y888888888888888888888888888888P" """"""""""Y888baa,_ aP' ` ""Ybaa, ,aadP" "' `Ya dP ` "b, ,d" ' Yb 8l 8l_____8l 8l [8l 8l"""""8l 8l] 8l d8 8b 8l 8l dP/ \Yb 8l 8l ,dP/ \Yb, 8l 8l ,adP'/ \`Yba, 8l Yb ,adP' `Yba, dP Yb ,aadP' `Ybaa, dP `Yb ,aadP' `Ybaa, dP' `Ybaaaaad8P "' `" Y8baaaaadP'
          Hide
          Stuart Lamour added a comment - - edited

          as developers i've always thought we should be optimising, not increasing code.

          surely instead of adding more dom elements to every page, removing one bug in the javascript is a huge win?

          i'm sure i read that moodle guide for integration says code has to be better, not worse.

          @mary ?

          your code is a workaround, not a fix.

          the fix is to fix the js so it works and no theme has to do this workaround.

          Show
          Stuart Lamour added a comment - - edited as developers i've always thought we should be optimising, not increasing code. surely instead of adding more dom elements to every page, removing one bug in the javascript is a huge win? i'm sure i read that moodle guide for integration says code has to be better, not worse. @mary ? your code is a workaround, not a fix. the fix is to fix the js so it works and no theme has to do this workaround.
          Hide
          Stuart Lamour added a comment -

          grep and replaced 'div.'+CSS.BLOCKREGION with CSS.BLOCKREGION - should do the trick?

          Show
          Stuart Lamour added a comment - grep and replaced 'div.'+CSS.BLOCKREGION with CSS.BLOCKREGION - should do the trick?
          Hide
          Damyon Wiese added a comment -

          Nice art Mary!

          Stuart - I think the div/aside issue will be dealt with in MDL-32608 - but that is a riskier change and we are in code freeze now so I felt it was better to integrate this solution. From reading MDL-32608 - there may be more changes required than just changing that selector (I think the js dynamically generates those block containers in some cases).

          Show
          Damyon Wiese added a comment - Nice art Mary! Stuart - I think the div/aside issue will be dealt with in MDL-32608 - but that is a riskier change and we are in code freeze now so I felt it was better to integrate this solution. From reading MDL-32608 - there may be more changes required than just changing that selector (I think the js dynamically generates those block containers in some cases).
          Hide
          Mary Evans added a comment -

          That may be so Stuart, but as I see it we are regressing with some of the Bootstrap changes, besides Rome was not built in a day, so please bear with us until other things are fixed.

          Thanks.

          Show
          Mary Evans added a comment - That may be so Stuart, but as I see it we are regressing with some of the Bootstrap changes, besides Rome was not built in a day, so please bear with us until other things are fixed. Thanks.
          Hide
          Bas Brands added a comment -

          @stuart,

          I have tried the fix for JavaScript but it did not seem to work. Did your fix work for Moodle 2.5?

          Show
          Bas Brands added a comment - @stuart, I have tried the fix for JavaScript but it did not seem to work. Did your fix work for Moodle 2.5?
          Hide
          Stuart Lamour added a comment - - edited

          the issue i'm seeing, even with mary's https://github.com/lazydaisy/moodle/compare/master...MDL-38898_master is that the js adds a dom element when there is no post-region.

          where it appends this in the dom breaks the bootstrap layout.

          to reproduce :
          turn editing on in a site with only pre-region
          try and drag a block into the js appended post-region

          results :
          in the dom the js appends the post-region after the pre-region which breaks the layout.

          this would be the case for this js with any theme/layout with multiple regions that didn't use something like the http://alistapart.com/article/holygrail layout.

          Show
          Stuart Lamour added a comment - - edited the issue i'm seeing, even with mary's https://github.com/lazydaisy/moodle/compare/master...MDL-38898_master is that the js adds a dom element when there is no post-region. where it appends this in the dom breaks the bootstrap layout. to reproduce : turn editing on in a site with only pre-region try and drag a block into the js appended post-region results : in the dom the js appends the post-region after the pre-region which breaks the layout. this would be the case for this js with any theme/layout with multiple regions that didn't use something like the http://alistapart.com/article/holygrail layout.
          Hide
          Mary Evans added a comment -

          Can you create a new bug issue for this. As I think it is the same as one that is jinxed the Holy Grail too.

          Show
          Mary Evans added a comment - Can you create a new bug issue for this. As I think it is the same as one that is jinxed the Holy Grail too.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          I feel myself really alone tonight! So was time to push your fixes upstream!

          "Lest we forget. We will remember them."

          Thanks and ciao!

          Show
          Eloy Lafuente (stronk7) added a comment - I feel myself really alone tonight! So was time to push your fixes upstream! "Lest we forget. We will remember them." Thanks and ciao!
          Hide
          Mary Evans added a comment - - edited

          @Eloy: There is an error in Clean/layout/general.php probably caused by Damyon when he added fix to Simple/layout/general.php when Integrating this issue. For some reason he missed my patch in MDL-39250 which he clearly did not check.

          Should I fix this in MDL-39250 if I change subject title to read Clean theme does not support drag-n-drop?

          I need to reopen MDL-39250 as Damyon closed it as a duplicate!

          Show
          Mary Evans added a comment - - edited @Eloy: There is an error in Clean/layout/general.php probably caused by Damyon when he added fix to Simple/layout/general.php when Integrating this issue. For some reason he missed my patch in MDL-39250 which he clearly did not check. Should I fix this in MDL-39250 if I change subject title to read Clean theme does not support drag-n-drop? I need to reopen MDL-39250 as Damyon closed it as a duplicate!
          Hide
          Mary Evans added a comment -

          I knew this would happen!

          Show
          Mary Evans added a comment - I knew this would happen!
          Hide
          Mary Evans added a comment -

          I think Stuart that the problem is in the CSS.
          The fact body classes are generated seems to have been overlooked in bootstrap as a means to identify different layouts. eg., .side-pre-only, .side-post-only, and .content-only. .blocks-moving.side-pre-only, .blocks-moving.side-post-only

          Show
          Mary Evans added a comment - I think Stuart that the problem is in the CSS. The fact body classes are generated seems to have been overlooked in bootstrap as a means to identify different layouts. eg., .side-pre-only, .side-post-only, and .content-only. .blocks-moving.side-pre-only, .blocks-moving.side-post-only
          Hide
          Stuart Lamour added a comment -

          i'm still confused why this issue is closed and fixed.

          mary?

          Show
          Stuart Lamour added a comment - i'm still confused why this issue is closed and fixed. mary?
          Hide
          Mary Evans added a comment -

          The fact it is closed means it has been integrated, faults and all and is beyond return.
          I fixed the problem I found in MDL-39250.

          However, you are correct with the bug you have found in that the drag'n'drop tries to find a block region is a page where none exist, because the region is not listed in the config.php, layout/frontpage being a prime example.

          This is now being tracked in MDL-39382

          Show
          Mary Evans added a comment - The fact it is closed means it has been integrated, faults and all and is beyond return. I fixed the problem I found in MDL-39250 . However, you are correct with the bug you have found in that the drag'n'drop tries to find a block region is a page where none exist, because the region is not listed in the config.php, layout/frontpage being a prime example. This is now being tracked in MDL-39382
          Hide
          Stuart Lamour added a comment -

          by adding the extra divs to the dom it breaks the ability to move blocks from one region to another when only one region has blocks in.

          this is true even when the config is set for two regions.

          its odd this didn't show up in testing -leaving this bug in seems much more serious than not having drag n drop.

          Show
          Stuart Lamour added a comment - by adding the extra divs to the dom it breaks the ability to move blocks from one region to another when only one region has blocks in. this is true even when the config is set for two regions. its odd this didn't show up in testing -leaving this bug in seems much more serious than not having drag n drop.
          Hide
          Mary Evans added a comment - - edited

          Hi Stuart,

          I noticed this odd behaviour as well, but since this issue is closed can you open another tracker so we can look at this new problem in detail, and try and find a fix for it too?

          I think, like you have said in the past, it's the way the JS is written for the holy-grail layout, and so it fails if the layout is different.

          Thanks
          Mary

          Show
          Mary Evans added a comment - - edited Hi Stuart, I noticed this odd behaviour as well, but since this issue is closed can you open another tracker so we can look at this new problem in detail, and try and find a fix for it too? I think, like you have said in the past, it's the way the JS is written for the holy-grail layout, and so it fails if the layout is different. Thanks Mary
          Hide
          Mark Aberdour added a comment -

          Hi folks, sorry am so late to this conversation. I installed the latest 2.5 this afternoon. Not being able to move a block into an empty column is a massive issue from an admin user's or course creator's perspective. It totally breaks Moodle and should surely be considered a blocker, yet this issue appears to be 'fixed' and 'closed'?! I am working with a new test site with just a couple of sample courses, and it restricts me to a 2 column layout, I cannot add a right hand column at all. Hugely restrictive to end users.

          Does it really need a new ticket opening? As I see it the ticket was opened, tested, fixed and closed however testing missed the bug, hence the ticket should just be reopened. We already have a closed duplicate at MDL-39250 and what looks like another duplicate at #MDL-39382, do we really need a 4th ticket for the same issue? I am no developer though, maybe there are subtle differences in these three tickets, but they look the same to my non-developer eye

          Anyway, if a new ticket is what it will take to fix it, I will gladly open one, if that is all that is preventing this being fixed. Please let me know if you want me to do that.

          Thanks

          Show
          Mark Aberdour added a comment - Hi folks, sorry am so late to this conversation. I installed the latest 2.5 this afternoon. Not being able to move a block into an empty column is a massive issue from an admin user's or course creator's perspective. It totally breaks Moodle and should surely be considered a blocker, yet this issue appears to be 'fixed' and 'closed'?! I am working with a new test site with just a couple of sample courses, and it restricts me to a 2 column layout, I cannot add a right hand column at all. Hugely restrictive to end users. Does it really need a new ticket opening? As I see it the ticket was opened, tested, fixed and closed however testing missed the bug, hence the ticket should just be reopened. We already have a closed duplicate at MDL-39250 and what looks like another duplicate at # MDL-39382 , do we really need a 4th ticket for the same issue? I am no developer though, maybe there are subtle differences in these three tickets, but they look the same to my non-developer eye Anyway, if a new ticket is what it will take to fix it, I will gladly open one, if that is all that is preventing this being fixed. Please let me know if you want me to do that. Thanks
          Hide
          Andrew Davis added a comment -

          As it would be best to get this resolved asap I've raised MDL-39573.

          Show
          Andrew Davis added a comment - As it would be best to get this resolved asap I've raised MDL-39573 .

            People

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

              Dates

              • Created:
                Updated:
                Resolved: