Moodle
  1. Moodle
  2. MDL-33143

Unable to insert images into Creole wiki when already previewing

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.3
    • Fix Version/s: 2.3
    • Component/s: Wiki (2.x)
    • Labels:

      Description

      Discovered during MDLQA-2017. When doing step 6, if you try to add a different image when already previewing nothing happens and you get the following Javascript error:

      "Error: document.forms.mform1 is undefined
      Source File: ../lib/javascript.php/331/mod/wiki/editors/wiki/buttons.js
      Line: 2"

        Gliffy Diagrams

          Issue Links

            Activity

            Hide
            Ankit Agarwal added a comment -

            It is because the form's id in QA page is mform2 instead of mform1 as expected by the JS

            Show
            Ankit Agarwal added a comment - It is because the form's id in QA page is mform2 instead of mform1 as expected by the JS
            Hide
            Rajesh Taneja added a comment -

            Thanks for discovering this Charles.

            It seems this bug has been there for quite sometime. As stated in MDL-31584, we use wikieditor form element on editing page. While constructing MoodleQuickForm, we use static counter ($formcounter), which gets incremented and suffixed to id.

            As in this case we have one JS, serving two forms (wikieditor mform element and wiki_editor.php). So, I am updating mform id with known "mform1" value, to make it compatible. Probably when we plan to fix MDL-31584 and use YUI, we can remove this hard-coding.

            I will create a new issue to back-port this on 21 and 22.

            Show
            Rajesh Taneja added a comment - Thanks for discovering this Charles. It seems this bug has been there for quite sometime. As stated in MDL-31584 , we use wikieditor form element on editing page. While constructing MoodleQuickForm, we use static counter ($formcounter), which gets incremented and suffixed to id. As in this case we have one JS, serving two forms (wikieditor mform element and wiki_editor.php). So, I am updating mform id with known "mform1" value, to make it compatible. Probably when we plan to fix MDL-31584 and use YUI, we can remove this hard-coding. I will create a new issue to back-port this on 21 and 22.
            Hide
            Sam Hemelryk added a comment -

            Hi Raj,

            The change is simple enough, clean, and appears to work fine.
            It concerns me a little mform2 is being used at the ID as it is the second moodleform to be instantiated (a static id is used within MoodleQuickForm::MoodleQuickForm). Overriding the ID like this would cause breakage if that first form were to be ever actually displayed. Although with 5min of looking I couldn't pinpoint its instantiation (didn't try very hard a debug in MoodleQuickForm constructor would light the path).
            So I'm betting that this change is 95% safe and is likely to not cause problems so fine to go in.
            Perhaps however you would like to add a comment about the dodgy nature of having to force it.
            The proper solution lies in refactoring the button.js to not rely on the formid which is auto-generated an instead require some other id somewhere, form element or wrapper. But that would take more time than we have right now I think.

            Anyway gets a +1 from me.

            Cheers
            Sam

            Show
            Sam Hemelryk added a comment - Hi Raj, The change is simple enough, clean, and appears to work fine. It concerns me a little mform2 is being used at the ID as it is the second moodleform to be instantiated (a static id is used within MoodleQuickForm::MoodleQuickForm). Overriding the ID like this would cause breakage if that first form were to be ever actually displayed. Although with 5min of looking I couldn't pinpoint its instantiation (didn't try very hard a debug in MoodleQuickForm constructor would light the path). So I'm betting that this change is 95% safe and is likely to not cause problems so fine to go in. Perhaps however you would like to add a comment about the dodgy nature of having to force it. The proper solution lies in refactoring the button.js to not rely on the formid which is auto-generated an instead require some other id somewhere, form element or wrapper. But that would take more time than we have right now I think. Anyway gets a +1 from me. Cheers Sam
            Hide
            Rajesh Taneja added a comment -

            Thanks for the review Sam.

            I agree with you, the right solution lies in button.js, but it needs substantial work. Also, as we have plan to go towards OUwiki, I thought this is easy (Hack) fix.
            Well, I thought of searching mform class in JS, but IE doesn't support getElementsByClass, so didn't went that way.

            I added comment above the fix, can you please suggest what else you want me to add in it?

            Show
            Rajesh Taneja added a comment - Thanks for the review Sam. I agree with you, the right solution lies in button.js, but it needs substantial work. Also, as we have plan to go towards OUwiki, I thought this is easy (Hack) fix. Well, I thought of searching mform class in JS, but IE doesn't support getElementsByClass, so didn't went that way. I added comment above the fix, can you please suggest what else you want me to add in it?
            Hide
            Sam Hemelryk added a comment -

            Hi Raj,

            I think the message needs to more clearly identify this as code we don't want to be used elsewhere.
            Probably something along the lines of (and this is just a suggestion feel free to rewrite and be creative):

            // BEWARE HACK: In order for things to work we need to override the form id and set it to form1.
            // The first form to be instantiated never gets displayed so this should be safe.
            

            So that its clear its a hack which helps ensure people don't get any bright ideas about doing this elsewhere.

            Cheers
            Sam

            Show
            Sam Hemelryk added a comment - Hi Raj, I think the message needs to more clearly identify this as code we don't want to be used elsewhere. Probably something along the lines of (and this is just a suggestion feel free to rewrite and be creative): // BEWARE HACK: In order for things to work we need to override the form id and set it to form1. // The first form to be instantiated never gets displayed so this should be safe. So that its clear its a hack which helps ensure people don't get any bright ideas about doing this elsewhere. Cheers Sam
            Hide
            Rajesh Taneja added a comment -

            Thanks Sam,

            I have updated the branch and now pushing it for integration review.

            Show
            Rajesh Taneja added a comment - Thanks Sam, I have updated the branch and now pushing it for integration review.
            Hide
            Aparup Banerjee added a comment -

            This has been integrated now. Please re-run the test in the MDLQA.

            Show
            Aparup Banerjee added a comment - This has been integrated now. Please re-run the test in the MDLQA.
            Hide
            Charles Fulton added a comment -

            The test now passes.

            Show
            Charles Fulton added a comment - The test now passes.
            Hide
            Frédéric Massart added a comment -

            Test successful on master!

            Show
            Frédéric Massart added a comment - Test successful on master!
            Hide
            Eloy Lafuente (stronk7) added a comment -

            This issue has been integrated upstream and is now available both via git and cvs (and in some hours, via mirrors and downloads).

            Thanks!

            Show
            Eloy Lafuente (stronk7) added a comment - This issue has been integrated upstream and is now available both via git and cvs (and in some hours, via mirrors and downloads). Thanks!

              People

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

                Dates

                • Created:
                  Updated:
                  Resolved: