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

      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"

        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: