Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Accessibility improvements for submission report pdf #5099

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

robinmolen
Copy link
Contributor

Closes #5047

Changes

  • Added semantic relationships between field label and field content.
  • When a field is left empty by the user, the submission report will display 'No information provided' in place of the missing field content.
  • Removed leading and trailing whitespace in pdf metadata title

Checklist

Check off the items that are completed or not relevant.

  • Impact on features

    • Checked copying a form
    • Checked import/export of a form
    • Config checks in the configuration overview admin page
    • Problem detection in the admin email digest is handled
  • Release management

    • I have labelled the PR as "needs-backport" accordingly
  • I have updated the translations assets (you do NOT need to provide translations)

    • Ran ./bin/makemessages_js.sh
    • Ran ./bin/compilemessages_js.sh
  • Dockerfile/scripts

    • Updated the Dockerfile with the necessary scripts from the ./bin folder
  • Commit hygiene

    • Commit messages refer to the relevant Github issue
    • Commit messages explain the "why" of change, not the how

@robinmolen robinmolen requested a review from vaszig February 11, 2025 11:19
@robinmolen robinmolen linked an issue Feb 11, 2025 that may be closed by this pull request
4 tasks
Copy link
Contributor

@vaszig vaszig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure if we should have the translations in this PR. I remember we do it in the release flow (let me know if something has changed).

@@ -73,6 +74,15 @@ def __call__(self, component: ComponentT, value: Any) -> str:
formatted_values = (
force_str(self.format(component, value)) for value in values
)

# Check if formatted_values isn't empty
formatted_values_copy, formatted_values = tee(formatted_values)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't used this before, it seems nice instead of a list I guess, but I do not know how it behaves concerning performance and efficiency.

Copy link
Contributor Author

@robinmolen robinmolen Feb 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the documentation again, i think it would be fine (this setup can be improved though..). It copies the formatted_values iterable object, so we can check the content on the copy without moving through the original iterable.

But yeah, i'll have a look if i can create a solution using lists

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yeah, this can be greatly simplified with list.. Changing it 👍

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I didn't say to change it. I just said that I haven't used it and I am not aware of how it works!If you think though that it should be changed to a list ok. I thought you have discussed this with Sergei and he suggested this

@robinmolen
Copy link
Contributor Author

I am not sure if we should have the translations in this PR. I remember we do it in the release flow (let me know if something has changed).

Ah yeah, you're right.. I'll remove them from the PR 👍

Added aria tags that showcase that field label and field value are related
…n report pdf

To indicate when a field has been left empty by the user, show "No information provided" in the place where the filled in information would have been.
This also helps screen reader users identify which fields aren't filled in.
Without the `spaceless` tags the pdf will be generated with leading and trailing whitespace. This whitespace only visible/noticeable when you look at the title in the pdf metadata.
@robinmolen robinmolen force-pushed the feature/5047-pdf-a11y-improvements branch from ead9eb0 to fef4e04 Compare February 20, 2025 11:18
Comment on lines +124 to +134
return (
format_html(
"<ul>{values}</ul>",
values=format_html_join(
"",
"<li>{}</li>",
((selected_label,) for selected_label in selected_labels),
),
)
if len(selected_labels) > 0
else ""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

codewise I think a guard clause would be more readable:

if not selected_labels:
    return ""
return format_html(...)

this also makes it more easy to see where we're generating (potentially unsafe) HTML that needs to escape inputs.

@@ -76,19 +78,23 @@ <h2 class="subtitle">{{ submission_step_node.render }}</h2>
{% if component_node.component.type == "fieldset" %}
<span class="submission-step-row__fieldset-label">{{ component_node.label }}</span>
{% else %}
<div class="submission-step-row__label">
<span id="{{ component_node.key }}" class="submission-step-row__label">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if these are sufficiently unique if you use repeating groups, have you tested that?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Accessibility improvements - PDF
3 participants