Adds banner type as ARIA label to banner article elements

[danchamp]

Fixes #345:

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

[andybroomfield]

@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.

[markconroy]

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

[danchamp]

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

[danchamp]

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

[andybroomfield]

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?)

[danchamp]

@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:

<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?

[waako]

Thanks for the background on implementation, that's helpful

• I can see the logic in keeping html_id preprocess, reduce risk of forgetting to set html_id in the twig template override.
• The fact that title is optional was another reason I wanted the type_of_alert_label separate from display_title
• Alert type being optional as well means makes sense to provide fallback value, such as Announcement, then no risk of landmark just being "region".

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>

Probably viable, although it would need to be outside the {% if display_title %} block.

If navigation is by headers, would we actully need to change this to a region. I'm still feel that the landmark is the alert banner block as a whole, not the indivual banners (and there could be more than one).

The problem, as always, is the versatility of this module, if it was showing 4 "basic" announcements each with a title, then having the block container being <section aria-label="Announcements"> would make sense and be less verbose for screen-reader users navigating by landmarks, as you've said they can navigate by headers within the landmark.
However, if there were 1 "critical" alert, 1 "minor" alert and 2 "announcements" then it seems important that the "critical" alert at least is identified separately.

Maybe a more helpful approach would be:

• Set block container div to section with aria-label="Announcements"
• Output the alert type (if set) in the alert banner as agreed above
• If Alert type: Emergency, then change article to section with aria-labelledby="html_id"

Would a minor alert or death of a notable person warrant being highlighted in landmarks as individual region ?

For the 1.4.1 Use of Color failure, we are agreed that's a bigger change and needs input on what visual mechanism is best or least impactful to implement.

[andybroomfield]

@waako See latest push, though I might now make an accessible label in preprocess instead and set an aria-label instead of a labelled-by.

[waako]

@andybroomfield aria-labelledby is preferred as it references HTML content on the page whereas aria-label is not translatable - not talking about Drupal.t here but via scripts or services (hosted, OS provided or third-party) etc.

I understand why you might want to use preprocess for the accessible label, to keep some logic out of twig template.
However, this brings us back to using 2 IDs for the aria-labelledby, it would simplify the output, and if no title then no ID, therefore ignored and just reads out type_of_alert_label value.

Would this work and appease keeping things in preprocess ?

• Define alert_title_id and alert_type_label_id in preprocess, so "if title" set alert_title_id to html_id + '--title' else '' (empty string so ignored in aria-labelledby)
• Update the type_of_alert_label in preprocess to have default Announcement value if type of alert not set

Then it simplifies the twig template logic

{% if display_title %}
    <h2 class="localgov-alert-banner__title" id="{{ alert_title_id }}">
      {{ content.title }}
    </h2> 
 {% endif %}
 <p class="visually-hidden" id="{{ alert_type_label_id }}">{{ type_of_alert_label }}</p>

No need for any wrapping div, and risk of nesting/logic confusion ?
p.s.: Spotted that currently the id="{{ html_id ~ '-label' }}" set both on div and h2

[andybroomfield]

How do we deal with a situation where there is no title and no label?

[andybroomfield]

Theres also the issue of translation that we need to check.... Not sure how thats going to work becuase these values are part of the field config.

[andybroomfield]

@willguv This is the alert banner issue with the missing type of alert label.
If we make a product change, it will be to include either the text of the type of alert or another non-colour way like an icon inside the banner.

[andybroomfield]

The test failure here is becuase the label can't be found as the defination is
array(4) { ["00--announcement"]=> string(12) "Announcement" ["20--minor"]=> string(11) "Minor alert" ["50--major"]=> string(9) "Emergency" ["90--notable-person"]=> string(25) "Death of a notable person" }

I think thats the test thats wrong though, although we should correct that as it is possible to set up a type of alert without a label.

[andybroomfield]

I'm moving this back to draft whilst we continue work on it.

[andybroomfield]

Alt is #376 (just show it using manage display).
I'm concerned that the type of alerts are really internal names, and may not be suitable for public display.

[andybroomfield]

My instinct is that we maybe should'nt set this here, and this is something a theme should handle, as we don't actully know where someone will place the block. I can see value in wrapping the whole alert banner block in a landmark and using headers for navigation... I do think we need to split this from the colour / label issue which we could do by just including it in manage display as per #376, see my concern about the type of alerts beiing internal names so might need renaming.