We were only showing these actions to users with small screens and to
mouse users on hover. Keyboard users or users with a touch screen of a
medium or large size could never find out the actions were there.
The button looked just like regular text and it wasn't obvious that it
was an interactive element.
So we're styling it like the "Hide" link (which should actually be a
button, by the way).
Note buttons by default don't have a `cursor: pointer` property because
they're usually styled in a way that makes it obvious they're
interactive elements. Since that isn't the case here, we're adding that
extra hint for mouse users.
Also note all these links/buttons will look like regular text to color-
blind users. And if they use a touchscreen, they won't be able to hover
on them to see the cursor change. We might need to change these elements
in the future.
It didn't have a `for` attribute and so it wasn't correctly associated
with its input. That means clicking on / touching the label didn't have
the effect of focusing on the field, and screen readers wouldn't
announce the label.
The button was announced as expanded when the form was hidden and as
collapsed when the form was shown.
This is because Foundation sets the expanded attribute based on whether
the class to toggle already exists. Since initially the form had the
"hide" class and the button toggled that class, Foundation checked that
the class was already present and so set the button as expanded.
So we're changing the toggler class for a class we don't use at all,
just so Foundation initiall sets `aria-expanded=false` and then changes
it to `aria-expanded=true` after the button is clicked. Then we're
ignoring this class completely and are styling this form with CSS
instead.
We could also use a toggler class like "visible" and write something
like:
```
.add-related-content + form:not(.visible) {
display: none;
}
```
However, using ARIA attributes is more robust as it guarantees the
styles will always be in sync with what screen reader users experience.
And we could also remove all the Foundation toggler functionality and
use our own JavaScript to handle the button state. We might do so in the
future.
We're removing the parenthesis after page expectations (as we do almost
everywhere) and we're replacing `have_selector` with `have_css`, since
on that file we were using `have_css` everywhere except in one place.
This way the relationship between the two elements is more obvious. And
since now the button and the form are siblings, it's easier to find one
based on the other using CSS or JavaScript.
We were using a placeholder to indicate content which should be part of
a label.
It might be better to actually use "Link address" as a label instead of
"URL". I'm using "URL" because it's used in other places in the admin
section.
We were using a paragraph before the field, and then a field with no
label and a redundant placeholder. This causes accessibility issues for
screen reader users, who will not hear the label being announced when
entering the field, and to users who click on the label expecting it to
automatically focus on its related field (which is the standard browser
behavior).
Using placeholders having similar (or identical) text as already present
as a label has a few issues.
First, it's a distraction. Reading the same information twice is
useless, requires an extra effort, and might even frustrate users.
Second, if users start typing before reading the placeholder and see it
disappear, they might think they're missing relevant information,
delete what they typed, and read the placeholder. That will get them
nowhere.
Finally, we display placeholders using a text offering very low contrast
against the background, so users don't think the placeholder is an
actual value entered in the field. Using such low contrast makes the
text hard to read, particularly for users with visual impairments.
So we're removing these placeholders.
This commit only deals with placeholder texts with similar (or
identical) texts as the label text. There might be other places where we
should replace placeholder texts with labels, but that's a different
topic.
A button cannot be inside an anchor tag, and it might confuse some
browsers or screen readers.
We're also making it clear in the tests that the intention is to use a
button there by using `click_button` instead of `click_on` since the
latter also clicks on links.
While debugging JavaScript is certainly useful, enabling it generates
about 100 extra HTTP requests because we include about 100 JavaScript
files (including external dependencies and files written by us).
Depending on the browser configuration, this might make the browser take
a very long time processing these requests.
On my machine, with these changes, refreshing a page on Firefox takes
about 1 second, while previously it took about 6-8 seconds. With
Chromium, there doesn't seem to be much difference.
Developers are encouraged to temporarily turn debugging while debugging
JavaScript (which is a task I personally do about once a month) if that
makes debugging easier for them.
This change doesn't affect our CSS files, since for CSS we use Sass
instead of the asset pipeline. Sass already compiles all CSS files into
one in the development environment.
We were using an `@extend` selector inside a `@supports` condition,
which didn't generate the `@supports` clause as we intended to, so
browsers with no support for mask images were still applying properties
which were meant for browsers with support for mask images.
Nowadays there are many users with a screen of 1920px (or higher) width
but who don't change their font-size preferences and keep them at 16px,
which is the default in most browsers.
On pages which use the whole window width, that results in unreasonably
long lines which are very hard to read. That's the main reason why many
sites (including CONSUL) use rules like `max-width: 75rem` for the body
or other elements.
However, on extra large screens this causes the content to be in the
middle of the screen while most of the screen is empty, and the text
might also be too small for that resolution, making it harder to read.
There are a few approaches to solve this problem.
Using just viewport units like `font-size: calc(4vw + 4vh + 2vmin);` is
indeed responsive, but it's got an important flaw: it ignores user
preferences, and so the font size will be the same for users who prefer
a 16px size and for users who prefer a 32px size.
Using `calc(1em + 0.2vw)` or similar might make the text too big on
small screens.
Finally, using `max(1em, 1vw)` would work in a similar way to what we're
doing, but zooming in and out would only work when the viewport width is
less than 100em (at that moment, 1em == 1vw).
So we're taking an approach where zooming in and out always works at
least to a certain degree (due to the 0.25em part) and the font size is
increased gradually when we reach the point where the viewport width is
bigger than 100em. We're using 0.25em since it will be exactly 4px with
the default browser configuration, and so calculations are easier than
with (for example) 0.3em.
Disclaimer: I've tested this feature on several devices with different
screen sizes and resolutions, but I must admit there might be cases I'm
unaware of where there are side effects for certain combinations of
screen size/resolution/dpi. In general, though, the side effects should
be similar to what happens when users increase the font size in their
browser's preferences.
Note we're using `Max` instead of `max` because Sass can't handle the
CSS `max` function, as mentioned in commit cdfa23fc6.
When the "Or fill the following form" text was translated to another
language or customized by administrators, the text could span over two
lines. Since the element had a fixed height, it could overlap with the
text below it.
So now we're using an element with a relative position to achieve the
same effect (have the contents of the elements on top of its border).
This test will fail if the ID of the created record is the same as the
ID of the first user we create in the test. The chance is very low due
to the `rand(9999999)` which causes the test to fail just once every ten
million times. However, why assigning the ID in the first place? Without
it, the test will never fail due to conflicting IDs.
The test "updates officer_assignment_id_log if amount changes" failed
when the ID we assigned when creating the records was the same as the ID
of the first officer assignment created during that test. It's recently
happened while running our test suite [1] with the following error:
```
1) Poll::Recount logging changes updates officer_assignment_id_log if
amount changes
Failure/Error: poll_recount.officer_assignment =
create(:poll_officer_assignment, id: 101)
ActiveRecord::RecordNotUnique:
PG::UniqueViolation: ERROR: duplicate key value violates unique
constraint "poll_officer_assignments_pkey"
DETAIL: Key (id)=(101) already exists.
```
We're also removing the IDs in the "updates officer_assignment_id_log if
amount changes" test to avoid any possible issues, even if in this test
I think no other officer assignments are created and so there can't be
another record with the same ID.
[1] https://github.com/consul/consul/runs/2818889296
We've seen a few CONSUL repositories where compiled assets have
accidentally been added to version control. It's pretty easy if you use
something like `git add .` before creating a commit and you've compiled
the assets locally.
Having these assets in version control doesn't help and in certain
environments it might even have side effects. For instance, we might try
updating the source code of a Sass file and might wonder why these
changes are ignored in some test or development environments.
Using pixels to define font sizes has an important problem: it ignores
user settings. No matter whether users set their font size to 16px (the
default font size in most browsers), to 18px (like I do) or to 32 px
(like users with huge screens or with a visual disability); the size
will not change.
Even if most browsers can zoom to somehow overcome this issue, it's
still annoying. And, in our case, we use relative units most of the time
but absolute units in some places. This leads to situations where some
of the text gets larger when users increase their font size while some
of the text remains the same. Sometimes this results in titles having a
smaller size than regular text below it.
The solution is using relative units everywhere. Quoting the Web
Accessibility Initiative guide for styles [1]:
> The user needs to be able to resize the text to 200% of its size
> anywhere on the page, without the text being cut off or overlapping
> other text. The font size should be defined in relative units, such as
> percentages, em or rem. It is not possible to zoom text set in pixels
> independently from the rest of the page in some browsers.
[1] https://www.w3.org/WAI/tutorials/page-structure/styling/
When administrators disabled features and users tried to access them
with the browser, we were responding with a 500 "Internal Server Error"
page, which in my humble opinion was incorrect. There was no error at
all; the server worked exactly as expected.
I think a 403 "Forbidden" code is better; since that feature is
disabled, we're refusing to let users access it.
We could also respond with a 404 "Not found", although I wonder whether
that'll be confusing when administrators temporarily or accidentally
disable a feature.
A similar thing might happen if we responded with a 410 "Gone" code.
Actually this case might be more confusing since users aren't that
familiar with this code.
In any case, all these options are better than the 500 error.
Note we don't cast negative votes when users remove their support. That
way we provide compatibility for institutions who have implemented real
negative votes (in case there are / will be any), and we also keep the
database meaningful: it's not that users downvoted something; they
simply removed their upvote.
Co-Authored-By: Javi Martín <javim@elretirao.net>
Co-Authored-By: Julian Nicolas Herrero <microweb10@gmail.com>
This way it's easier to find the keys: keys related to the
`Budgets::Investments::VotesComponent` class are in the
`budgets.investments.votes` namespace.
We're making a couple of exceptions; we're not modifying the `supports`
nor the `support_title` keys because they're used in other places.
Since we're going to add an action to remove supports, having a separate
controller makes things easier.
Note there was a strange piece of code which assumed users were not
verified if they couldn't vote investments. Now the code is also
strange, since it assumes users are not verified if they can't create
votes. We might need to revisit these conditions if our logic changes in
the future.
It was hard to notice what was going on when supporting one investment
which was at the bottom of the investment index page.
I wonder whether we should add the title of the investment to this text;
I'm not doing so because we don't do that anywhere else.
In the previous commit I mentioned:
> If I'm right, the `investment_votes` instance variable only exists to
> avoid several database queries to get whether the current user has
> supported each of the investments.
>
> However, that doesn't make much sense when only one investment is
> shown.
Now let's discuss the case when there are several investments, like in
the investments index:
* There are 10 investments per page by default
* Each query takes less than a millisecond
* We still make a query per investment to check whether the current user
voted in a different group
* AFAIK, there have been no performance tests showing these
optimizations make the request to the investments index significantly
faster
* These optimizations make the code way more complex than it is without
them
Considering all these points, I'm removing the optimizations. I'm fine
with adding `includes` calls to preload records and avoid N+1 queries
even if there are no performance tests showing they make the application
faster because the effect on the code complexity is negligible. But
that's not the case here.
Note we're using `defined?` instead of the `||=` operator because the
`||=` operator will not serve its purpose when the result of the
operation returns `false`.
If I'm right, the `investment_votes` instance variable only exists to
avoid several database queries to get whether the current user has
supported each of the investments.
However, that doesn't make much sense when only one investment is shown.
In this case, the number of queries stays the same, and so we can
simplify the code by rendering the component with an optional parameter.
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/
