Always set an explicit button type, and bump html-validate from 8.2.0 to 8.3.0

[dependabot]

Bumps html-validate from 8.2.0 to 8.3.0.

Release notes

Sourced from html-validate's releases.

v8.3.0

8.3.0 (2023-08-20)

Features

• rules: new rule no-implicit-button-type (38efd72), closes #221

Bug Fixes

• html5: <label> cannot have empty for (3626e1a), closes #223
• html5: element-required-attributes allows <button> without type (use no-implicit-button-type instead) (d32f492), closes #221

Changelog

Sourced from html-validate's changelog.

8.3.0 (2023-08-20)

Features

• rules: new rule no-implicit-button-type (38efd72), closes #221

Bug Fixes

• html5: <label> cannot have empty for (3626e1a), closes #223
• html5: element-required-attributes allows <button> without type (use no-implicit-button-type instead) (d32f492), closes #221

Commits

• 6e952b1 chore(release): 8.3.0
• f2bc6e5 Merge branch 'feature/button-type' into 'master'
• d32f492 fix(html5): element-required-attributes allows \<button> without type (u...
• 38efd72 feat(rules): new rule no-implicit-button-type
• 2b989e1 Merge branch 'ci/dep-scanner' into 'master'
• 539fd7f ci: remove deprecated license-scanner job
• 33b84fa Merge branch 'bugfix/empty-label-for' into 'master'
• 90b7d00 refactor: get test cases up to spec
• 3626e1a fix(html5): \<label> cannot have empty for
• a2d552e chore(deps): update dependency sass to v1.66.1
• Additional commits viewable in compare view

Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting @dependabot rebase.

---

Dependabot commands and options

You can trigger Dependabot actions by commenting on this PR:

• @dependabot rebase will rebase this PR
• @dependabot recreate will recreate this PR, overwriting any edits that have been made to it
• @dependabot merge will merge this PR after your CI passes on it
• @dependabot squash and merge will squash and merge this PR after your CI passes on it
• @dependabot cancel merge will cancel a previously requested merge and block automerging
• @dependabot reopen will reopen this PR if it is closed
• @dependabot close will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually
• @dependabot show <dependency name> ignore conditions will show all of the ignore conditions of the specified dependency
• @dependabot ignore <dependency name> major version will close this group update PR and stop Dependabot creating any more for the specific dependency's major version (unless you unignore this specific dependency's major version or upgrade to it yourself)
• @dependabot ignore <dependency name> minor version will close this group update PR and stop Dependabot creating any more for the specific dependency's minor version (unless you unignore this specific dependency's minor version or upgrade to it yourself)
• @dependabot ignore <dependency name> dependency will close this group update PR and stop Dependabot creating any more for the specific dependency (unless you unignore this specific dependency or upgrade to it yourself)
• @dependabot unignore <dependency name> dependency will remove all of the ignore conditions of the specified dependency
• @dependabot unignore <dependency name> <ignore condition> will remove the ignore condition of the specified dependency and ignore conditions

[36degrees]

Looks like the behaviour of the type option for buttons differs between <input type="button"> and <button>.

The documentation for the type option says:

Type of input or button – button, submit or reset. Defaults to submit. This has no effect on a elements.

This is true for <input type="button"> where the type attribute is always included:

govuk-frontend/packages/govuk-frontend/src/govuk/components/button/template.njk

Line 56 in f6b0c3e

<input value="{{ params.text }}" type="{{ params.type if params.type else 'submit' }}" {{- buttonAttributes | safe }} {{- commonAttributes | safe }}>

However when the <button> element is used, the type attribute is only included if the type option is set:

govuk-frontend/packages/govuk-frontend/src/govuk/components/button/template.njk

Line 49 in f6b0c3e

<button {%- if params.value %} value="{{ params.value }}"{% endif %}{%- if params.type %} type="{{ params.type }}"{% endif %} {{- buttonAttributes | safe }} {{- commonAttributes | safe }}>

We might want to either make this consistent or update the documentation.

Do we want to fix as part of this PR or disable the rule and create an issue to address this later?

[colinrotherham]

@dependabot rebase

[github-actions]

📋 Stats

File sizes

File	Size
dist/govuk-frontend-4.6.0.min.css	118.07 KiB
dist/govuk-frontend-4.6.0.min.js	47.1 KiB
dist/govuk-frontend-ie8-4.6.0.min.css	79.27 KiB
packages/govuk-frontend/dist/govuk/all.bundle.js	72.34 KiB
packages/govuk-frontend/dist/govuk/all.bundle.mjs	67.95 KiB
packages/govuk-frontend/dist/govuk/all.mjs	4 KiB
packages/govuk-frontend/dist/govuk/govuk-frontend-component.mjs	359 B
packages/govuk-frontend/dist/govuk/govuk-frontend.min.js	34.28 KiB
packages/govuk-frontend/dist/govuk/i18n.mjs	5.83 KiB

Modules

File	Size
all.mjs	64.5 KiB
components/accordion/accordion.mjs	20.86 KiB
components/button/button.mjs	3.67 KiB
components/character-count/character-count.mjs	20.46 KiB
components/checkboxes/checkboxes.mjs	4.37 KiB
components/error-summary/error-summary.mjs	4.96 KiB
components/exit-this-page/exit-this-page.mjs	15.42 KiB
components/header/header.mjs	2.3 KiB
components/notification-banner/notification-banner.mjs	3.5 KiB
components/radios/radios.mjs	3.37 KiB
components/skip-link/skip-link.mjs	2.12 KiB
components/tabs/tabs.mjs	8.04 KiB

View stats and visualisations on the review app

---

Action run for 95366f7

[colinrotherham]

@36degrees For the Design System failures we should probably be consistent and add missing button types in the PR

It'll catch future problems should buttons be moved into <form> wrappers and the default changes

Maybe the package will detect that in future

Here though, we can override?

Update: Changing button/template.njk isn't breaking so a quick commit?

[colinrotherham]

@36degrees I've pushed a commit, but with strings attached

The Design System uses govukButton() without type: 'button' for "Hide cookie message"

https://github.com/alphagov/govuk-design-system/blob/0c456f2da38acb82df510c8beda879e5b3888a80/views/partials/_cookie-banner.njk#L50-L51

With the pushed fix it'll default to type: 'submit' as we've not followed our own documentation

But really it's implicitly a submit button by default so still not a breaking change

[romaricpascal]

I don’t think we need to explicitly set the type to fulfil our documentation as the platform does that for us:

submit: The button submits the form data to the server. This is the default if the attribute is not specified for buttons associated with a <form>, or if the attribute is an empty or invalid value.
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/button#type

I see how that could help people debug buttons meant for JavaScript inadvertently submitting forms (though they probably should submit anyway for progressive enhancement or be injected via JavaScript with type="button" if they’re meant for JS only).

Regarding the Design System cookie banner, missing the type does not make any difference to the behaviour, so it’s not a big deal that this button defaults to being a submit.

It doesn’t really feel like working with the grain of the platform for us to explicitly, redundantly set the type. Plus that makes it look like setting a default type to submit is something we've done (so have to support/handle) while we've just made visible what the platform does.

Just wanted to checked that the platform was already handling the default before we commit into setting it ourselves, as changing afterwards will be a breaking change. I'd be very fine with disabling that rule which I see more geared towards checking the HTML of full pages than library code.

If we decide to go ahead (don't consider that comment blocking), it might be worth a little mention in the CHANGELOG, explaining that it helps debugging?

[36degrees]

I don't have a strong opinion on whether we should always set the type attribute, but I do think it'd be worth making the behaviour consistent between input type="button" and button.

[colinrotherham]

I don't have a strong opinion on whether we should always set the type attribute, but I do think it'd be worth making the behaviour consistent between input type="button" and button.

@36degrees I think that decides it?

Otherwise we'd let this example get an implicit type="text" as the default:

{{ govukButton({
  text: "Save and continue",
  element: "input"
}) }}

When you'd expect input type="button"

[romaricpascal]

Fair catch on the input bit, let's default to type="submit" then 😄

Going onwards, would we want to reconsider generating input tags from this macro? Is there a reason nowadays to prefer an <input type="button|submit|reset"> to a <button>? It seems it would also close a gap where you could generate a text input (or other kind of valid non-button input type) with the button macro by using element: "input", type: "text" as options?

[36degrees]

Hah. Somehow, I had not made the connection that we needed the type attribute for input otherwise it would be a text input! 🤦🏻

I still think it's a little bit confusing that we say the type option 'defaults to submit' but that it's explicitly set for inputs and implicitly set for buttons. But maybe not worth making a change for.

[36degrees]

Looks like we commented at the same time!

Going onwards, would we want to reconsider generating input tags from this macro? Is there a reason nowadays to prefer an to a ? It seems it would also close a gap where you could generate a text input (or other kind of valid non-button input type) with the button macro by using element: "input", type: "text" as options?

Yeah, we had a draft issue on https://github.com/orgs/alphagov/projects/41/views/1 to deprecate and remove this functionality. I think the only reason we've historically supported both options is because of bugs in IE6 and IE7(!)

Maybe something to deprecate in v5.x and remove in v6?

[colinrotherham]

Thanks all

Shall I keep the commit to always default to type="submit" for consistency reasons then?

Maybe something to deprecate in v5.x and remove in v6?

Yeah. Can always go back to an implicit type in v6

[colinrotherham]

For some history, we moved to implicit types back in:

• Simplify button types to avoid unnecessary type attribute #1538