In the registration form, after changing the username, we were removing
the message about whether a username was available. However, we were
also removing every `<small>` tag on the page. This affected the demo
branch, where we add a `<small>` tag on every page.
So we're now removing a specific element instead.
This rule was introduced in RuboCop 1.76.0 to ensure methods ending
in '?' return boolean.
This commit applies suggested renames and code cleanup:
- Renames 'is_active?' to 'active_class' since it returns a string
- Renames 'parsed_value' to 'in_favor?' and 'is_request_active' to end with '?'
for boolean semantics
- Skips false positives like 'save', 'auto_labels' or 'save_requiring_finish_signup',
which are not predicate methods.
Bumps [rubocop](https://github.com/rubocop/rubocop) from 1.71.2 to 1.75.8.
- [Release notes](https://github.com/rubocop/rubocop/releases)
- [Changelog](https://github.com/rubocop/rubocop/blob/master/CHANGELOG.md)
- [Commits](https://github.com/rubocop/rubocop/compare/v1.71.2...v1.75.8)
---
updated-dependencies:
- dependency-name: rubocop
dependency-version: 1.75.8
dependency-type: direct:development
update-type: version-update:semver-minor
...
Notes:
This commit also includes several style and lint fixes required after
updating RuboCop:
- Removed redundant parentheses now detected by improved
'Style/RedundantParentheses' (1.72 and 1.75.3).
- Replaced ternary expressions with logical OR when the ternary was
returning 'true', as flagged by 'Style/RedundantCondition' (1.73).
- Adjusted block variables to resolve new 'Lint/ShadowingOuterLocalVariable'
offenses (1.75), helping avoid future conflicts during upgrades with
'rails app:updates'
Signed-off-by: dependabot[bot] <support@github.com>
Since commit c5103d3025, the styles from application.scss no longer apply
to app/views/dashboard/poster/index.pdf.erb.
To recover the text-center alignment, we add the rule in dashboard.scss.
Note that we also remove text-center from the h2, since it wasn't applied
and there's no need to recover it because it doesn't exist in the preview
either.
We were using an <a> tag wrapping the whole content of the banner in
order to make the whole banner clickable. However, that made the text of
the link less concise, affecting people using screen readers. So,
instead, we're using the `card` mixin, which we introduced in commit
f285dfcba.
We're making this change now because the HTML5 Sanitizer that we're
about to enable in the next commit was handling the whitespace inside
the banner differently, causing one test to fail, and we didn't find a
different way to fix it.
We were getting the following warning after upgrading to Rails 7.1:
```
Rendered shared/_social_media_meta_tags.html.erb
DEPRECATION WARNING: Passing the class as positional argument is
deprecated and will be removed in Rails 7.2.
Please pass the class as a keyword argument:
serialize :metadata, type: Object
```
Apparently, the `ids` method, which originally was implemented as
`pluck(:id)`, sometimes returned duplicate ids [1]. Fixing that issue
generated another issue [2] when combining `.includes` and `order`.
There was an attempt to fix it [3], but it still doesn't fix the case
where the ordering column belongs to an association [4].
This means that, when upgrading to Rails 7.1, calling `.ids` on
`budget.investments.includes(:heading).sort_by_random.for_render`
results in an invalid SQL statement:
```
ActiveRecord::StatementInvalid:
PG::GroupingError: ERROR: column "ids.ordering" must appear in
the GROUP BY clause or be used in an aggregate function
LINE 1: ... = $4 GROUP BY "budget_investments"."id" ORDER BY
ids.orderi...
```
To solve it, we could once again use `.pluck(:id)` instead of `ids`, but
I'm not sure whether there would be a risk to get duplicate IDs in some
cases. We cannot call `.reorder` or `.unscope(:order)` either because we
need the IDs to be ordered randomly.
So we're removing the `includes(:heading)` part when getting the IDs.
Since using `includes` generates a separate query that doesn't affect
the query to get the IDs, removing it makes no difference.
Another option would be to remove the `for_render` call, since we're
including the headings to avoid the N+1 queries problem, but we're doing
so without benchmarks showing that it does actually make a difference.
[1] Issue 46455 in https://github.com/rails/rails
[2] Issue 48080 in https://github.com/rails/rails
[3] Pull request 48101 in https://github.com/rails/rails
[4] Issue 50263 in https://github.com/rails/rails
We were using a placeholder, which is way less accessible than a label.
One issue here (which also happened before, but is now more obvious) is
that, when adding several options, there will be many fields with the
same label.
Another issue is that, for some languages, we're using texts like "Add a
closed answer", which might be confusing because we might be editing an
existing answer. The proper solution would probably be using the text
"Option 1", "Option 2", ... I'm not doing so right now because I'm not
sure that's a good option and because changing the text would mean
losing the existing translations.
This way the fields are easier to use, and we can get rid of the
placeholders.
Note we're simplifying the `answer_result_value` in order to make it
easier to return `0` instead of `nil` when the field is empty.
Also note there's a small change of behavior here; previously, these
fields were empty by default, and now their value is `0` by default, so
blindly clicking the "Save" button would send `0` instead of an empty
value. I don't think it's a big deal, though, but we need to keep that
in mind.
Back when we added all the missing labels (changes that we merged in
commit c34cab282), we forgot about fields which had placeholdes, since
Axe doesn't report an error when there are placeholders but there aren't
labels.
In this case, we were using an invalid <label> tag for the question
options, and <h3> tags as labels for the votes.
Using standard labels solves the issue.
Saying that we're supposed to introduce a descriptive title in a field
labelled as "Title" is redundant. Besides, the text of the placeholder
was barely distinguishable, making it harder to fill in the form.
The link to the comments page was an "expand" icon, which was confusing
because it wasn't really expanding the contents of the sidebar but going
to an entirely different page. Furthermore, it didn't have any text for
people using screen readers, which is why Axe was reporting the
following accessibility error:
```
link-name: Links must have discernible text (serious)
https://dequeuniversity.com/rules/axe/4.10/link-name?application=axeAPI
The following 1 node violate this rule:
Selector: #annotation-link > a
HTML: <a href="/legislation/processes/75/draft_versions/30/annotations/8?sub_annotation_ids=">
<span class="icon-expand" aria-hidden="true"></span>
</a>
Fix all of the following:
- Element is in tab order and does not have accessible text
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
```
So we're removing the icon and turning the "N comments" text into a
link, so it's easier to guess that the link takes us to the page showing
all the comments for this annotation.
Just like we do with the rest of the phases.
The reason why we're making this change right now is that we were
getting an accessibility error with processes with no result publication
date:
```
link-name: Links must have discernible text (serious)
https://dequeuniversity.com/rules/axe/4.10/link-name?application=axeAPI
The following 1 node violate this rule:
Selector: p:nth-child(6) > a
HTML: <a href="/legislation/processes/39/result_publication">
<strong></strong>
</a>
Fix all of the following:
- Element is in tab order and does not have accessible text
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
```
This way we simplify the CSS and, in the case of the "check" icon, using
an SVG icon instead of an icon font offers several advantages, as
mentioned in commit 925f04e3f.
People using screen readers had no idea what these links were about (not
that the icons are very usable for people seeing them either... but
that's a different topic). Axe was reporting this error:
```
link-name: Links must have discernible text (serious)
https://dequeuniversity.com/rules/axe/4.10/link-name?application=axeAPI
The following 1 node violate this rule:
Selector: #dashboard_action_2_execute
HTML: <a id="dashboard_action_2_execute" class="unchecked-link"
rel="nofollow" data-method="post"
href="/proposals/16-proposal-6-title/dashboard/actions/2/execute">
<span class="unchecked"></span>
</a>
Fix all of the following:
- Element is in tab order and does not have accessible text
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
```
When using a link, people using screen readers might think they're going
to a new page where the password is going to be shown. With a button,
they get a better idea about what to expect.
Furthermore, with a button, we can use the `aria-pressed` attribute to
indicate whether the password is currently being shown.
We were using an icon for this link, but people who can't see the icon
couldn't know what the link was about. Axe was reporting the following
accessibility error:
```
link-name: Links must have discernible text (serious)
https://dequeuniversity.com/rules/axe/4.10/link-name?application=axeAPI
The following 1 node violate this rule:
Selector: .show-password
HTML: <a href="#" class="show-password">
<span class="icon-eye"></span>
</a>
Fix all of the following:
- Element is in tab order and does not have accessible text
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
```
This happened when previewing banners in the "new banner form", which
might cause accessibility issues when people access the list of links on
the page.
We were getting the following accessibility error:
```
link-name: Links must have discernible text (serious)
https://dequeuniversity.com/rules/axe/4.9/link-name?application=axeAPI
The following node violate this rule:
Selector: a[href$="new"]
HTML: <a href="/admin/banners/new"><h2></h2><h3></h3></a>
Fix all of the following:
- Element is in tab order and does not have accessible text
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
- Element has no title 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.
Just like we do everywhere else. We're also removing the wrong ARIA
attributes that we added in commit c18479e3a, which caused an
accessibility issue reported by Axe:
```
aria-required-children: Certain ARIA roles must contain particular
children (critical)
https://dequeuniversity.com/rules/axe/4.10/aria-required-children?application=axeAPI
The following 1 node violate this rule:
Selector: .tabs-content
HTML: <div class="tabs-content"
data-tabs-content="information-texts-tabs" role="tablist">
Fix any of the following:
- Element has children which are not allowed: ul[tabindex]
```
Although, in this case, it would probably be better to have different
pages instead of tabs, so loading the page doesn't take too long.
