A comment on the pull request for this branch
pointed out that it's not clear why the 'items'
list is deleted and then reassigned in
extend_params:
https://github.com/alphagov/notifications-admin/pull/3770#pullrequestreview-573067465
The simple reason is that we want to use
merge_jsonlike to merge params and
param_extensions (passed in as extensions) but
merge_jsonlike doesn't merge lists correctly.
I realised that if we just make merge_jsonlike
merge lists correctly, we can use it for
everything extend_params does.
This commit does that, and replaces all calls to
extend_params with merge_jsonlike.
Because extend_params is used across many form
field classes, and so many pages, I took the
following precautions after making those changes:
1. found every use of param_extensions
2. looked at the merges onto params that each would
cause and deduped them to a final list of 6(!)
3. tested pages containing fields from that list
4. added new testcases to the merge_jsonlike tests
for any merges that exist in our codebase but
not in our tests
Current behaviour is to check item-against-item
and merge based on whether items match, irrelevant
of position. This doesn't produce the results we
need for our usecases (merging data to send to
GOVUK Frontend components).
We actually want:
- items to be compared based on their position
- new primitive items at the same position to
overwrite existing ones
- dicts or lists at the same position to be merged
For example,
Starting with this list:
[{"name": "option-1", "value": "1"}]
Merging in this list:
[{"hint": {"text": "Choose one option"}}]
You currently get this:
[
{"name": "option-1", "value": "1"},
{"hint": {"text": "Choose one option"}}
]
We want to get this:
[
{
"name": "option-1", "value": "1",
"hint": {"text": "Choose one option"}
}
]
The OrganisationAgreementSignedForm class has a
bug causing it to render different HTML when the
page loads to when you subsequently refresh it.
This commit proposes a change to the extend_params
function to fix it.
extend_params, is used by the
OrganisationAgreementSignedForm, as well as all
the other WTForms field classes we added to wrap
GOVUK Frontend components. Fixing it should
therefore fix any similar bugs with them.
All of these fields send a dict of configuration
data to the GOVUK Frontend component when they
call it, at render time. This dict is 'JSON-like',
meaning it's values can be all the primitives as
well as lists and dicts. This also means it can go
quite deep.
Extending the default configuration
The classes have a default dict of this data kept
privately in the params variable. They let you
change it by passing in an argument called
param_extensions on instantiation, after that,
through an attribute of the same name and at
render time as the same argument (in templates).
The extend_params function
The param_extensions dict is used as a collection
of changes to make to the default params dict.
The changes are applied by the extend_params
function. Its code deletes part of the
param_extensions, a side effect that didn't seem a
problem because it isn't used after the function
has run.
The bug
The bug was only with the part of the HTML that
got its data from the part of the param_extensions
dict that was deleted by extend_params. The class
with the bug set param_extensions when the field
is instantiated, as part of its parent form
definition.
My guess is that param_extensions was stored in
memory, as part of the form class, and reused
when the page refreshed. At that point,
extend_params had deleted part of its data,
causing the bug.
jQuery.attr returns `undefined` if an element does not have an
attribute. We want an empty string, rather than the default of coercing
`undefined` to the string `'undefined'`.
The content length message was making the page jumpy and causing reflows
in three ways. This commit addresses each of those ways:
As the user scrolled
---
The footer went from fixed to sticky and the spacing around the message
changed. This change in spacing was needed so that the message looked
right in both contexts.
I think the best way to resolve this is to not use the sticky footer
when editing text message or broadcast templates.
On my 1440×900 screen I can fit a 5 fragment text message, plus the
‘will be charged as 5 text messages’ message, plus the save button.
Our top 10 screen resolutions according to our analytics are:
Position | Resolution | Percentage of users
---------|------------|--------------------
1 | 1920x1080 | 27.37%
2 | 1280×720 | 11.07%
3 | 1366×768 | 8.88%
4 | 1536×864 | 5.79%
5 | 1440×900 | 4.52%
6 | 1600×900 | 3.71%
7 | 1280×1024 | 3.10%
8 | 1680×1050 | 2.42%
9 | 1920×1200 | 2.33%
10 | 2560×1440 | 1.99%
When the page first loaded
---
The message is empty so takes up no space, then the javascript fires
and inserts the message, taking up a line of space.
This is resolved by making the empty message take up space with a
non-breaking space character.
When the user first typed
---
We previously didn’t show any message until the user started typing.
This meant that, with the above fix, there was a larger than normal
empty space between the textarea and the save button.
This is resolved by always showing the message, even when the user
hasn’t typed anything yet.
***
These are design decisions which made sense when the message was
displayed along side the button, but we’ve had to change now that the
message is above the button.
We feel that this is more appropriate because it’s part of the
information you’re agreeing to before you hit submit.
Sometimes users can missing information that doesn’t start left-aligned
to the column they’re interacting with.
It also makes it closer to the Design System component.
We’re keeping it in the sticky footer, so that it’s always visible no
matter where in the message you’re scrolled to (this means you won’t
have to edited to content then scroll down to check whether you’ve
made it fit).
This looks tidy, and because of the sticky footer it means the message
is always visible, even if your template is quite long. So no matter
where you’re scrolled to in the template you don’t have to scroll to the
bottom to see the count update.
The endpoint that count characters should be pretty low-load because it
won’t talk to the database (unless, on the first request, the user and
service aren’t cached in Redis).
The response size is also very small, only one line of text wrapped in a
single `<span>`, so won’t be as CPU-intensive to render as a whole page.
Still, we don’t want to completely hammer the server if a user types
very quickly.
This commit adds some throttling, so that we wait until there’s a
certain amount of delay between keystrokes before firing off the request
to the backend.
I’ve set the delay at 150ms. At normal typing speed this makes the lag
feel fairly imperceptible – it feels like you get an updated count in
response to most keystrokes. It’s only if you really mash the keyboard
that the count won’t update until you take a breath.
This commit copies the same ARIA attributes that are added to the
character count component[1] in the GOV.UK Design System.
This means that screen reader users will hear the count message when
they stop typing.
1. https://design-system.service.gov.uk/components/character-count/
This commit adds some Javascript that makes AJAX requests as the users
changes the content of their template.
It then takes the content returned by the backend and inserts it in the
page.
Users sending text messages are sometimes unaware that long messages
will cost more.
Users sending broadcast messages need to be aware that there’s a
character limit, so they can take this into account when planning their
messages.
This commit adds an endpoint which counts the number of characters in
some template content, and returns a snippet of useful info about how
long the message is.
In subsequent commits we’ll be able to use AJAX to fetch this snippet as
the user types.
There’s a surprising amount of complexity in counting the length of
messages. So we’ll need to do this in Python because it would be too
convoluted to re-implement the length counting in client side code, let
alone ensuring it had parity with its Python equivalent.
As formatters we can use them in Jinja or Python code.
It also means we don’t need to import them every time we want to use
them – they’re always available in the template context.
For now this doesn’t remove the macros, it just aliases them to the
formatters. This gives us confidence that the formatters are working the
same way the old macros did, and reduces the diff size of each commit.
We have lots of functions for converting various types of data into
strings to be displayed to the user somewhere.
This commit collects all these functions into their own module, rather
than having them cluttering up `app/__init__.py` or buried amongst
various other things that have ended up in `app/utils.py`.
`app/utils.py` is a bit of a dumping ground for things we don’t have a
better place for.
We now have a place and structure for storing ‘model’ code (‘model’ in
the model, view, controller (MVC) sense of the word).
This commit moves the spreadsheet model to that place.
We added an extra, hidden, <input> to our /sign-in
and /register forms to stop Chrome's form
heuristics filling the fields in wrong.
See the commit that added it:
33b15cdec6
This removes it after testing with the following
Chrome/OS combinations and all working without the
hack:
- Chrome 87, Windows 7
- Chrome 86, Windows 7
- Chrome 86, Windows 10
- Chrome 85, Windows 7
- Chrome 80, Windows 10
- Chrome 81, Windows 7
- Chrome 52, Windows 7
- Chrome 68, Windows 7
- Chrome 78, Windows 8.1
- Chrome 86, Windows 8.1
These combinations were based on the most-used
versions recorded in our analytics for the last 3
months.
Looks like this was added in:
4a226a7a29
...and used in the spark_bar_field macro. That
macro was removed in:
89b88ee4cb
..but the import was missed out.
An accessiblity audit done as part of Notify's
service assessment raised the following problem
with our big_number component.
When you turn CSS off, the sentence in the
component is split onto separate lines.
This was because the number part is wrapped in a
<div> which browsers were interpreting as being a
separate sentence to the label.
So "1 letter", where "letter" is the label, was
seen as:
"1"
"letter"
The accessibility expert consulted on this pointed
out that this would sound confusing for users of
screen readers when moving through the document
sentence by sentence.
These changes:
- make the <div>s into <span>s which are 'phrasing
content' and so are interpreted as part of the
same sentence
- change the CSS so the number will still sit
on top of its label text
The HTML5 spec has a section on how browsers
should arrange text into paragraphs that explains
what was happening in more detail:
https://www.w3.org/TR/html52/dom.html#paragraphs
We shouldn’t have a page where someone can look up any other user’s
email address based on their user ID.
We also don’t want a page where a malicious user could send someone an
link which would get them invited to the service.
Restricting the invite to be populated just from users in their own
organisation doesn’t mitigate against this stuff completely, but they
probably have a way of finding out the email address of someone in their
organisation already.
At the moment users must be invited to join a service. But this means:
- users must know that a service already exists
- they need to know who to ask for an invite
If the user doesn’t know these thing then sometimes they just go ahead
and set up a new service. Which means they have to get all the way to
the point of requesting to go live before we tell them that there’s
already a service with a similar name or purpose.
So we should let users:
1. discover what other services exist in their organisation
2. apply to join a service
3. automatically notify the service managers of their interest
4. be invited by a service manager
5. accept the invite
This commit implements step 4. We can just link them to the invite form
in step 3., but we should make it easy for them to send the invite,
without having to copy and paste email addresses.
So this commit let the invite form be pre-populated with an existing
user’s email address.
We added this code in
https://github.com/alphagov/notifications-admin/pull/3371/files to
account for Flask Login renaming its cookies. We wanted our apps to be
compatible with the old and new names, so people didn’t get logged out
when we rolled out the change.
Now that all the cookies with the old names will have expired (some
weekends have passed since March) we can remove this loop.
In very old browsers it used to be that you could only make 2 concurrent
requests from the same origin.
So base64 encoding of images into CSS was an optimisation that became
popular because it reduced the number of separate requests.
However base64 encoding images has a few disadvantages:
- it increases the size of the image by about 30%
- it increases the size of the CSS file, which is a
[render blocking resource](https://web.dev/render-blocking-resources/)
so makes the page appear to load more slowly for the sake of some
images which, on most pages, never get used
- GZipping things that are already compressed (for example PNG data) is
very CPU intensive, and might be why Cloudfront sometimes gives up
Removing the inlining of images reduces the size of the CSS we’re
sending to the browser considerably:
–| Before | After | Saving
---|---|---|---
Uncompressed | 198kb | 164kb | 17%
Compressed | 38kb | 23kb | 39%
When looking at Google’s PageSpeed Insights tool as part of the
compression work I noticed a suggestion that we preload our font files.
The tool suggests this should save about 300ms on first page load time.
***
Our font files are referenced from our CSS. This means that the browser
has to download and parse the CSS before it knows where to find the font
files. This means the requests happen in sequence.
We can make the requests happen in parallel by using a `<link>` tag with
`rel=preload`. This tells the browser to start downloading the fonts
before it’s even started downloading the CSS (the CSS will be the next
thing to start downloading, since it’s the next `<link>` element in the
head of the HTML).
Downloading fonts before things like images is important because once
the font is downloaded it causes the layout to repaint, and shift
everything around. So the page doesn’t feel stable until after the fonts
have loaded.
Google call this [cumulative layout shift](https://web.dev/cls/) which
is a score for how much the page moves around. A lower score means a
better experience (and, less importantly for us, means the page might
rank higher in search results)
We’re only preloading the WOFF2 fonts because only modern browsers
support preload, and these browsers also all support WOFF2.
We set an empty `crossorigin` attribute (which means anonymous-mode)
because the preload request needs to match the origin’s CORS mode. See
https://developer.mozilla.org/en-US/docs/Web/HTML/Preloading_content#CORS-enabled_fetches
for more details.
We set `as=font` because this helps the browser use the correct content
security policy, and prioritise which requests to make first.
It’s fiddly having to scroll within a small textbox to see all the
content. Let’s make the box expand to fit the contents like we do
elsewhere. This was removed by accident when we stopped highlighting
placeholders in broadcast templates in
https://github.com/alphagov/notifications-admin/pull/3672/files
Depends on:
- [ ] https://github.com/alphagov/notifications-utils/pull/826/files
Adds error messages for when the content of a broadcast template is too
long.
The error message is explicit when this is cause by non-GSM characters.
We may not want to expose this complexity to our users, but it’s useful
for now while we’re testing things out.
