-
Notifications
You must be signed in to change notification settings - Fork 332
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Avoid invalid nesting of <span>
within a <dd>
in summary list
#2323
Conversation
684d9be
to
a4c6fc2
Compare
I should have tested this more thoroughly cross-browser before raising – unfortunately IE11 and Safari (at least) still omit the border on the row when the 'cell' is missing 😩 So this might be a non-starter. |
a4c6fc2
to
4a7a515
Compare
2f2c485
to
7b67409
Compare
7b67409
to
7af828e
Compare
Updated the PR to replace the It'd be great to find a way to avoid the need for the pseudo element, but I haven't been able to. I still think that moving the border to the row makes sense, even if we need to keep the empty cell. |
7af828e
to
b1e3217
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me! Thank you so much for taking the time to make the commit messages so helpful 🙏
I've tested in Assistiv Labs with Windows High Contrast Mode and Edge, and NVDA and Firefox. I've also had a look at it with Safari on iOS v13.2 using BrowserStack. It looked fine everywhere. I also had a look at it in latest Chrome and Safari on macOS, with and without styles, comparing with the old version, and the design looked to be consistent there.
Suggested release note entry…
The Design System does not currently include an example of a summary list where only some rows have actions – we might want to consider adding one? In which case we could then link to the Design System for an example. @EoinShaughnessy thoughts? |
@36degrees Release note content looks really good. Would just suggest a slight rewording:
I put 'HTML' in the title instead of 'markdown' so we're not using 2 descriptors for the same thing. |
We wrap each name-value group in the summary list's `<dl>` with a grouping `<div>` element, which is valid according to the current WHATWG HTML spec [1]. At the time we wrote these tests, this was a relatively recent change to the HTML spec, and the axe rules had not been updated to allow it. Since then, the underlying axe rules have been updated to allow wrapping `<div>` elements [2], so we no longer need to disable the `dlitem` and `definition-list` (released in axe-core 3.2.0 [3]) [1]: https://html.spec.whatwg.org/multipage/grouping-content.html#the-dl-element [2]: dequelabs/axe-core#1284 [3]: https://github.com/dequelabs/axe-core/blob/develop/CHANGELOG.md#320-2019-03-04
When a summary list has actions in only some of the rows, we 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 `<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. Add a failing test that covers this by running Axe on the example that has rows both with and without actions. The test fails with: > Summary list › rows › actions › passes accessibility tests when some rows do not have actions > > expect(received).toHaveNoViolations(expected) > > Expected the HTML found at $('dl') to have no violations: > > <dl class="govuk-summary-list"> > > Received: > > "<dl> elements must only directly contain properly-ordered <dt> and <dd> groups, <script>, <template> or <div> elements (definition-list)" > > Fix all of the following: > List element has direct children that are not allowed inside <dt> or <dd> elements > > You can find more information on this issue here: > https://dequeuniversity.com/rules/axe/4.2/definition-list?application=axeAPI [1]: https://html.spec.whatwg.org/multipage/grouping-content.html#the-dl-element
As described in the previous commit, 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. However, it's not valid for a `<span>` to be a direct child of a `<dl>`. Replace the empty `span.govuk-summary-list__actions` with an `:after` pseudo-element as suggested by @adamliptrot-oc [1]. 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]: #1806 (comment)
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 [1] as in the separated borders model (the initial border model) – 'rows, columns, row groups, and column groups cannot have borders' [2]. Unfortunately some browsers, including IE11 and Safari, still omit the border where a row is missing cells – so we do still need to provide the empty pseudo-element 'cell' for rows without actions. [1]: https://drafts.csswg.org/css2/#collapsing-borders [2]: 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).
Rather than setting an explicit width for the 'value' column, we can leave the width out and it'll expand to use the full remaining width once the widths of the key and actions columns – both of which have defined widths – have been taken into account.
<span>
in summary list<span>
within a <dd>
in summary list
b1e3217
to
746569d
Compare
746569d
to
8edcd65
Compare
I've raised an issue about this here: alphagov/govuk-design-system#1948 |
When a summary list has actions in only some of the rows, we currently include an empty
span
with thegovuk-summary-list__actions
class in the rows that do not have actions: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
, andactions
elements). Omitting theactions
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 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.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'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. This requires a new modifier classgovuk-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)Summary of breaking changes
Where a summary list includes a mix of rows both with and without actions, for each row without any actions:
<span class="govuk-summary-list__actions"></span>
govuk-summary-list__row--no-actions
modifier class to the row itself (the wrapping<div>
that has the classgovuk-summary-list__row
)If you have written a template or macro for this component, you'll need to update the template or macro so that for rows without actions it:
<span class="govuk-summary-list__actions"></span>
govuk-summary-list__row--no-actions
modifier class to the row itselfIf you are using our provided Nunjucks templates, you should not need to make any changes as this will happen automatically when you update.
Closes #1806