From e69f1fd0944697d8a62886451f8704d3e5401553 Mon Sep 17 00:00:00 2001 From: Matthew Bernhardt Date: Mon, 10 Nov 2025 17:45:34 -0500 Subject: [PATCH 1/3] Define new helper for tab links, with aria ** Why are these changes being introduced: We want to add an `aria-current="page"` attribute to the currently active tab link on a results page, so that assistive technologies can parse which is the active tab. This requires a conditional link_to call, as well as an update to the javascript which handles tab changes. ** Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/use-180 ** How does this address that need: Because we can't (apparently) write a conditional inside an argument to a link_to call, we have to maintain the conditional check elsewhere - so to prevent the source_tabs partial from becoming a mess of multiple conditionas, we define a new search helper function, link_to_tab. This takes a string as an argument, and is capable of outputting both a link to the active tab, as well as to an inactive tab. We also update loading_spinner.js to be able to maintain the state of the aria-current attribute. ** Document any side effects to this change: The tests for this helper function _should_ be robust enough, but I worry that they're a bit too clever. I didn't want to just write a string comparison assertion, because that feels very fragile given how much this partial has been changing lately. The assertions as written should focus on the values being present or not, but still be robust to other changes. Also note that, for now, the source_tabs partial is still a bit bespoke in that there are explicitly two calls to the various tabs. I'd originally intended to define a list in the controller, but managing the list of tabs in this way feels awkward and like it doesn't gain anything since the tabs are entwined with so much of the rest of the application. Abstracting the tab order and contents to a list doesn't save us from other complexity. --- app/helpers/search_helper.rb | 13 +++++++++++++ app/javascript/loading_spinner.js | 8 +++++--- app/views/search/_source_tabs.html.erb | 10 ++-------- test/helpers/search_helper_test.rb | 21 +++++++++++++++++++++ 4 files changed, 41 insertions(+), 11 deletions(-) diff --git a/app/helpers/search_helper.rb b/app/helpers/search_helper.rb index d638c769..b8f4ea34 100644 --- a/app/helpers/search_helper.rb +++ b/app/helpers/search_helper.rb @@ -22,6 +22,19 @@ def link_to_result(result) end end + def link_to_tab(target) + if @active_tab == target.downcase + link_to target, results_path(params.permit(:q, :per_page, :booleanType, :tab).merge(tab: target.downcase)), + aria: { current: "page" }, + class: "active tab-link", + data: { turbo_frame: "search-results", turbo_action: "advance" } + else + link_to target, results_path(params.permit(:q, :per_page, :booleanType, :tab).merge(tab: target.downcase)), + class: "tab-link", + data: { turbo_frame: "search-results", turbo_action: "advance" } + end + end + def view_online(result) return unless result[:source_link].present? diff --git a/app/javascript/loading_spinner.js b/app/javascript/loading_spinner.js index 1c35c393..e734b880 100644 --- a/app/javascript/loading_spinner.js +++ b/app/javascript/loading_spinner.js @@ -23,15 +23,17 @@ document.addEventListener('turbo:frame-render', function(event) { // console.log(`Updated tab input value to: ${queryParam}`); } - // update active tab styling - // remove active class from all tabs + // update tab links to reflect new state. This is a two-step process: + // 1. Reset all tabs to base condition document.querySelectorAll('.tab-link').forEach((tab) => { tab.classList.remove('active'); + tab.removeAttribute('aria-current'); }); - // add active class to current tab + // 2. Add "active" class and aria-current attribute to the newly-active tab link const currentTabLink = document.querySelector(`.tab-link[href*="tab=${queryParam}"]`); if (currentTabLink) { currentTabLink.classList.add('active'); + currentTabLink.setAttribute('aria-current', 'page'); } // Clear the pending action diff --git a/app/views/search/_source_tabs.html.erb b/app/views/search/_source_tabs.html.erb index 7f02c4cf..0e2978d0 100644 --- a/app/views/search/_source_tabs.html.erb +++ b/app/views/search/_source_tabs.html.erb @@ -1,14 +1,8 @@ \ No newline at end of file diff --git a/test/helpers/search_helper_test.rb b/test/helpers/search_helper_test.rb index ccb83f6a..3ee91aac 100644 --- a/test/helpers/search_helper_test.rb +++ b/test/helpers/search_helper_test.rb @@ -199,6 +199,27 @@ class SearchHelperTest < ActionView::TestCase assert_equal 'Sample Document Title', link_to_result(result) end + test 'link_to_tab builds a link without an aria attribute when that tab is not active' do + @active_tab = 'bar' + actual = link_to_tab('Foo') + + assert_select Nokogiri::HTML::Document.parse( actual ), 'a' do |link| + assert_select '[class*=?]', 'active', count: 0 + assert_select '[class*=?]', 'tab-link' + assert_select '[aria-current=?]', 'page', count: 0 + end + end + + test 'link_to_tab builds a link that includes an aria attribute when that tab is active' do + @active_tab = 'foo' + actual = link_to_tab('Foo') + + assert_select Nokogiri::HTML::Document.parse( actual ), 'a' do |link| + assert_select link, '[class*=?]', 'active' + assert_select link, '[aria-current=?]', 'page', count: 1 + end + end + test 'primo_search_url generates correct Primo URL' do query = 'machine learning' expected_url = 'https://mit.primo.exlibrisgroup.com/discovery/search?query=any%2Ccontains%2Cmachine+learning&vid=01MIT_INST%3AMIT' From 200128753ef2888e17487455652a0b0dbdfd61e6 Mon Sep 17 00:00:00 2001 From: Matthew Bernhardt Date: Wed, 12 Nov 2025 11:16:01 -0500 Subject: [PATCH 2/3] Move spinner control to our javascript --- app/assets/stylesheets/partials/_loading_spinner.scss | 2 +- app/javascript/loading_spinner.js | 6 ++++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/app/assets/stylesheets/partials/_loading_spinner.scss b/app/assets/stylesheets/partials/_loading_spinner.scss index 8439a413..a7ad6dcd 100644 --- a/app/assets/stylesheets/partials/_loading_spinner.scss +++ b/app/assets/stylesheets/partials/_loading_spinner.scss @@ -8,7 +8,7 @@ } // Pagination overlay when loading -[busy]:not([no-spinner]) { +.spinner { position: relative; min-height: 400px; display: block; diff --git a/app/javascript/loading_spinner.js b/app/javascript/loading_spinner.js index e734b880..01a55b83 100644 --- a/app/javascript/loading_spinner.js +++ b/app/javascript/loading_spinner.js @@ -35,6 +35,8 @@ document.addEventListener('turbo:frame-render', function(event) { currentTabLink.classList.add('active'); currentTabLink.setAttribute('aria-current', 'page'); } + // Remove the spinner now that things are ready + document.getElementById('search-results').classList.remove('spinner'); // Clear the pending action window.pendingFocusAction = null; @@ -53,6 +55,10 @@ document.addEventListener('click', function(event) { // Handle tab clicks if (clickedElement.closest('.tab-navigation')) { + // Throw the spinner on the search results immediately + document.getElementById('search-results').classList.add('spinner'); + + // Position the window at the top of the results window.scrollTo({ top: 0, behavior: 'smooth' }); window.pendingFocusAction = 'tab'; } From f35d4ca8d310957dad649ea9a2b737ebab5263c5 Mon Sep 17 00:00:00 2001 From: Matthew Bernhardt Date: Wed, 12 Nov 2025 11:17:12 -0500 Subject: [PATCH 3/3] Eagerly update tab UI This moves the manipulation of the tabs to the click action (via a standalone function) rather than in the turbo:frame-render action. --- app/javascript/loading_spinner.js | 34 ++++++++++++++++++++----------- 1 file changed, 22 insertions(+), 12 deletions(-) diff --git a/app/javascript/loading_spinner.js b/app/javascript/loading_spinner.js index 01a55b83..98186dc4 100644 --- a/app/javascript/loading_spinner.js +++ b/app/javascript/loading_spinner.js @@ -1,3 +1,19 @@ +// Update the tab UI to reflect the newly-requested state. This function is called +// by a click event handler in the tab UI. It follows a two-step process: +function swapTabs(new_target) { + // 1. Reset all tabs to base condition + document.querySelectorAll('.tab-link').forEach((tab) => { + tab.classList.remove('active'); + tab.removeAttribute('aria-current'); + }); + // 2. Add "active" class and aria-current attribute to the newly-active tab link + const currentTabLink = document.querySelector(`.tab-link[href*="tab=${new_target}"]`); + if (currentTabLink) { + currentTabLink.classList.add('active'); + currentTabLink.setAttribute('aria-current', 'page'); + } +} + // Loading spinner behavior for pagination (Turbo Frame updates) document.addEventListener('turbo:frame-render', function(event) { if (window.pendingFocusAction === 'pagination') { @@ -23,18 +39,6 @@ document.addEventListener('turbo:frame-render', function(event) { // console.log(`Updated tab input value to: ${queryParam}`); } - // update tab links to reflect new state. This is a two-step process: - // 1. Reset all tabs to base condition - document.querySelectorAll('.tab-link').forEach((tab) => { - tab.classList.remove('active'); - tab.removeAttribute('aria-current'); - }); - // 2. Add "active" class and aria-current attribute to the newly-active tab link - const currentTabLink = document.querySelector(`.tab-link[href*="tab=${queryParam}"]`); - if (currentTabLink) { - currentTabLink.classList.add('active'); - currentTabLink.setAttribute('aria-current', 'page'); - } // Remove the spinner now that things are ready document.getElementById('search-results').classList.remove('spinner'); @@ -55,11 +59,17 @@ document.addEventListener('click', function(event) { // Handle tab clicks if (clickedElement.closest('.tab-navigation')) { + const clickedParams = new URLSearchParams(clickedElement.search); + const newTab = clickedParams.get('tab'); + // Throw the spinner on the search results immediately document.getElementById('search-results').classList.add('spinner'); // Position the window at the top of the results window.scrollTo({ top: 0, behavior: 'smooth' }); + + swapTabs(newTab); + window.pendingFocusAction = 'tab'; } });