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
    • Rank:
      47575

      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.

        Activity

        Hide
        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 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
        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
        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 Glancy added a comment -

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

        Show
        Marina Glancy added a comment - I don't know how to reproduce. When it happens again I'll make sure to write here
        Hide
        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
        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
        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
        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 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 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
        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
        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
        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
        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 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 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
        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
        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
        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
        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 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 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: