We were using Foundation's accordion menu to open/close nested lists of
links. Unfortunately, Foundation's accordion makes it impossible to
access links in nested links using the keyboard [1] (note the issue is
closed, but in the latest version of Foundation, 6.8.1, it's still
present, and Foundation's development is mostly discontinued).
Furtheremore, it adds the `menuitem` role to links, but ARIA menus are
not ment for navigation but for application behavior and, since it
doesn't add the `menubar` or `menu` roles to the parent elements, it
results in accessibility issues for people using screen readers (also
reported by the Axe accessibility testing engine).
So we need to implement our own solution. We're using the most commonly
used pattern: a buttton with the `aria-expanded` attribute. And, for
people using browsers where JavaScript hasn't loaded, we're keeping the
submenus open at all times (just like we were doing until now), and
we're disabling the buttons (since they do nothing without JavaScript).
This might not be an ideal solution, but it's probably good enough, and
way better than what we had until now.
We've also considered using the <details> and <summary> elements instead
of using buttons to open/close items on the list. However, these
elements still present some accessibility issues [2], and the transition
between open and closed can't be animated unless we overwrite the
`click` event with JavaScript. The pattern of using these elements to
open/close a nested list of links isn't common either, and some people
using screen readers might get confused when entering/leaving the nested
list.
We tried other approaches to get the animation effect, all of them based
on adding `[aria-expanded="false"]:not([disabled]) + * { display: none;
}` to the CSS file.
Unfortunately, animation using CSS isn't feasible right now because
browsers can't animate a change form `height: 0` to `height: auto`.
There are some hacks like animating the `max-height` or the `flex-grow`
property, but the resulting animation is inconsistent. A perfect
animation can be done using the `grid-template-rows` property [3], but
it requires adding a grid container and only works in Firefox and recent
versions of Chrome and similar browsers.
Getting to a solution with JavaScript was also tricky. With the
following approach, `slideToggle()` opened the menu the first time, even
if it was already open (not sure why):
```
toggle_buttons.on("click", function() {
$(this).attr("aria-expanded", !JSON.parse($(this).attr("aria-expanded")));
$(this).next().slideToggle();
});
```
This made the arrow turn after the menu had slided instead of doing it
at the same time:
```
toggle_buttons.on("click", function() {
var button = $(this);
button.next().slideToggle(function() {
button.attr("aria-expanded",
!JSON.parse(button.attr("aria-expanded")));
});
}
```
With this, everything disappeared quickly:
```
toggle_buttons.on("click", function() {
var expanded = JSON.parse($(this).attr("aria-expanded"));
if (expanded) {
$(this).next().slideUp();
} else {
$(this).next().slideDown();
}
$(this).attr("aria-expanded", !expanded);
}
```
So, in the end, we're hiding the nested link lists with JavaScript
instead of CSS.
[1] Issue 12046 in https://github.com/foundation/foundation-sites
[2] https://www.scottohara.me/blog/2022/09/12/details-summary.html
[3] https://css-tricks.com/css-grid-can-do-auto-height-transitions
This rule was added in rubocop-capybara 2.19.0. We were following it
about 85% of the time.
Now we won't have to check both have_css and have_selector when
searching the code.
We were displaying documents in five places, and in five different ways.
Sometimes with the metadata in parenthesis after the title, sometimes
with the metadata below the title, sometimes without metadata, sometimes
with an icon in front of the document, and sometimes with a separate
link to download the file.
So we're now displaying the same thing everywhere. Not sure whether this
is the best solution, but at least it's consistent.
We aren't unifying the way we display a list of documents, though, since
different sections look pretty different and I'm not sure whether the
same style would look well everywhere.
Note that we're renaming the `document` HTML class in the documents
table to `document-row` so the styles for the `document` class don't
apply here.
We were using a "Download file" link in one place, while in another
place we had an additional column where the name of the document was a
link to download it.
This way we don't have to write `"spec/fixtures/files"` every time.
Note this method isn't included in factories. We could include it like
so:
```
FactoryBot::SyntaxRunner.class_eval do
include ActiveSupport::Testing::FileFixtures
self.file_fixture_path = RSpec.configuration.file_fixture_path
end
```
However, I'm not sure about the possible side effects, and since we only
use attachments in a few factories, there isn't much gain in applying
the monkey-patch.
This way we fix a bug we mentioned in commit 930bb753c which caused
links to documents to be broken when editing their title because the
title was used to generate the URL of the document.
Note we're still using Paperclip to render cached attachments because
this is the only case where we store files with just Paperclip and not
Active Storage.
With Active Storage, we render attachments just like any other resource,
using `polymorphic_path`. Paperclip included the `url` method in the
model; since the model doesn't have access to the request parameters
(like the host), this was inconvenient because it wasn't possible to
generate absolute URLs with Paperclip.
In order to simplify the code and make it similar to the way we used
Paperclip, we're adding a `variant` method accepting the name of a
variant and returning the variant.
These tests were checking the URLs of documents and images pointed to
the URL generated by the `attachment.url` method. In order to do so, we
were running database queries after starting the process running the
browser, which is sometimes causing database inconsistencies when
running the tests.
So I'm simply removing the URL check. The tests are slightly less useful
now, but it isn't like they were 100% right in the first place. After
all, if the `attachment.url` method wasn't working properly, the tests
were still passing.
Links acting like buttons have a few disadvantages.
First, screen readers will announce them as "links". Screen reader users
usually associate links with "things that get you somewhere" and buttons
with "things that perform an action". So when something like "Delete,
link" is announced, they'll probably think this is a link which will
take them to another page where they can delete a record.
Furthermore, the URL of the link for the "destroy" action might be the
same as the URL for the "show" action (only one is accessed with a
DELETE request and the other one with a GET request). That means screen
readers could announce the link like "Delete, visited link", which is
very confusing.
They also won't work when opening links in a new tab, since opening
links in a new tab always results in a GET request to the URL the link
points to.
Finally, submit buttons work without JavaScript enabled, so they'll work
even if the JavaScript in the page hasn't loaded (for whatever reason).
For all these reasons (and probably many more), using a button to send
forms is IMHO superior to using links.
There's one disadvantage, though. Using `button_to` we create a <form>
tag, which means we'll generate invalid HTML if the table is inside
another form. If we run into this issue, we need to use `button_tag`
with a `form` attribute and then generate a form somewhere else inside
the HTML (with `content_for`).
Note we're using `button_to` with a block so it generates a <button>
tag. Using it in a different way the text would result in an <input />
tag, and input elements can't have pseudocontent added via CSS.
The following code could be a starting point to use the `button_tag`
with a `form` attribute. One advantage of this approach is screen
readers wouldn't announce "leaving form" while navigating through these
buttons. However, it doesn't work in Internet Explorer.
```
ERB:
<% content_for(:hidden_content, form_tag(path, form_options) {}) %>
<%= button_tag text, button_options %>
Ruby:
def form_id
path.gsub("/", "_")
end
def form_options
{ id: form_id, method: options[:method] }
end
def button_options
html_options.except(:method).merge(form: form_id)
end
Layout:
<%= content_for :hidden_content %> # Right before the `</body>`
```
JavaScript is used by about 98% of web users, so by testing without it
enabled, we're only testing that the application works for a very
reduced number of users.
We proceeded this way in the past because CONSUL started using Rails 4.2
and truncating the database between JavaScript tests with database
cleaner, which made these tests terribly slow.
When we upgraded to Rails 5.1 and introduced system tests, we started
using database transactions in JavaScript tests, making these tests much
faster. So now we can use JavaScript tests everywhere without critically
slowing down our test suite.
We were repeating the same code over and over (with a few variants) to
setup tests which require an administrator. We can use a tag and
simplify the code.
This rule was added in Rubocop 0.89.0. However, there are some false
positives when we don't use interpolation but simply concatenate in
order to avoid long lines. Even if there weren't false positives, there
are places where we concatenate to emphasize the point that we're adding
a certain character to a text.
We might reconsider this rule in the future, since we generally prefer
interpolation over concatenation.
