Merge pull request #2323 from alphagov/summary-list-avoid-empty-span

Avoid invalid nesting of `<span>` within a `<dd>` in summary list

CHANGELOG.md

@@ -12,9 +12,18 @@ You must remove this setting. Otherwise, you would have to conditionally add ove
 
 This was added in [pull request 1963: Remove deprecated `$govuk-border-width-form-element-error` setting](https://github.com/alphagov/govuk-frontend/pull/1963).
 
+#### Update the HTML for summary lists
+
+We've updated the HTML for the summary list component to avoid nesting `<span>` elements within `<dd>` elements, which is invalid HTML. This update only affects summary lists that include a mix of rows with and without actions.
+
+Do not include an empty `<span class="govuk-summary-list__actions"></span>` within the rows that do not have any actions. Instead, add the `govuk-summary-list__row--no-actions` modifier class to the row.
+
+This change was introduced in [pull request #2323](https://github.com/alphagov/govuk-frontend/pull/2323).
+
 ## Fixes
 
 - [#2255: Fix conditionally revealed questions getting out of sync when multiple sets of radios and checkboxes contain inputs with the same name](https://github.com/alphagov/govuk-frontend/pull/2255)
+- [#2323: Avoid invalid nesting of `<span>` within a `<dd>` in summary list](https://github.com/alphagov/govuk-frontend/pull/2323)
 
 ## 3.14.0 (Feature release)
 

src/govuk/components/summary-list/_index.scss

@@ -6,21 +6,30 @@
       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;
     }
   }
 
+  // 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;
+  }
+
   .govuk-summary-list__key,
   .govuk-summary-list__value,
   .govuk-summary-list__actions {
@@ -31,7 +40,6 @@
       padding-top: govuk-spacing(2);
       padding-right: govuk-spacing(4);
       padding-bottom: govuk-spacing(2);
-      border-bottom: 1px solid $govuk-border-colour;
     }
   }
 
@@ -63,16 +71,6 @@
     @include govuk-media-query($until: tablet) {
       margin-bottom: govuk-spacing(3);
     }
-    @include govuk-media-query($from: tablet) {
-      width: 50%;
-    }
-  }
-
-  // Expand width when value is last column (no action)
-  .govuk-summary-list__value:last-child {
-    @include govuk-media-query($from: tablet) {
-      width: 70%;
-    }
   }
 
   .govuk-summary-list__value > p {
@@ -109,36 +107,30 @@
 
   // 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;
     }
 
+    // Increase padding by 1px to compensate for 'missing' border
     @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;
       }
     }
   }
 
   // No border on specific rows
   .govuk-summary-list__row--no-border {
-    @include govuk-media-query($until: tablet) {
-      border: 0;
-    }
+    border: 0;
 
+    // Increase padding by 1px to compensate for 'missing' border
     @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;
       }
     }
   }

src/govuk/components/summary-list/template.njk

@@ -16,7 +16,7 @@
 <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 %}
-      <div class="govuk-summary-list__row {%- if row.classes %} {{ row.classes }}{% endif %}">
+      <div class="govuk-summary-list__row {%- if anyRowHasActions and not row.actions.items %} govuk-summary-list__row--no-actions{% endif %} {%- if row.classes %} {{ row.classes }}{% endif %}">
         <dt class="govuk-summary-list__key {%- if row.key.classes %} {{ row.key.classes }}{% endif %}">
           {{ row.key.html | safe if row.key.html else row.key.text }}
         </dt>
@@ -37,9 +37,6 @@
               </ul>
             {% endif %}
           </dd>
-        {% elseif anyRowHasActions %}
-          {# Add dummy column to extend border #}
-          <span class="govuk-summary-list__actions"></span>
         {% endif %}
       </div>
     {% endif %}

src/govuk/components/summary-list/template.test.js

@@ -14,14 +14,7 @@ describe('Summary list', () => {
     it('passes accessibility tests', async () => {
       const $ = render('summary-list', examples.default)
 
-      const results = await axe($.html(), {
-        rules: {
-          // In newer versions of the HTML specification wrapper
-          // <div>s are allowed in a definition list
-          dlitem: { enabled: false },
-          'definition-list': { enabled: false }
-        }
-      })
+      const results = await axe($.html())
       expect(results).toHaveNoViolations()
     })
   })
@@ -217,19 +210,42 @@ describe('Summary list', () => {
         expect($action.length).toEqual(0)
       })
 
-      it('adds dummy action columns when only some rows have actions', async () => {
+      describe('when only some rows have actions', () => {
         const $ = render('summary-list', examples['with some actions'])
+        const $component = $('.govuk-summary-list')
+
+        it('passes accessibility tests', async () => {
+          const results = await axe($.html())
+          expect(results).toHaveNoViolations()
+        })
+
+        it('does not add no-actions modifier class to rows with actions', () => {
+          // The first row has actions
+          const $firstRow = $component.find('.govuk-summary-list__row:first-child')
+          expect($firstRow.hasClass('govuk-summary-list__row--no-actions')).toBeFalsy()
+        })
 
+        it('adds no-actions modifier class to rows without actions', () => {
+          // The second row does not have actions
+          const $secondRow = $component.find('.govuk-summary-list__row:nth-child(2)')
+          expect($secondRow.hasClass('govuk-summary-list__row--no-actions')).toBeTruthy()
+        })
+      })
+
+      describe('when no rows have actions', () => {
+        const $ = render('summary-list', examples.default)
         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')
+        it('passes accessibility tests', async () => {
+          const results = await axe($.html())
+          expect(results).toHaveNoViolations()
+        })
 
-        // Second action column is a dummy
-        expect($action[1].tagName).toEqual('span')
-        expect($($action[1]).text()).toEqual('')
+        it('does not add no-actions modifier class to any of the rows', () => {
+          // The first row has actions
+          const $rows = $component.find('.govuk-summary-list__row')
+          expect($rows.hasClass('govuk-summary-list__row--no-actions')).toBeFalsy()
+        })
       })
     })
   })