From 9aa4a7267a18acc4f73e036b02ce3c4bbb06cc7a Mon Sep 17 00:00:00 2001
From: Chris Hill-Scott <me@quis.cc>
Date: Thu, 1 Feb 2018 13:32:27 +0000
Subject: [PATCH] Use `<button>` not `<input>`s for form submission
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Both `<button type='submit'>Submit<button>` and
`<input type='submit' value='Submit'>` can be used to submit a form.

We have historically[1] used `<input>` because it’s better-supported by
IE6 in that:
- the `submit` attribute is mandatory on `<button>`, not on `<input>`
- the `innerHTML` of a button will be submitted to the server, not the
  value (as in other browsers)

Reasons to now use `<button>` instead:
- IE6/7 support is no longer a concern (especially with deprecation of
  TLS 1.0 on the way)
- Because an `<input>` element can’t have children, the pseudo-element
  hack[2] used to ensure the top edge of the button is clickable doesn’t
  work. We’re seeing this bug[3] affect real users in research.

1. We inhereted our buttons from Digital Marketplace, here is me making
   that change in their code:  https://github.com/alphagov/digitalmarketplace-frontend-toolkit/commit/8df7e2e79e53f85e26ca69d0439a5c7879323cd0#diff-b1420f7b7a25657d849edf90a70ef541
2. https://github.com/alphagov/govuk_frontend_toolkit/commit/24e1906c0d908f0e15609ad2376feb7dd56f5731#diff-ef0e4eb6f1e90b44b0c3fe39dce274a4R79

3. https://github.com/alphagov/govuk_elements/issues/545
---
 app/templates/components/banner.html             | 2 +-
 app/templates/components/file-upload.html        | 2 +-
 app/templates/components/page-footer.html        | 2 +-
 app/templates/views/api/keys.html                | 2 +-
 app/templates/views/check/ok.html                | 2 +-
 app/templates/views/notifications.html           | 2 +-
 app/templates/views/notifications/check.html     | 2 +-
 app/templates/views/platform-admin/index.html    | 2 +-
 app/templates/views/platform-admin/services.html | 2 +-
 app/templates/views/templates/template.html      | 4 ++--
 tests/app/main/views/test_api_integration.py     | 3 ++-
 tests/app/main/views/test_api_keys.py            | 3 ++-
 tests/app/main/views/test_jobs.py                | 2 +-
 tests/app/main/views/test_organisations.py       | 2 +-
 tests/app/main/views/test_send.py                | 2 +-
 15 files changed, 18 insertions(+), 16 deletions(-)

diff --git a/app/templates/components/banner.html b/app/templates/components/banner.html
index d797525d4..7329968a5 100644
--- a/app/templates/components/banner.html
+++ b/app/templates/components/banner.html
@@ -13,7 +13,7 @@
     {% if delete_button %}
       <form method='post'>
         <input type="hidden" name="csrf_token" value="{{ csrf_token() }}" />
-        <input type="submit" class="button" name="delete" value="{{ delete_button }}" />
+        <button type="submit" class="button" name="delete">{{ delete_button }}</button
       </form>
     {% endif %}
   </div>
diff --git a/app/templates/components/file-upload.html b/app/templates/components/file-upload.html
index 6cbb52e74..a8bfe8cba 100644
--- a/app/templates/components/file-upload.html
+++ b/app/templates/components/file-upload.html
@@ -26,6 +26,6 @@
     {% endif %}
     <label class="file-upload-filename" for="{{ field.name }}"></label>
     <input type="hidden" name="csrf_token" value="{{ csrf_token() }}" />
-    <input type="submit" class="file-upload-submit" value="Submit" />
+    <button type="submit" class="file-upload-submit">Submit</button>
   </form>
 {% endmacro %}
diff --git a/app/templates/components/page-footer.html b/app/templates/components/page-footer.html
index 5dcc69e4f..64959cdcd 100644
--- a/app/templates/components/page-footer.html
+++ b/app/templates/components/page-footer.html
@@ -11,7 +11,7 @@
   <div class="page-footer">
     {% if button_text %}
       <input type="hidden" name="csrf_token" value="{{ csrf_token() }}" />
-      <input type="submit" class="button{% if destructive %}-destructive{% endif %}" value="{{ button_text }}" />
+      <button type="submit" class="button{% if destructive %}-destructive{% endif %}">{{ button_text }}</button>
     {% endif %}
     {% if back_link %}
       <a class="page-footer-back-link" href="{{ back_link }}">{{ back_link_text }}</a>
diff --git a/app/templates/views/api/keys.html b/app/templates/views/api/keys.html
index 95d797331..7e53cd4ab 100644
--- a/app/templates/views/api/keys.html
+++ b/app/templates/views/api/keys.html
@@ -18,7 +18,7 @@
         </p>
         <form method='post'>
           <input type="hidden" name="csrf_token" value="{{ csrf_token() }}" />
-          <input type="submit" class="button" name="delete" value="Confirm" />
+          <button type="submit" class="button" name="delete">Confirm</button>
         </form>
       {% endcall %}
     </div>
diff --git a/app/templates/views/check/ok.html b/app/templates/views/check/ok.html
index aa9ca1cc8..16c8cb265 100644
--- a/app/templates/views/check/ok.html
+++ b/app/templates/views/check/ok.html
@@ -37,7 +37,7 @@
         ) }}
       {% endif %}
       {% if template.template_type != 'letter' or not request.args.from_test %}
-      <input type="submit" class="button" value="Send {{ count_of_recipients }} {{ message_count_label(count_of_recipients, template.template_type, suffix='') }}" />
+      <button type="submit" class="button">Send {{ count_of_recipients }} {{ message_count_label(count_of_recipients, template.template_type, suffix='') }}</button>
       {% else %}
         <a href="{{ url_for('main.check_messages_preview', service_id=current_service.id, template_type=template.template_type, upload_id=upload_id, filetype='pdf') }}" download class="button">Download as a printable PDF</a>
       {% endif %}
diff --git a/app/templates/views/notifications.html b/app/templates/views/notifications.html
index 29ee7e695..1b47e8906 100644
--- a/app/templates/views/notifications.html
+++ b/app/templates/views/notifications.html
@@ -34,7 +34,7 @@
       </div>
       <div class="column-one-quarter align-button-with-textbox">
         <input type="hidden" name="csrf_token" value="{{ csrf_token() }}">
-        <input type="submit" class="button" value="Search">
+        <button type="submit" class="button">Search</button>
       </div>
     </form>
 
diff --git a/app/templates/views/notifications/check.html b/app/templates/views/notifications/check.html
index a58eee691..8619daf79 100644
--- a/app/templates/views/notifications/check.html
+++ b/app/templates/views/notifications/check.html
@@ -51,7 +51,7 @@
       <input type="hidden" name="csrf_token" value="{{ csrf_token() }}" />
       {% if not error %}
         {% if template.template_type != 'letter' or not request.args.from_test %}
-        <input type="submit" class="button" value="Send 1 {{ message_count_label(1, template.template_type, suffix='') }}" />
+        <button type="submit" class="button">Send 1 {{ message_count_label(1, template.template_type, suffix='') }}</button>
         {% else %}
           <a href="{{ url_for('main.check_messages_preview', service_id=current_service.id, template_type=template.template_type, upload_id=upload_id, filetype='pdf') }}" download class="button">Download as a printable PDF</a>
         {% endif %}
diff --git a/app/templates/views/platform-admin/index.html b/app/templates/views/platform-admin/index.html
index ef9a57952..5210e188f 100644
--- a/app/templates/views/platform-admin/index.html
+++ b/app/templates/views/platform-admin/index.html
@@ -22,7 +22,7 @@
       {{ textbox(form.end_date, hint="Enter end date in format YYYY-MM-DD") }}
       {{ checkbox(form.include_from_test_key) }}
       </br>
-      <input type="submit" class="button">
+      <button type="submit" class="button">Filter</button>
     </form>
   </details>
 
diff --git a/app/templates/views/platform-admin/services.html b/app/templates/views/platform-admin/services.html
index a37267499..975514381 100644
--- a/app/templates/views/platform-admin/services.html
+++ b/app/templates/views/platform-admin/services.html
@@ -107,7 +107,7 @@
         {{ textbox(form.end_date, hint="Enter end date in format YYYY-MM-DD") }}
         {{ checkbox(form.include_from_test_key) }}
         </br>
-        <input type="submit" class="button">
+        <button type="submit" class="button">Filter</button>
       </form>
     </details>
 
diff --git a/app/templates/views/templates/template.html b/app/templates/views/templates/template.html
index e99f56609..4344f761b 100644
--- a/app/templates/views/templates/template.html
+++ b/app/templates/views/templates/template.html
@@ -23,7 +23,7 @@
         </ul>
         <form method='post'>
           <input type="hidden" name="csrf_token" value="{{ csrf_token() }}" />
-          <input type="submit" class="button" name="delete" value="Confirm" />
+          <button type="submit" class="button" name="delete">Confirm</button>
         </form>
       {% endcall %}
     </div>
@@ -37,7 +37,7 @@
         {% endif %}
         <form method='post'>
           <input type="hidden" name="csrf_token" value="{{ csrf_token() }}" />
-          <input type="submit" class="button" name="delete" value="Confirm" />
+          <button type="submit" class="button" name="delete">Confirm</button>
         </form>
       {% endcall %}
     </div>
diff --git a/tests/app/main/views/test_api_integration.py b/tests/app/main/views/test_api_integration.py
index 50b9731b7..99deaeb28 100644
--- a/tests/app/main/views/test_api_integration.py
+++ b/tests/app/main/views/test_api_integration.py
@@ -265,7 +265,8 @@ def test_should_show_confirm_revoke_api_key(
     )
     assert normalize_spaces(page.select('.banner-dangerous')[0].text) == (
         'Are you sure you want to revoke this API key? '
-        '‘some key name’ will no longer let you connect to GOV.UK Notify.'
+        '‘some key name’ will no longer let you connect to GOV.UK Notify. '
+        'Confirm'
     )
     assert mock_get_api_keys.call_args_list == [
         call(
diff --git a/tests/app/main/views/test_api_keys.py b/tests/app/main/views/test_api_keys.py
index a874eacad..3b6dc37d3 100644
--- a/tests/app/main/views/test_api_keys.py
+++ b/tests/app/main/views/test_api_keys.py
@@ -242,7 +242,8 @@ def test_should_show_confirm_revoke_api_key(
     )
     assert normalize_spaces(page.select('.banner-dangerous')[0].text) == (
         'Are you sure you want to revoke this API key? '
-        '‘some key name’ will no longer let you connect to GOV.UK Notify.'
+        '‘some key name’ will no longer let you connect to GOV.UK Notify. '
+        'Confirm'
     )
     assert mock_get_api_keys.call_args_list == [
         call(
diff --git a/tests/app/main/views/test_jobs.py b/tests/app/main/views/test_jobs.py
index 2b812ca3f..0b8bb0bde 100644
--- a/tests/app/main/views/test_jobs.py
+++ b/tests/app/main/views/test_jobs.py
@@ -255,7 +255,7 @@ def test_should_show_scheduled_job(
         template_id='5d729fbd-239c-44ab-b498-75a985f3198f',
         version=1,
     )
-    assert page.find('input', {'type': 'submit', 'value': 'Cancel sending'})
+    assert page.select_one('button[type=submit]').text.strip() == 'Cancel sending'
 
 
 def test_should_cancel_job(
diff --git a/tests/app/main/views/test_organisations.py b/tests/app/main/views/test_organisations.py
index 270185e74..14828e655 100644
--- a/tests/app/main/views/test_organisations.py
+++ b/tests/app/main/views/test_organisations.py
@@ -148,7 +148,7 @@ def test_shows_temp_logo_after_uploading_logo(request_post_manage_org_redirect):
 
 def test_save_enabled_after_uploading_logo(request_post_manage_org_redirect):
     page, _ = request_post_manage_org_redirect
-    assert not page.select_one('div.page-footer input.button').has_attr('disabled')
+    assert not page.select_one('div.page-footer button.button').has_attr('disabled')
 
 
 def test_deletes_previous_temp_logo_after_uploading_logo(logged_in_platform_admin_client, mocker, fake_uuid):
diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py
index 606f877c1..4431d589e 100644
--- a/tests/app/main/views/test_send.py
+++ b/tests/app/main/views/test_send.py
@@ -611,7 +611,7 @@ def test_send_test_doesnt_show_file_contents(
     assert page.select('h1')[0].text.strip() == 'Preview of Two week reminder'
     assert len(page.select('table')) == 0
     assert len(page.select('.banner-dangerous')) == 0
-    assert page.select('input[type=submit]')[0]['value'].strip() == 'Send 1 text message'
+    assert page.select_one('button[type=submit]').text.strip() == 'Send 1 text message'
 
 
 @pytest.mark.parametrize('endpoint, template_mock, expected_recipient', [