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.

Sign up for GitHub

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

Jump to bottom

A11y/announce placeholders #1801

Merged
merged 6 commits into from
Apr 18, 2024
Merged

A11y/announce placeholders #1801

merged 6 commits into from
Apr 18, 2024

Conversation

amazingphilippe
Copy link
Contributor

Summary | Résumé

Fixes cds-snc/notification-planning#1456

This slightly overhauls how placeholders are rendered in the admin interface.

  • replaces SPAN elements with MARK, which is apparently more suited for this type of thing
  • Adds pseudo elements before and after the MARK elements to announce the placeholder.
  • Refactors the placeholder CSS so its less repetititititve

Test instructions | Instructions pour tester la modification

  1. Have at least 2 templates ready:
    2. SMS template with a conditional and custom content
    3. Email template with a conditional and custom content
  2. With voiceover on chrome
  3. Start reading the template previews
  4. Each placeholder should announce a "begins" and "ends"
  5. Conditional placeholders should announce "if" and "then"
  6. Visually, there should be no difference from prod.

@github-actions GitHub Actions
Copy link

🧪 Review environment

https://umc2kl5u6zgtof3agf7ucnmmia0kyxap.lambda-url.ca-central-1.on.aws/

andrewleith
andrewleith reviewed Apr 12, 2024
View reviewed changes
Copy link
Member

@andrewleith andrewleith left a comment

Choose a reason for hiding this comment

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

This doesn't appear to work, <span class="placeholder">...</span> is still in the template preview. I think you'd need to modify utils to change the way template previews are rendered.

That said, this technique buries language strings inside of CSS, which may make them hard to find for future developers. I wonder if there is a way to keep the language strings outside of CSS?

Could an alternative be to output these strings into the markup using the sr-only class?

@amazingphilippe amazingphilippe mentioned this pull request Apr 12, 2024
Merged
5 tasks
@amazingphilippe
Copy link
Contributor Author

This doesn't appear to work, ... is still in the template preview. I think you'd need to modify utils to change the way template previews are rendered.

We have a matching utils PR. Since this PR has no effect, it would be safer to merge before.

That said, this technique buries language strings inside of CSS, which may make them hard to find for future developers. I wonder if there is a way to keep the language strings outside of CSS?

Discussing this further, we also don't really want to start adding translations in utils, and try to keep most of it in admin.

Could an alternative be to output these strings into the markup using the sr-only class?

A few things to think through:

  1. We should think of more integrated ways to deal with translation strings in templates, javascripts and now CSS.
  2. We should see how cypress tests react to pseudo content.
  3. Investigate if we can translate some sr-only spans on the admin side when we call the template from utils.

andrewleith
andrewleith requested changes Apr 15, 2024
View reviewed changes
Copy link
Member

@andrewleith andrewleith left a comment

Choose a reason for hiding this comment

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

This works well, just a few questions about how we refer to variables and some commented out markup that should likely be removed.

app/templates/views/templates/_template.html Outdated
@@ -29,4 +29,5 @@ <h2 class="heading-medium">{{ _('Ready to send?') }}</h2>
{{ template|string|translate_preview_template }}
</div>

<!-- <div id="placeholder_tag" role="tooltip" aria-hidden="true">Placeholder variable</div> -->
Copy link
Member

Choose a reason for hiding this comment

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

Should this be removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes! Thanks

app/assets/stylesheets/tailwind/components/placeholder.css Outdated

/* BEGINS or ENDS */
[lang*="en"] mark[class*="placeholder"]::before {
content: " [" var(--name) " begins] ";
Copy link
Member

Choose a reason for hiding this comment

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

What is the purpose of the square brackets here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe they ensure a short pause. Looking at this table, only Voiceover would pause. NVDA and JAWS would announce "brackets" twice.

JAWS always announces everything. Only pauses at commas, semicolons and ellipses

NVDA would pause with regular parenthesis ()

I don't mind using semicolons, nobody will see them!

app/assets/stylesheets/tailwind/components/placeholder.css

/* PLACEHOLDER TYPES */
[lang*="en"] mark.placeholder {
--name: "custom content";
Copy link
Member

Choose a reason for hiding this comment

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

The phrase "custom content" confused me here. Why not call this "variable" or "variable content" instead?

Copy link
Member

Choose a reason for hiding this comment

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

I just looked through the guidance and see that we do indeed call this "custom content" - but I still this think is a confusing way to refer to it. Maybe we should update the guidance? @YedidaZalik what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whenever we do update the guidance, we'll have other things to clean up within admin. I suggest we be consistent for now, and decide if we want to re-consider this term. We also have similar work planned in cds-snc/notification-planning#1350

Copy link
Member

Choose a reason for hiding this comment

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

Yeah makes sense to me!

Copy link

Choose a reason for hiding this comment

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

+1 "custom content" was chosen by Yael a year or so ago

@amazingphilippe amazingphilippe force-pushed the a11y/announce-placeholders branch from 0bb133b to 0a47076 Compare April 18, 2024 15:47
@amazingphilippe amazingphilippe merged commit f6670a4 into main Apr 18, 2024
9 checks passed
@amazingphilippe amazingphilippe deleted the a11y/announce-placeholders branch April 18, 2024 17:04
andrewleith added a commit that referenced this pull request Apr 23, 2024
@andrewleith
Revert "A11y/announce placeholders (#1801)"
576b0ab 
This reverts commit f6670a4.
@andrewleith andrewleith mentioned this pull request Apr 23, 2024
Merged
andrewleith added a commit that referenced this pull request Apr 23, 2024
@andrewleith
Revert/pr 1801 (#1817)
10e0a7c 
* Revert "A11y/announce placeholders (#1801)"

This reverts commit f6670a4.

* chore: regen css
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@YedidaZalik YedidaZalik YedidaZalik left review comments

@andrewleith andrewleith andrewleith approved these changes

Assignees
No one assigned
Labels
a11y
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Template formatting use of special characters. Infos and relationships
3 participants
@amazingphilippe @andrewleith @YedidaZalik