Use real check boxes to select goals

We were using list items with the checkbox role. This is probably
confusing since list items have a listitem role, and so we were
basically using a list with no listitem.

We could add a `<span role="checkbox">` tag inside the list item, but
using real checkboxes is probably easier. We're also adding a test to
verify the checkboxes native behavior is compatible with our code.

Note we're using the "enter" key to toggle the "checked" status of the
SDG. This is probably not intuitive for screen reader users who might
try to submit the form using the "enter" key after selecting a goal.

However, the alternative would be even less intuitive for sighted
keyboard users, since for them these icons look like links or buttons
and they would accidentally submit the form when trying to select a
goal.

Since we haven't come up with a better interface yet, for now we're
following the principle of least damage; we consider submitting the form
against a user's will is less annoying than forcing users to move to a
different field before being able to submit the form.

Also note we can't write `check` in the tests because Capybara will try
to click the checkbox, which is hidden by the image in the label.

app/assets/javascripts/sdg/related_list_selector.js

@@ -30,11 +30,11 @@
             var keep_goal = $(amsify_suggestags.selector).val().split(",").some(function(selected_value) {
               return App.SDGRelatedListSelector.goal_code(value) === App.SDGRelatedListSelector.goal_code(selected_value);
             });
-            App.SDGRelatedListSelector.goal_element(value).attr("aria-checked", keep_goal);
+            App.SDGRelatedListSelector.goal_element(value).prop("checked", keep_goal);
             App.SDGRelatedListSelector.manage_remove_help(amsify_suggestags, value);
           },
           afterAdd: function(value) {
-            App.SDGRelatedListSelector.goal_element(value).attr("aria-checked", true);
+            App.SDGRelatedListSelector.goal_element(value).prop("checked", true);
             App.SDGRelatedListSelector.manage_add_help(amsify_suggestags, value);
           },
           keepLastOnHoverTag: false,
@@ -48,23 +48,24 @@
       }
     },
     manage_icons: function(amsify_suggestags) {
-      $("[role='checkbox']").on("click keydown", function(event) {
+      $(".sdg-related-list-selector .goals input").on("change", function() {
         var goal_id = this.dataset.code;
 
-        if (event.type === "click" || (event.type === "keydown" && [13, 32].indexOf(event.keyCode) >= 0)) {
-          if (amsify_suggestags.isPresent(goal_id)) {
-            amsify_suggestags.removeTag(goal_id, false);
-          } else {
-            amsify_suggestags.addTag(goal_id, false);
-          }
-
+        if (amsify_suggestags.isPresent(goal_id)) {
+          amsify_suggestags.removeTag(goal_id, false);
+        } else {
+          amsify_suggestags.addTag(goal_id, false);
+        }
+      }).on("keydown", function(event) {
+        if (event.keyCode === 13) {
+          $(this).trigger("click");
           event.preventDefault();
           event.stopPropagation();
         }
       });
     },
     goal_element: function(value) {
-      return $("li[data-code=" + App.SDGRelatedListSelector.goal_code(value) + "]");
+      return $(".sdg-related-list-selector .goals [data-code=" + App.SDGRelatedListSelector.goal_code(value) + "]");
     },
     goal_code: function(value) {
       return value.toString().split(".")[0];

app/assets/stylesheets/mixins.scss

@@ -240,7 +240,7 @@
   width: calc(100% + #{$spacing});
   width: calc(100% + #{$max-spacing});
 
-  li {
+  > * {
     margin-bottom: 0;
     margin-left: $spacing;
     margin-left: $max-spacing;

app/assets/stylesheets/sdg/related_list_selector.scss

@@ -12,22 +12,18 @@
     }
   }
 
-  label + ul {
+  .goals {
     $spacing: 0.5%;
     @include sdg-goal-list($spacing);
 
-    li {
+    legend {
+      @include element-invisible;
+    }
+
+    label {
       min-width: $sdg-icon-min-width;
       width: calc(100% / 17 - #{$spacing});
 
-      &[aria-checked=true] img {
-        opacity: 0.15;
-      }
-
-      &:focus {
-        outline: $outline-focus;
-      }
-
       &:hover {
         cursor: pointer;
       }
@@ -36,6 +32,18 @@
         width: 100%;
       }
     }
+
+    input {
+      @include element-invisible;
+
+      &:focus + label {
+        outline: $outline-focus;
+      }
+
+      &:checked + label img {
+        opacity: 0.15;
+      }
+    }
   }
 
   .input-section {

app/components/sdg/related_list_selector_component.html.erb

@@ -2,13 +2,12 @@
   <div class="input-section">
     <%= f.label :related_sdg_list %>
 
-    <ul aria-label="<%= t("sdg.related_list_selector.goal_list") %>">
-      <% goals.each do |goal| %>
-        <li data-code="<%= goal.code %>" role="checkbox" aria-checked="<%= checked?(goal.code) %>" tabindex="0">
-          <%= render SDG::Goals::IconComponent.new(goal) %>
-        </li>
+    <fieldset class="goals">
+      <legend><%= t("sdg.related_list_selector.goal_list") %></legend>
+      <%= f.collection_check_boxes(:sdg_goal_ids, goals, :id, :code) do |checkbox_form| %>
+        <%= goal_field(checkbox_form) %>
       <% end %>
-    </ul>
+    </fieldset>
 
     <%= f.text_field :related_sdg_list,
                       class: "input",

app/components/sdg/related_list_selector_component.rb

@@ -5,10 +5,6 @@ class SDG::RelatedListSelectorComponent < ApplicationComponent
     @f = form
   end
 
-  def checked?(code)
-    f.object.sdg_goals.find_by(code: code).present?
-  end
-
   def sdg_related_suggestions
     goals_and_targets.map { |goal_or_target| suggestion_tag_for(goal_or_target) }
   end
@@ -39,6 +35,13 @@ class SDG::RelatedListSelectorComponent < ApplicationComponent
       SDG::Goal.order(:code)
     end
 
+    def goal_field(checkbox_form)
+      goal = checkbox_form.object
+
+      checkbox_form.check_box(data: { code: goal.code }) +
+        checkbox_form.label { render(SDG::Goals::IconComponent.new(goal)) }
+    end
+
     def text_for(goal_or_target)
       if goal_or_target.class.name == "SDG::Goal"
         t("sdg.related_list_selector.goal_identifier", code: goal_or_target.code)

spec/support/common_actions.rb

@@ -45,7 +45,9 @@ module CommonActions
   end
 
   def click_sdg_goal(code)
-    find("li[data-code='#{code}']").click
+    within(".sdg-related-list-selector .goals") do
+      find("[data-code='#{code}'] + label").click
+    end
   end
 
   def remove_sdg_goal_or_target_tag(code)

spec/system/sdg_management/relations_spec.rb

@@ -368,7 +368,7 @@ describe "SDG Relations", :js do
         visit sdg_management_edit_legislation_process_path(process)
         click_sdg_goal(1)
 
-        expect(find("li[data-code='1']")["aria-checked"]).to eq "true"
+        expect(find("input[data-code='1']")).to be_checked
       end
 
       scenario "when remove a last tag related to a Goal, the icon will not be checked" do
@@ -380,15 +380,32 @@ describe "SDG Relations", :js do
         visit sdg_management_edit_legislation_process_path(process)
         remove_sdg_goal_or_target_tag(1)
 
-        expect(find("li[data-code='1']")["aria-checked"]).to eq "true"
+        expect(find("input[data-code='1']")).to be_checked
 
         remove_sdg_goal_or_target_tag(1.1)
 
-        expect(find("li[data-code='1']")["aria-checked"]).to eq "true"
+        expect(find("input[data-code='1']")).to be_checked
 
         remove_sdg_goal_or_target_tag("1.1.1")
 
-        expect(find("li[data-code='1']")["aria-checked"]).to eq "false"
+        expect(find("input[data-code='1']")).not_to be_checked
+      end
+
+      context "when we have a Goal and a related Target selected" do
+        scenario "we can remove and add same Goal always keeping the icon as checked" do
+          process = create(:legislation_process, title: "SDG process")
+          process.sdg_goals = [SDG::Goal[1]]
+          process.sdg_targets = [SDG::Target[1.1]]
+
+          visit sdg_management_edit_legislation_process_path(process)
+          click_sdg_goal(1)
+
+          expect(find("input[data-code='1']")).to be_checked
+
+          click_sdg_goal(1)
+
+          expect(find("input[data-code='1']")).to be_checked
+        end
       end
     end