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

          Attachments

            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