Define new helper for tab links, with aria #271
Conversation
Pull Request Test Coverage Report for Build 19311046157Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
** 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.
This moves the manipulation of the tabs to the click action (via a standalone function) rather than in the turbo:frame-render action.
matt-bernhardt
force-pushed
the
use-180
branch
from
1736278 to
f35d4ca
Compare
JPrevost
left a comment
JPrevost
left a comment
There was a problem hiding this comment.
I've added comments to three changes that don't seem necessary. They work fine, but when I revert the CSS to what it was and remove the manual spinner class manipulation I literally see no difference locally. I'm curious if I'm missing something or if this is preferred for some reason?
| currentTabLink.classList.add('active'); | ||
| } | ||
| // Remove the spinner now that things are ready | ||
| document.getElementById('search-results').classList.remove('spinner'); |
There was a problem hiding this comment.
I think the manual addition/removal adds complexity without improving the outcome in a meaningful way. Would you mind either clarifying why this is better or changing it back?
There was a problem hiding this comment.
Let me test without this change - my thinking initially was the spinner wasn't appearing early enough - which seemed to be the case when I simulated low-bandwidth connections working locally.
I'll re-check now that everything is in place.
There was a problem hiding this comment.
Ok, I've set up a comparison to show the behavior I was noticing. This applies to two review apps:
- The review app for this PR, which has had the spinner CSS class reverted back to the prior state, and keys off commit
b261749, can be seen at https://timdex-ui-pi-use-180-tee54nbsx.herokuapp.com/results?booleanType=AND&q=archives&tab=primo - A comparison review app, which still has the spinner defined in a CSS class and added/removed manually. This app keys off commit
f35d4ca, which just prior to the tip of this PR. This can be seen at https://timdex-ui-pi-use-180-or-ixjq7j.herokuapp.com/results?booleanType=AND&q=archives&tab=primo
Both apps use the "simulate search latency" feature, but there's another component to this - simulating slow network connections via the browser. The test sequence is:
- In your browser, set a network throttle via dev tools (Firefox allows me to go all the way down to 50 Kbps, although the behavior I can see emerges as a flicker at 4G speeds. It is quite pronounced at 2G speeds).
- Load a page of resuts, similar to the URL above.
- Flip back and forth between tabs - I think it might become more pronounced after doing this a few times, but I can't say that for sure. What you're looking for is that the spinner gets removed just before the tab content actually changes. When the browser throttle is particularly pronounced (i.e. the user has very slow internet connection), there can be a seconds-long gap between the spinner disappearing and the new tab content becoming visible.
I've found that once I've noticed this stutter, I find that I can notice it even on faster connections - but the stutter is minimal / non-existent if we remove the spinner manually as in the other review app.
I may be overthinking this - I like the approach of letting Turbo Frames control the spinner - but having seen that there are cases where the spinner doesn't get removed at the right moment, I worry about inducing confusion when a user starts to scroll after the spinner disappears, only to have a new batch of results appear after a moment.
| const newTab = clickedParams.get('tab'); | ||
|
|
||
| // Throw the spinner on the search results immediately | ||
| document.getElementById('search-results').classList.add('spinner'); |
There was a problem hiding this comment.
I think the manual addition/removal adds complexity without improving the outcome in a meaningful way. Would you mind either clarifying why this is better or changing it back?
|
|
||
| // Pagination overlay when loading | ||
| [busy]:not([no-spinner]) { | ||
| .spinner { |
There was a problem hiding this comment.
I think the manual addition/removal adds complexity without improving the outcome in a meaningful way. Would you mind either clarifying why this is better or changing it back?
| @@ -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) { | |||
There was a problem hiding this comment.
Great call consolidating this logic into a function
| end | ||
| end | ||
|
|
||
| def link_to_tab(target) |
There was a problem hiding this comment.
Much cleaner in the view now and easier to test. ❤️
|
@matt-bernhardt I concur with your assessment with simulated low bandwidth devices. Please add your fix back in and we can move towards merge. |
|
@JPrevost I've restored the .spinner class and explicit handling |
This makes a pair of connected changes which focus on the process of switching tabs to see different sets of search results:
loading-spinner.jsfor updates after the initial page load.turbo:frame-renderevent. We also immediately update the tabs' display as part of the same click handling sequence. The results panel's loading spinner is then removed when the new frame content renders.This adds two tests for the helper function (but nothing for the javascript side of things).
I've tested this behavior in Firefox, Chrome, and Safari on a Mac, and using Firefox, Chrome, and Edge on a Windows 11 box. This sequence seems to work as desired on a visual basis. I've also checked the behavior with WAVE and ANDI, and listened to the interaction with VoiceOver. I've got some questions there which I'll share with Darcy for confirmation during our formal a11y testing.
Dave has confirmed that the behavior in the review app is what he was intending.
Ticket
How to see this behavior
The review app for this PR has all the changes, as well as the feature flag enabled that simulates slower API responses - so you've got a chance to see the spinner in action (thanks @JPrevost )
Side effects / notes:
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.
Developer
Accessibility
New ENV
Approval beyond code review
Additional context needed to review
E.g., if the PR includes updated dependencies and/or data
migration, or how to confirm the feature is working.
Code Reviewer
Code
added technical debt.
Documentation
(not just this pull request message).
Testing