-
Notifications
You must be signed in to change notification settings - Fork 325
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
Remove non-operational value parameter from file upload component #5311
base: main
Are you sure you want to change the base?
Conversation
For security reasons, file inputs cannot have the value property set to anything except for an empty string. This parameter has thus never actually worked for anything other than to output a non-functioning HTML attribute. We can probably get rid of it.
📋 StatsFile sizes
Modules
View stats and visualisations on the review app Action run for 0f5188d |
Rendered HTML changes to npm packagediff --git a/packages/govuk-frontend/dist/govuk/components/file-upload/template-with-value.html b/packages/govuk-frontend/dist/govuk/components/file-upload/template-with-value.html
deleted file mode 100644
index 68d350f46..000000000
--- a/packages/govuk-frontend/dist/govuk/components/file-upload/template-with-value.html
+++ /dev/null
@@ -1,6 +0,0 @@
-<div class="govuk-form-group">
- <label class="govuk-label" for="file-upload-4">
- Upload a photo
- </label>
- <input class="govuk-file-upload" id="file-upload-4" name="file-upload-4" type="file" value="C:\fakepath\myphoto.jpg">
-</div>
Action run for 0f5188d |
Other changes to npm packagediff --git a/packages/govuk-frontend/dist/govuk/components/file-upload/fixtures.json b/packages/govuk-frontend/dist/govuk/components/file-upload/fixtures.json
index 4a36de2a3..132c05002 100644
--- a/packages/govuk-frontend/dist/govuk/components/file-upload/fixtures.json
+++ b/packages/govuk-frontend/dist/govuk/components/file-upload/fixtures.json
@@ -55,22 +55,6 @@
"screenshot": false,
"html": "<div class=\"govuk-form-group govuk-form-group--error\">\n <label class=\"govuk-label\" for=\"file-upload-3\">\n Upload a file\n </label>\n <div id=\"file-upload-3-hint\" class=\"govuk-hint\">\n Your photo may be in your Pictures, Photos, Downloads or Desktop folder. Or in an app like iPhoto.\n </div>\n <p id=\"file-upload-3-error\" class=\"govuk-error-message\">\n <span class=\"govuk-visually-hidden\">Error:</span> Error message goes here\n </p>\n <input class=\"govuk-file-upload govuk-file-upload--error\" id=\"file-upload-3\" name=\"file-upload-3\" type=\"file\" aria-describedby=\"file-upload-3-hint file-upload-3-error\">\n</div>"
},
- {
- "name": "with value",
- "options": {
- "id": "file-upload-4",
- "name": "file-upload-4",
- "value": "C:\\fakepath\\myphoto.jpg",
- "label": {
- "text": "Upload a photo"
- }
- },
- "hidden": false,
- "description": "",
- "previewLayoutModifiers": [],
- "screenshot": false,
- "html": "<div class=\"govuk-form-group\">\n <label class=\"govuk-label\" for=\"file-upload-4\">\n Upload a photo\n </label>\n <input class=\"govuk-file-upload\" id=\"file-upload-4\" name=\"file-upload-4\" type=\"file\" value=\"C:\fakepath\myphoto.jpg\">\n</div>"
- },
{
"name": "with label as page heading",
"options": {
diff --git a/packages/govuk-frontend/dist/govuk/components/file-upload/macro-options.json b/packages/govuk-frontend/dist/govuk/components/file-upload/macro-options.json
index 5db62cb10..63bf01af9 100644
--- a/packages/govuk-frontend/dist/govuk/components/file-upload/macro-options.json
+++ b/packages/govuk-frontend/dist/govuk/components/file-upload/macro-options.json
@@ -11,12 +11,6 @@
"required": true,
"description": "The ID of the input."
},
- {
- "name": "value",
- "type": "string",
- "required": false,
- "description": "Optional initial value of the input."
- },
{
"name": "disabled",
"type": "boolean",
Action run for 0f5188d |
From a cursory search, folks are indeed using the value param, even if it's almost certainly not doing anything. Probably this is going to break tests, which isn't necessarily the end of the world but does mean we'd be putting out a knowlingly breaking change. I'm going to suggest we follow due process and deprecate this instead. I reckon we can still remove the test and instead of removing the param from the docs, maybe just adding a note that it's deprecated or even replacing the existing docs with a deprecation note. |
Playing it safe and putting this through the deprecation and removal process. I've put this in the v6 milestone and added a PR to deprecate it here: #5330 |
For security reasons, all contemporary web browsers disallow the
value
attribute to be set to anything other than an empty string, be that via HTML or JavaScript, effectively making it a read only attribute. This parameter has thus never actually done anything other than output a non-functioning HTML attribute value. As such, we should probably get rid of it.Our tests for this functionality appear to have been passing because it was comparing the value of the HTML attribute against what it was set to, without regarding that the underlying API property didn't match either of them.
Changes
value
parameter and related code from the component.value
parameter documentation.Thoughts
Would we consider this a breaking change? One the one hand, it's a change to the component API that has been in place since 2017. On the other, it's been non-operational that entire time.
There's a possibility that other services or forks have implemented it, on the basis of the same (flawed) test we had and which may have their own tests fail with its removal. I'm not sure if that speculation is reason enough to consider it breaking, however.