From a2f6e03516c730832f296fe8a2d6b659bcb41152 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Javi=20Mart=C3=ADn?= <javim@elretirao.net>
Date: Tue, 29 Jun 2021 00:11:03 +0200
Subject: [PATCH] Move advanced search toggle button inside the form

Since forms are landmarks, screen reader users might navigate to the
form. But then they were going to find an empty form with no way to
toggle it.

Moving the button inside the form means screen reader users navigating
to the form will find the button to toggle it.

It also helps us simplifying the code; there's no need to use
data-attributes to communicate whether the form should be visible since
now we can easily use the button `aria-expanded` attribute.

We could further simplify the JavaScript if we used a CSS rule to
show/hide the form fields based on the toggle button `aria-expanded`
attribute. However, implementing the "slide" animation we use when
toggling the form with CSS is difficult and unreliable.
---
 app/assets/javascripts/advanced_search.js     |  9 ++---
 app/assets/stylesheets/advanced_search.scss   | 39 ++++++++++---------
 .../shared/advanced_search_component.html.erb |  6 +--
 .../shared/advanced_search_component_spec.rb  |  2 +-
 spec/system/advanced_search_spec.rb           |  4 +-
 5 files changed, 29 insertions(+), 31 deletions(-)

diff --git a/app/assets/javascripts/advanced_search.js b/app/assets/javascripts/advanced_search.js
index 89f8be04f..d987fda8a 100644
--- a/app/assets/javascripts/advanced_search.js
+++ b/app/assets/javascripts/advanced_search.js
@@ -1,9 +1,6 @@
 (function() {
   "use strict";
   App.AdvancedSearch = {
-    advanced_search_terms: function() {
-      return $("#js-advanced-search").data("advanced-search-terms");
-    },
     toggle_date_options: function() {
       if ($("#js-advanced-search-date-min").val() === "custom") {
         $("#js-custom-date").show();
@@ -18,15 +15,15 @@
 
       toggle_button.removeAttr("hidden");
 
-      if (App.AdvancedSearch.advanced_search_terms()) {
+      if (toggle_button.attr("aria-expanded") === "true") {
         App.AdvancedSearch.toggle_date_options();
       } else {
-        $("#js-advanced-search").hide();
+        toggle_button.next().hide();
       }
       toggle_button.on({
         click: function() {
           $(this).attr("aria-expanded", !JSON.parse($(this).attr("aria-expanded")));
-          $("#js-advanced-search").slideToggle();
+          $(this).next().slideToggle();
         }
       });
       $("#js-advanced-search-date-min").on({
diff --git a/app/assets/stylesheets/advanced_search.scss b/app/assets/stylesheets/advanced_search.scss
index 394a0d0b6..b6bf6c5ec 100644
--- a/app/assets/stylesheets/advanced_search.scss
+++ b/app/assets/stylesheets/advanced_search.scss
@@ -1,23 +1,6 @@
-.advanced-search {
-  color: $link;
-  cursor: pointer;
-  margin: $line-height 0;
-
-  @include breakpoint(medium) {
-    float: right;
-    margin-bottom: 0;
-    margin-top: $line-height / 4;
-    position: absolute;
-    right: 0;
-  }
-
-  &:focus {
-    outline: $outline-focus;
-  }
-}
-
 .advanced-search-form {
   @include grid-row-nest;
+  position: relative;
 
   @include breakpoint(large) {
     .filter {
@@ -43,6 +26,26 @@
     }
   }
 
+  > [aria-expanded] {
+    @include xy-gutters;
+    color: $link;
+    cursor: pointer;
+    margin-top: $line-height;
+    margin-bottom: $line-height;
+
+    @include breakpoint(medium) {
+      float: right;
+      margin-bottom: 0;
+      margin-top: $line-height / 4;
+      position: absolute;
+      right: 0;
+    }
+
+    &:focus {
+      outline: $outline-focus;
+    }
+  }
+
   .general-search,
   .filter,
   .submit {
diff --git a/app/components/shared/advanced_search_component.html.erb b/app/components/shared/advanced_search_component.html.erb
index f221eed99..1155ef057 100644
--- a/app/components/shared/advanced_search_component.html.erb
+++ b/app/components/shared/advanced_search_component.html.erb
@@ -1,11 +1,9 @@
-<div class="relative">
+<%= form_tag request.path, id: "advanced_search_form", class: "advanced-search-form", method: :get do %>
   <button type="button" aria-expanded="<%= advanced_search.present? %>" id="js-advanced-search-title" class="advanced-search small" hidden>
     <%= t("shared.advanced_search.title") %>
   </button>
-</div>
 
-<%= form_tag request.path, id: "advanced_search_form", class: "advanced-search-form", method: :get do %>
-  <div id="js-advanced-search" data-advanced-search-terms="<%= advanced_search.present? %>">
+  <div>
     <div class="general-search">
       <label for="search">
         <%= t("shared.advanced_search.general") %>
diff --git a/spec/components/shared/advanced_search_component_spec.rb b/spec/components/shared/advanced_search_component_spec.rb
index 8dd88d147..1bb232cb0 100644
--- a/spec/components/shared/advanced_search_component_spec.rb
+++ b/spec/components/shared/advanced_search_component_spec.rb
@@ -13,7 +13,7 @@ describe Shared::AdvancedSearchComponent, type: :component do
     it "hides the button to show the form" do
       render_inline component
 
-      expect(page).to have_button "Advanced search", visible: :hidden
+      expect(page.find("form")).to have_button "Advanced search", visible: :hidden
     end
   end
 
diff --git a/spec/system/advanced_search_spec.rb b/spec/system/advanced_search_spec.rb
index f6f296663..8303f9bea 100644
--- a/spec/system/advanced_search_spec.rb
+++ b/spec/system/advanced_search_spec.rb
@@ -351,7 +351,7 @@ describe "Advanced search" do
 
         click_button "Filter"
 
-        within "#js-advanced-search" do
+        within ".advanced-search-form" do
           expect(page).to have_selector("input[name='search'][value='Schwifty']")
           expect(page).to have_select("advanced_search[official_level]", selected: "Official position 1")
           expect(page).to have_select("advanced_search[date_min]", selected: "Last 24 hours")
@@ -370,7 +370,7 @@ describe "Advanced search" do
 
         expect(page).to have_content("citizen proposals cannot be found")
 
-        within "#js-advanced-search" do
+        within ".advanced-search-form" do
           expect(page).to have_select("advanced_search[date_min]", selected: "Customized")
           expect(page).to have_selector("input[name='advanced_search[date_min]'][value*='#{7.days.ago.strftime("%d/%m/%Y")}']")
           expect(page).to have_selector("input[name='advanced_search[date_max]'][value*='#{1.day.ago.strftime("%d/%m/%Y")}']")