Moodle
  1. Moodle
  2. MDL-26168

Cloze edit page doesn't render in IE8 for some themes

    Details

    • Type: Bug Bug
    • Status: Open
    • Priority: Minor Minor
    • Resolution: Unresolved
    • Affects Version/s: 2.0
    • Fix Version/s: STABLE backlog
    • Component/s: Questions
    • Labels:
    • Environment:
      Internet Explorer 8
    • Affected Branches:
      MOODLE_20_STABLE
    • Rank:
      16068

      Description

      Some clearfix DIVs are echoed, putting them in the wrong place which triggers quirks mode on IE8 or even breaks rendering (on some custom theme).

      These divs should be output via addElement to the form and not echoed directly.

      1. MDL_question_clearfix_move.patch
        2 kB
        Tony Levi
      2. mdl-26168-adam.patch
        2 kB
        Ashley Holman
      1. clozecorrecteddisplay.jpg
        143 kB

        Issue Links

          Activity

          Hide
          Tim Hunt added a comment -

          Patch looks good to me. I'm about to forward it for inclusion in next week's build.

          Show
          Tim Hunt added a comment - Patch looks good to me. I'm about to forward it for inclusion in next week's build.
          Hide
          Tim Hunt added a comment -

          Tony, you clearly made that patch using git. Do you have somewhere public you can push your git stuff? If so, if you can set up a branch like https://github.com/timhunt/moodle/compare/master...MDL-26168, that makes my life a little bit easier. It also means that you get recorded as the committer of the fix.

          Alternatively, you can do the commit locally, and then use git format-patch to create the patch (http://kernel.org/pub/software/scm/git/docs/git-format-patch.html)

          Anyway, thanks for your contribution.

          Show
          Tim Hunt added a comment - Tony, you clearly made that patch using git. Do you have somewhere public you can push your git stuff? If so, if you can set up a branch like https://github.com/timhunt/moodle/compare/master...MDL-26168 , that makes my life a little bit easier. It also means that you get recorded as the committer of the fix. Alternatively, you can do the commit locally, and then use git format-patch to create the patch ( http://kernel.org/pub/software/scm/git/docs/git-format-patch.html ) Anyway, thanks for your contribution.
          Hide
          Ashley Holman added a comment -

          When using 'decode and verify question text' with cloze questions, blocks don't render and form sections under the verified questions don't display correctly.

          The problem is fixed with attached patch.

          Show
          Ashley Holman added a comment - When using 'decode and verify question text' with cloze questions, blocks don't render and form sections under the verified questions don't display correctly. The problem is fixed with attached patch.
          Hide
          Tim Hunt added a comment -

          I don't like this patch. The clearfix class is designed for use on divs that contain floats, and which need a clear at the end of the div. Therefore, it makes not sense to use it on an empty div. Also, I assume the divs in question were intentionally wrapping some logical unit of the page, and we should keep that. (Well, the HTML here is not exemplary, but it is OK.)

          Surely the correct fix is just to add

          #page-question-type-multianswer div.ablock.clearfix

          { clear: both; }

          to question/type/multianswer/styles.css? (Note, I have not tested this.)

          Show
          Tim Hunt added a comment - I don't like this patch. The clearfix class is designed for use on divs that contain floats, and which need a clear at the end of the div. Therefore, it makes not sense to use it on an empty div. Also, I assume the divs in question were intentionally wrapping some logical unit of the page, and we should keep that. (Well, the HTML here is not exemplary, but it is OK.) Surely the correct fix is just to add #page-question-type-multianswer div.ablock.clearfix { clear: both; } to question/type/multianswer/styles.css? (Note, I have not tested this.)
          Hide
          Myles Carrick added a comment -

          The wrapper divs don't appear here to be doing anything useful - and are breaking the layout in IE and Chrome. The content is always in fieldsets (which themselves are .clearfix). Without those two wrapper divs altogether it's working for me in IE 7&8, FF 3.x and FF4, and Chrome and the validation passes.

          Show
          Myles Carrick added a comment - The wrapper divs don't appear here to be doing anything useful - and are breaking the layout in IE and Chrome. The content is always in fieldsets (which themselves are .clearfix). Without those two wrapper divs altogether it's working for me in IE 7&8, FF 3.x and FF4, and Chrome and the validation passes.
          Hide
          Tony Levi added a comment -

          I think the final fix for this did entirely remove them, we were trying to avoid breaking what they were really used for (which is, as far as we could tell, nothing!)

          Show
          Tony Levi added a comment - I think the final fix for this did entirely remove them, we were trying to avoid breaking what they were really used for (which is, as far as we could tell, nothing!)
          Hide
          Mary Evans added a comment -

          Tim,

          I'm not sure if this is related, but I have always thought there is a bug in Base theme which does stop the flow in a page. If you add #page

          { border: 5px solid red;}

          to core.css in base theme it adds a 10px line at the top of the page, this shows that page flow is not working. However, if you put <div class="clearfix"></div> just before the last closing div tag for page in both general.php and frontpage.php you get a 5px red border around the page. This means that your outer page is enveloping the whole of the contents. Otherwise page as a container is doing nothing.

          I think this is what's happening to the edit page with the wrappers.
          I think your idea of a clearfix div is mis-guided. I've used this type of clear fix for the last 10 years, and works every time.

          Mary

          Show
          Mary Evans added a comment - Tim, I'm not sure if this is related, but I have always thought there is a bug in Base theme which does stop the flow in a page. If you add #page { border: 5px solid red;} to core.css in base theme it adds a 10px line at the top of the page, this shows that page flow is not working. However, if you put <div class="clearfix"></div> just before the last closing div tag for page in both general.php and frontpage.php you get a 5px red border around the page. This means that your outer page is enveloping the whole of the contents. Otherwise page as a container is doing nothing. I think this is what's happening to the edit page with the wrappers. I think your idea of a clearfix div is mis-guided. I've used this type of clear fix for the last 10 years, and works every time. Mary
          Hide
          Pierre Pichet added a comment -

          Have you done your tests on editing
          a Cloze question with more than one question type and( more important)
          on a Cloze question that is used in a quiz were valid attempts have been done
          when you change the question type of one of the sub question
          so that all the features appear ?

          Show
          Pierre Pichet added a comment - Have you done your tests on editing a Cloze question with more than one question type and( more important) on a Cloze question that is used in a quiz were valid attempts have been done when you change the question type of one of the sub question so that all the features appear ?
          Hide
          Pierre Pichet added a comment - - edited

          the problem is related to using div mixed with addElement('header')
          like line 173

                   //   $mform->addElement('html', '</div>');
          
                      $this->negative_diff =$countsavedsubquestions - $countsubquestions ;
                      if ( ($this->negative_diff > 0 ) ||$this->qtype_change || ($this->used_in_quiz && $this->negative_diff != 0)){
                          $mform->addElement('header', 'additemhdr', get_string('warningquestionmodified','qtype_multianswer'));
                      }
                      if($this->negative_diff > 0) {
                          //$this->used_in_quiz
                          $mform->addElement('static', 'alert1', "<strong>".get_string('questiondeleted','qtype_multianswer')."</strong>",get_string('questionsless','qtype_multianswer',$this->negative_diff));
                      }
                      if($this->qtype_change ) {
                          $mform->addElement('static', 'alert1', "<strong>".get_string('questiontypechanged','qtype_multianswer')."</strong>",get_string('questiontypechangedcomment','qtype_multianswer'));
                      }
          

          removing these div and /div seems to solve the problem that was not located only in IE but also in Firefox.

          Sorry for these div that were put in developping the code and not removed to more standard format as I was more concern to review the code that print all the subquestions elements correctly

          This code was put in October 21, 2009 so quite old as view from 2,0

          Show
          Pierre Pichet added a comment - - edited the problem is related to using div mixed with addElement('header') like line 173 // $mform->addElement('html', '</div>'); $this->negative_diff =$countsavedsubquestions - $countsubquestions ; if ( ($this->negative_diff > 0 ) ||$this->qtype_change || ($this->used_in_quiz && $this->negative_diff != 0)){ $mform->addElement('header', 'additemhdr', get_string('warningquestionmodified','qtype_multianswer')); } if($this->negative_diff > 0) { //$this->used_in_quiz $mform->addElement('static', 'alert1', "<strong>".get_string('questiondeleted','qtype_multianswer')."</strong>",get_string('questionsless','qtype_multianswer',$this->negative_diff)); } if($this->qtype_change ) { $mform->addElement('static', 'alert1', "<strong>".get_string('questiontypechanged','qtype_multianswer')."</strong>",get_string('questiontypechangedcomment','qtype_multianswer')); } removing these div and /div seems to solve the problem that was not located only in IE but also in Firefox. Sorry for these div that were put in developping the code and not removed to more standard format as I was more concern to review the code that print all the subquestions elements correctly This code was put in October 21, 2009 so quite old as view from 2,0
          Hide
          Pierre Pichet added a comment -

          Mary,
          "I think your idea of a clearfix div is mis-guided. I've used this type of clear fix for the last 10 years, and works every time."

          Perhaps you did not notice the reediting of all theme code by Tim for 2,0

          Show
          Pierre Pichet added a comment - Mary, "I think your idea of a clearfix div is mis-guided. I've used this type of clear fix for the last 10 years, and works every time." Perhaps you did not notice the reediting of all theme code by Tim for 2,0
          Hide
          Pierre Pichet added a comment - - edited

          A view of the Cloze result once the divs have been removed.
          Same on Explorer or Firefox or Safari

          Show
          Pierre Pichet added a comment - - edited A view of the Cloze result once the divs have been removed. Same on Explorer or Firefox or Safari
          Hide
          Tim Hunt added a comment -

          So, if I am interpreting correctly, I think you are reaching some sort of consensus here. I would be really grateful if someone could make a patch for the fix that everyone agrees on, then I will review and commit it.

          Show
          Tim Hunt added a comment - So, if I am interpreting correctly, I think you are reaching some sort of consensus here. I would be really grateful if someone could make a patch for the fix that everyone agrees on, then I will review and commit it.
          Hide
          Chris Brainerd added a comment -

          I'm seeing this issue in Firefox 6.0 Mac as well as Opera 11.51 Win32, Moodle 2.0.3 with a slightly modified boxxie theme. mdl-26168-adam.patch has fixed it beautifully. Just wanted to vote for its resolution in the core.

          Show
          Chris Brainerd added a comment - I'm seeing this issue in Firefox 6.0 Mac as well as Opera 11.51 Win32, Moodle 2.0.3 with a slightly modified boxxie theme. mdl-26168-adam.patch has fixed it beautifully. Just wanted to vote for its resolution in the core.
          Hide
          Tim Hunt added a comment -

          Yes, but there is lots of discussion above that agrees that patch takes the wrong approach, and suggests a better approach.

          If someone can make a patch that follows that better approach, I will review it. If not -one else works on this patch, nothing will happen. I have other priorities.

          Show
          Tim Hunt added a comment - Yes, but there is lots of discussion above that agrees that patch takes the wrong approach, and suggests a better approach. If someone can make a patch that follows that better approach, I will review it. If not -one else works on this patch, nothing will happen. I have other priorities.
          Hide
          Pierre Pichet added a comment -

          So let's put it on the top and more news after the weekend

          Show
          Pierre Pichet added a comment - So let's put it on the top and more news after the weekend
          Hide
          Pierre Pichet added a comment - - edited

          Tim,
          The commit for MDL-26168 in 2,0 is ready.
          However I found that some langstrings need to be fixed in 2.0 and 2.1.
          i.e.
          $storemess = " STORED QTYPE ".$question_type_names[$this->savedquestiondisplay->options->questions[$sub]->qtype];

          What is the simplest way to proceed so that you have the minimum of commits to handle.
          .Modify the bug name and text so to put everything in one commit?
          .Create a substask ?
          ...

          Show
          Pierre Pichet added a comment - - edited Tim, The commit for MDL-26168 in 2,0 is ready. However I found that some langstrings need to be fixed in 2.0 and 2.1. i.e. $storemess = " STORED QTYPE ".$question_type_names[$this->savedquestiondisplay->options->questions [$sub] ->qtype]; What is the simplest way to proceed so that you have the minimum of commits to handle. .Modify the bug name and text so to put everything in one commit? .Create a substask ? ...
          Hide
          Tim Hunt added a comment -

          No need for sub-tasks.

          What you should aim for in git is:

          • a single branch
          • that starts from the latest MOODLE_20_STABLE branch
          • and contains any number of commits. That number may be 1 for simple changes, but for more complex changes it can be helpful to have several commits that break the change down into logical steps.

          Then you should post the information about the branch here. If you are using github, then one very good way to make it clear what changes you have done is to give a compare URL like https://github.com/timhunt/moodle/compare/MOODLE_20_STABLE...MDL-29528_20

          That is just the ideal to aim at. Any reasonable approximation that lets my get the code is fine.

          Show
          Tim Hunt added a comment - No need for sub-tasks. What you should aim for in git is: a single branch that starts from the latest MOODLE_20_STABLE branch and contains any number of commits. That number may be 1 for simple changes, but for more complex changes it can be helpful to have several commits that break the change down into logical steps. Then you should post the information about the branch here. If you are using github, then one very good way to make it clear what changes you have done is to give a compare URL like https://github.com/timhunt/moodle/compare/MOODLE_20_STABLE...MDL-29528_20 That is just the ideal to aim at. Any reasonable approximation that lets my get the code is fine.
          Hide
          Pierre Pichet added a comment -

          Tim,
          How shuld we handle plural in language strings.
          there can be one or more questions added to an already used in a quiz cloze question.

          acutal strings are

          $string['questionnadded'] = 'Question added';

          $string['questionsmore'] = '{$a} question(s) more than in the multianswer question stored in the database';

          Should we create a special code and singular and plural versions of the strings or change to

          $string['questionsadded'] = 'Question(s) added';

          Incidently I came to that case as $string['questionnadded'] needs to be corrected to 'questionsadded'

          Show
          Pierre Pichet added a comment - Tim, How shuld we handle plural in language strings. there can be one or more questions added to an already used in a quiz cloze question. acutal strings are $string ['questionnadded'] = 'Question added'; $string ['questionsmore'] = '{$a} question(s) more than in the multianswer question stored in the database'; Should we create a special code and singular and plural versions of the strings or change to $string ['questionsadded'] = 'Question(s) added'; Incidently I came to that case as $string ['questionnadded'] needs to be corrected to 'questionsadded'
          Hide
          Tim Hunt added a comment -

          There is not a good way to handle plurals. This is a known problems. (See the first main section 'A Localization Horror Story: It Could Happen To You' of http://search.cpan.org/dist/Locale-Maketext/lib/Locale/Maketext/TPJ13.pod for a great summary of the problem.)

          So, the 'right' way to do it in Moodle is:

          1. If possible, rewrite the string to avoid the problem. A good example of this is the string "Attempts: 1" used in the quiz UI.

          2. If that does not work, use strings like '{$a} question(s) more than in the multianswer question stored in the database';

          Show
          Tim Hunt added a comment - There is not a good way to handle plurals. This is a known problems. (See the first main section 'A Localization Horror Story: It Could Happen To You' of http://search.cpan.org/dist/Locale-Maketext/lib/Locale/Maketext/TPJ13.pod for a great summary of the problem.) So, the 'right' way to do it in Moodle is: 1. If possible, rewrite the string to avoid the problem. A good example of this is the string "Attempts: 1" used in the quiz UI. 2. If that does not work, use strings like '{$a} question(s) more than in the multianswer question stored in the database';
          Hide
          Tim Hunt added a comment -

          This issue was assigned to me automatically, however I will not be able to work on this issue in the immediate future. In order to create a truer sense of the state of this issue and to allow other developers to have chance to become involved, I am removing myself as the assignee of this issue.

          For more information, see http://docs.moodle.org/dev/Changes_to_issue_assignment

          Show
          Tim Hunt added a comment - This issue was assigned to me automatically, however I will not be able to work on this issue in the immediate future. In order to create a truer sense of the state of this issue and to allow other developers to have chance to become involved, I am removing myself as the assignee of this issue. For more information, see http://docs.moodle.org/dev/Changes_to_issue_assignment

            People

            • Votes:
              2 Vote for this issue
              Watchers:
              5 Start watching this issue

              Dates

              • Created:
                Updated: