From 12cb9359f74048ee30ed4be73c42e80101d8e7fb Mon Sep 17 00:00:00 2001 From: Oliver Byford Date: Wed, 18 Aug 2021 16:54:13 +0100 Subject: [PATCH] Avoid invalid empty actions span in summary list MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 is 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 `
` 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 `` to be a direct child of a `
`. This has been flagged by automated testing tools such as Axe (as seen in https://github.com/alphagov/govuk-frontend/issues/1806) – although we're not aware of any issues that it would cause for users in practise. Simplify the code 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] Unfortunately some browsers, including IE11 and Safari, still omit the border where a row is missing cells – so we still need to provide an empty cell for rows without actions. Replace the empty `span.govuk-summary-list__actions` with an `:after` pseudo-element as suggested by @adamliptrot-oc [4]. This requires a new modifier class `govuk-summary-list__row--no-actions` to be applied to any rows that do not have actions (unless none of the rows in the summary list have actions) [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). [4]: https://github.com/alphagov/govuk-frontend/issues/1806#issuecomment-667993696 --- src/govuk/components/summary-list/_index.scss | 46 +++++++++---------- .../components/summary-list/template.njk | 5 +- .../components/summary-list/template.test.js | 15 ------ 3 files changed, 22 insertions(+), 44 deletions(-) diff --git a/src/govuk/components/summary-list/_index.scss b/src/govuk/components/summary-list/_index.scss index 3e9b60ac5d..bea68c8180 100644 --- a/src/govuk/components/summary-list/_index.scss +++ b/src/govuk/components/summary-list/_index.scss @@ -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; @@ -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; } } @@ -109,37 +110,32 @@ // 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; } } + + // Provide an empty 'cell' for rows that don't have actions – otherwise the + // bottom border is not drawn for that part of the row in some browsers. + .govuk-summary-list__row--no-actions:after { + content: ""; + display: table-cell; + } } diff --git a/src/govuk/components/summary-list/template.njk b/src/govuk/components/summary-list/template.njk index 8434378922..e6ad25cf2e 100644 --- a/src/govuk/components/summary-list/template.njk +++ b/src/govuk/components/summary-list/template.njk @@ -16,7 +16,7 @@
{% for row in params.rows %} {% if row %} -
+
{{ row.key.html | safe if row.key.html else row.key.text }}
@@ -37,9 +37,6 @@ {% endif %} - {% elseif anyRowHasActions %} - {# Add dummy column to extend border #} - {% endif %}
{% endif %} diff --git a/src/govuk/components/summary-list/template.test.js b/src/govuk/components/summary-list/template.test.js index 8e1b69e4ee..8343ef2b48 100644 --- a/src/govuk/components/summary-list/template.test.js +++ b/src/govuk/components/summary-list/template.test.js @@ -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('') - }) }) }) })