Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-42756

Action menu not operable in docked blocks.

    Details

      Description

      While testing MDL-42739 I discovered the action menu is not operation within a docked block.

      To replicate:

      1. Log in as admin
      2. Turn on editing
      3. Dock a block
      4. Refresh the current page.
      5. Toggle the display of the docked.
      6. Notice that the block actions have not initialised.

      I've tested in standard and can confirm the issue there, we should also of course test bootstrapbase based themes.

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              Hide
              samhemelryk Sam Hemelryk added a comment -

              Added Andrew - he may have some ideas.

              Show
              samhemelryk Sam Hemelryk added a comment - Added Andrew - he may have some ideas.
              Hide
              dobedobedoh Andrew Nicols added a comment -

              Yup - missing #dockeditempanel .commands img.actionmenu

              We should probably actually make the existing one more generic now that the caret has landed too

              img.actionmenu {
              width: auto;
              }

              Show
              dobedobedoh Andrew Nicols added a comment - Yup - missing #dockeditempanel .commands img.actionmenu We should probably actually make the existing one more generic now that the caret has landed too img.actionmenu { width: auto; }
              Hide
              dobedobedoh Andrew Nicols added a comment -

              Hmm, so the reason that it's not working is more complicated and I think I'm going to play slopey shoulders and pass this back to you Sam.

              The selectors are all correct, but the block is removed from the DOM before we set up the actionmenu so we don't set it up.
              When a block is subsequently displayed, we don't set it up later.

              There are a couple of potential fixes for this:

              • stop removing the blocks from the DOM when we dock them, and instead move them to the correct location and hide them in the UI until they're made visible somehow;
              • bubble a global event which can be picked up by various things; or
              • both of the above.

              I favour both but with a priority on the first option. The event bubbling is just a nicety to aid us in the future when we start to add blocks via AJAX alone.

              Show
              dobedobedoh Andrew Nicols added a comment - Hmm, so the reason that it's not working is more complicated and I think I'm going to play slopey shoulders and pass this back to you Sam. The selectors are all correct, but the block is removed from the DOM before we set up the actionmenu so we don't set it up. When a block is subsequently displayed, we don't set it up later. There are a couple of potential fixes for this: stop removing the blocks from the DOM when we dock them, and instead move them to the correct location and hide them in the UI until they're made visible somehow; bubble a global event which can be picked up by various things; or both of the above. I favour both but with a priority on the first option. The event bubbling is just a nicety to aid us in the future when we start to add blocks via AJAX alone.
              Hide
              lazydaisy Mary Evans added a comment -

              Hi Andrew, if it is any help I tend to test Base directly as it gives you a more accurate 'state of play'. The only thing in base is that you need to add the option in config.php to allow docking like so...
               

              $THEME->enable_dock = true;

              Bootstrap themes are not yet dockable, MDL-38923 got turned down as it was too late and missed the freeze, but there is something not right with the code so you might like to cast an eye over it and see if it is as bad as what Jason was implying in his summing up. As far as I could see though it works great.

              Show
              lazydaisy Mary Evans added a comment - Hi Andrew, if it is any help I tend to test Base directly as it gives you a more accurate 'state of play'. The only thing in base is that you need to add the option in config.php to allow docking like so...   $THEME->enable_dock = true; Bootstrap themes are not yet dockable, MDL-38923 got turned down as it was too late and missed the freeze, but there is something not right with the code so you might like to cast an eye over it and see if it is as bad as what Jason was implying in his summing up. As far as I could see though it works great.
              Hide
              dobedobedoh Andrew Nicols added a comment -

              Sam Hemelryk,

              Have you had any success with a solution to this issue? A number of users have reported it in MDL-43410.

              Cheers,

              Andrew

              Show
              dobedobedoh Andrew Nicols added a comment - Sam Hemelryk , Have you had any success with a solution to this issue? A number of users have reported it in MDL-43410 . Cheers, Andrew
              Hide
              aolley Adam Olley added a comment -

              Sam Hemelryk Just wondering if this has had any work done on it in the past year before I take a look at it.

              Show
              aolley Adam Olley added a comment - Sam Hemelryk Just wondering if this has had any work done on it in the past year before I take a look at it.
              Hide
              lazydaisy Mary Evans added a comment - - edited

              Some work was done on docking the blocks earlier this year, however I think this problem still exists.

              Show
              lazydaisy Mary Evans added a comment - - edited Some work was done on docking the blocks earlier this year, however I think this problem still exists.
              Hide
              aolley Adam Olley added a comment -

              So when the panel is shown, the content of the panel gets set to the dockitem to be shown, and when hidden, it gets removed. Simplest solution I've found so far is to just trigger the actionmenu enhancement on the panel node whenever we show it.

              Show
              aolley Adam Olley added a comment - So when the panel is shown, the content of the panel gets set to the dockitem to be shown, and when hidden, it gets removed. Simplest solution I've found so far is to just trigger the actionmenu enhancement on the panel node whenever we show it.
              Hide
              cibot CiBoT added a comment -

              +1 code verified against automated checks.

              Checked MDL-42756 using repository: git://github.com/aolley/moodle.git

              More information about this report

              Show
              cibot CiBoT added a comment - +1 code verified against automated checks. Checked MDL-42756 using repository: git://github.com/aolley/moodle.git MOODLE_27_STABLE (branch: MDL-42756_27 | CI Job ) master (branch: MDL-42756 | CI Job ) More information about this report
              Hide
              aolley Adam Olley added a comment -

              Currently breaks dock when editing is off... patch on the way.

              Show
              aolley Adam Olley added a comment - Currently breaks dock when editing is off... patch on the way.
              Hide
              aolley Adam Olley added a comment -

              Patch includes check to only attempt the enhancement if actionmenu is available (i.e. editing is on)

              Show
              aolley Adam Olley added a comment - Patch includes check to only attempt the enhancement if actionmenu is available (i.e. editing is on)
              Hide
              cibot CiBoT added a comment -

              +1 code verified against automated checks.

              Checked MDL-42756 using repository: git://github.com/aolley/moodle.git

              More information about this report

              Show
              cibot CiBoT added a comment - +1 code verified against automated checks. Checked MDL-42756 using repository: git://github.com/aolley/moodle.git MOODLE_27_STABLE (branch: MDL-42756_27 | CI Job ) master (branch: MDL-42756 | CI Job ) More information about this report
              Hide
              samhemelryk Sam Hemelryk added a comment - - edited

              Hi Adam,

              Nice solution!

              The only thing I noted was that it would be nice to fix the size of the icon for this at the same time.
              There is some CSS to fix the width of the icon for non-docked blocks:

              .block .header .commands img.actionmenu {
                 width:auto;
              }
              

              We just need to extend it with something like _ .block .dockeditempanel_hd .commans img.actionmenu_

              Feel free to move this forward to integration when you like.

              Cheers
              Sam

              Show
              samhemelryk Sam Hemelryk added a comment - - edited Hi Adam, Nice solution! The only thing I noted was that it would be nice to fix the size of the icon for this at the same time. There is some CSS to fix the width of the icon for non-docked blocks: .block .header .commands img.actionmenu { width:auto; } We just need to extend it with something like _ .block .dockeditempanel_hd .commans img.actionmenu_ Feel free to move this forward to integration when you like. Cheers Sam
              Hide
              aolley Adam Olley added a comment - - edited

              Updated with css change for bootstrapbase.

              Also added a 28 stable branch since 2.8 is out now.

              Sam Hemelryk Unfortunately I don't have whatever permissions are needed to skip past the peer review step and to submit for integration review.

              Show
              aolley Adam Olley added a comment - - edited Updated with css change for bootstrapbase. Also added a 28 stable branch since 2.8 is out now. Sam Hemelryk Unfortunately I don't have whatever permissions are needed to skip past the peer review step and to submit for integration review.
              Hide
              cibot CiBoT added a comment -

              Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!

              Show
              cibot CiBoT added a comment - Moving this issue to current integration cycle, will be reviewed soon. Thanks for the hard work!
              Hide
              lazydaisy Mary Evans added a comment -

              Just a note to say there is no such CSS selector class with the name "hidepanemicon" which is just above the style in your commit for this issue. I noticed it while fixing MDL-48202 and have open MDL-48246 to fix it.

              Show
              lazydaisy Mary Evans added a comment - Just a note to say there is no such CSS selector class with the name "hidepanemicon" which is just above the style in your commit for this issue. I noticed it while fixing MDL-48202 and have open MDL-48246 to fix it.
              Hide
              poltawski Dan Poltawski added a comment -

              Thanks Adam, integrated to master, 28 and 27

              Show
              poltawski Dan Poltawski added a comment - Thanks Adam, integrated to master, 28 and 27
              Hide
              rajeshtaneja Rajesh Taneja added a comment -

              Thanks for fixing this Adam,

              Works as expected, Passing...

              Show
              rajeshtaneja Rajesh Taneja added a comment - Thanks for fixing this Adam, Works as expected, Passing...
              Hide
              poltawski Dan Poltawski added a comment -

              Thanks for your contributions! This change is now available from the main moodle.git repository and will shortly be available on download.moodle.org.

              Optimism is an occupational hazard of programming; feedback is the treatment.
              – Kent Beck

              Show
              poltawski Dan Poltawski added a comment - Thanks for your contributions! This change is now available from the main moodle.git repository and will shortly be available on download.moodle.org. Optimism is an occupational hazard of programming; feedback is the treatment. – Kent Beck

                People

                • Votes:
                  4 Vote for this issue
                  Watchers:
                  9 Start watching this issue

                  Dates

                  • Created:
                    Updated:
                    Resolved:
                    Fix Release Date:
                    12/Jan/15