Skip to content

Commit 4708f00

Browse files
spaceninjaspaceninja
authored
Add is_sectioning check to Card and Overview (#1936)
This PR updates the template for the Card component and Overview object to check if `tag_name` is a sectioning element like `article` or `section`, and then change the default for `header_tag_element` and `footer_tag_element` to either `header`/`footer` or `div`. This avoids an issue when `header` elements are rendered in a `div`, it causes useless "banner" landmarks to display in the VO Rotor. Fixes #1934
1 parent adae056 commit 4708f00

File tree

7 files changed

+185
-17
lines changed

7 files changed

+185
-17
lines changed

‎.changeset/sharp-years-think.md‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
"@cloudfour/patterns": patch
3+
---
4+
5+
Update Card and Overview to only use `header` and `footer` elements if the containing element is an `article` or `section`.

‎src/components/card/card.test.ts‎

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,54 @@
1+
import path from 'path';
2+
3+
import type { ElementHandle } from 'pleasantest';
4+
import { getAccessibilityTree, withBrowser } from 'pleasantest';
5+
6+
import { loadTwigTemplate } from '../../../test-utils.js';
7+
8+
/** Helper to load the Twig template file */
9+
const template = loadTwigTemplate(path.join(__dirname, './demo/single.twig'));
10+
const divTemplate = loadTwigTemplate(path.join(__dirname, './demo/div.twig'));
11+
12+
describe('Card component', () => {
13+
test(
14+
'should use header/footer with article',
15+
withBrowser(async ({ utils, page }) => {
16+
await utils.injectHTML(
17+
await template({
18+
show_heading: true,
19+
show_footer: true,
20+
})
21+
);
22+
23+
const body = await page.evaluateHandle<ElementHandle>(
24+
() => document.body
25+
);
26+
expect(await getAccessibilityTree(body, { includeText: false }))
27+
.toMatchInlineSnapshot(`
28+
article
29+
banner
30+
heading "Lorem ipsum dolor sit amet" (level=2)
31+
contentinfo
32+
`);
33+
})
34+
);
35+
36+
test(
37+
'should not use header/footer with div',
38+
withBrowser(async ({ utils, page }) => {
39+
await utils.injectHTML(
40+
await divTemplate({
41+
show_heading: true,
42+
show_footer: true,
43+
})
44+
);
45+
46+
const body = await page.evaluateHandle<ElementHandle>(
47+
() => document.body
48+
);
49+
expect(
50+
await getAccessibilityTree(body, { includeText: false })
51+
).toMatchInlineSnapshot(`heading "Lorem ipsum dolor sit amet" (level=2)`);
52+
})
53+
);
54+
});

‎src/components/card/card.twig‎

Lines changed: 21 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1,32 +1,40 @@
1-
{% set tag_name = tag_name|default('article') %}
2-
{% set header_tag_name = header_tag_name|default('header') %}
3-
{% set footer_tag_name = footer_tag_name|default('footer') %}
4-
5-
{% set heading_level = heading_level|default(2) %}
1+
{% set _tag_name = tag_name|default('article') %}
2+
{#
3+
Using `header` inside a `div` causes pointless "banner" landmarks in
4+
the VoiceOver rotor. As a result, we set the default header/footer
5+
element to `div` if the `tag_name` is anything but `article` or `section`.
6+
#}
7+
{% set _is_sectioning = _tag_name in ['article', 'section'] %}
8+
{% set _default_header_tag = _is_sectioning ? 'header' : 'div' %}
9+
{% set _default_footer_tag = _is_sectioning ? 'footer' : 'div' %}
10+
{% set _header_tag_name = header_tag_name|default(_default_header_tag) %}
11+
{% set _footer_tag_name = footer_tag_name|default(_default_footer_tag) %}
12+
13+
{% set _heading_level = heading_level|default(2) %}
614

715
{% set _heading_block %}{% block heading %}{% endblock %}{% endset %}
816
{% set _cover_block %}{% block cover %}{% endblock %}{% endset %}
917
{% set _content_block %}{% block content %}{% endblock %}{% endset %}
1018
{% set _footer_block %}{% block footer %}{% endblock %}{% endset %}
1119

12-
<{{ tag_name }} class="
20+
<{{ _tag_name }} class="
1321
c-card
1422
{% if href %}c-card--with-link{% endif %}
1523
{% if class %}{{ class }}{% endif %}"
1624
{% if heading_id and _heading_block is not empty %}aria-labelledby="{{heading_id}}"{% endif %}>
1725

1826
{% if _heading_block is not empty %}
19-
<{{ header_tag_name }} class="c-card__header">
20-
<h{{ heading_level }} class="c-card__heading"{% if heading_id %} id="{{heading_id}}"{% endif %}>
27+
<{{ _header_tag_name }} class="c-card__header">
28+
<h{{ _heading_level }} class="c-card__heading"{% if heading_id %} id="{{heading_id}}"{% endif %}>
2129
{% if href %}
2230
<a href="{{ href }}" class="c-card__link">
2331
{{ _heading_block }}
2432
</a>
2533
{% else %}
2634
{{ _heading_block }}
2735
{% endif %}
28-
</h{{ heading_level }}>
29-
</{{ header_tag_name }}>
36+
</h{{ _heading_level }}>
37+
</{{ _header_tag_name }}>
3038
{% endif %}
3139

3240
{% if _cover_block is not empty %}
@@ -42,9 +50,9 @@
4250
{% endif %}
4351

4452
{% if _footer_block is not empty %}
45-
<{{ footer_tag_name }} class="c-card__footer">
53+
<{{ _footer_tag_name }} class="c-card__footer">
4654
{{ _footer_block }}
47-
</{{ footer_tag_name }}>
55+
</{{ _footer_tag_name }}>
4856
{% endif %}
4957

50-
</{{ tag_name }}>
58+
</{{ _tag_name }}>

‎src/components/card/demo/div.twig‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
{# Used for tests #}
2+
{% embed '@cloudfour/components/card/card.twig' with { tag_name: 'div' } %}
3+
{% block heading %}
4+
{%- if show_heading -%}
5+
Lorem ipsum dolor sit amet
6+
{%- endif -%}
7+
{% endblock %}
8+
{% block cover %}
9+
{%- if show_cover -%}
10+
<img src="https://placeimg.com/800/450/animals" alt="">
11+
{%- endif -%}
12+
{% endblock %}
13+
{% block content %}
14+
{%- if show_content -%}
15+
<p>Consectetur adipiscing elit. Fusce tempor ut ex nec scelerisque. Quisque dui tortor, tempus et tempor in, rhoncus eu massa. Vestibulum dolor erat, vestibulum eget velit eu, dignissim hendrerit tortor.</p>
16+
{%- endif -%}
17+
{% endblock %}
18+
{% block footer %}
19+
{%- if show_footer -%}
20+
<p>{{'now'|date('M j, Y')}}</p>
21+
{%- endif -%}
22+
{% endblock %}
23+
{% endembed %}

‎src/objects/overview/demo/div.twig‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
{# Used for tests #}
2+
{% embed '@cloudfour/objects/overview/overview.twig' with { overview_tag: 'div' } %}
3+
{% block header %}
4+
Header
5+
{% endblock %}
6+
{% block actions %}
7+
Actions
8+
{% endblock %}
9+
{% block content %}
10+
Content
11+
{% endblock %}
12+
{% endembed %}

‎src/objects/overview/overview.test.ts‎

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,56 @@
1+
import path from 'path';
2+
3+
import type { ElementHandle } from 'pleasantest';
4+
import { getAccessibilityTree, withBrowser } from 'pleasantest';
5+
6+
import { loadTwigTemplate } from '../../../test-utils.js';
7+
8+
/** Helper to load the Twig template file */
9+
const template = loadTwigTemplate(path.join(__dirname, './demo/basic.twig'));
10+
const divTemplate = loadTwigTemplate(path.join(__dirname, './demo/div.twig'));
11+
12+
describe('Overview object', () => {
13+
test(
14+
'should use header with section',
15+
withBrowser(async ({ utils, page }) => {
16+
await utils.injectHTML(
17+
await template({
18+
show_heading: true,
19+
show_footer: true,
20+
})
21+
);
22+
23+
const body = await page.evaluateHandle<ElementHandle>(
24+
() => document.body
25+
);
26+
expect(await getAccessibilityTree(body)).toMatchInlineSnapshot(`
27+
region
28+
banner
29+
text "Header"
30+
text "Actions"
31+
text "Content"
32+
`);
33+
})
34+
);
35+
36+
test(
37+
'should not use header with div',
38+
withBrowser(async ({ utils, page }) => {
39+
await utils.injectHTML(
40+
await divTemplate({
41+
show_heading: true,
42+
show_footer: true,
43+
})
44+
);
45+
46+
const body = await page.evaluateHandle<ElementHandle>(
47+
() => document.body
48+
);
49+
expect(await getAccessibilityTree(body)).toMatchInlineSnapshot(`
50+
text "Header"
51+
text "Actions"
52+
text "Content"
53+
`);
54+
})
55+
);
56+
});

‎src/objects/overview/overview.twig‎

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,24 @@
1-
<{{overview_tag|default('section')}}
1+
{% set _overview_tag = overview_tag|default('section') %}
2+
{#
3+
Using `header` inside a `div` causes pointless "banner" landmarks in
4+
the VoiceOver rotor. As a result, we set the default header element
5+
to `div` if the `overview_tag` is anything but `article` or `section`.
6+
#}
7+
{% set _is_sectioning = _overview_tag in ['article', 'section'] %}
8+
{% set _default_header_tag = _is_sectioning ? 'header' : 'div' %}
9+
{% set _header_tag_name = header_tag_name|default(_default_header_tag) %}
10+
11+
<{{ _overview_tag }}
212
class="o-overview"
313
{% if labelledby_id %}aria-labelledby="{{labelledby_id}}"{% endif %}
414
>
5-
<header class="o-overview__header">
15+
<{{ _header_tag_name }} class="o-overview__header">
616
{% block header %}{% endblock %}
7-
</header>
17+
</{{ _header_tag_name }}>
818
<div class="o-overview__actions">
919
{% block actions %}{% endblock %}
1020
</div>
1121
<div class="o-overview__content">
1222
{% block content %}{% endblock %}
1323
</div>
14-
</{{overview_tag|default('section')}}>
24+
</{{ _overview_tag }}>

0 commit comments

Comments
 (0)