Fwd port fix to OC-89: Improve MCQ/MRQ choice HTML

problem_builder/public/css/questionnaire.css

 .mentoring .questionnaire .choices-list {
+    display: table;
     position: relative;
+    width: 100%;
+    border-spacing: 0 6px;
     padding-top: 10px;
     margin-bottom: 10px;
 }
 
 .mentoring .questionnaire .choice-result {
-    display: inline-block;
+    display: table-cell;
     width: 40px;
-    vertical-align: middle;
+    vertical-align: top;
     cursor: pointer;
     float: none;
 }
 
 .mentoring .questionnaire .choice {
     overflow-y: hidden;
+    display: table-row;
 }
 
 .mentoring .questionnaire .choice-result.checkmark-correct,
@@ -71,26 +75,13 @@
 }
 
 .mentoring .choices-list .choice-selector {
-    margin-right: 5px;
+    display: table-cell;
+    vertical-align: top;
+    width: 28px;
 }
 
 .mentoring .choice-label {
-    display: inline-block;
-    margin-top: 8px;
-    margin-bottom: 5px;
+    display: table-cell;
+    vertical-align: top;
     line-height: 1.3;
 }
-
-.mentoring .choices-list .choice-text > .xblock-light-child * {
-    vertical-align: middle;
-}
-
-.mentoring .choices-list .choice-text > .xblock-light-child,
-.mentoring .choices-list .choice-text > .xblock-light-child > .html_child {
-    /*
-    HTML Light Child content is wrapped in two divs: div.xblock-light-child and just div
-    On the other hand, choice are usually rendered inline.
-    Hence, we render first two divs inline, than all the actual content of HTML is rendered as is
-    */
-    display: inline-block;
-}

problem_builder/questionnaire.py

@@ -113,6 +113,15 @@ class QuestionnaireAbstractBlock(StudioEditableXBlockMixin, StudioContainerXBloc
 
         return block
 
+    @property
+    def html_id(self):
+        """
+        A short, simple ID string used to uniquely identify this question.
+
+        This is only used by templates for matching <input> and <label> elements.
+        """
+        return unicode(id(self))  # Unique as long as all choices are loaded at once
+
     def student_view(self, context=None):
         name = getattr(self, "unmixed_class", self.__class__).__name__
 

problem_builder/templates/html/mcqblock.html

@@ -7,9 +7,14 @@
     {% for choice in custom_choices %}
     <div class="choice">
       <div class="choice-result fa icon-2x"></div>
-      <label class="choice-label">
-        <input class="choice-selector" type="radio" name="{{ self.name }}" value="{{ choice.value }}"{% if self.student_choice == choice.value %} checked{% endif %} />
-          <span class="choice-text">{{ choice.content|safe }}</span>
+      <div class="choice-selector">
+        <input id="choice-{{ self.html_id }}-{{ forloop.counter }}" type="radio"
+          name="{{ self.name }}" value="{{ choice.value }}"
+          {% if self.student_choice == choice.value %} checked{% endif %}
+        />
+      </div>
+      <label class="choice-label" for="choice-{{ self.html_id }}-{{ forloop.counter }}">
+          {{ choice.content|safe }}
       </label>
       <div class="choice-tips-container">
         <div class="choice-tips"></div>

problem_builder/templates/html/mrqblock.html

@@ -7,11 +7,14 @@
     {% for choice in custom_choices %}
     <div class="choice">
       <div class="choice-result fa icon-2x"></div>
-      <label class="choice-label">
-        <input class="choice-selector" type="checkbox" name="{{ self.name }}"
-               value="{{ choice.value }}"
-               {% if choice.value in self.student_choices %} checked{% endif %} />
-        <span class="choice-text">{{ choice.content|safe }}</span>
+      <div class="choice-selector">
+        <input id="choice-{{ self.html_id }}-{{ forloop.counter }}" type="checkbox"
+         name="{{ self.name }}" value="{{ choice.value }}"
+         {% if choice.value in self.student_choices %} checked{% endif %}
+        />
+      </div>
+      <label class="choice-label" for="choice-{{ self.html_id }}-{{ forloop.counter }}">
+        {{ choice.content|safe }}
       </label>
       <div class="choice-tips-container">
         <div class="choice-tips"></div>

problem_builder/templates/html/ratingblock.html

@@ -4,32 +4,37 @@
     <p>{{ self.question }}</p>
   </legend>
   <div class="choices-list">
-    {% for value in '12345' %}
+    {% for i in '12345' %}
       <div class="choice">
         <div class="choice-result fa icon-2x"></div>
-        <label class="choice-label">
-          <input class="choice-selector" type="radio" name="{{ self.name }}" value="{{ value }}"
-                 {% if self.student_choice == value %}checked{% endif %} />
-          {{ value }}
-          {% if forloop.first %}
-            <span class="low">- {{ self.low }}</span>
-          {% endif %}
-          {% if forloop.last %}
-            <span class="low">- {{ self.high }}</span>
-          {% endif %}
+        <div class="choice-selector">
+          <input id="choice-{{ self.html_id }}-{{i}}" type="radio"
+            name="{{ self.name }}" value="{{i}}"
+            {% if self.student_choice == i %} checked{%else%} data-student-choice='{{self.student_choice}}'{% endif %}
+            />
+        </div>
+        <label class="choice-label" for="choice-{{ self.html_id }}-{{i}}">
+          {{i}}
+          {% if i == '1' %} - {{ self.low }}{% endif %}
+          {% if i == '5' %} - {{ self.high }}{% endif %}
         </label>
         <div class="choice-tips-container">
           <div class="choice-tips"></div>
         </div>
       </div>
     {% endfor %}
+
     {% for choice in custom_choices %}
     <div class="choice">
       <div class="choice-result fa icon-2x"></div>
-      <label class="choice-label">
-        <input class="choice-selector" type="radio" name="{{ self.name }}" value="{{ choice.value }}"
-               {% if self.student_choice == '{{ choice.value }}' %} checked{% endif %} />
-          <span class="choice-text">{{ choice.content|safe }}</span>
+      <div class="choice-selector">
+        <input id="choice-{{ self.html_id }}-custom{{ forloop.counter }}" type="radio"
+          name="{{ self.name }}" value="{{ choice.value }}"
+          {% if self.student_choice == choice.value %} checked{%else%} data-student-choice='{{self.student_choice}}'{% endif %}
+        />
+      </div>
+      <label class="choice-label" for="choice-{{ self.html_id }}-custom{{ forloop.counter }}">
+          {{ choice.content|safe }}
       </label>
       <div class="choice-tips-container">
         <div class="choice-tips"></div>

problem_builder/tests/integration/test_mcq.py

@@ -45,6 +45,9 @@ class MCQBlockTest(MentoringBaseTest):
         """
         mcq_legend.click()
 
+    def _get_choice_label_text(self, choice):
+        return choice.find_element_by_css_selector('label').text
+
     def _get_inputs(self, choices):
         return [choice.find_element_by_css_selector('input') for choice in choices]
 
@@ -71,20 +74,17 @@ class MCQBlockTest(MentoringBaseTest):
         self.assertEqual(mcq1_legend.text, 'Question 1\nDo you like this MCQ?')
         self.assertEqual(mcq2_legend.text, 'Question 2\nHow do you rate this MCQ?')
 
-        mcq1_choices = mcq1.find_elements_by_css_selector('.choices .choice label')
-        mcq2_choices = mcq2.find_elements_by_css_selector('.rating .choice label')
-
-        self.assertEqual(len(mcq1_choices), 3)
-        self.assertEqual(len(mcq2_choices), 6)
-        self.assertEqual(mcq1_choices[0].text, 'Yes')
-        self.assertEqual(mcq1_choices[1].text, 'Maybe not')
-        self.assertEqual(mcq1_choices[2].text, "I don't understand")
-        self.assertEqual(mcq2_choices[0].text, '1 - Not good at all')
-        self.assertEqual(mcq2_choices[1].text, '2')
-        self.assertEqual(mcq2_choices[2].text, '3')
-        self.assertEqual(mcq2_choices[3].text, '4')
-        self.assertEqual(mcq2_choices[4].text, '5 - Extremely good')
-        self.assertEqual(mcq2_choices[5].text, "I don't want to rate it")
+        mcq1_choices = mcq1.find_elements_by_css_selector('.choices .choice')
+        mcq2_choices = mcq2.find_elements_by_css_selector('.rating .choice')
+
+        self.assertListEqual(
+            [self._get_choice_label_text(choice) for choice in mcq1_choices],
+            ["Yes", "Maybe not", "I don't understand"]
+        )
+        self.assertListEqual(
+            [self._get_choice_label_text(choice) for choice in mcq2_choices],
+            ['1 - Not good at all', '2', '3', '4', '5 - Extremely good', "I don't want to rate it"]
+        )
 
         mcq1_choices_input = self._get_inputs(mcq1_choices)
         mcq2_choices_input = self._get_inputs(mcq2_choices)
@@ -172,13 +172,13 @@ class MCQBlockTest(MentoringBaseTest):
         mcq_legend = mcq.find_element_by_css_selector('legend')
         self.assertEqual(mcq_legend.text, 'Question\nWhat do you like in this MRQ?')
 
-        mcq_choices = mcq.find_elements_by_css_selector('.choices .choice label')
+        mcq_choices = mcq.find_elements_by_css_selector('.choices .choice')
 
         self.assertEqual(len(mcq_choices), 4)
-        self.assertEqual(mcq_choices[0].text, 'Its elegance')
-        self.assertEqual(mcq_choices[1].text, 'Its beauty')
-        self.assertEqual(mcq_choices[2].text, "Its gracefulness")
-        self.assertEqual(mcq_choices[3].text, "Its bugs")
+        self.assertListEqual(
+            [self._get_choice_label_text(choice) for choice in mcq_choices],
+            ['Its elegance', 'Its beauty', "Its gracefulness", "Its bugs"]
+        )
 
         mcq_choices_input = self._get_inputs(mcq_choices)
         self.assertEqual(mcq_choices_input[0].get_attribute('value'), 'elegance')
@@ -200,7 +200,7 @@ class MCQBlockTest(MentoringBaseTest):
 
         for index, expected_feedback in enumerate(item_feedbacks):
             choice_wrapper = choices_list.find_elements_by_css_selector(".choice")[index]
-            choice_wrapper.find_element_by_css_selector(".choice-selector").click()  # clicking on actual radio button
+            choice_wrapper.find_element_by_css_selector(".choice-selector input").click()  # click actual radio button
             submit.click()
             self.wait_until_disabled(submit)
             item_feedback_icon = choice_wrapper.find_element_by_css_selector(".choice-result")
@@ -220,8 +220,8 @@ class MCQBlockTest(MentoringBaseTest):
         result = []
         # this could be a list comprehension, but a bit complicated one - hence explicit loop
         for choice_wrapper in questionnaire.find_elements_by_css_selector(".choice"):
-            choice_label = choice_wrapper.find_element_by_css_selector("label .choice-text")
-            result.append(choice_label.get_attribute('innerHTML'))
+            choice_label = choice_wrapper.find_element_by_css_selector("label")
+            result.append(choice_label.get_attribute('innerHTML').strip())
 
         return result
 
@@ -249,7 +249,7 @@ class MCQBlockTest(MentoringBaseTest):
         submit = mentoring.find_element_by_css_selector('.submit input.input-main')
         self.assertFalse(submit.is_enabled())
 
-        inputs = choices_list.find_elements_by_css_selector('input.choice-selector')
+        inputs = choices_list.find_elements_by_css_selector('.choice-selector input')
         self._selenium_bug_workaround_scroll_to(choices_list)
         inputs[0].click()
         inputs[1].click()