I recently found that there is a problem with the
overuse of lists online for sequences of content
that don't need to be in a list. I think that's
what we're doing with our map key.
The problem is large enough that browsers include
heuristics to determine if lists should be
presented as such to the accessibility API (and so
to assistive tech' like screen readers). This
thread contains more details:
https://twitter.com/cookiecrook/status/1337226933822603270
Based on the metric described in the thread:
'If all of the styles that make it “list-like”
have been removed, it’s no longer relevant to
convey it as a list.'
...or
'if a sighted user doesn’t need to know it’s a
list, why would a screen reader user need to know
or want to know?'
Based on that, I think the items in our map key
should be paragraphs, not items in a list.
Also, they read really well as sentences when
announced by a screen reader, partly because of
the extra (hidden) info @quis added to them.
This also doesn't change their visual appearance.
The Siteimprove accessibility testing tool raised
an issue with our having a div with role=region in
the page that was:
- empty
- with no label
The status has a role of 'region' which makes it a
generic landmark. This means that, unlike <nav>
or <footer>, users don't get any hint of its
purpose from its HTML tag. If we did want this,
we'd have to give it a label to explain that.
I don't think we do want those things. I think
it's more of a sentence that sits between the
searchbox and the results, just saying how many
are there.
That being the case, we should just remove the
role. It's also what design system do with their
character count, which is similar:
https://design-system.service.gov.uk/components/character-count/
Unlike that component, I don't think we need to
use aria-describedby to set the status as the
description of the searchbox because it describes
the results, not the search term.
This is currently spelt incorrectly though it
seemed to work nonetheless. Can only assume this
is a common error, for this attribute or all
attribute names, so browsers work it out.
This makes the spelling match the spec:
https://www.w3.org/TR/wai-aria/#aria-describedby
They have the service navigation inside the <main>
block. This means you can't bypass it when you use
the skiplink.
This copies other layouts that inherit from
admin_template.html and sets the contents of the
'main' template block so the service navigation is
placed before the <main> tag.
Previous attempt: https://github.com/alphagov/notifications-admin/pull/4048
The problem with the previous attempt is that the assets built on
CI become part of the build artefact used for production [1]. This
switches back to my original approach of using environment.sh, but
with a technique to cope with it being absent on CI. I've tested it
works with and without an environment.sh file.
Note that "npm install" is fine to be on a separate line, since a
non-zero exit code will always cause "make" to stop.
[1]: https://github.com/alphagov/notifications-aws/blob/master/concourse/templates/admin.yml.j2#L47
This depends on an environment variable being set when the assets
are built in a development context [1]. Otherwise, the assets get
their '/static' prefix stripped like they do for production, which
isn't compatible with serving them under '/static' in development.
[1]: 66e5022198/gulpfile.js (L37-L41)
Since we’ve introduced the ‘on behalf of’ wording to the go live ticket
(to talk about who the agreement has been signed on behalf of) it’s
confusing to use the same terminology to talk about the organisation
for whom the agreement has been accepted.
We can make the `as_agreement_statement_for_go_live_request` method less
complex by offloading some of the content it returns to the presentation
layer.
Jinja is a better language for doing complex templating. And we can use
the global Jinja scope to automatically get access to things like
`current_user` and our formatters.
We can’t use the latest version of importlib-metadata because it’s
pinned to <4.3 by the newest version of flake8.
The conflict is caused by:
The user requested importlib-metadata==4.8.1
click 8.0.1 depends on importlib-metadata; python_version < "3.8"
pytest 6.2.5 depends on importlib-metadata>=0.12; python_version < "3.8"
flake8 4.0.1 depends on importlib-metadata<4.3; python_version < "3.8"
Our `make freeze-requirements` task doesn’t catch this because it
doesn’t look at dependencies in `requirements-for-test.txt`. Therefore
it only freezes the version that `click` is specifying, which is the
latest version.
Pinning the version in `requirements.in` gets around this.
Previously this test asserted on `current_user.is_authenticated`. That
isn’t possible now because the object imported into tests isn’t the same
one the app is using.
A different proxy for whether the user is signed in is whether they have
a user id in their session, because we set this every time they sign in:
ff32e73d9b/app/models/user.py (L162)
This is the newest version.
Pyup is complaining about vulnerabilities in version 1.0.1, specifically
> Werkzeug version 2.0.2 improves the security of the debugger cookies.
> "SameSite" attribute is set to "Strict" instead of "None", and the
> secure flag is added when on HTTPS.
Previously we were using whatever version of Werkzeug that Flask
specified this pins it to get rid of the vulnerability without having to
upgrade everything at once.
This requires a few changes to tests which were relying on importing
`session` and `current_user` from Flask. Previously it seemed that
importing these in the tests referred to the same object that was being
used in the app. This appears to no longer be the case. This commit
works around that by:
- using a context manager to get the contents of the session, like we
already do in most tests
- asserting that the mock which logs the user in is being called with
the right values, rather than looking at the state of the
`current_user` object (which was probably giving false certainty
anyway)
