Axe was reporting an accessibility error:
```
Found 1 accessibility violation:
1) aria-command-name: ARIA commands must have an accessible name
(serious)
https://dequeuniversity.com/rules/axe/4.11/aria-command-name?application=axeAPI
The following 1 node violate this rule:
Selector: .leaflet-marker-icon
HTML: <div class="leaflet-marker-icon map-marker
leaflet-zoom-animated leaflet-interactive"
tabindex="0" role="button">
<div class="map-icon"></div>
</div>
Fix any of the following:
- Element does not have text that is visible to screen readers
- aria-label attribute does not exist or is empty
- aria-labelledby attribute does not exist, references elements
that do not exist or references elements that are empty
- Element has no title attribute
```
Using the title of the proposal/investment as the text of the marker is
definitely a good solution when there are several markers on the map.
Not sure whether there's a better option when there's only one marker,
though.
Note that we aren't providing a proper aria-label for markers on the map
we use in the form to create a proposal or an investment. Adding one
isn't trivial given the current code, and keyboard users can't add a
marker in the first place. We'll have to revisit this issue when we add
keyboard support for this.
We're also changing a test to make sure that titles with quotes in their
names don't break the markup due to an invalid aria-label attribute.
People using screen readers might have a hard time knowing what a
progressbar is about unless we provide a label for it. Axe was reporting
failures like:
```
aria-progressbar-name: ARIA progressbar nodes must have an accessible
name (serious)
https://dequeuniversity.com/rules/axe/4.10/aria-progressbar-name?application=axeAPI
The following 1 node violate this rule:
Selector: .progress
HTML: <div class="progress" role="progressbar" tabindex="0"
aria-valuenow="0.0" aria-valuemin="0" aria-valuemax="100">
<div class="progress-meter" style="width: 0.0%"></div>
</div>
Fix any of the following:
- aria-label attribute does not exist or is empty
- aria-labelledby attribute does not exist, references
elements that do not exist or references elements that are empty
- Element has no title attribute
```
Note that, in the case of the ballot progressbar, it's easier to use
`aria-labelledby`, while in other place it's easier to use `aria-label`,
so we using the easier solution in each scenario.
All the expectations checked after the `click_link "Check my votes"`
action were already true before clicking the link, meaning the test
could finish before the request did.
It's possible that this request caused a test run 8274, job 2 [1], since
a multitenancy test failed and a possible cause could have been
simultaneous requests to both a tenant subdomain and the application's
main domain. The failure was:
```
1) Multitenancy PostgreSQL extensions work for tenants
Failure/Error: expect(page).to have_content "Proposal created
successfully."
expected to find text "Proposal created successfully." in
"Language: \n
\nEnglish\nDeutsch\nEspañol\nFrançais\nNederlands\nPortuguês
brasileiro\n中文\n Notifications\nMy content\nMy account\nSign
out\nDebates\nYou are in\nProposals\nVoting\nCollaborative
legislation\nParticipatory budgeting\nSDG\nHelp\nProposals\nCreate new
proposal\nHow do citizen proposals work?\nRecommendations for creating a
proposal\nDo not use capital letters for the proposal title or for whole
sentences. On the internet, this is considered shouting. And nobody
likes being shouted at.\nAny proposal or comment suggesting illegal
action will be deleted, as well as those intending to sabotage the
debate spaces. Anything else is allowed.\nEnjoy this space and the
voices that fill it. It belongs to you too.\n×\n1 error prevented this
Proposal from being saved.\nPlease check the marked fields to know how
to correct them:\nREQUIRED FIELDS\nProposal title\nProposal
summary\n(maximum 200 characters)\ntsvector for María the
Martian\nProposal text\n Format\n ◢\n If you are human, ignore
this field\nOPTIONAL FIELDS\nExternal video URL\nYou may add a link to
YouTube or Vimeo\nDescriptive image\nYou can upload one image of
following content types: jpg, up to 1 MB.\nAdd image\nDocuments\nYou can
upload up to a maximum of 3 documents of following content types: pdf,
up to 3 MB per file.\nAdd new document\nTags\nTag this proposal. You can
choose from proposed categories or add your own\nCategories\n\nFull name
of the person submitting the proposal\n(individually or as
representative of a collective; will not be displayed publically)\ncan't
be blank, is too short (minimum is 6 characters)\nSustainable
Development Goals and Targets\nYou can choose one or several SDGs
aligned with your citizen proposal\nGoals and Targets\nYou can introduce
the code of a specific goal/target or a text to find one. For more
information visit the SDG help page.\nI agree to the Privacy Policy and
the Terms and conditions of use\nOpen government\nThis portal uses the
CONSUL DEMOCRACY application which is open-source
software.\nParticipation\nDecide how to shape the city you want to live
in.\nCONSUL DEMOCRACY, 2024 Privacy Policy Terms and conditions of use
Accessibility"
```
Note the `can't be blank, is too short` reference to the responsible
name, which is only checked when user verification is not skipped. In
this test, the `mars` tenant skips the verification while the default
tenant does not. The mentioned possibility of simultaneous requests
might have caused the issue.
[1] https://github.com/consuldemocracy/consuldemocracy/actions/runs/11747689680/job/32730131655
Even after disabling the turbolinks previews in the previous commit
(which is still necessary, even with the changes in this commit), these
tests were still finishing before the "Go back" requests did. To
reproduce an issue caused by this behavior, run:
```
rspec spec/system/budgets/ballots_spec.rb:425 spec/system/users_auth_spec.rb:701 --seed 40358
```
Apparently, a `have_current_path` expectation isn't enough to check that
the request has finished and it only checks that the request to that
path has started or it's being processed.
Adding an additional expectation to check that the content of the page
has changed solves the issue.
Note we're excluding a few files:
* Configuration files that weren't generated by us
* Migration files that weren't generated by us
* The Gemfile, since it includes an important comment that must be on
the same line as the gem declaration
* The Budget::Stats class, since the heading statistics are a mess and
having shorter lines would require a lot of refactoring
We were rendering the whole sidebar again, which wasn't necessary since
most of it remains unchanged. This resulted in complicated code to pass
the necessary information to render the same map that was already
rendered. Furthermore, when users had been moving the map around or
zooming out, we were resetting the map to its default settings after
voting, which was potentially annoying.
This also fixes the wrong investments being displayed in the map after
voting; only the investments on the current page were displayed in this
case while the index displayed all of them.
When voting investment projects, the sidebar was rendered without the
`@heading_content_blocks` being set. That resulted in a 500 error when
the heading had content blocks.
By extracting the logic to a component, we make sure the heading content
blocks are properly set every time this code is rendered, no matter
which controller is rendering the view.
Defining a behavior on hover means making it different for people using
a keyboard or a touchscreen (most of the population, nowadays).
In this case, we had an accessibility issue where the message wouldn't
disappear once it appeared. That meant that, after tabbing through all
the links and buttons in, for instance, the debates index, the page
would be filled with "participation not allowed" messages, and in order
to see the information about how many people have voted, reloading the
page was required.
For touchscreen users the behavior was similar to what we get on hover,
although we've found some inconsistencies when trying to support several
elements on the same page.
We think in proposals it makes sense to hide the "support" button when
users click on it, and the same applies to the buttonsto support and
vote investment projects. However, we aren't hiding the buttons to
agree/disagree with a debate in order to keep the information about the
current number of people agreeing and disagreeing visible.
Note we're removing some support spec methods because after these
changes the duplication isn't as obvious as it was in the past.
As mentioned in commits 5311daadf and bb958daf0, using links combined
with JavaScript to generate POST requests to the server has a few
issues.
We're also improving the keyboard access. Previously, the links were
focusable and clickable with the keyboard. Now we're disabling the
buttons when voting isn't allowed.
Since these elements can no longer be focused, we're adding an element
with `tabindex="0"` so the "participation not allowed" message is shown,
like we do in most places.
Note we're slightly changing one test because now when hovering over the
button on Chrome, the "participation not allowed" text isn't shown; it's
only shown when hovering on the parts of the `div.ballot` element
outside the button. Since we're already rewriting the behavior of the
"participation not allowed" text in a different pull request, we aren't
going to fix this behavior.
We only need to define one `in_browser`, which is the one opening the
session as an administrator.
This change is done to simplify the code, although there's a small
chance it might also make the test stop failing in our CI. Sometimes in
our CI the first `visit` in the `in_browser(:admin)` block fails for
unknown reasons, rendering a blank page.
Even after the previous changes, this test is still failing sometimes
(although now it fails for a different reason). We're doing this change
in order to discard it as the reason why the test is failing.
There seems to be an issue with capybara or chromedriver which results
in `fill_in` sometimes appending to an input rather than overwriting
[1], causing some tests to fail under certain circumstances.
Clearing fields before filling them in solves the issue.
Note we're now getting warnings on all tests using the rack driver. I
haven't found a way to avoid the `clear: :backspace` option in
non-JavaScript tests, so to avoid the annoying warnings we should reduce
the number of tests using the rack driver even more.
[1] See issue 2419 in https://github.com/teamcapybara/capybara/issues
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.
System tests are used to test the application from the user's point of
view. To test for specific exceptions, particularly regarding
authorization permissions, controller tests fit better.
Another option would be to test the page displayed shows a certain text,
like "Internal server error". I'm choosing controller tests because
they're faster and we're basically testing the same scenario many times
and we've already got a test checking what happens when users access a
page raising an exception.
Note we're absolutely positioning the link instead of the icon; that way
keyboard users will be able to focus on the icon. Until now, users would
focus on an empty link.
For the same reason, we couldn't use `click_link` with Capybara, since
it would fail to click an empty link. Now we can.
Co-authored-by: Javi Martín <javim@elretirao.net>
Since the `@ballot_referer` variable was only set in the lines
controller, it didn't work when we accessed the ballot page without
adding a line.
Note it still doesn't work if we access the ballot page directly by
entering the URL in the browser's address bar.
There was a big difference between the current budget and a specific
budget landing page. This didn't really make too much sense. Also, it
was not possible to know how a draft participatory budget will look
before it was published.
By unifying those two views now they will look quite similar and it
will be possible for administrators to preview any draft budget and to
know how the budget will look like before actually publishing it.
It was added because a test failed without turbolinks. However, writing
the test so it doesn't update the database at the same time the browser
is doing a request also solves the problem and makes the test more
robust.
By default, Capybara only finds visible elements, so adding the
`visible: true` option is usually redundant.
We were using it sometimes to make it an obvious contrast with another
test using `visible: false`. However, from the user's perspective, we
don't care whether the element has been removed from the DOM or has been
hidden, so we can just test that the visible selector can't be found.
Besides, using `visible: false` means the test will also pass if the
element is present and visible. However, we want the test to fail if the
element is visible. That's why a couple of JavaScript-dependant tests
were passing even when JavaScript was disabled.
These tests were supposed to check the link to vote is hidden when users
don't have permission to vote. However, they aren't testing that, since
the `visible: false` option also matches visible elements. The links are
actually considered visible since they're displayed by the browser;
there's just another element on top of them.
Using `obscured: true` instead of `visible: false` solves the issue.
However, while the `obscured` option is true when the element is hidden
by another element, it's also true when the element is not currently
visible in the browser window, so in some cases we need to scroll so the
condition is effective.
