Using links combined with JavaScript to generate POST requests to the
browser has a few issues.
An obvious one is that it doesn't work for users without JavaScript
enabled (which lately I've noticed are more common than I thought, even
though I've been one of them for years). These users will reach a 404
page.
Since CONSUL isn't currently designed to work without JavaScript
enabled, let's ignore this one for now.
A not so obvious issue is screen reader users might expect the link to
take them somewhere instead of performing an action (in this case,
sending a form to the server).
There might be more issues I'm unaware of. Quoting DHH [1]:
"Turning ahrefs into POSTs is a bit of an anti-pattern, especially for
a11y reasons. Better to use button_to with a styling."
So we're using a button instead. This way we can also simplify the code
and make the button disabled for unidentified users, which automatically
makes it impossible to focus it using the keyboard.
A possible disadvantage of using `button_to` is it will create a form
tag which will be announced to screen readers as a form landmark. I've
tested it with my screen reader and everything worked fine for me, but
some screen reader users might interact with these forms in a different
way and find it confusing, particularly in the case where the button is
disabled.
With this change, we're only changing links for buttons in one place.
There are other places where we should do similar changes.
[1] See issue 33 in https://github.com/hotwired/turbo-rails/
We meant to check that the link to support a different investment didn't
have an alert dialog. However, we were testing by trying to support the
same investment twice, and here there was no link at all, so the
assertion "there's no link with a dialog" always passed even when the
application didn't behave properly.
Now we check there's a link there, and that link doesn't have an
associated alert dialog.
In regular system tests, we prefer testing against the text of the link
because it's easier to recognize which link we're talking about.
I was wondering whether we should remove the `title` attribute
completely and use just the `aria-label` attribute. Quoting The Paciello
Group blog [1]:
"If you want to hide content from mobile and tablet users as well as
assistive tech users and keyboard only users, use the title attribute."
However, there's a case where mouse users might find the title attribute
useful. When visiting an investment page, the connection between the
"Support" link and the investment is not as clear as it is in the
investments index page, so it might not be clear what you're supporting.
As mentioned, though, this information will only be relevant for mouse
users, who nowadays represent less than half of our users.
[1] https://www.tpgi.com/using-the-html-title-attribute-updated/
This way blind screen reader users will know which project they're
supporting. In a list of investments, context might not be clear when a
link saying "Support" or "Support this project" is announced, but a link
saying "Support Renovate sidewalks in Main Street" is less ambiguous.
When identified users accessed the investments page, we were using the
`aria-hidden` attribute to hide this link from screen readers since
unidentified users can't support investments.
However, the link was still focusable using keyboard navigation. This
resulted in screen reader users reaching the link and being able to
click it, but getting no feedback at all about where they were or what
they were clicking on.
This is why the fourth ARIA rule says: "Do not use role="presentation" or
aria-hidden="true" on a focusable element" [1].
So we're replacing the link with a non-interactive element instead, like
we do in other places like proposals.
The accessibility of this part of the page for unidentified users still
has some issues; here we're only improving it up to a certain point.
[1] https://www.w3.org/TR/using-aria/#4thrule
The `js-` prefix (which I admit I'm not fond of) is usually used to
indicate the class is used by JavaScript files, not for using it in test
files. And in all the other similar tests, we're using the `in-favor`
class instead of the `js-in-favor` class.
This reverts commit 83fe74d53.
When supporting an investment in the management section through the
investment view, we were accessing an action in the public investments
controller. This meant the manager was the one supporting the investment
(as they'd be the `current_user` in this controller) and not the managed
user.
In the case of groups with many headings, voting the first time requires
a confirmation and then a regular (non-AJAX) request takes place. In
this case, users were redirected to the public area instead of remaining
in the management area.
Using the proper URL to vote solves the problem.
Note there was a comment about one of these tests failing in Travis.
Most probably the test failed because there was no expectation after
clicking the link with the investment title, so the "Support" button
(which is also present in the index page) was clicked before the
investment view was loaded.
The test was failing sometimes because the conditions we were checking
were the same before and after clicking the "Pending" link. So there was
a race condition between the request generated by clicking the "Pending"
link and the order to click the "Confirm moderation" link. This
sometimes resulted in the "Confirm moderation" link not being correctly
clicked.
The key was declared twice and so the first one ("Support this project")
was overwritten.
We're grouping all keys related to the investment list together in order
to reduce the chances of this issue happening again (or, at least, in
this part of the code).
The color we used offered a contrast of 3.94 against the background. The
minimum requirement for AA level is a contrast of 4.5, and we usually
aim for a contrast of 5 at least.
So we're making the text slightly darker so it's easier to read.
An image without an alt text is invalid HTML and is confusing for screen
reader users.
This is just a quick patch which partially solves this problem. The
image doesn't necessarily need to be a decorative one, so administrators
should have the option to provide an alternative text for the images.
Note one of the tests dealing with random results is a bit flaky; since
it's a permutation selecting 7 objects out of 12, it will fail about
once every 4 million times. We think this is acceptable.
Co-Authored-By: Julian Nicolas Herrero <microweb10@gmail.com>
In the form of creating a new investment was hiding the name of the
group if it had only one heading, but could be confusing to users if
there are, for example, five different groups of one heading.
The solution:
- If the budget has one group and one heading, the heading selector is
hidden.
- If the budget has one group and more than one heading, the group name
is hidden.
- If the budget has more than one group, the group name appears
regardless of the number of headings.
Note we're using an extra `<span>` element but we could use a CSS grid
layout instead. We're not using it because browser compatibility is only
94.56% at the time of writing.
The next warning is flooding the tests log:
`Options passed to Node#set but the RackTest driver
doesn't support any - ignoring`
As the rack test driver does not support any option, we
get rid of all `default_set_options` before each system
spec that uses this driver.
