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

chooserdialogue shouldn't access form elements by id

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 2.3
    • Fix Version/s: 2.4
    • Component/s: JavaScript
    • Labels:
    • Testing Instructions:
      Hide

      It's advisable to have your JS console open for these tests

      • Open a course page
      • Turn editing on
      • Open the Activity Chooser
      • Hit the Cancel button
        • Confirm that the submit button is disabled
        • Confirm that the chooser closes correctly without error
      • Re-open the chooser
        • Confirm that the submit button is disabled
      • Select a module
        • Confirm that the submit button is now enabled
      • Click the submit button
        • Confirm that the correct modedit page opens
      Show
      It's advisable to have your JS console open for these tests Open a course page Turn editing on Open the Activity Chooser Hit the Cancel button Confirm that the submit button is disabled Confirm that the chooser closes correctly without error Re-open the chooser Confirm that the submit button is disabled Select a module Confirm that the submit button is now enabled Click the submit button Confirm that the correct modedit page opens
    • Affected Branches:
      MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-34352-master-2

      Description

      It's possible (and likely) that there will be more than one chooserdialogue available at a time. At present, we're using IDs to access certain fields:

      • jumplink
      • submitbutton
      • cancelbutton

      But it's a breach of the XHTML spec to use the same ID multiple times.

      We need to use class selectors rather than IDs

        Gliffy Diagrams

          Attachments

            Issue Links

              Activity

              Hide
              fred Frédéric Massart added a comment -

              Code looks good Andrew. Pushing for integration!

              Show
              fred Frédéric Massart added a comment - Code looks good Andrew. Pushing for integration!
              Hide
              samhemelryk Sam Hemelryk added a comment - - edited

              Umm sorry, just one thing to check up / clarify.
              It looks like ./course/yui/modchooser/modchooser.js is still referencing #jump.

              Show
              samhemelryk Sam Hemelryk added a comment - - edited Umm sorry, just one thing to check up / clarify. It looks like ./course/yui/modchooser/modchooser.js is still referencing #jump.
              Hide
              poltawski Dan Poltawski added a comment -

              The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

              TIA and ciao

              Show
              poltawski Dan Poltawski added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
              Hide
              samhemelryk Sam Hemelryk added a comment -

              Sorry, sending this back Andrew it looks to me like that modchooser.js needs to be updated as well.
              Unfortunately I am neither 100% sure or confident enough to make the changes during integration and I think its best you have a look at it yourself.
              Feel free to correct it and push it back up for integration, or correct me

              Cheers
              Sam

              Show
              samhemelryk Sam Hemelryk added a comment - Sorry, sending this back Andrew it looks to me like that modchooser.js needs to be updated as well. Unfortunately I am neither 100% sure or confident enough to make the changes during integration and I think its best you have a look at it yourself. Feel free to correct it and push it back up for integration, or correct me Cheers Sam
              Hide
              cibot CiBoT added a comment -

              Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

              Show
              cibot CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
              Hide
              dobedobedoh Andrew Nicols added a comment -

              Apologies Sam,

              When I wrote this I was also working on another related commit.
              The assigning of the jumplink in modchooser is actually superflous as it's set in chooserdialogue::display_chooser() so I've removed it - I'm going to be doing additional work on this in another issue to improve things further.

              Show
              dobedobedoh Andrew Nicols added a comment - Apologies Sam, When I wrote this I was also working on another related commit. The assigning of the jumplink in modchooser is actually superflous as it's set in chooserdialogue::display_chooser() so I've removed it - I'm going to be doing additional work on this in another issue to improve things further.
              Hide
              samhemelryk Sam Hemelryk added a comment -

              Cool thanks for clearing that up Andrew, looking at this again now

              Show
              samhemelryk Sam Hemelryk added a comment - Cool thanks for clearing that up Andrew, looking at this again now
              Hide
              samhemelryk Sam Hemelryk added a comment -

              Thanks Andrew, things look spot on. This has been integrated now.

              Show
              samhemelryk Sam Hemelryk added a comment - Thanks Andrew, things look spot on. This has been integrated now.
              Hide
              abgreeve Adrian Greeve added a comment -

              I confirmed that the submit button was disabled when it should be and enabled when an activity or resourse is selected.
              I'm always re-directed to the correct modedit page as well.
              No error were discovered during the testing process.
              Test passed

              Show
              abgreeve Adrian Greeve added a comment - I confirmed that the submit button was disabled when it should be and enabled when an activity or resourse is selected. I'm always re-directed to the correct modedit page as well. No error were discovered during the testing process. Test passed
              Hide
              nebgor Aparup Banerjee added a comment -

              yay, it works!

              This issue has been put through rigorous processes and finally swam upstream along with some 65 others this week.

              Thank you all for taking the time to get us here.

              cheers!

              Show
              nebgor Aparup Banerjee added a comment - yay, it works! This issue has been put through rigorous processes and finally swam upstream along with some 65 others this week. Thank you all for taking the time to get us here. cheers!

                People

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

                  Dates

                  • Created:
                    Updated:
                    Resolved:
                    Fix Release Date:
                    3/Dec/12