Merge pull request #296 from DFE-Digital/change-govuk-submit-from-inp…

…ut-to-button

Change `govuk_submit` from an `input` element to a `button` element

lib/govuk_design_system_formbuilder/builder.rb

@@ -843,7 +843,7 @@ def govuk_check_box(attribute_name, value, unchecked_value = false, hint: {}, la
 
     # Generates a submit button, green by default
     #
-    # @param text [String] the button text
+    # @param text [String,Proc] the button text. When a +Proc+ is provided its contents will be rendered within the button element
     # @param warning [Boolean] makes the button red ({https://design-system.service.gov.uk/components/button/#warning-buttons warning}) when true
     # @param secondary [Boolean] makes the button grey ({https://design-system.service.gov.uk/components/button/#secondary-buttons secondary}) when true
     # @param classes [Array,String] Classes to add to the submit button
@@ -853,16 +853,17 @@ def govuk_check_box(attribute_name, value, unchecked_value = false, hint: {}, la
     #   client-side validation provided by the browser. This is to provide a more consistent and accessible user
     #   experience
     # @param disabled [Boolean] makes the button disabled when true
-    # @option kwargs [Hash] kwargs additional arguments are applied as attributes to the +input+ element
+    # @option kwargs [Hash] kwargs additional arguments are applied as attributes to the +button+ element
     # @param block [Block] When content is passed in via a block the submit element and the block content will
     #   be wrapped in a +<div class="govuk-button-group">+ which will space the buttons and links within
     #   evenly.
     # @raise [ArgumentError] raised if both +warning+ and +secondary+ are true
     # @return [ActiveSupport::SafeBuffer] HTML output
     # @note Only the first additional button or link (passed in via a block) will be given the
     #   correct left margin, subsequent buttons will need to be manually accounted for
-    # @note This helper always renders an +<input type='submit'>+ tag, HTML content is not supported inside. You
-    #   can place +<button>+ tags inside the form to have the same effect
+    # @note This helper always renders an +<button type='submit'>+ tag. Previous versions of this gem rendered
+    #   a +`<input type='submit'>' tag instead, but there is a {https://github.com/alphagov/govuk_elements/issues/545 longstanding bug}
+    #   with this approach where the top few pixels don't initiate a submission when clicked.
     # @see https://design-system.service.gov.uk/components/button/#stop-users-from-accidentally-sending-information-more-than-once
     #   GOV.UK double click prevention
     #

lib/govuk_design_system_formbuilder/elements/submit.rb

@@ -9,7 +9,7 @@ def initialize(builder, text, warning:, secondary:, classes:, prevent_double_cli
 
         fail ArgumentError, 'buttons can be warning or secondary' if warning && secondary
 
-        @text                 = text
+        @text                 = build_text(text)
         @prevent_double_click = prevent_double_click
         @warning              = warning
         @secondary            = secondary
@@ -26,6 +26,17 @@ def html
 
     private
 
+      def build_text(text)
+        case text
+        when String
+          text
+        when Proc
+          capture { text.call }
+        else
+          fail(ArgumentError, %(text must be a String or Proc))
+        end
+      end
+
       def button_group
         Containers::ButtonGroup.new(@builder, buttons).html
       end
@@ -35,11 +46,12 @@ def buttons
       end
 
       def submit
-        @builder.submit(@text, **attributes(@html_attributes))
+        @builder.tag.button(@text, **attributes(@html_attributes))
       end
 
       def options
         {
+          type: 'submit',
           formnovalidate: !@validate,
           disabled: @disabled,
           class: classes,

spec/govuk_design_system_formbuilder/builder/configuration/submit_configuration_spec.rb

@@ -25,15 +25,15 @@
         end
 
         specify 'should use the default value when no override supplied' do
-          expect(subject).to have_tag('input', with: { type: 'submit', value: default_submit_button_text })
+          expect(subject).to have_tag('button', with: { type: 'submit' }, text: default_submit_button_text)
         end
 
         context %(overriding with 'Engage') do
           let(:submit_button_text) { 'Engage' }
           let(:args) { [method, submit_button_text] }
 
           specify 'should use supplied value when overridden' do
-            expect(subject).to have_tag('input', with: { type: 'submit', value: submit_button_text })
+            expect(subject).to have_tag('button', with: { type: 'submit' }, text: submit_button_text)
           end
         end
       end
@@ -55,14 +55,14 @@
         end
 
         specify 'should have no formnovalidate attribute' do
-          expect(parsed_subject.at_css('input').attributes.keys).not_to include('formnovalidate')
+          expect(parsed_subject.at_css('button').attributes.keys).not_to include('formnovalidate')
         end
 
         context %(overriding with false) do
           let(:kwargs) { { validate: false } }
 
           specify 'should have a formnovalidate attribute' do
-            expect(subject).to have_tag('input', with: { type: 'submit', formnovalidate: 'formnovalidate' })
+            expect(subject).to have_tag('button', with: { type: 'submit', formnovalidate: 'formnovalidate' })
           end
         end
       end

spec/govuk_design_system_formbuilder/builder/submit_spec.rb

@@ -12,37 +12,37 @@
     it_behaves_like 'a field that supports custom branding'
 
     it_behaves_like 'a field that supports custom classes' do
-      let(:element) { 'input' }
+      let(:element) { 'button' }
       let(:default_classes) { %w(govuk-button) }
       let(:block_content) { -> { %(Example) } }
     end
 
     it_behaves_like 'a field that allows extra HTML attributes to be set' do
-      let(:described_element) { 'input' }
+      let(:described_element) { 'button' }
       let(:expected_class) { 'govuk-button' }
     end
 
-    specify 'output should be a submit input' do
-      expect(subject).to have_tag('input', with: { type: 'submit' })
+    specify 'output should be a button element' do
+      expect(subject).to have_tag('button', with: { type: 'submit' })
     end
 
     specify 'button should have the correct classes' do
-      expect(subject).to have_tag('input', with: { class: 'govuk-button' })
+      expect(subject).to have_tag('button', with: { class: 'govuk-button' })
     end
 
     specify 'button should have the correct text' do
-      expect(subject).to have_tag('input', with: { value: text })
+      expect(subject).to have_tag('button', text: text)
     end
 
     specify 'button should have the govuk-button data-module' do
-      expect(subject).to have_tag('input', with: { 'data-module' => 'govuk-button' })
+      expect(subject).to have_tag('button', with: { 'data-module' => 'govuk-button' })
     end
 
     context 'when no value is passed in' do
       subject { builder.send(method) }
 
       specify %(it should default to 'Continue) do
-        expect(subject).to have_tag('input', with: { value: 'Continue' })
+        expect(subject).to have_tag('button', text: 'Continue')
       end
     end
 
@@ -51,15 +51,15 @@
         subject { builder.send(*args.push('Create'), warning: true) }
 
         specify 'button should have the warning class' do
-          expect(subject).to have_tag('input', with: { class: %w(govuk-button govuk-button--warning) })
+          expect(subject).to have_tag('button', with: { class: %w(govuk-button govuk-button--warning) })
         end
       end
 
       context 'secondary' do
         subject { builder.send(*args.push('Create'), secondary: true) }
 
         specify 'button should have the secondary class' do
-          expect(subject).to have_tag('input', with: { class: %w(govuk-button govuk-button--secondary) })
+          expect(subject).to have_tag('button', with: { class: %w(govuk-button govuk-button--secondary) })
         end
       end
 
@@ -75,22 +75,22 @@
         subject { builder.send(*args.push('Create'), classes: %w(custom-class--one custom-class--two)) }
 
         specify 'button should have the custom class' do
-          expect(subject).to have_tag('input', with: { class: %w(govuk-button custom-class--one custom-class--two) })
+          expect(subject).to have_tag('button', with: { class: %w(govuk-button custom-class--one custom-class--two) })
         end
       end
     end
 
     describe 'preventing double clicks' do
       specify 'data attribute should be present by default' do
-        expect(subject).to have_tag('input', with: { 'data-prevent-double-click' => true })
+        expect(subject).to have_tag('button', with: { 'data-prevent-double-click' => true })
       end
 
       context 'when disabled' do
         subject { builder.send(*args.push(text), prevent_double_click: false) }
 
         specify 'data attribute should not be present by default' do
           expect(
-            parsed_subject.at_css('input').attributes.keys
+            parsed_subject.at_css('button').attributes.keys
           ).not_to include('data-prevent-double-click')
         end
       end
@@ -112,8 +112,8 @@
 
       specify 'should wrap the buttons and extra content in a button group' do
         expect(subject).to have_tag('div', with: { class: 'govuk-button-group' }) do
-          with_tag('input', value: text)
-          with_tag('a', with: { href: target })
+          with_tag('button', text: 'Continue')
+          with_tag('a', text: text, with: { href: target })
         end
       end
     end
@@ -123,23 +123,23 @@
         subject { builder.send(*args) }
 
         specify 'should have attribute formnovalidate' do
-          expect(subject).to have_tag('input', with: { type: 'submit', formnovalidate: 'formnovalidate' })
+          expect(subject).to have_tag('button', with: { type: 'submit', formnovalidate: 'formnovalidate' })
         end
       end
 
       context 'when validate is false' do
         subject { builder.send(*args, validate: false) }
 
         specify 'should have attribute formnovalidate' do
-          expect(subject).to have_tag('input', with: { type: 'submit', formnovalidate: 'formnovalidate' })
+          expect(subject).to have_tag('button', with: { type: 'submit', formnovalidate: 'formnovalidate' })
         end
       end
 
       context 'when validate is true' do
         subject { builder.send(*args, validate: true) }
 
         specify 'should have attribute formnovalidate' do
-          expect(parsed_subject.at_css('input').attributes.keys).not_to include('formnovalidate')
+          expect(parsed_subject.at_css('button').attributes.keys).not_to include('formnovalidate')
         end
       end
     end
@@ -148,17 +148,39 @@
       context 'when disabled is false' do
         subject { builder.send(*args.push('Create')) }
 
-        specify 'input should not have the disabled attribute' do
-          expect(parsed_subject.at_css('input').attributes.keys).not_to include('disabled')
+        specify 'button should not have the disabled attribute' do
+          expect(parsed_subject.at_css('button').attributes.keys).not_to include('disabled')
         end
       end
 
       context 'when disabled is true' do
         subject { builder.send(*args.push('Create'), disabled: true) }
 
-        specify 'input should have the disabled attribute' do
-          expect(parsed_subject.at_css('input').attributes.keys).to include('disabled')
-          expect(subject).to have_tag('input', with: { class: %w(govuk-button govuk-button--disabled) })
+        specify 'button should have the disabled attribute' do
+          expect(parsed_subject.at_css('button').attributes.keys).to include('disabled')
+          expect(subject).to have_tag('button', with: { class: %w(govuk-button govuk-button--disabled) })
+        end
+      end
+    end
+
+    describe 'setting the button content with a proc' do
+      let(:custom_text) { "Click me!" }
+      let(:custom_wrapper) { :strong }
+      let(:custom_content) { builder.content_tag(custom_wrapper, custom_text) }
+
+      subject { builder.send(*args, custom_content) }
+
+      specify "renders the custom content inside the button element" do
+        expect(subject).to have_tag("button") do
+          with_tag(custom_wrapper, text: custom_text)
+        end
+      end
+
+      context "when the button content is invalid" do
+        subject { builder.send(*args, Date.today) }
+
+        specify "fails with an appropriate error message" do
+          expect { subject }.to raise_error(ArgumentError, /text must be a String or Proc/)
         end
       end
     end