-
Notifications
You must be signed in to change notification settings - Fork 320
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
Add HTML validation rules for missing elements #4417
Conversation
📋 StatsFile sizes
Modules
View stats and visualisations on the review app Action run for f4f16a0 |
cf03ebb
to
fca508c
Compare
b9b689e
to
bee54bd
Compare
bee54bd
to
4efd4d5
Compare
4efd4d5
to
23f1bf5
Compare
022350c
to
5529af2
Compare
23f1bf5
to
a762f66
Compare
a762f66
to
3f69de1
Compare
3f69de1
to
e2311a8
Compare
24a4a1d
to
b2c68f7
Compare
e2311a8
to
77cfc8a
Compare
23d6855
to
9164d53
Compare
77cfc8a
to
4341ec9
Compare
const html = outdent` | ||
<div id="content" class="govuk-width-container"> | ||
${render(componentName, { | ||
context: fixture.options, | ||
fixture | ||
})} | ||
</div> | ||
` |
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.
I think this could be slightly misleading because this looks very much like one of our main wrapper elements, which I don't think is something you'd ever associate with a component using aria-describedby
.
Can we make it clearer what this is for by:
- adding the extra element after the rendered component
- changing the ID to something like
test-target-element
(and in the corresponding examples) - adding a comment to explain what it's for
const html = outdent` | |
<div id="content" class="govuk-width-container"> | |
${render(componentName, { | |
context: fixture.options, | |
fixture | |
})} | |
</div> | |
` | |
const html = outdent` | |
${render(componentName, { | |
context: fixture.options, | |
fixture | |
})} | |
<div id="test-target-element"> | |
<!-- Target for references in examples (e.g. aria-describedby) so no-missing-references can pass --> | |
</div> | |
` |
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.
@36degrees Mind if I tag @romaricpascal in?
I thought the same as you but we agreed it was acceptable for the Skip link accessibility tests
I'd like to make your suggestion there too
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.
Not sure I follow, sorry – how is the skip link involved here?
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.
Ah I'll explain, it was covered in dev catch-up after your holiday
E.g. Skip link failed Accessibility tests (JavaScript error) without a target element
So we decided that a dummy element ID was acceptable for now, but due to hard-coded defaults like default("#content", true)
we added id="content"
to the component preview wrapper just like in this PR
I like your suggestion so perhaps we can annotate the test ID in both wrappers?
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.
Here's the existing component preview wrapper with id="content"
added in 3ed9d1f
govuk-frontend/shared/lib/components.js
Lines 209 to 211 in 3ed9d1f
<div id="content" class="govuk-width-container"> | |
${render(componentName, options)} | |
</div> |
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.
Thanks, that's helpful context 👍🏻
I think annotating the test ID in both wrappers sounds like a good solution.
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.
Morning 👋
I've swapped all the #content
references in 3ed9d1f to test-target-element
Whilst the HTML validation tests lack the <div id="content">
wrapper, it's still needed by:
npx jest --selectProjects "Accessibility tests"
npx jest --selectProjects "JavaScript component tests"
But that's only because it's hard coded in href="{{ params.href | default("#content", true) }}"
Without it we'll see JavaScript warnings for every Skip link example:
console.warn
Initialising `new SkipLink()` with example 'no href' threw:
ElementError: Skip link: Target content (`id="content"`) not found
Think we're looking good now, ready for review again 👍
See comments on: #4417 (comment)
We previously enabled the rule `no-missing-references` but lots of examples target `id="test-target-element"` on the preview container which isn’t available when only the component is rendered For example from fieldset.yaml ``` - name: with describedBy hidden: true options: describedBy: test-target-element legend: text: Which option? ```
4341ec9
to
f4f16a0
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.
Looks great, thanks for making that change 👍🏻 Think it's a lot clearer what's going on now.
Totally agree. Thanks for pushing for it |
See comments on: #4417 (comment)
Add HTML validation rules for missing elements
This PR enables extra HTML validation rules:
input-missing-label
– Require input to have a labelno-missing-references
– Require all form field and ARIA references to existBut also adds the HTML validation preset
html-validate:document
for testing govuk/template.njkThe good news is we’re fully valid already, but we’d be extra safe 😅
Missing elements
To resolve missing elements, we could use
render()
renderPreview()
to render the entire pageBut instead I've taken the
id="content"
wrapper like we did for Skip link accessibility tests in #4346 and updated it with @36degrees's idea to use a dummy element instead:This passes validation for examples that add
aria-describedby
etc such as in fieldset.yaml