Skip to content

Commit

Permalink
Avoid invalid empty actions span in summary list
Browse files Browse the repository at this point in the history
When a summary list has actions in only some of the rows, we currently include an empty `span` with the `govuk-summary-list__actions` class in the rows that do not have actions:

```
dl.govuk-summary-list
  div.govuk-summary-list__row
     dt.govuk-summary-list__key
     dd.govuk-summary-list__value
     dd.govuk-summary-list__actions <-- List of actions
  div.govuk-summary-list__row
     dt.govuk-summary-list__key
     dd.govuk-summary-list__value
     span.govuk-summary-list__actions <-- Empty
```

This was previously required because (from the tablet breakpoint upwards) the summary list is displayed as a table, and the bottom border on each 'row' is applied to the individual 'cells' within that row (the `key`, `value`, and `actions` elements). Omitting the `actions` element from a row therefore means that that part of the row does not have a bottom border.

However, this is invalid HTML as the content model for the `<dl>` element [1] only allows for 'either: Zero or more groups each consisting of one or more dt elements followed by one or more dd elements, optionally intermixed with script-supporting elements. Or: One or more div elements, optionally intermixed with script-supporting elements.'

As such, it's not valid for a `<span>` to be a direct child of a `<dl>`. This has been flagged by automated testing tools such as Axe (as seen in #1806) – although we're not aware of any issues that it would cause for users in practise.

Avoid the need for the empty 'cell' by moving the bottom border to the row itself. This requires us to set `border-collapse: collapse` in order to use the collapsing border model [2] as in the separated borders model (the initial border model) – 'rows, columns, row groups, and column groups cannot have borders' [3]

Remove the empty `span.govuk-summary-list__actions` from the template – although it does still work perfectly well with the existing markup, so I don't believe this is technically a breaking change.

[1]: https://html.spec.whatwg.org/multipage/grouping-content.html#the-dl-element
[2]: https://drafts.csswg.org/css2/#collapsing-borders
[3]: https://drafts.csswg.org/css2/#:~:text=Rows%2C%20columns%2C%20row%20groups%2C%20and%20column%20groups%20cannot%20have%20borders%20(i.e.%2C%20user%20agents%20must%20ignore%20the%20border%20properties%20for%20those%20elements).
  • Loading branch information
36degrees committed Aug 18, 2021
1 parent 2b41586 commit a4c6fc2
Show file tree
Hide file tree
Showing 3 changed files with 14 additions and 49 deletions.
39 changes: 14 additions & 25 deletions src/govuk/components/summary-list/_index.scss
Original file line number Diff line number Diff line change
Expand Up @@ -6,15 +6,17 @@
display: table;
width: 100%;
table-layout: fixed; // Required to allow us to wrap words that overflow.
border-collapse: collapse;
}
margin: 0; // Reset default user agent styles
@include govuk-responsive-margin(6, "bottom");
}

.govuk-summary-list__row {
border-bottom: 1px solid $govuk-border-colour;

@include govuk-media-query($until: tablet) {
margin-bottom: govuk-spacing(3);
border-bottom: 1px solid $govuk-border-colour;
}
@include govuk-media-query($from: tablet) {
display: table-row;
Expand All @@ -31,7 +33,6 @@
padding-top: govuk-spacing(2);
padding-right: govuk-spacing(4);
padding-bottom: govuk-spacing(2);
border-bottom: 1px solid $govuk-border-colour;
}
}

Expand Down Expand Up @@ -109,37 +110,25 @@

// No border on entire summary list
.govuk-summary-list--no-border {
@include govuk-media-query($until: tablet) {
.govuk-summary-list__row {
border: 0;
}
.govuk-summary-list__row {
border: 0;
}

@include govuk-media-query($from: tablet) {
.govuk-summary-list__key,
.govuk-summary-list__value,
.govuk-summary-list__actions {
// Remove 1px border, add 1px height back on
padding-bottom: govuk-spacing(2) + 1px;
border: 0;
}
.govuk-summary-list__key,
.govuk-summary-list__value,
.govuk-summary-list__actions {
padding-bottom: govuk-spacing(2) + 1px;
}
}

// No border on specific rows
.govuk-summary-list__row--no-border {
@include govuk-media-query($until: tablet) {
border: 0;
}
border: 0;

@include govuk-media-query($from: tablet) {
.govuk-summary-list__key,
.govuk-summary-list__value,
.govuk-summary-list__actions {
// Remove 1px border, add 1px height back on
padding-bottom: govuk-spacing(2) + 1px;
border: 0;
}
.govuk-summary-list__key,
.govuk-summary-list__value,
.govuk-summary-list__actions {
padding-bottom: govuk-spacing(2) + 1px;
}
}
}
9 changes: 0 additions & 9 deletions src/govuk/components/summary-list/template.njk
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,6 @@
</a>
{% endmacro -%}

{# Determine if we need 2 or 3 columns #}
{% set anyRowHasActions = false %}
{% for row in params.rows %}
{% set anyRowHasActions = true if row.actions.items | length else anyRowHasActions %}
{% endfor -%}

<dl class="govuk-summary-list {%- if params.classes %} {{ params.classes }}{% endif %}"{% for attribute, value in params.attributes %} {{attribute}}="{{value}}"{% endfor %}>
{% for row in params.rows %}
{% if row %}
Expand All @@ -37,9 +31,6 @@
</ul>
{% endif %}
</dd>
{% elseif anyRowHasActions %}
{# Add dummy column to extend border #}
<span class="govuk-summary-list__actions"></span>
{% endif %}
</div>
{% endif %}
Expand Down
15 changes: 0 additions & 15 deletions src/govuk/components/summary-list/template.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -216,21 +216,6 @@ describe('Summary list', () => {

expect($action.length).toEqual(0)
})

it('adds dummy action columns when only some rows have actions', async () => {
const $ = render('summary-list', examples['with some actions'])

const $component = $('.govuk-summary-list')
const $action = $component.find('.govuk-summary-list__actions')

// First action column contains link text
expect($action[0].tagName).toEqual('dd')
expect($($action[0]).text()).toContain('Edit')

// Second action column is a dummy
expect($action[1].tagName).toEqual('span')
expect($($action[1]).text()).toEqual('')
})
})
})
})

0 comments on commit a4c6fc2

Please sign in to comment.