From fb981bfbbe5162f5374613f489866dd6746ffa35 Mon Sep 17 00:00:00 2001
From: Jillian Vogel <jill@opencraft.com>
Date: Tue, 23 Jul 2019 16:23:40 +0930
Subject: [PATCH] Aids styleability of CAPA checkbox and radio problems

by making CAPA <input> elements siblings of their <label>s, instead of children.

Also:

* Moves radio submitted status block down below the problem
  to match the checkbox problem status blocks.
* Marks submitted choicegroup answers with a class
---
 .../lib/capa/capa/templates/choicegroup.html  |  48 ++++----
 .../capa/capa/tests/test_input_templates.py   |  20 ++--
 .../lib/xmodule/xmodule/css/capa/display.scss | 110 ++++++++++--------
 .../xmodule/xmodule/js/src/capa/display.js    |  63 +++++-----
 common/test/acceptance/pages/lms/problem.py   |  29 +++--
 .../tests/lms/test_problem_types.py           |   2 +-
 6 files changed, 149 insertions(+), 123 deletions(-)

diff --git a/common/lib/capa/capa/templates/choicegroup.html b/common/lib/capa/capa/templates/choicegroup.html
index f4b39c7969..6d9b1276e0 100644
--- a/common/lib/capa/capa/templates/choicegroup.html
+++ b/common/lib/capa/capa/templates/choicegroup.html
@@ -18,47 +18,39 @@ import six
         <p class="question-description" id="${description_id}">${description_text}</p>
       % endfor
         % for choice_id, choice_label in choices:
+          <%
+            label_class = 'response-label field-label label-inline'
+            input_class = 'field-input input-' + input_type
+            input_checked = ''
+
+            if is_radio_input(choice_id) or (input_type != 'radio' and choice_id in value):
+              input_class += ' submitted'
+              if status.classname and not show_correctness == 'never':
+                label_class += ' choicegroup_' + status.classname
+          %>
           <div class="field">
-            <%
-              label_class = 'response-label field-label label-inline'
-            %>
-
-            <label id="${id}-${choice_id}-label"
-                ## If the student has selected this choice...
-                % if is_radio_input(choice_id):
-
-                    % if status.classname and not show_correctness == 'never':
-                        <% label_class += ' choicegroup_' + status.classname %>
-                    % endif
-                % endif
-                class="${label_class}"
-                ${describedby_html}
-                >
-                <input type="${input_type}" name="input_${id}${name_array_suffix}" id="input_${id}_${choice_id}" class="field-input input-${input_type}" value="${choice_id}"
+            <input type="${input_type}" name="input_${id}${name_array_suffix}" id="input_${id}_${choice_id}"
+              class="${input_class}" value="${choice_id}"
                 ## If the student selected this choice...
                 % if is_radio_input(choice_id):
                 checked="true"
                 % elif input_type != 'radio' and choice_id in value:
                 checked="true"
                 % endif
-                /> ${HTML(choice_label)}
-                % if is_radio_input(choice_id):
-                  % if not show_correctness == 'never' and status.classname != 'unanswered':
-                    <%include file="status_span.html" args="status=status, status_id=id"/>
-                  % endif
-                % endif
+            /><label id="${id}-${choice_id}-label" for="input_${id}_${choice_id}"
+                class="${label_class}"
+                ${describedby_html}
+                > ${HTML(choice_label)}
             </label>
           </div>
         % endfor
         <span id="answer_${id}"></span>
     </fieldset>
     <div class="indicator-container">
-        % if input_type == 'checkbox' or status.classname == 'unanswered':
-          % if show_correctness != 'never':
-            <%include file="status_span.html" args="status=status, status_id=id"/>
-          % else:
-            <%include file="status_span.html" args="status=status, status_id=id, hide_correctness=True"/>
-          % endif
+        % if show_correctness != 'never':
+          <%include file="status_span.html" args="status=status, status_id=id"/>
+        % else:
+          <%include file="status_span.html" args="status=status, status_id=id, hide_correctness=True"/>
         % endif
     </div>
     % if show_correctness == "never" and (value or status not in ['unsubmitted']):
diff --git a/common/lib/capa/capa/tests/test_input_templates.py b/common/lib/capa/capa/tests/test_input_templates.py
index 82342698b4..9e599db576 100644
--- a/common/lib/capa/capa/tests/test_input_templates.py
+++ b/common/lib/capa/capa/tests/test_input_templates.py
@@ -118,8 +118,8 @@ class TemplateTestCase(unittest.TestCase):
         If no elements are found, the assertion fails.
         """
         element_list = xml_root.xpath(xpath)
-        self.assertGreater(len(element_list), 0, "Could not find element at '%s'" % str(xpath))
-
+        self.assertGreater(len(element_list), 0, "Could not find element at '%s'\n%s" %
+                           (str(xpath), etree.tostring(xml_root)))
         if exact:
             self.assertEqual(text, element_list[0].text.strip())
         else:
@@ -345,7 +345,7 @@ class ChoiceGroupTemplateTest(TemplateTestCase):
     def test_option_marked_correct(self):
         """
         Test conditions under which a particular option
-        (not the entire problem) is marked correct.
+        and the entire problem is marked correct.
         """
         conditions = [
             {'input_type': 'radio', 'value': '2'},
@@ -359,14 +359,14 @@ class ChoiceGroupTemplateTest(TemplateTestCase):
             xpath = "//label[contains(@class, 'choicegroup_correct')]"
             self.assert_has_xpath(xml, xpath, self.context)
 
-            # Should NOT mark the whole problem
-            xpath = "//div[@class='indicator-container']/span"
-            self.assert_no_xpath(xml, xpath, self.context)
+            # Should also mark the whole problem
+            xpath = "//div[@class='indicator-container']/span[@class='status correct']"
+            self.assert_has_xpath(xml, xpath, self.context)
 
     def test_option_marked_incorrect(self):
         """
         Test conditions under which a particular option
-        (not the entire problem) is marked incorrect.
+        and the entire problem is marked incorrect.
         """
         conditions = [
             {'input_type': 'radio', 'value': '2'},
@@ -380,9 +380,9 @@ class ChoiceGroupTemplateTest(TemplateTestCase):
             xpath = "//label[contains(@class, 'choicegroup_incorrect')]"
             self.assert_has_xpath(xml, xpath, self.context)
 
-            # Should NOT mark the whole problem
-            xpath = "//div[@class='indicator-container']/span"
-            self.assert_no_xpath(xml, xpath, self.context)
+            # Should also mark the whole problem
+            xpath = "//div[@class='indicator-container']/span[@class='status incorrect']"
+            self.assert_has_xpath(xml, xpath, self.context)
 
     def test_never_show_correctness(self):
         """
diff --git a/common/lib/xmodule/xmodule/css/capa/display.scss b/common/lib/xmodule/xmodule/css/capa/display.scss
index 05355bd052..32bba7cda7 100644
--- a/common/lib/xmodule/xmodule/css/capa/display.scss
+++ b/common/lib/xmodule/xmodule/css/capa/display.scss
@@ -222,52 +222,6 @@ div.problem {
     &::after {
       @include margin-left($baseline*0.75);
     }
-
-    &:hover {
-      border: 2px solid $blue;
-    }
-
-    &.choicegroup_correct {
-      @include status-icon($correct, $checkmark-icon);
-
-      border: 2px solid $correct;
-
-      // keep green for correct answers on hover.
-      &:hover {
-        border-color: $correct;
-      }
-    }
-
-    &.choicegroup_partially-correct {
-      @include status-icon($partially-correct, $asterisk-icon);
-
-      border: 2px solid $partially-correct;
-
-      // keep green for correct answers on hover.
-      &:hover {
-        border-color: $partially-correct;
-      }
-    }
-
-    &.choicegroup_incorrect {
-      @include status-icon($incorrect, $cross-icon);
-
-      border: 2px solid $incorrect;
-
-      // keep red for incorrect answers on hover.
-      &:hover {
-        border-color: $incorrect;
-      }
-    }
-
-    &.choicegroup_submitted {
-      border: 2px solid $submitted;
-
-      // keep blue for submitted answers on hover.
-      &:hover {
-        border-color: $submitted;
-      }
-    }
   }
 
   .indicator-container {
@@ -284,6 +238,41 @@ div.problem {
   input[type="checkbox"] {
     @include margin(($baseline/4) ($baseline/2) ($baseline/4) ($baseline/4));
   }
+
+  input {
+    &:focus,
+    &:hover {
+      & + label {
+        border: 2px solid $blue;
+      }
+    }
+
+    &,
+    &:focus,
+    &:hover {
+      & + label.choicegroup_correct {
+        @include status-icon($correct, $checkmark-icon);
+
+        border: 2px solid $correct;
+      }
+
+      & + label.choicegroup_partially-correct {
+        @include status-icon($partially-correct, $asterisk-icon);
+
+        border: 2px solid $partially-correct;
+      }
+
+      & + label.choicegroup_incorrect {
+        @include status-icon($incorrect, $cross-icon);
+
+        border: 2px solid $incorrect;
+      }
+
+      & + label.choicegroup_submitted {
+        border: 2px solid $submitted;
+      }
+    }
+  }
 }
 
 // +Problem - Choice Group
@@ -292,6 +281,10 @@ div.problem {
   .choicegroup {
     @extend %choicegroup-base;
 
+    .field {
+      position: relative;
+    }
+
     label {
       @include padding($baseline/2);
       @include padding-left($baseline*1.9);
@@ -308,6 +301,9 @@ div.problem {
 
       position: absolute;
       top: em(9);
+      width: $baseline*1.1;
+      height: $baseline*1.1;
+      z-index: 1;
     }
 
     legend {
@@ -1628,6 +1624,17 @@ div.problem .imageinput.capa_inputtype {
     top: 3px;
     width: 25px;
     height: 20px;
+
+    &.unsubmitted,
+    &.unanswered {
+      .status-icon {
+        content: '';
+      }
+
+      .status-message {
+        display: none;
+      }
+    }
   }
 
   .correct {
@@ -1658,6 +1665,17 @@ div.problem .annotation-input {
     top: 3px;
     width: 25px;
     height: 20px;
+
+    &.unsubmitted,
+    &.unanswered {
+      .status-icon {
+        content: '';
+      }
+
+      .status-message {
+        display: none;
+      }
+    }
   }
 
   .correct {
diff --git a/common/lib/xmodule/xmodule/js/src/capa/display.js b/common/lib/xmodule/xmodule/js/src/capa/display.js
index 728b047bc7..5ec25bfc19 100644
--- a/common/lib/xmodule/xmodule/js/src/capa/display.js
+++ b/common/lib/xmodule/xmodule/js/src/capa/display.js
@@ -230,16 +230,16 @@
                 // Render 'x point(s) possible (un/graded, results hidden)' if no current score provided.
                 if (graded) {
                     progressTemplate = ngettext(
-                        // Translators: %(num_points)s is the number of points possible (examples: 1, 3, 10).;
-                        '%(num_points)s point possible (graded, results hidden)',
-                        '%(num_points)s points possible (graded, results hidden)',
+                        // Translators: {num_points} is the number of points possible (examples: 1, 3, 10).;
+                        '{num_points} point possible (graded, results hidden)',
+                        '{num_points} points possible (graded, results hidden)',
                         totalScore
                     );
                 } else {
                     progressTemplate = ngettext(
-                        // Translators: %(num_points)s is the number of points possible (examples: 1, 3, 10).;
-                        '%(num_points)s point possible (ungraded, results hidden)',
-                        '%(num_points)s points possible (ungraded, results hidden)',
+                        // Translators: {num_points} is the number of points possible (examples: 1, 3, 10).;
+                        '{num_points} point possible (ungraded, results hidden)',
+                        '{num_points} points possible (ungraded, results hidden)',
                         totalScore
                     );
                 }
@@ -248,14 +248,14 @@
                 // But if staff has overridden score to a non-zero number, show it
                 if (graded) {
                     progressTemplate = ngettext(
-                        // Translators: %(num_points)s is the number of points possible (examples: 1, 3, 10).;
-                        '%(num_points)s point possible (graded)', '%(num_points)s points possible (graded)',
+                        // Translators: {num_points} is the number of points possible (examples: 1, 3, 10).;
+                        '{num_points} point possible (graded)', '{num_points} points possible (graded)',
                         totalScore
                     );
                 } else {
                     progressTemplate = ngettext(
-                        // Translators: %(num_points)s is the number of points possible (examples: 1, 3, 10).;
-                        '%(num_points)s point possible (ungraded)', '%(num_points)s points possible (ungraded)',
+                        // Translators: {num_points} is the number of points possible (examples: 1, 3, 10).;
+                        '{num_points} point possible (ungraded)', '{num_points} points possible (ungraded)',
                         totalScore
                     );
                 }
@@ -264,25 +264,25 @@
                 if (graded) {
                     progressTemplate = ngettext(
                         // This comment needs to be on one line to be properly scraped for the translators.
-                        // Translators: %(earned)s is the number of points earned. %(possible)s is the total number of points (examples: 0/1, 1/1, 2/3, 5/10). The total number of points will always be at least 1. We pluralize based on the total number of points (example: 0/1 point; 1/2 points);
-                        '%(earned)s/%(possible)s point (graded)', '%(earned)s/%(possible)s points (graded)',
+                        // Translators: {earned} is the number of points earned. {possible} is the total number of points (examples: 0/1, 1/1, 2/3, 5/10). The total number of points will always be at least 1. We pluralize based on the total number of points (example: 0/1 point; 1/2 points);
+                        '{earned}/{possible} point (graded)', '{earned}/{possible} points (graded)',
                         totalScore
                     );
                 } else {
                     progressTemplate = ngettext(
                         // This comment needs to be on one line to be properly scraped for the translators.
-                        // Translators: %(earned)s is the number of points earned. %(possible)s is the total number of points (examples: 0/1, 1/1, 2/3, 5/10). The total number of points will always be at least 1. We pluralize based on the total number of points (example: 0/1 point; 1/2 points);
-                        '%(earned)s/%(possible)s point (ungraded)', '%(earned)s/%(possible)s points (ungraded)',
+                        // Translators: {earned} is the number of points earned. {possible} is the total number of points (examples: 0/1, 1/1, 2/3, 5/10). The total number of points will always be at least 1. We pluralize based on the total number of points (example: 0/1 point; 1/2 points);
+                        '{earned}/{possible} point (ungraded)', '{earned}/{possible} points (ungraded)',
                         totalScore
                     );
                 }
             }
-            progress = interpolate(
+            progress = edx.StringUtils.interpolate(
                 progressTemplate, {
                     earned: curScore,
                     num_points: totalScore,
                     possible: totalScore
-                }, true
+                }
             );
             return this.$('.problem-progress').text(progress);
         };
@@ -379,7 +379,7 @@
         Problem.prototype.render = function(content, focusCallback) {
             var that = this;
             if (content) {
-                this.el.html(content);
+                edx.HtmlUtils.setHtml(this.el, edx.HtmlUtils.HTML(content));
                 return JavascriptLoader.executeModuleScripts(this.el, function() {
                     that.setupInputTypes();
                     that.bind();
@@ -389,7 +389,7 @@
                 });
             } else {
                 return $.postWithPrefix('' + this.url + '/problem_get', function(response) {
-                    that.el.html(response.html);
+                    edx.HtmlUtils.setHtml(that.el, edx.HtmlUtils.HTML(response.html));
                     return JavascriptLoader.executeModuleScripts(that.el, function() {
                         that.setupInputTypes();
                         that.bind();
@@ -560,11 +560,12 @@
                                 }
                             ));
                         }
-                        fd.append(element.id, file);
+                        fd.append(element.id, file);  // xss-lint: disable=javascript-jquery-append
                     }
                     if (element.files.length === 0) {
                         fileNotSelected = true;
-                        fd.append(element.id, ''); // In case we want to allow submissions with no file
+                        // In case we want to allow submissions with no file
+                        fd.append(element.id, '');  // xss-lint: disable=javascript-jquery-append
                     }
                     if (requiredFiles.length !== 0) {
                         requiredFilesNotSubmitted = true;
@@ -575,18 +576,21 @@
                         ));
                     }
                 } else {
-                    fd.append(element.id, element.value);
+                    fd.append(element.id, element.value);  // xss-lint: disable=javascript-jquery-append
                 }
             });
             if (fileNotSelected) {
                 errors.push(gettext('You did not select any files to submit.'));
             }
-            errorHtml = '<ul>\n';
+            errorHtml = '';
             for (i = 0, len = errors.length; i < len; i++) {
                 error = errors[i];
-                errorHtml += '<li>' + error + '</li>\n';
+                errorHtml = edx.HtmlUtils.joinHtml(
+                    errorHtml,
+                    edx.HtmlUtils.interpolateHtml(edx.HtmlUtils.HTML('<li>{error}</li>'), {error: error})
+                );
             }
-            errorHtml += '</ul>';
+            errorHtml = edx.HtmlUtils.interpolateHtml(edx.HtmlUtils.HTML('<ul>{errors}</ul>'), {errors: errorHtml});
             this.gentle_alert(errorHtml);
             abortSubmission = fileTooLarge || fileNotSelected || unallowedFileSubmitted || requiredFilesNotSubmitted;
             if (abortSubmission) {
@@ -965,6 +969,7 @@
                 return $(element).find('input').on('input', function() {
                     var $p;
                     $p = $(element).find('span.status');
+                    $p.removeClass('correct incorrect submitted');
                     return $p.parent().removeAttr('class').addClass('unsubmitted');
                 });
             },
@@ -1002,6 +1007,7 @@
                 return $(element).find('input').on('input', function() {
                     var $p;
                     $p = $(element).find('span.status');
+                    $p.removeClass('correct incorrect submitted');
                     return $p.parent().removeClass('correct incorrect').addClass('unsubmitted');
                 });
             }
@@ -1069,8 +1075,8 @@
                 results = [];
                 for (i = 0, len = answer.length; i < len; i++) {
                     choice = answer[i];
-                    $inputLabel = $element.find('#input_' + inputId + '_' + choice).parent('label');
-                    $inputStatus = $inputLabel.find('#status_' + inputId);
+                    $inputLabel = $element.find('#input_' + inputId + '_' + choice + ' + label');
+                    $inputStatus = $element.find('#status_' + inputId);
                     // If the correct answer was already Submitted before "Show Answer" was selected,
                     // the status HTML will already be present. Otherwise, inject the status HTML.
 
@@ -1078,12 +1084,13 @@
                     // will be marked as "unanswered". In that case, for correct answers update the
                     // classes accordingly.
                     if ($inputStatus.hasClass('unanswered')) {
-                        $inputStatus.removeAttr('class').addClass('status correct');
+                        edx.HtmlUtils.append($inputLabel, edx.HtmlUtils.HTML(correctStatusHtml));
                         $inputLabel.addClass('choicegroup_correct');
                     } else if (!$inputLabel.hasClass('choicegroup_correct')) {
                         // If the status HTML is not already present (due to clicking Submit), append
                         // the status HTML for correct answers.
                         edx.HtmlUtils.append($inputLabel, edx.HtmlUtils.HTML(correctStatusHtml));
+                        $inputLabel.removeClass('choicegroup_incorrect');
                         results.push($inputLabel.addClass('choicegroup_correct'));
                     }
                 }
@@ -1182,7 +1189,7 @@
                             types[key](context, value);
                         }
                     });
-                    container.html(canvas);
+                    edx.HtmlUtils.setHtml(container, edx.HtmlUtils.HTML(canvas));
                 } else {
                     console.log('Answer is absent for image input with id=' + id); // eslint-disable-line no-console
                 }
diff --git a/common/test/acceptance/pages/lms/problem.py b/common/test/acceptance/pages/lms/problem.py
index 6ee2cf9c38..c51f2d9e84 100644
--- a/common/test/acceptance/pages/lms/problem.py
+++ b/common/test/acceptance/pages/lms/problem.py
@@ -489,30 +489,39 @@ class ProblemPage(PageObject):
         solution_selector = '.solution-span div.detailed-solution'
         return self.q(css=solution_selector).is_present()
 
-    def is_choice_highlighted(self, choice, choices_list):
+    def is_choice_highlighted(self, choice, choices_list, show_answer=True):
         """
         Check if the given answer/choice is highlighted for choice group.
+
+        show_answer: if set, then requires each choice to be marked with a status.
+            If not set, then the status can be elswhere in the problem.
         """
-        choice_status_xpath = (u'//fieldset/div[contains(@class, "field")][{{0}}]'
-                               u'/label[contains(@class, "choicegroup_{choice}")]'
-                               u'/span[contains(@class, "status {choice}")]'.format(choice=choice))
-        any_status_xpath = u'//fieldset/div[contains(@class, "field")][{0}]/label/span'
-        for choice in choices_list:
-            if not self.q(xpath=choice_status_xpath.format(choice)).is_present():
+        if show_answer:
+            choice_status_xpath = (u'//fieldset/div[contains(@class, "field")][{{0}}]'
+                                   u'/label[contains(@class, "choicegroup_{choice}")]'
+                                   u'/span[contains(@class, "status {choice}")]'.format(choice=choice))
+            any_status_xpath = u'//fieldset/div[contains(@class, "field")][{0}]/label/span'
+        else:
+            choice_status_xpath = (u'//fieldset/div[contains(@class, "field")][{{0}}]'
+                                   u'/label[contains(@class, "choicegroup_{choice}")]'.format(choice=choice))
+            any_status_xpath = u'//div[contains(@class, "indicator-container")]/span[contains(@class, "status")]'
+
+        for possible_choice in choices_list:
+            if not self.q(xpath=choice_status_xpath.format(possible_choice)).is_present():
                 return False
 
             # Check that there is only a single status span, as there were some bugs with multiple
             # spans (with various classes) being appended.
-            if not len(self.q(xpath=any_status_xpath.format(choice)).results) == 1:
+            if not len(self.q(xpath=any_status_xpath.format(possible_choice)).results) == 1:
                 return False
 
         return True
 
-    def is_correct_choice_highlighted(self, correct_choices):
+    def is_correct_choice_highlighted(self, correct_choices, show_answer=True):
         """
         Check if correct answer/choice highlighted for choice group.
         """
-        return self.is_choice_highlighted('correct', correct_choices)
+        return self.is_choice_highlighted('correct', correct_choices, show_answer)
 
     def is_submitted_choice_highlighted(self, correct_choices):
         """
diff --git a/common/test/acceptance/tests/lms/test_problem_types.py b/common/test/acceptance/tests/lms/test_problem_types.py
index 5048909d5e..54d3cb379e 100644
--- a/common/test/acceptance/tests/lms/test_problem_types.py
+++ b/common/test/acceptance/tests/lms/test_problem_types.py
@@ -786,7 +786,7 @@ class MultipleChoiceProblemTypeTest(MultipleChoiceProblemTypeBase, ProblemTypeTe
 
         # After submit, the answer should be marked as correct.
         self.problem_page.click_submit()
-        self.assertTrue(self.problem_page.is_correct_choice_highlighted(correct_choices=[3]))
+        self.assertTrue(self.problem_page.is_correct_choice_highlighted(correct_choices=[3], show_answer=False))
 
         # Switch to an incorrect answer. This will hide the correctness indicator.
         self.answer_problem('incorrect')