Merge pull request #3568 from projectblacklight/bl8-backport-facet-modal-fixes

jcoyne · web-flow · commit 77ceb36475aa · 2025-04-16T13:20:09.000-05:00
[8.x] Backport facet modal bugfixes
diff --git a/app/assets/stylesheets/blacklight/_bootstrap_overrides.scss b/app/assets/stylesheets/blacklight/_bootstrap_overrides.scss
@@ -39,4 +39,8 @@ dialog.modal[open] {
 
   max-height: unset; // override user-agent dialog
   max-width: unset; // override user-agent dialog
+
+  &::backdrop {
+    background-color: var(--bl-modal-backdrop-bg);
+  }
 }
diff --git a/app/assets/stylesheets/blacklight/_facets.scss b/app/assets/stylesheets/blacklight/_facets.scss
@@ -207,12 +207,6 @@
 /* Sidenav
 -------------------------------------------------- */
 
-.facet-pagination {
-  &.top {
-    padding: $modal-inner-padding;
-  }
-}
-
 .pivot-facet {
   &.show {
     display: flex;
@@ -272,3 +266,9 @@ $facet-toggle-height: $facet-toggle-width !default;
       $facet-toggle-width auto no-repeat;
   }
 }
+
+/* Facet browse pages & modals
+-------------------------------------------------- */
+.facet-filters:not(:has(*)) {
+  display: none !important;
+}
diff --git a/app/assets/stylesheets/blacklight/_modal.scss b/app/assets/stylesheets/blacklight/_modal.scss
@@ -3,25 +3,26 @@
 }
 
 .modal-content {
-  .facet-pagination.top {
-  	display: none;
-  }
-
   .page-sidebar {
     display: none;
   }
 
   .blacklight-modal-close {
     display: block;
   }
-
-  .prev_next_links.btn-group .btn {
-    border: 0;
-  }
 }
 
 // app/views/catalog/facet.html.erb may be rendered as a modal or a whole page.
 // When it's a whole page, don't show the close button.
 .blacklight-modal-close {
 	display: none;
 }
+
+// When modal content is rendered outside of a modal, Bootstrap css variables like
+// --bs-modal-footer-border-width are undefined (only in .modal scope). So we apply
+// light styling to the modal footer when it's not in a modal.
+.modal-footer:not(.modal .modal-footer) {
+  border-top: var(--bs-border-width) solid var(--bs-border-color);
+  padding-top: 1rem;
+  margin-top: 1rem;
+}
diff --git a/app/assets/stylesheets/blacklight/_pagination.scss b/app/assets/stylesheets/blacklight/_pagination.scss
@@ -10,7 +10,5 @@
 }
 
 .pagination {
-  @media (max-width: breakpoint-max(sm)) {
-    flex-wrap: wrap;
-  }
+  flex-wrap: wrap;
 }
diff --git a/app/assets/stylesheets/blacklight/blacklight_defaults.scss b/app/assets/stylesheets/blacklight/blacklight_defaults.scss
@@ -20,4 +20,7 @@ $body-secondary-color: rgba($body-color, 0.75) !default;
   --bl-constraint-remove-hover-border-color: #bb2d3b;
 
   --bl-field-name-color: var(--bs-secondary-color);
+
+  /* emulate Bootstrap backdrop bg & opacity */
+  --bl-modal-backdrop-bg: rgba(0, 0, 0, 0.5);
 }
diff --git a/app/components/blacklight/facet_field_pagination_component.html.erb b/app/components/blacklight/facet_field_pagination_component.html.erb
@@ -1,10 +1,10 @@
 <div class="prev_next_links btn-group">
   <%= helpers.link_to_previous_page @facet_field.paginator, raw(t('views.pagination.previous')), params: @facet_field.search_state.to_h, param_name: param_name, class: 'btn btn-link', data: { blacklight_modal: "preserve" } do %>
-    <%= content_tag :span, raw(t('views.pagination.previous')), class: 'disabled btn' %>
+    <%= content_tag :a, raw(t('views.pagination.previous')), class: 'disabled btn btn-link text-decoration-none', aria: { disabled: true } %>
   <% end %>
 
   <%= helpers.link_to_next_page @facet_field.paginator, raw(t('views.pagination.next')), params: @facet_field.search_state.to_h, param_name: param_name, class: 'btn btn-link',  data: { blacklight_modal: "preserve" } do %>
-    <%= content_tag :span, raw(t('views.pagination.next')), class: 'disabled btn' %>
+    <%= content_tag :a, raw(t('views.pagination.next')), class: 'disabled btn btn-link text-decoration-none', aria: { disabled: true } %>
   <% end %>
 </div>
 
diff --git a/app/components/blacklight/search/facet_suggest_input.html.erb b/app/components/blacklight/search/facet_suggest_input.html.erb
@@ -3,7 +3,10 @@
 </label>
 <%= text_field_tag "facet_suggest_#{facet.key}",
   nil,
-  class: "facet-suggest form-control",
-  data: {facet_field: facet.key},
+  class: "facet-suggest form-control mb-3",
+  data: {
+    facet_field: facet.key,
+    facet_search_context: presenter.view_context.search_facet_path(id: facet.key)
+  },
   placeholder: I18n.t('blacklight.search.form.search.placeholder')
 %>
diff --git a/app/javascript/blacklight/facet_suggest.js b/app/javascript/blacklight/facet_suggest.js
@@ -4,20 +4,42 @@ const FacetSuggest = async (e) => {
   if (e.target.matches('.facet-suggest')) {
     const queryFragment = e.target.value?.trim();
     const facetField = e.target.dataset.facetField;
+    const facetArea = document.querySelector('.facet-extended-list');
+    const prevNextLinks = document.querySelectorAll('.prev_next_links');
+
     if (!facetField) { return; }
 
-    const urlToFetch = `/catalog/facet_suggest/${facetField}/${queryFragment}${window.location.search}`
+    // Get the search params from the current query so the facet suggestions
+    // can retain that context.
+    const facetSearchContext = e.target.dataset.facetSearchContext;
+    const url = new URL(facetSearchContext, window.location.origin);
+
+    // Drop facet.page so a filtered suggestion list will always start on page 1
+    url.searchParams.delete('facet.page');
+    const facetSearchParams = url.searchParams.toString();
+
+    const urlToFetch = `/catalog/facet_suggest/${facetField}/${queryFragment}?${facetSearchParams}`;
+
     const response = await fetch(urlToFetch);
     if (response.ok) {
         const blob = await response.blob()
         const text = await blob.text()
-    
+
         const facetArea = document.querySelector('.facet-extended-list');
-    
+
         if (text && facetArea) {
             facetArea.innerHTML = text
         }
     }
+
+    // Hide the prev/next links when a user enters text in the facet
+    // suggestion input. They don't work with a filtered list.
+    prevNextLinks.forEach(element => {
+      element.classList.toggle('invisible', !!queryFragment);
+    });
+
+    // Add a class to distinguish suggested facet values vs. regular.
+    facetArea.classList.toggle('facet-suggestions', !!queryFragment);
   }
 };
 
diff --git a/app/views/catalog/facet.html.erb b/app/views/catalog/facet.html.erb
@@ -1,14 +1,14 @@
 <%= render Blacklight::System::ModalComponent.new do |component| %>
-  <% component.with_prefix do %>
-    <div class="facet-pagination top row justify-content-between">
-      <%= render :partial=>'facet_pagination' %>
-    </div>
-  <% end %>
-
   <% component.with_title { facet_field_label(@facet.key) } %>
-  <%= render Blacklight::Search::FacetSuggestInput.new(facet: @facet, presenter: @presenter) %>
 
-  <%= render partial: 'facet_index_navigation' if @facet.index_range && @display_facet.index? %>
+  <div class="facet-filters card card-body bg-light p-3 mb-3 border-0">
+    <%= render Blacklight::Search::FacetSuggestInput.new(facet: @facet, presenter: @presenter) %>
+    <%= render partial: 'facet_index_navigation' if @facet.index_range && @display_facet.index? %>
+  </div>
+
+  <div class="facet-pagination top d-flex flex-wrap w-100 justify-content-between border-bottom pb-3 mb-3">
+    <%= render :partial=>'facet_pagination' %>
+  </div>
 
   <div class="facet-extended-list">
     <%= render Blacklight::FacetComponent.new(display_facet: @display_facet,
@@ -17,7 +17,7 @@
   </div>
 
   <% component.with_footer do %>
-    <div class="facet-pagination bottom flex-row justify-content-between">
+    <div class="facet-pagination bottom d-flex flex-wrap w-100 justify-content-between">
       <%= render :partial=>'facet_pagination' %>
     </div>
   <% end %>
diff --git a/spec/components/blacklight/search/facet_suggest_input_spec.rb b/spec/components/blacklight/search/facet_suggest_input_spec.rb
@@ -5,9 +5,11 @@
 RSpec.describe Blacklight::Search::FacetSuggestInput, type: :component do
   let(:facet) { Blacklight::Configuration::FacetField.new key: 'language_facet', suggest: true }
   let(:presenter) { instance_double(Blacklight::FacetFieldPresenter) }
+  let(:view_context) { double(ActionView::Base) }
 
   before do
-    allow(presenter).to receive(:label).and_return 'Language'
+    allow(presenter).to receive_messages(label: 'Language', view_context: view_context)
+    allow(view_context).to receive(:search_facet_path).and_return('/catalog/facet/language_facet')
   end
 
   it 'has an input with the facet-suggest class, which the javascript needs to find it' do
@@ -20,6 +22,12 @@
     expect(rendered.css('input[data-facet-field="language_facet"]').count).to eq 1
   end
 
+  it 'has an input with the data-facet-search-context attribute, which the javascript needs to determine the current search context' do
+    allow(view_context).to receive(:search_facet_path).and_return('/catalog/facet/language_facet?f%5Bformat%5D%5B%5D=Book&facet.prefix=R&facet.sort=index&q=tibet&search_field=all_fields')
+    rendered = render_inline(described_class.new(facet: facet, presenter: presenter))
+    expect(rendered.css('input[data-facet-search-context="/catalog/facet/language_facet?f%5Bformat%5D%5B%5D=Book&facet.prefix=R&facet.sort=index&q=tibet&search_field=all_fields"]').count).to eq 1
+  end
+
   it 'has a visible label that is associated with the input' do
     rendered = render_inline(described_class.new(facet: facet, presenter: presenter))
     label = rendered.css('label').first
diff --git a/spec/features/facets_spec.rb b/spec/features/facets_spec.rb
@@ -99,12 +99,15 @@
     end
   end
 
-  describe 'Facet modal' do
-    context 'when configured' do
+  describe 'Facet modal content' do
+    context 'when facet is configured with suggest: true' do
+      # BL8: turn on the suggest feature for testing (off by default)
       before do
-        enabled = CatalogController.blacklight_config.dup
-        enabled.facet_fields[:subject_ssim].merge!({ suggest: true })
-        allow(CatalogController).to receive(:blacklight_config).and_return enabled
+        CatalogController.blacklight_config.facet_fields[:subject_ssim].suggest = true
+      end
+
+      after do
+        CatalogController.blacklight_config.facet_fields[:subject_ssim].delete(:suggest)
       end
 
       it 'allows the user to filter a long list of facet values', :js do
@@ -114,6 +117,7 @@
 
         fill_in 'facet_suggest_subject_ssim', with: "ag"
 
+        expect(page).to have_css '.facet-suggestions'
         expect(page).to have_link 'Old age'
         expect(page).to have_css 'a.facet-select', count: 2
       end
@@ -122,19 +126,68 @@
         visit '/catalog/facet/subject_ssim?q=tibet&search_field=all_fields'
         fill_in 'facet_suggest_subject_ssim', with: 'la'
 
+        expect(page).to have_css '.facet-suggestions'
         expect(page).to have_link 'Tibetan language'
+        expect(page).to have_no_link 'Law'
         expect(page).to have_css 'a.facet-select', count: 1
       end
 
-      it 'allows the user to filter more than once', :js do
+      it 'allows the user to toggle the sort, then filter', :js do
         visit '/catalog/facet/subject_ssim'
-        expect(page).to have_no_link 'Old age' # This is on the second page of facet values
-        expect(page).to have_css 'a.facet-select', count: 20
 
-        fill_in 'facet_suggest_subject_ssim', with: "ag"
+        fill_in 'facet_suggest_subject_ssim', with: 'po'
 
-        expect(page).to have_link 'Old age'
-        expect(page).to have_link('Old age', href: '/?f%5Bsubject_ssim%5D%5B%5D=Old+age')
+        expect(page).to have_css '.facet-suggestions'
+        expect(page).to have_css('.facet-values li:nth-child(1)', text: 'Political plays, Japanese')
+        expect(page).to have_css('.facet-values li:nth-child(2)', text: 'Military weapons')
+        expect(page).to have_css('.facet-values li:nth-child(3)', text: 'Political science')
+
+        first(:link, 'A-Z Sort').click
+
+        expect(page).to have_css('.facet-suggest[data-facet-search-context*="facet.sort=index"]')
+
+        fill_in 'facet_suggest_subject_ssim', with: 'po'
+
+        expect(page).to have_css '.facet-suggestions'
+        expect(page).to have_css('.facet-values li:nth-child(1)', text: 'Military weapons')
+        expect(page).to have_css('.facet-values li:nth-child(2)', text: 'Political plays, Japanese')
+        expect(page).to have_css('.facet-values li:nth-child(3)', text: 'Political science')
+      end
+
+      it 'allows the user to choose a starting letter, then filter', :js do
+        visit '/catalog/facet/subject_ssim'
+
+        first(:link, 'A-Z Sort').click
+        expect(page).to have_css('.facet-suggest[data-facet-search-context*="facet.sort=index"]')
+
+        click_on 'M'
+        expect(page).to have_css('.facet-suggest[data-facet-search-context*="facet.prefix=M"]')
+
+        fill_in 'facet_suggest_subject_ssim', with: 'te'
+
+        expect(page).to have_css '.facet-suggestions'
+        expect(page).to have_link 'Maternity insurance'
+        expect(page).to have_no_link 'Teaching'
+      end
+
+      it 'hides previous/next links when filtering', :js do
+        visit '/catalog/facet/subject_ssim'
+        expect(page).to have_link 'Next »'
+
+        fill_in 'facet_suggest_subject_ssim', with: 'te'
+        expect(page).to have_css '.facet-suggestions'
+        expect(page).to have_no_link 'Next »'
+
+        fill_in 'facet_suggest_subject_ssim', with: ''
+        expect(page).to have_no_css '.facet-suggestions'
+        expect(page).to have_link 'Next »'
+      end
+    end
+
+    context 'when facet is NOT configured with suggest: true' do
+      it 'does not offer the user a way to filter the list of facet values' do
+        visit '/catalog/facet/subject_ssim'
+        expect(page).to have_no_field 'facet_suggest_subject_ssim'
       end
     end
   end
diff --git a/spec/views/catalog/facet.html.erb_spec.rb b/spec/views/catalog/facet.html.erb_spec.rb
@@ -4,11 +4,14 @@
   let(:display_facet) { double }
   let(:blacklight_config) { Blacklight::Configuration.new }
   let(:component) { instance_double(Blacklight::FacetComponent) }
+  let(:facet_suggest_input) { instance_double(Blacklight::Search::FacetSuggestInput) }
 
   before do
     allow(Blacklight::FacetComponent).to receive(:new).and_return(component)
+    allow(Blacklight::Search::FacetSuggestInput).to receive(:new).and_return(facet_suggest_input)
     allow(view).to receive(:render).and_call_original
     allow(view).to receive(:render).with(component)
+    allow(view).to receive(:render).with(facet_suggest_input)
 
     blacklight_config.add_facet_field 'xyz', label: "Facet title"
     allow(view).to receive(:blacklight_config).and_return(blacklight_config)
@@ -22,6 +25,11 @@
     expect(rendered).to have_css 'h1', text: "Facet title"
   end
 
+  it "renders the facet suggest input" do
+    render
+    expect(view).to have_received(:render).with(facet_suggest_input)
+  end
+
   it "renders facet pagination" do
     render
     expect(rendered).to have_content 'pagination'

Original file line number	Diff line number	Diff line change
`@@ -39,4 +39,8 @@ dialog.modal[open] {`
`39`	`39`
`40`	`40`	`max-height: unset; // override user-agent dialog`
`41`	`41`	`max-width: unset; // override user-agent dialog`
	`42`	`+`
	`43`	`+ &::backdrop {`
	`44`	`+ background-color: var(--bl-modal-backdrop-bg);`
	`45`	`+ }`
`42`	`46`	`}`
Original file line number	Diff line number	Diff line change
`@@ -10,7 +10,5 @@`
`10`	`10`	`}`
`11`	`11`
`12`	`12`	`.pagination {`
`13`		`- @media (max-width: breakpoint-max(sm)) {`
`14`		`- flex-wrap: wrap;`
`15`		`- }`
	`13`	`+ flex-wrap: wrap;`
`16`	`14`	`}`
Original file line number	Diff line number	Diff line change
`@@ -20,4 +20,7 @@ $body-secondary-color: rgba($body-color, 0.75) !default;`
`20`	`20`	`--bl-constraint-remove-hover-border-color: #bb2d3b;`
`21`	`21`
`22`	`22`	`--bl-field-name-color: var(--bs-secondary-color);`
	`23`	`+`
	`24`	`+ /* emulate Bootstrap backdrop bg & opacity */`
	`25`	`+ --bl-modal-backdrop-bg: rgba(0, 0, 0, 0.5);`
`23`	`26`	`}`