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

Adds banner type as ARIA label to banner article elements #346

Open
wants to merge 6 commits into
base: 1.8.x
Choose a base branch
from

Conversation

danchamp
Copy link
Collaborator

@danchamp danchamp commented Jul 5, 2024

Fixes #345:

  • Sets banner type variable in template_preprocess
  • Changes wrapper element to
    and adds ARIA label and role

@andybroomfield
Copy link
Contributor

@danchamp You can ignore the test fail as it's fixed in 1.8.x. Depending on how soon you need this, you could change the base branch to 1.8.x instead on 1.x and make it part of the refactor work.

@@ -32,7 +32,7 @@
]
%}

<article {{ attributes.addClass(classes) }}>
<article {{ attributes.addClass(classes) }} role="region" aria-label="{{ type_of_alert_label }}">
Copy link
Member

Choose a reason for hiding this comment

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

If we intend for this to be a "region" should we remove <article> and replace it with <section>?

It seems strange to have an article element reassigned as a region.

I think MDN agrees with me: https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/region_role#prefer_html

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@markconroy Thanks for the pointer, that would make sense and I'll update it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@andybroomfield Thanks, I'll move it to 1.8x, it's not urgent so can go with the refactor.

Copy link
Contributor

Choose a reason for hiding this comment

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

The change to article (from div) was made in #123. I'm reading up on MDN Region role which states that the <section> tag is preffered without a aria-role. TBH it's a little confusing as I thought it was best to avoid section if something more suitable was in places (maybe note?

Is the reason to move away from article so you can apply an aria-label for screen readers? Would a span with screen reader text be more approriate, using the Drupal standard of <span class="visually-hidden">label="{{ type_of_alert_label }}</span> be more appropriate?

Should we make the block as a whole a region (aside?)

Copy link
Collaborator Author

@danchamp danchamp Jul 10, 2024

Choose a reason for hiding this comment

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

@andybroomfield You're right, if we use <section> then the explicit role is redundant and should be removed. I've updated the template.

The benefit of using an element with an implicit or explicit role is that it makes it a navigational landmark and easier for AT users to be aware of. Example from the demo site with this is place and landmarks listed:

alert-banner-landmarks

<article> has no implicit role, and we'd be relying on users of AT to identify the type of alert by another route - headings perhaps, or by linear access of something like that hidden <span>.

There may be a better way to achieve the aim - a visible label with the alert type maybe? We're currently relying on sighted users to perceive and correctly interpret the colours for each type, so should maybe be more explicit to all users?

Copy link
Collaborator Author

@danchamp danchamp Jul 10, 2024

Choose a reason for hiding this comment

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

@waako Thanks for this, it all makes total sense. I've updated the template, leaving the translation issue for now:

alert-banner-unique-landmarks

Would some punctuation between the type and title in the ARIA label help here?

Copy link
Contributor

Choose a reason for hiding this comment

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

@danchamp Since the alert banners are translatable and the fields your using are content, they should be the translated versions anyway (worth checking). I assume screenreaders arn't going to read the title out twice?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@andybroomfield Yes to the content fields already being translatable, I think the only element that isn't covered is the hard-coded 'Announcement' fallback string if a type isn't set for some reason. It's an edge case though, and may not be worth the effort of adding it as a user-definable variable?

Screen readers shouldn't read the title twice, since the two places it's included are in different contexts - label for the landmark, and the actual content of the hidden para.

Copy link

Choose a reason for hiding this comment

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

TLDR; Keep type of alert label separate from alert title in html, and onward discussion needing in term of 1.4.1 Use of color

Unique ID

I'd forgotten about Drupal's clean_unique_id twig filter.
So shouldn't need the whole html_id logic, just id="{{ alert_type|clean_unique_id }}".
But jumping ahead of myself probably best define an alert_id: {% set alert_id = alert_type|clean_unique_id %} then have flexibility to do id="{{ alert_id ~ '-label'}}"

Type of alert label placement

I can fully understand why you would render the type of alert label inside the heading, as a prefix, however I worry that this will:

  • Result in a non-equivalent experience between visual and non-visual users
  • Be confusing to non-blind screen-reader users

Technically this is [not a failure of 2.5.3 Label in name, but does not feel right nevertheless.

My initial reasoning for outputting the <p id="{{ alert_id ~ '--type' }}" class="visually-hidden">{{ type_of_alert_label }}</p> after the title in source order, was that if screen-reader uses jumps to alert h2 directly, they still get the context of alert type when reading on from h2, but it's no hard rule.
Either way, adding punctuation like colon at end of type of alert label is perfectly reasonable here I'd say.

Of course having the type of alert label render as hidden text only available to certain assistive technologies does bring up the (colour) elephant in the room

Use of colour

The ideal solution to the general issue raised by #345 (1.4.1 Use of Color) would be to visibly render the type of alert label on the page.
This would introduce a change to any existing sites and introduces design decisions that I don't feel in a position to answer, maybe @msayoung might have some thoughts on best approach to this, also if and whom would need buy-in from ?

Sorry for the essay, been cogitating this and it grew into a mammoth

Copy link
Contributor

@andybroomfield andybroomfield Jul 13, 2024

Choose a reason for hiding this comment

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

We can revert the last commit if we want to avoid the type inside the label.

Lets do the following:

  • Keep the html_id inside preprocess as those who override the template will need access to it.
  • The type of alert field is optional and may not always be present, especially on custom alert banners. It's presence cannot be replied upon and if it is missing it should not be assumed to be of a particular type so for now lets leave that off the label if it is not present. (It's canonically not of any type so assuming announcement is incorrect).
  • I think displaying the type of alert is a product change and would have to go to product and content design teams, lets not force that on sites. Lets do the accessibility bug fix for now and take it to the product and content groups if they want to make it visible.
  • Can we just use a single id for the label, even if it means a wrapper around the h2 and the type. <div id="{{ html_id ~ '--label' }}"><h2>{{ title }}</h2><p class="visually-hidden">{{ type_of_alert_label }}</p></div>

@danchamp danchamp force-pushed the fix/345-add-banner-type-accessible-name branch from e84eddb to 3112e60 Compare July 9, 2024 17:58
@danchamp danchamp changed the base branch from 1.x to 1.8.x July 9, 2024 18:00
@danchamp danchamp requested review from andybroomfield and removed request for msayoung July 9, 2024 18:02
@danchamp danchamp marked this pull request as ready for review July 9, 2024 18:03
@andybroomfield andybroomfield changed the base branch from 1.8.x to 1.8.refactor July 10, 2024 16:11
@andybroomfield andybroomfield deleted the branch 1.8.x July 11, 2024 08:58
@andybroomfield andybroomfield changed the base branch from 1.8.refactor to 1-8-refactor July 11, 2024 09:17
@andybroomfield andybroomfield changed the base branch from 1-8-refactor to 1.8.x July 11, 2024 09:25
Use meaningful ID based on localgov_alert_banner entitiy ID using
Drupal html::unique_id method (will always generate a guranteed unique id).
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.

Alert banners convey meaning using colour alone by types having styling reflecting their purpose
4 participants