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

Filepicker icon/details/list buttons not working

    Details

    • Testing Instructions:
      Hide
      1. Create 1 course (topics format, all other settings default)
      2. Add a couple of images to the course (via drag and drop, of course!)
      3. Click on the news forum
      4. Click on 'Add a new topic'
      5. Click the image icon in the HTML editor + 'Find or upload an image...'
      6. Click on 'Server files' (but reproducible with all other repositories)
      7. Click on the 'Icons / Details / Tree' buttons
        • Ensure the view changes as each button is selected, and that the relevant button is displayed as 'selected'

      As noted by Mariana below, this should be tested before the patch is applied to ensure that your current system configuration was exhibiting the problem.

      Show
      Create 1 course (topics format, all other settings default) Add a couple of images to the course (via drag and drop, of course!) Click on the news forum Click on 'Add a new topic' Click the image icon in the HTML editor + 'Find or upload an image...' Click on 'Server files' (but reproducible with all other repositories) Click on the 'Icons / Details / Tree' buttons Ensure the view changes as each button is selected, and that the relevant button is displayed as 'selected' As noted by Mariana below, this should be tested before the patch is applied to ensure that your current system configuration was exhibiting the problem.
    • Affected Branches:
      MOODLE_24_STABLE, MOODLE_25_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull Master Branch:
      MDL-37831_filepicker_buttons

      Description

      In Moodle 2.3, I can select the 'server files' repository type, then click on the 'icon / details / list' buttons on the top-right and the view changes.

      With Moodle 2.4 / master branch these buttons are not doing anything.

      I've traced through the code and the following is called by the render function (in filepicker.js):
      this.fpnode.all('.fp-vb-icons,.fp-vb-tree,.fp-vb-details').
      on('click', this.viewbar_clicked, this);
      But the function viewbar_clicked() is never reached when I click on the buttons (according to the breakpoint I added in Chrome) - the default link action ('#') instead happens and the page scrolls up to the top.

      I've tried this with Chrome (Debian), Chrome (Windows 7), Iceweasel (Debian) and with a clean install of Moodle 2.5dev.

      To see if it helped, I tried adding the "this.fpnode.all ... 'click' ..." code (above) to the end of the viewbar_set_enabled function, but that didn't improve things.

        Gliffy Diagrams

          Activity

          Hide
          marina Marina Glancy added a comment -

          I have noticed it too but for me they sometimes work and sometimes don't and I am not sure in which cases exactly. This must be related to some changes done in 2.4. Fred, does it trigger anything for you?

          Show
          marina Marina Glancy added a comment - I have noticed it too but for me they sometimes work and sometimes don't and I am not sure in which cases exactly. This must be related to some changes done in 2.4. Fred, does it trigger anything for you?
          Hide
          fred Frédéric Massart added a comment -

          Hum, that does seem to work on my instances. I have changed a few things in the area so I might be responsible for a regression but I can't think of any issue related yet. Could you try to send through the list of files in the current folder when you're changing the view? I wonder if a funky file name could be the cause of this.

          Show
          fred Frédéric Massart added a comment - Hum, that does seem to work on my instances. I have changed a few things in the area so I might be responsible for a regression but I can't think of any issue related yet. Could you try to send through the list of files in the current folder when you're changing the view? I wonder if a funky file name could be the cause of this.
          Hide
          marina Marina Glancy added a comment -

          I don't know how to reproduce. When it happens again I'll make sure to write here

          Show
          marina Marina Glancy added a comment - I don't know how to reproduce. When it happens again I'll make sure to write here
          Hide
          davosmith Davo Smith added a comment -

          These were the steps I took to reproduce:

          1. Clone a fresh copy of the Moodle master branch
          2. Install - my system is Debian, PHP 5.4.4-11, Apache 2.2.22, MySQL 14.14
          3. Using Chrome 24.0.1312.56 or Iceweasel 10.0.12
          4. Logged in as admin, create 1 course (topics format, all other settings default)
          5. Add a couple of images to the course (via drag and drop, of course )
          6. Click on the news forum
          7. Click on 'Add a new topic'
          8. Click the image icon + 'Find or upload an image...'
          9. Click on 'Server files' (but reproducable with all other repositories)
          10. Click on the 'Icons / Details / Tree' buttons => nothing happens
          Show
          davosmith Davo Smith added a comment - These were the steps I took to reproduce: Clone a fresh copy of the Moodle master branch Install - my system is Debian, PHP 5.4.4-11, Apache 2.2.22, MySQL 14.14 Using Chrome 24.0.1312.56 or Iceweasel 10.0.12 Logged in as admin, create 1 course (topics format, all other settings default) Add a couple of images to the course (via drag and drop, of course ) Click on the news forum Click on 'Add a new topic' Click the image icon + 'Find or upload an image...' Click on 'Server files' (but reproducable with all other repositories) Click on the 'Icons / Details / Tree' buttons => nothing happens
          Hide
          davosmith Davo Smith added a comment -

          This certainly seems to be a YUI issue.

          After tracing through the YUI code, I eventually reached the 'add' function on line 188 of yuilib/build/simpleyui/simpleyui.js

          This expected an element to add an event to.
          This received an array of elements.
          As the array had neither an 'addEventListener' nor an 'attachEvent' function, the event was never attached to the elements.

          Going back to filepicker.js and changing (line 1323):

          // assign callbacks for view mode switch buttons
          this.fpnode.all('.fp-vb-icons,.fp-vb-tree,.fp-vb-details').
          on('click', this.viewbar_clicked, this);

          to:

          // assign callbacks for view mode switch buttons
          this.fpnode.one('.fp-vb-icons').on('click', this.viewbar_clicked, this);
          this.fpnode.one('.fp-vb-tree').on('click', this.viewbar_clicked, this);
          this.fpnode.one('.fp-vb-details').on('click', this.viewbar_clicked, this);

          Fixed the problem for me.

          Show
          davosmith Davo Smith added a comment - This certainly seems to be a YUI issue. After tracing through the YUI code, I eventually reached the 'add' function on line 188 of yuilib/build/simpleyui/simpleyui.js This expected an element to add an event to. This received an array of elements. As the array had neither an 'addEventListener' nor an 'attachEvent' function, the event was never attached to the elements. Going back to filepicker.js and changing (line 1323): // assign callbacks for view mode switch buttons this.fpnode.all('.fp-vb-icons,.fp-vb-tree,.fp-vb-details'). on('click', this.viewbar_clicked, this); to: // assign callbacks for view mode switch buttons this.fpnode.one('.fp-vb-icons').on('click', this.viewbar_clicked, this); this.fpnode.one('.fp-vb-tree').on('click', this.viewbar_clicked, this); this.fpnode.one('.fp-vb-details').on('click', this.viewbar_clicked, this); Fixed the problem for me.
          Hide
          marina Marina Glancy added a comment -

          that sounds really odd! Can we make sure during testing that the same scenario before patch has error and after patch does not

          Show
          marina Marina Glancy added a comment - that sounds really odd! Can we make sure during testing that the same scenario before patch has error and after patch does not
          Hide
          davosmith Davo Smith added a comment -

          Skipping 'peer review' phase as change is very small and has already been discussed - please let me know if I shouldn't do this in the future.

          Show
          davosmith Davo Smith added a comment - Skipping 'peer review' phase as change is very small and has already been discussed - please let me know if I shouldn't do this in the future.
          Hide
          stronk7 Eloy Lafuente (stronk7) added a comment -

          Davo, does need to land to 24_STABLE too? It's not 100% clear for me. By cherrypick?

          Show
          stronk7 Eloy Lafuente (stronk7) added a comment - Davo, does need to land to 24_STABLE too? It's not 100% clear for me. By cherrypick?
          Hide
          damyon Damyon Wiese added a comment -

          Pushed this to 2.4 and master (cherry-pick and quick test). Incremented the version number for the JS changes.

          Thanks Davo.

          Show
          damyon Damyon Wiese added a comment - Pushed this to 2.4 and master (cherry-pick and quick test). Incremented the version number for the JS changes. Thanks Davo.
          Hide
          markn Mark Nelson added a comment -

          This was working for me in Stable and Integration. I was not able to replicate the issue prior to this commit.

          Show
          markn Mark Nelson added a comment - This was working for me in Stable and Integration. I was not able to replicate the issue prior to this commit.
          Hide
          markn Mark Nelson added a comment -

          Ah nvm, I was able to replicate the issue in stable. I was navigating to the file picker through another means. This does work. Passing.

          Show
          markn Mark Nelson added a comment - Ah nvm, I was able to replicate the issue in stable. I was navigating to the file picker through another means. This does work. Passing.
          Hide
          damyon Damyon Wiese added a comment -

          Congratulations this fix has been added to Moodle!

          You may want to dedicate this issue to someone special on this Valentines day.

          Thanks!

          Show
          damyon Damyon Wiese added a comment - Congratulations this fix has been added to Moodle! You may want to dedicate this issue to someone special on this Valentines day. Thanks!

            People

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

              Dates

              • Created:
                Updated:
                Resolved:
                Fix Release Date:
                11/Mar/13