Moodle
  1. Moodle
  2. MDL-36422

Accessibility bug in Quiz when trying to do manual grading

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 2.2.5, 2.3.2
    • Fix Version/s: 2.2.6, 2.3.3
    • Component/s: Quiz
    • Labels:
    • Testing Instructions:
      Hide

      1. Create a test with couple of questions
      2. Attempt the test with some student accounts
      3. Login as teacher and try to do manual grading on the attempts
      4. Make sure the HTML is correctly displayed
      5. Try on a few different browsers

      Show
      1. Create a test with couple of questions 2. Attempt the test with some student accounts 3. Login as teacher and try to do manual grading on the attempts 4. Make sure the HTML is correctly displayed 5. Try on a few different browsers
    • Affected Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE
    • Fixed Branches:
      MOODLE_22_STABLE, MOODLE_23_STABLE
    • Pull from Repository:
    • Pull Master Branch:
      MDL-36422-master
    • Rank:
      45247

      Description

      Found another regression related from http://tracker.moodle.org/browse/MDL-34570 in the latest MOODLE_23_STABLE release.
      If you open up any test with some attempts and try to do manual grading, then the UI view is messed up.

      Note: This happens in latest weekly release, not in the 2.3.2 release from September.
      Any weekly release after MDL-34570 has been integrated.

      1. binarious_theme2.jpg
        50 kB
      2. binarius_theme1.jpg
        102 kB
      3. custom_theme1.jpg
        124 kB
      4. custom_theme2.jpg
        90 kB

        Issue Links

          Activity

          Hide
          Tõnis Tartes added a comment - - edited

          I also found the cause of that..
          question/behaviour/rendererbase.php around line 89 -

                  $commenteditor = html_writer::tag('div', html_writer::tag('textarea', s($commenttext),
                          array('id' => $id, 'name' => $inputname, 'rows' => 10, 'cols' => 60)));
                  $commenteditor .= html_writer::end_tag('div');
          

          I think this line is unneccessary - $commenteditor .= html_writer::end_tag('div'); - because html_writer::tag automatically closes the div.
          If i commented that line out, the view was fixed.

          gitlog link - https://github.com/moodle/moodle/commit/d3187c299a14659bd5ec5ccc3e4a243ba3659583

          Show
          Tõnis Tartes added a comment - - edited I also found the cause of that.. question/behaviour/rendererbase.php around line 89 - $commenteditor = html_writer::tag('div', html_writer::tag('textarea', s($commenttext), array('id' => $id, 'name' => $inputname, 'rows' => 10, 'cols' => 60))); $commenteditor .= html_writer::end_tag('div'); I think this line is unneccessary - $commenteditor .= html_writer::end_tag('div'); - because html_writer::tag automatically closes the div. If i commented that line out, the view was fixed. gitlog link - https://github.com/moodle/moodle/commit/d3187c299a14659bd5ec5ccc3e4a243ba3659583
          Hide
          Tim Hunt added a comment -

          I think your analysis of the problem is wrong. That end_tag matches a corresponding start_tag higher up the code. Also, that end_tag line was not changed as part of MDL-34570.

          Show
          Tim Hunt added a comment - I think your analysis of the problem is wrong. That end_tag matches a corresponding start_tag higher up the code. Also, that end_tag line was not changed as part of MDL-34570 .
          Hide
          Tim Hunt added a comment -

          Fred, any chance you could take a quick look at this regression? It would be good to fix it before 2.3.3 if that is possible.

          Show
          Tim Hunt added a comment - Fred, any chance you could take a quick look at this regression? It would be good to fix it before 2.3.3 if that is possible.
          Hide
          Tõnis Tartes added a comment -

          I could'nt find any start_tag before that end_tag inside /question/behaviour/rendererbase.php, thats why i suspect that line as the cause of this problem..
          Maybe it was just accidentally added or maybe indeed the start_tag begins from another file which would make finding it harder i guess..
          Although it would be nice if this would be fixed before 2.3.3, probably more people will find this bug and complain that the blocks are misplaced..

          Show
          Tõnis Tartes added a comment - I could'nt find any start_tag before that end_tag inside /question/behaviour/rendererbase.php, thats why i suspect that line as the cause of this problem.. Maybe it was just accidentally added or maybe indeed the start_tag begins from another file which would make finding it harder i guess.. Although it would be nice if this would be fixed before 2.3.3, probably more people will find this bug and complain that the blocks are misplaced..
          Hide
          Frédéric Massart added a comment -

          There you go. I removed the closing </div>. Thanks for reporting this Tonis, I have to admit that on my installation the difference is not visible, but the HTML was incorrect so I fixed it.

          Could you please provide a screenshot of you are experiencing? Along with some eventual settings that you might have set (or themes). That might help improving the testing instructions.

          Cheers!
          Fred

          Show
          Frédéric Massart added a comment - There you go. I removed the closing </div>. Thanks for reporting this Tonis, I have to admit that on my installation the difference is not visible, but the HTML was incorrect so I fixed it. Could you please provide a screenshot of you are experiencing? Along with some eventual settings that you might have set (or themes). That might help improving the testing instructions. Cheers! Fred
          Hide
          Dan Poltawski added a comment -

          Expedited integration review because Fred asked nicely on behalf of Tim (and its easy )

          Show
          Dan Poltawski added a comment - Expedited integration review because Fred asked nicely on behalf of Tim (and its easy )
          Hide
          Dan Poltawski added a comment -

          Intgerated to 22, 23 and master. Thanks Fred

          Show
          Dan Poltawski added a comment - Intgerated to 22, 23 and master. Thanks Fred
          Hide
          Tim Hunt added a comment -

          Thanks all.

          Show
          Tim Hunt added a comment - Thanks all.
          Hide
          Mark Nelson added a comment -

          Page looked fine to me, no breakage here.

          Show
          Mark Nelson added a comment - Page looked fine to me, no breakage here.
          Hide
          Tõnis Tartes added a comment -

          Attaching couple of screenshots.
          2 Screenshots from a custom theme and couple of screenshots from one Moodles default theme.

          Show
          Tõnis Tartes added a comment - Attaching couple of screenshots. 2 Screenshots from a custom theme and couple of screenshots from one Moodles default theme.
          Hide
          Tõnis Tartes added a comment -

          With the fix applied, it shows everything correctly.

          Show
          Tõnis Tartes added a comment - With the fix applied, it shows everything correctly.
          Hide
          Frédéric Massart added a comment -

          Thanks for testing this Mark and Tonis!

          Show
          Frédéric Massart added a comment - Thanks for testing this Mark and Tonis!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Amazed. Inspired. Grateful. That’s how your generosity makes me feel.

          (not really)

          Closing, thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Amazed. Inspired. Grateful. That’s how your generosity makes me feel. (not really) Closing, thanks!

            People

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

              Dates

              • Created:
                Updated:
                Resolved: