From 4bdc8b15dfce0dc700d2e0c3f10ce3abdd9c0b8f Mon Sep 17 00:00:00 2001 From: Stephen Nelson Date: Wed, 29 Jul 2026 09:21:30 +0930 Subject: [PATCH 01/22] Improve CI resillience --- .github/workflows/test.yml | 10 ++++++++++ spec/support/capybara.rb | 4 ++++ 2 files changed, 14 insertions(+) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index cba953c..3d14f34 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -19,3 +19,13 @@ jobs: - name: Test run: | bin/ci + env: + FERRUM_PROCESS_TIMEOUT: 30 + - name: Archive screenshots and logs + if: failure() + uses: actions/upload-artifact@v7 + with: + name: test-failure-outputs + path: | + spec/dummy/tmp/screenshots + spec/dummy/log diff --git a/spec/support/capybara.rb b/spec/support/capybara.rb index 65771fe..dc3bda2 100644 --- a/spec/support/capybara.rb +++ b/spec/support/capybara.rb @@ -19,6 +19,10 @@ # required for docker (github-ci) browser_options: { "no-sandbox": nil }, } + + # Boot the shared browser outside any example so no spec's timing includes + # the first Chrome launch. The driver memoises it, so later groups no-op. + Capybara.current_session.driver.browser end config.include Capybara::RSpecMatchers, type: :request From 66ffd14ac3dc75810933ad2759c519411ee39f8d Mon Sep 17 00:00:00 2001 From: Stephen Nelson Date: Wed, 22 Jul 2026 12:54:42 +0930 Subject: [PATCH 02/22] Attachments: tests and examples with multiple files and upstream govuk_field_field --- .../app/controllers/profiles_controller.rb | 4 +- spec/dummy/app/models/profile.rb | 1 + spec/dummy/app/views/profiles/_form.html.erb | 2 + spec/dummy/app/views/profiles/show.html.erb | 18 +++ spec/requests/profile_gallery_spec.rb | 108 ++++++++++++++++++ spec/system/file_field_spec.rb | 9 +- 6 files changed, 139 insertions(+), 3 deletions(-) create mode 100644 spec/requests/profile_gallery_spec.rb diff --git a/spec/dummy/app/controllers/profiles_controller.rb b/spec/dummy/app/controllers/profiles_controller.rb index 96a0ec6..c747604 100644 --- a/spec/dummy/app/controllers/profiles_controller.rb +++ b/spec/dummy/app/controllers/profiles_controller.rb @@ -60,7 +60,9 @@ def profile_params :name, :email, :bio, :active, :age, :status, :country, :description, :avatar, :cv, # govuk_date_field submits multiparameter date components - "born_on(1i)", "born_on(2i)", "born_on(3i)" + "born_on(1i)", "born_on(2i)", "born_on(3i)", + # has_many_attached submits an array of uploads / signed ids + gallery: [] ) end end diff --git a/spec/dummy/app/models/profile.rb b/spec/dummy/app/models/profile.rb index f4b9927..ddf3628 100644 --- a/spec/dummy/app/models/profile.rb +++ b/spec/dummy/app/models/profile.rb @@ -72,6 +72,7 @@ class Profile < ApplicationRecord has_rich_text :description has_one_attached :avatar has_one_attached :cv + has_many_attached :gallery COUNTRIES = ["Australia", "New Zealand", "United Kingdom", "Canada", "Ireland"].freeze diff --git a/spec/dummy/app/views/profiles/_form.html.erb b/spec/dummy/app/views/profiles/_form.html.erb index be4590a..39836a6 100644 --- a/spec/dummy/app/views/profiles/_form.html.erb +++ b/spec/dummy/app/views/profiles/_form.html.erb @@ -13,7 +13,9 @@ <%= f.govuk_combobox :country, Profile::COUNTRIES %> <%= f.govuk_rich_textarea :description %> <%= f.govuk_image_field :avatar, optional: true %> + <%= f.govuk_image_field :gallery, multiple: true, optional: true %> <%= f.govuk_document_field :cv, optional: true %> + <%= f.govuk_file_field :cv, optional: true, javascript: true %> <%= f.govuk_submit %> <% end %> diff --git a/spec/dummy/app/views/profiles/show.html.erb b/spec/dummy/app/views/profiles/show.html.erb index 42414e6..f0ffdc5 100644 --- a/spec/dummy/app/views/profiles/show.html.erb +++ b/spec/dummy/app/views/profiles/show.html.erb @@ -11,6 +11,24 @@
<%= profile.country %>
Description
<%= profile.description %>
+
Avatar
+
+ <% if profile.avatar.attached? %> + <%= image_tag profile.avatar, alt: profile.name %> + <% end %> +
+
Gallery
+
+ <% profile.gallery.each do |image| %> + <%= image_tag image, alt: image.filename.to_s %> + <% end %> +
+
CV
+
+ <% if profile.cv.attached? %> + <%= link_to profile.cv.filename.to_s, profile.cv %> + <% end %> +
<%= link_to("Edit", edit_profile_path(profile)) %> diff --git a/spec/requests/profile_gallery_spec.rb b/spec/requests/profile_gallery_spec.rb new file mode 100644 index 0000000..b1d0ce4 --- /dev/null +++ b/spec/requests/profile_gallery_spec.rb @@ -0,0 +1,108 @@ +# frozen_string_literal: true + +require "rails_helper" + +# Exercises updating Profile#gallery (has_many_attached) through the controller. +# +# The gallery field round-trips as an array of `profile[gallery][]` values that +# mirrors the rendered form: a leading blank entry, then one hidden input per +# retained file carrying that blob's signed id, then the type=file input (empty +# in these specs — new files arrive as signed ids from the async direct upload). +# +# Because has_many_attached replaces the whole collection on assignment, the set +# submitted IS the resulting set: +# * the blank keeps the param present so an otherwise-empty array still clears +# * a signed id that's present is retained; one that's dropped is removed +# * a brand-new signed id is added +RSpec.describe "Updating a profile's gallery" do + let(:profile) { Profile.create!(name: "Ada Lovelace", email: "ada@example.com") } + + # Mirrors the submitted field order: blank first, then a signed id per file. + def gallery_params(*signed_ids) + { profile: { gallery: ["", *signed_ids] } } + end + + # A blob that exists but isn't attached to the profile yet — stands in for a + # file the browser has already direct-uploaded and injected as a hidden input. + def uploaded_blob(filename) + ActiveStorage::Blob.create_and_upload!( + io: File.open(file_fixture("avatar.png")), + filename:, + content_type: "image/png", + ) + end + + # Attaches a file directly, as though it were saved on a previous request. + def attach_existing(filename) + profile.gallery.attach(io: File.open(file_fixture("avatar.png")), filename:, content_type: "image/png") + profile.gallery.blobs.find { |blob| blob.filename.to_s == filename } + end + + describe "adding a single file (none => 1)" do + let(:blob) { uploaded_blob("added.png") } + + it "attaches the newly uploaded blob" do + patch profile_path(profile), params: gallery_params(blob.signed_id) + + expect(profile.reload.gallery.blobs).to contain_exactly(blob) + end + + it "redirects back to the profile" do + patch profile_path(profile), params: gallery_params(blob.signed_id) + + expect(response).to have_http_status(:see_other) + end + end + + describe "removing the only file (1 => none)" do + it "detaches it when just the blank entry is submitted" do + attach_existing("existing.png") + + expect do + patch profile_path(profile), params: gallery_params + end.to change { profile.reload.gallery.count }.from(1).to(0) + end + end + + describe "adding one and removing another (1 => 1)" do + it "replaces the old blob with the new one" do + attach_existing("existing.png") + new_blob = uploaded_blob("added.png") + + # The existing signed id is omitted (removed) and the new one submitted. + # contain_exactly is exhaustive: the new blob is the whole gallery. + patch profile_path(profile), params: gallery_params(new_blob.signed_id) + + expect(profile.reload.gallery.blobs).to contain_exactly(new_blob) + end + end + + describe "submitting an unchanged gallery (1 => 1)" do + it "keeps the same blob attached" do + existing = attach_existing("existing.png") + + patch profile_path(profile), params: gallery_params(existing.signed_id) + + expect(profile.reload.gallery.blobs).to contain_exactly(existing) + end + end + + # Without JS there's no async direct upload, so files arrive as real multipart + # uploads through the type=file input, appended after the blank entry (and any + # retained signed ids) in the same profile[gallery][] array. + describe "no-JS fallback: uploading through the file input" do + it "adds a single file (none => 1)" do + patch profile_path(profile), params: gallery_params(fixture_file_upload("avatar.png", "image/png")) + + expect(profile.reload.gallery.blobs.map { |blob| blob.filename.to_s }).to contain_exactly("avatar.png") + end + + it "adds multiple files (none => 2)" do + uploads = [fixture_file_upload("avatar.png", "image/png"), fixture_file_upload("avatar.png", "image/png")] + + expect do + patch profile_path(profile), params: gallery_params(*uploads) + end.to change { profile.reload.gallery.count }.from(0).to(2) + end + end +end diff --git a/spec/system/file_field_spec.rb b/spec/system/file_field_spec.rb index 94a58b6..e3b937c 100644 --- a/spec/system/file_field_spec.rb +++ b/spec/system/file_field_spec.rb @@ -11,7 +11,7 @@ it "previews the chosen image and can remove it", :aggregate_failures do visit new_profile_path - within(".govuk-image-field") do + within(avatar_field) do # No file chosen yet, so the preview is hidden. expect(page).to have_css("[data-govuk-image-field-target=preview]", visible: :hidden) @@ -58,7 +58,7 @@ it "highlights the field on dragenter and clears it on dragleave", :aggregate_failures do visit new_profile_path - field = find(".govuk-image-field") + field = avatar_field expect(field[:class]).not_to include("droppable") @@ -70,6 +70,11 @@ end end + # The form renders two image fields (avatar and gallery); scope to avatar's. + def avatar_field + find(".govuk-image-field:has(input[name='profile[avatar]'])") + end + # Dispatches a DragEvent carrying an (empty) DataTransfer onto the element, so # the controller's dragenter/dragleave handlers run exactly as they would for # a real drag interaction. Cuprite has no native drag-with-files API. From 31c307f8710bebda249fe5a8109a007d7d259014 Mon Sep 17 00:00:00 2001 From: Stephen Nelson Date: Wed, 22 Jul 2026 15:00:32 +0930 Subject: [PATCH 03/22] Attachments: config for using legacy file input components and JS --- lib/katalyst/govuk/form_builder/config.rb | 10 ++++++++++ ...{file_field_spec.rb => legacy_file_fields_spec.rb} | 11 ++++++++++- 2 files changed, 20 insertions(+), 1 deletion(-) rename spec/system/{file_field_spec.rb => legacy_file_fields_spec.rb} (93%) diff --git a/lib/katalyst/govuk/form_builder/config.rb b/lib/katalyst/govuk/form_builder/config.rb index 9874dfb..4c4c827 100644 --- a/lib/katalyst/govuk/form_builder/config.rb +++ b/lib/katalyst/govuk/form_builder/config.rb @@ -26,6 +26,16 @@ def image_mime_types=(value) end config.image_mime_types = %w[image/png image/gif image/jpeg image/webp].freeze + + def use_legacy_file_fields? + config.use_legacy_file_fields + end + + def use_legacy_file_fields=(value) + config.use_legacy_file_fields = value + end + + config.use_legacy_file_fields = false end end end diff --git a/spec/system/file_field_spec.rb b/spec/system/legacy_file_fields_spec.rb similarity index 93% rename from spec/system/file_field_spec.rb rename to spec/system/legacy_file_fields_spec.rb index e3b937c..25a9e33 100644 --- a/spec/system/file_field_spec.rb +++ b/spec/system/legacy_file_fields_spec.rb @@ -6,7 +6,16 @@ # govuk_document_field (app/javascript/katalyst/govuk/controllers/*). These # enhance a plain file input with a live preview, a "remove" button and # drag-and-drop, none of which can be observed without a real browser. -RSpec.describe "File field javascript" do +RSpec.describe "Legacy file fields javascript" do + delegate :config, to: :GOVUKDesignSystemFormBuilder + + around do |example| + config.use_legacy_file_fields = true + example.run + ensure + config.use_legacy_file_fields = false + end + describe "image field" do it "previews the chosen image and can remove it", :aggregate_failures do visit new_profile_path From 806e58a73f8a2640df9c650eadfb7bbcb6984da7 Mon Sep 17 00:00:00 2001 From: Stephen Nelson Date: Wed, 22 Jul 2026 15:01:00 +0930 Subject: [PATCH 04/22] Attachments: govuk_attachment_input with direct upload support --- .github/workflows/test.yml | 4 + .../katalyst/govuk/components/_index.scss | 1 + .../govuk/components/attachment/_index.scss | 3 + .../govuk/components/attachment/_mixin.scss | 47 +++ .../katalyst/govuk/form_builder/builder.rb | 139 ++++++- .../govuk/form_builder/elements/attachment.rb | 38 ++ .../govuk/form_builder/traits/attachment.rb | 134 +++++++ .../controllers/attachment_controller.js | 240 ++++++++++++ .../controllers/file_upload_controller.js | 352 ++++++++++++++++++ .../katalyst/govuk/controllers/index.js | 10 + spec/builders/document_field_spec.rb | 218 +++++++++++ spec/builders/gallery_field_spec.rb | 75 ++++ spec/builders/image_field_spec.rb | 271 ++++++++++++++ .../blocking_direct_uploads_controller.rb | 25 ++ spec/dummy/app/models/profile.rb | 2 +- .../examples/attachment/enhancement.html.erb | 21 ++ spec/dummy/app/views/profiles/_form.html.erb | 3 +- spec/dummy/config/routes.rb | 3 + spec/factories/profiles.rb | 1 + spec/rails_helper.rb | 11 +- spec/requests/profiles/avatar_spec.rb | 61 +++ spec/requests/profiles/cv_spec.rb | 51 +++ .../gallery_spec.rb} | 79 +++- spec/requests/profiles_spec.rb | 27 +- spec/support/attachment_field_helpers.rb | 56 +++ spec/support/attachment_request_helpers.rb | 31 ++ spec/support/direct_upload_helpers.rb | 48 +++ spec/support/factory_bot.rb | 15 + spec/system/attachment/drag_and_drop_spec.rb | 104 ++++++ spec/system/attachment/enhancement_spec.rb | 86 +++++ spec/system/attachment/no_upload_mode_spec.rb | 87 +++++ spec/system/attachment/remove_spec.rb | 98 +++++ spec/system/attachment/replace_spec.rb | 74 ++++ spec/system/attachment/round_trip_spec.rb | 55 +++ spec/system/attachment/status_spec.rb | 65 ++++ spec/system/attachment/upload_spec.rb | 102 +++++ 36 files changed, 2607 insertions(+), 30 deletions(-) create mode 100644 app/assets/stylesheets/katalyst/govuk/components/attachment/_index.scss create mode 100644 app/assets/stylesheets/katalyst/govuk/components/attachment/_mixin.scss create mode 100644 app/helpers/katalyst/govuk/form_builder/elements/attachment.rb create mode 100644 app/helpers/katalyst/govuk/form_builder/traits/attachment.rb create mode 100644 app/javascript/katalyst/govuk/controllers/attachment_controller.js create mode 100644 app/javascript/katalyst/govuk/controllers/file_upload_controller.js create mode 100644 spec/builders/document_field_spec.rb create mode 100644 spec/builders/gallery_field_spec.rb create mode 100644 spec/builders/image_field_spec.rb create mode 100644 spec/dummy/app/controllers/blocking_direct_uploads_controller.rb create mode 100644 spec/dummy/app/views/examples/attachment/enhancement.html.erb create mode 100644 spec/requests/profiles/avatar_spec.rb create mode 100644 spec/requests/profiles/cv_spec.rb rename spec/requests/{profile_gallery_spec.rb => profiles/gallery_spec.rb} (57%) create mode 100644 spec/support/attachment_field_helpers.rb create mode 100644 spec/support/attachment_request_helpers.rb create mode 100644 spec/support/direct_upload_helpers.rb create mode 100644 spec/support/factory_bot.rb create mode 100644 spec/system/attachment/drag_and_drop_spec.rb create mode 100644 spec/system/attachment/enhancement_spec.rb create mode 100644 spec/system/attachment/no_upload_mode_spec.rb create mode 100644 spec/system/attachment/remove_spec.rb create mode 100644 spec/system/attachment/replace_spec.rb create mode 100644 spec/system/attachment/round_trip_spec.rb create mode 100644 spec/system/attachment/status_spec.rb create mode 100644 spec/system/attachment/upload_spec.rb diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 3d14f34..05f7350 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -6,6 +6,10 @@ jobs: test: runs-on: ubuntu-latest steps: + - name: Install dependencies + env: + DEPENDENCIES: libvips + run: sudo apt-get install -y $DEPENDENCIES || (sudo apt-get update && sudo apt-get install -y $DEPENDENCIES) - uses: actions/checkout@v6 - uses: ruby/setup-ruby@v1 with: diff --git a/app/assets/stylesheets/katalyst/govuk/components/_index.scss b/app/assets/stylesheets/katalyst/govuk/components/_index.scss index c8a39f0..e06b770 100644 --- a/app/assets/stylesheets/katalyst/govuk/components/_index.scss +++ b/app/assets/stylesheets/katalyst/govuk/components/_index.scss @@ -1,3 +1,4 @@ +@use "attachment"; @use "govuk-frontend/dist/govuk/components/character-count"; @use "govuk-frontend/dist/govuk/components/checkboxes"; @use "combobox"; diff --git a/app/assets/stylesheets/katalyst/govuk/components/attachment/_index.scss b/app/assets/stylesheets/katalyst/govuk/components/attachment/_index.scss new file mode 100644 index 0000000..b1eec39 --- /dev/null +++ b/app/assets/stylesheets/katalyst/govuk/components/attachment/_index.scss @@ -0,0 +1,3 @@ +@use "mixin"; + +@include mixin.styles; diff --git a/app/assets/stylesheets/katalyst/govuk/components/attachment/_mixin.scss b/app/assets/stylesheets/katalyst/govuk/components/attachment/_mixin.scss new file mode 100644 index 0000000..c845ff5 --- /dev/null +++ b/app/assets/stylesheets/katalyst/govuk/components/attachment/_mixin.scss @@ -0,0 +1,47 @@ +@use "govuk-frontend/dist/govuk/base"; + +@mixin styles { + :where(.govuk-attachment) { + display: grid; + grid-template-areas: "preview caption actions"; + + .preview { + grid-area: preview; + } + + .caption { + grid-area: caption; + } + + .actions { + grid-area: actions; + } + + // The remove button requires JavaScript; without it the select is the + // visible control. + .actions button { + display: none; + } + } + + // With JavaScript running the button is the figure's only interactive + // control; the select still carries the submitted value but leaves the + // display, tab order, and accessibility tree. + .govuk-frontend-supported :where(.govuk-attachment) { + .actions select { + display: none; + } + + .actions button { + display: revert; + } + } + + // Hide duplicate inputs when multiple is not enabled, this allows reverting + // but the value from the last select will overwrite the others in save. + .govuk-file-upload-wrapper:has(input[type="file"]:not([multiple])) { + .govuk-attachment:has(+ .govuk-attachment) { + display: none; + } + } +} diff --git a/app/helpers/katalyst/govuk/form_builder/builder.rb b/app/helpers/katalyst/govuk/form_builder/builder.rb index fbae7e5..4778fa6 100644 --- a/app/helpers/katalyst/govuk/form_builder/builder.rb +++ b/app/helpers/katalyst/govuk/form_builder/builder.rb @@ -7,6 +7,9 @@ module Builder extend ActiveSupport::Concern included do + # Delegate image_tag for attachment previews + delegate :image_tag, to: :@template + # Overwrite GOVUK default to set small to true # @see GOVUKDesignSystemFormBuilder::Builder#govuk_collection_radio_buttons def govuk_collection_radio_buttons(attribute_name, collection, value_method, text_method = nil, @@ -248,6 +251,110 @@ def govuk_combobox(attribute_name, options_or_src = [], options: {}, label: {}, ).html end + # Generates an input of type +file+ with active storage and preview support. + # + # @param attribute_name [Symbol] The name of the attribute + # @option label text [String] the label text + # @option label tag [Symbol,String] the label's wrapper tag, intended to allow labels to act as page headings + # @option label size [String] the size of the label font, can be +xl+, +l+, +m+, +s+ or nil + # @option label hidden [Boolean] control the visability of the label. Hidden labels will stil be read by + # screenreaders + # @option label kwargs [Hash] additional arguments are applied as attributes on the +label+ element + # @param caption [Hash] configures or sets the caption content which is inserted above the label + # @option caption text [String] the caption text + # @option caption size [String] the size of the caption, can be +xl+, +l+ or +m+. Defaults to +m+ + # @option caption kwargs [Hash] additional arguments are applied as attributes on the caption +span+ element + # @param hint [Hash,Proc] The content of the hint. No hint will be added if 'text' is left +nil+. When a + # +Proc+ is supplied the hint will be wrapped in a +div+ instead of a +span+ + # @option hint text [String] the hint text + # @option hint kwargs [Hash] additional arguments are applied as attributes to the hint + # @option kwargs [Hash] kwargs additional arguments are applied as attributes to the +input+ element + # @param form_group [Hash] configures the form group + # @option form_group kwargs [Hash] additional attributes added to the form group + # @param before_input [String,Proc] the content injected before the input. No content will be added if left + # +nil+ + # @param after_input [String,Proc] the content injected after the input. No content will be added if left + # +nil+ + # @param choose_files_button_text [String] The text of the button that opens the file picker. Default is + # "Choose file". If javascript is not provided, this option will be ignored. + # @param drop_instruction_text [String] The text informing users they can drop files. Default is + # "or drop file". If javascript is not provided, this option will be ignored. + # @param multiple_files_chosen_text [Hash] The text displayed when multiple files have been chosen by the + # user. The component will replace the %{count} placeholder with the number of files selected. This uses + # the govuk-frontend pluralisation rules. If javascript is not provided, this option will be ignored. + # @param multiple_files_chosen_one_text [String] The text displayed when JavaScript is enabled and one file + # has been chosen by the user. The component will replace the %{count} placeholder with the number of files + # selected. This can also be set by passing a hash with key +one:+ to +multiple_files_chosen_text+. + # @param multiple_files_chosen_other_text [String] The text displayed when JavaScript is enabled and multiple + # files have been chosen by the user. The component will replace the %{count} placeholder with the number of + # files selected. This can also be set by passing a hash with key +other:+ to +multiple_files_chosen_text+. + # @param no_file_chosen_text [String] The text displayed when no file has been chosen by the user. Default is + # "No file chosen". If javascript is not provided, this option will be ignored. + # @param entered_drop_zone_text [String] The text announced by assistive technology when user drags files and + # enters the drop zone. Default is "Entered drop zone". If javascript is not provided, this option will be + # ignored. + # @param left_drop_zone_text [String] The text announced by assistive technology when user drags files and + # leaves the drop zone without dropping. Default is "Left drop zone". If javascript is not provided, this + # option will be ignored. + # @param & [Block] arbitrary HTML that will be rendered between the hint and the input + # + # @example A photo upload field with file type specifier and injected content + # = f.govuk_attachment_field :photo, label: { text: 'Upload your photo' }, accept: 'image/*' do + # + # p.govuk-inset-text + # | Explicit images will result in account termination + # + # @example A CV upload field with label as a proc + # = f.govuk_attachment_field :cv, label: -> { tag.h3('Upload your CV') } + # + # @see https://design-system.service.gov.uk/components/file-upload/ GOV.UK file upload + # @see https://design-system.service.gov.uk/styles/typography/#headings-with-captions Headings with captions + # @see https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/file MDN documentation for file upload + def govuk_attachment_field( + attribute_name, + label: {}, + caption: {}, + hint: {}, + form_group: {}, + before_input: nil, + after_input: nil, + choose_files_button_text: nil, + drop_instruction_text: nil, + multiple_files_chosen_text: nil, + multiple_files_chosen_one_text: nil, + multiple_files_chosen_other_text: nil, + no_file_chosen_text: nil, + entered_drop_zone_text: nil, + left_drop_zone_text: nil, + direct_upload: true, + direct_upload_url: (self.direct_upload_url if direct_upload), + **, + & + ) + Elements::Attachment.new( + self, + object_name, + attribute_name, + label:, + caption:, + hint:, + form_group:, + before_input:, + after_input:, + direct_upload_url:, + choose_files_button_text:, + drop_instruction_text:, + multiple_files_chosen_text:, + multiple_files_chosen_one_text:, + multiple_files_chosen_other_text:, + no_file_chosen_text:, + entered_drop_zone_text:, + left_drop_zone_text:, + **, + & + ).html + end + # Generates a file input element for uploading documents. # # @example A upload field with label as a proc @@ -261,9 +368,15 @@ def govuk_document_field(attribute_name, mime_types: config.document_mime_types, **, &) - Elements::Document.new( - self, object_name, attribute_name, label:, caption:, hint:, form_group:, mime_types:, **, & - ).html + if config.use_legacy_file_fields + Elements::Document.new( + self, object_name, attribute_name, label:, caption:, hint:, form_group:, mime_types:, **, & + ).html + else + govuk_attachment_field( + attribute_name, label:, caption:, hint:, form_group:, accept: mime_types&.join(","), **, & + ) + end end # Generates a file input element with a preview for uploading images. @@ -309,9 +422,15 @@ def govuk_image_field(attribute_name, mime_types: config.image_mime_types, **, &) - Elements::Image.new( - self, object_name, attribute_name, label:, caption:, hint:, form_group:, mime_types:, **, & - ).html + if config.use_legacy_file_fields + Elements::Image.new( + self, object_name, attribute_name, label:, caption:, hint:, form_group:, mime_types:, **, & + ).html + else + govuk_attachment_field( + attribute_name, label:, caption:, hint:, form_group:, accept: mime_types&.join(","), **, & + ) + end end # Keep track of whether we are inside a fieldset @@ -322,6 +441,14 @@ def fieldset_context private + def direct_upload_url + if @template.respond_to?(:rails_direct_uploads_url) + @template.rails_direct_uploads_url + elsif @template.respond_to?(:main_app) && @template.main_app.respond_to?(:rails_direct_uploads_url) + @template.main_app.rails_direct_uploads_url + end + end + def enum_values(attribute_name) object.class.defined_enums[attribute_name.to_s].keys end diff --git a/app/helpers/katalyst/govuk/form_builder/elements/attachment.rb b/app/helpers/katalyst/govuk/form_builder/elements/attachment.rb new file mode 100644 index 0000000..1269f75 --- /dev/null +++ b/app/helpers/katalyst/govuk/form_builder/elements/attachment.rb @@ -0,0 +1,38 @@ +# frozen_string_literal: true + +module Katalyst + module GOVUK + module FormBuilder + module Elements + class Attachment < GOVUKDesignSystemFormBuilder::Elements::File + include FormBuilder::Traits::Attachment + + def initialize(builder, object_name, attribute_name, direct_upload_url:, **, &) + super(builder, object_name, attribute_name, javascript: true, **, &) + + @direct_upload_url = direct_upload_url + + raise ArgumentError, "Unsupported attribute type #{value.class} for #{attribute_name}" unless attachment? + end + + def options + super.merge( + "data-direct-upload-url" => @direct_upload_url, + multiple: many?, + ) + end + + private + + def file + safe_join([attachment, @builder.file_field(@attribute_name, attributes(@html_attributes))]) + end + + def file_with_javascript_markup + tag.div(class: "#{brand}-file-upload-wrapper", data: { controller: "#{brand}-file-upload" }, **i18n_data) { file } + end + end + end + end + end +end diff --git a/app/helpers/katalyst/govuk/form_builder/traits/attachment.rb b/app/helpers/katalyst/govuk/form_builder/traits/attachment.rb new file mode 100644 index 0000000..c6f0841 --- /dev/null +++ b/app/helpers/katalyst/govuk/form_builder/traits/attachment.rb @@ -0,0 +1,134 @@ +# frozen_string_literal: true + +require "govuk_design_system_formbuilder" + +module Katalyst + module GOVUK + module FormBuilder + module Traits + # Generates attachment inputs and previews for ActiveStorage associations. + module Attachment + extend ActiveSupport::Concern + + include ActionView::Helpers::NumberHelper + + def attachment? + value.is_a?(ActiveStorage::Attached) + end + + def one? + value.is_a?(ActiveStorage::Attached::One) + end + + def many? + value.is_a?(ActiveStorage::Attached::Many) + end + + def value + @builder.object.send(@attribute_name) + end + + delegate :attached?, to: :value + + # @return [ActiveSupport::SafeBuffer|nil] + def attachment + return unless attached? + + if one? + attachment_for(value.blob) + elsif many? + safe_join(value.blobs.map { |blob| attachment_for(blob) }) + end + end + + # @param [ActiveStorage::Blob] blob + # @return [ActiveSupport::SafeBuffer|nil] + def attachment_for(blob) + tag.figure(class: "#{brand}-attachment", + aria: { labelledby: attachment_id_for(blob, :caption) }, + data: { controller: "#{brand}-attachment" }) do + safe_join([ + attachment_preview_for(blob), + attachment_caption_for(blob), + attachment_actions_for(blob), + ]) + end + end + + # A
with form elements for managing the attachment. + # @param [ActiveStorage::Blob] blob + # @return [ActiveSupport::SafeBuffer|nil] + def attachment_actions_for(blob) + return if blob.nil? + + tag.div(class: "actions") do + safe_join([ + attachment_input_for(blob), + attachment_remove_for(blob), + ]) + end + end + + # A + + + + +
+ + `; + + const figure = template.content.firstElementChild; + + figure.querySelector(".filename").textContent = file.name; + figure.querySelector(".size").textContent = humanSize(file.size); + + const [keep, remove] = figure.querySelectorAll("option"); + keep.textContent = file.name; + remove.textContent = `Remove ${file.name}`; + + figure + .querySelector("button") + .setAttribute("aria-label", `Remove ${file.name}`); + + // The figure carries its File until an attachment controller connects and + // claims it for upload; while it remains, the file is still in the input's + // FileList and will submit as ordinary multipart. + figure.file = file; + + return figure; +} + +const UNITS = ["Bytes", "KB", "MB", "GB", "TB", "PB"]; + +function humanSize(bytes) { + if (bytes === 1) return "1 Byte"; + if (bytes < 1024) return `${bytes} Bytes`; + + const exp = Math.min(Math.floor(Math.log2(bytes) / 10), UNITS.length - 1); + const value = Number((bytes / 1024 ** exp).toPrecision(3)); + + return `${value} ${UNITS[exp]}`; +} + +function createProgressTag(labelId, brand = "govuk") { + const progress = document.createElement("PROGRESS"); + progress.className = `${brand}-attachment-progress`; + if (labelId) progress.setAttribute("aria-labelledby", labelId); + progress.value = 0; + progress.max = 100; + return progress; +} diff --git a/app/javascript/katalyst/govuk/controllers/file_upload_controller.js b/app/javascript/katalyst/govuk/controllers/file_upload_controller.js new file mode 100644 index 0000000..e7f1f68 --- /dev/null +++ b/app/javascript/katalyst/govuk/controllers/file_upload_controller.js @@ -0,0 +1,352 @@ +import { Controller } from "@hotwired/stimulus"; +import { I18n } from "govuk-frontend/dist/govuk/i18n.mjs"; +import { FileUpload } from "govuk-frontend/dist/govuk/all.mjs"; +import { createAttachment } from "./attachment_controller"; + +export default class FileUploadController extends Controller { + connect() { + const fileInput = this.fileInput; + let uploadButton = this.uploadButton; + + this.i18n = new I18n(FileUpload.defaults.i18n, { locale: "en" }); + + if (!fileInput) throw new Error(`Missing file input for ${this.element}`); + + this.id = uploadButton?.id ?? fileInput.id; + + if (!uploadButton) { + // The label's `for` still points at the input's original id (now the + // button's id), so the label labels the button. Give it an id too, so + // the button's `aria-labelledby` reference resolves. + this.ensureLabelId(); + fileInput.id = `${this.id}-input`; + fileInput.toggleAttribute("hidden", true); + uploadButton = createUploadButton(this.id, this.i18n, fileInput); + fileInput.insertAdjacentElement("beforebegin", uploadButton); + } + + // Appended to the drop zone (not between button and input, whose + // adjacency the uploadButton getter relies on). + if (!this.announcements) this.element.appendChild(createAnnouncements()); + + uploadButton.addEventListener("click", this.onClick); + fileInput.addEventListener("change", this.onChange); + fileInput.addEventListener("govuk:upload", this.onUpload); + fileInput.addEventListener("govuk:remove", this.onRemove); + this.bindDraggingEvents(); + + // The injected button is not a real input, so it does not inherit the + // file input's disabled state; mirror it, and keep mirroring it if the + // input's `disabled` attribute changes at runtime. + this.updateDisabledState(); + this.observeDisabledState(); + + this.updateCount(); + } + + disconnect() { + this.uploadButton?.removeEventListener("click", this.onClick); + this.fileInput?.removeEventListener("change", this.onChange); + this.fileInput?.removeEventListener("govuk:upload", this.onUpload); + this.fileInput?.removeEventListener("govuk:remove", this.onRemove); + this.unbindDraggingEvents(); + this.disabledObserver?.disconnect(); + this.announcements?.remove(); + } + + // The label is rendered by the form group, outside the drop zone, and + // carries only a `for` (no id). The injected button references `${id}-label` + // in its `aria-labelledby`, so ensure the label has that id. + ensureLabelId() { + const label = document.querySelector(`label[for="${this.id}"]`); + + if (label && !label.id) label.id = `${this.id}-label`; + } + + updateDisabledState() { + const disabled = this.fileInput.disabled; + + this.uploadButton.disabled = disabled; + this.element.classList.toggle( + "govuk-file-upload-wrapper--disabled", + disabled, + ); + } + + observeDisabledState() { + this.disabledObserver = new MutationObserver((mutations) => { + for (const mutation of mutations) { + if (mutation.attributeName === "disabled") this.updateDisabledState(); + } + }); + + this.disabledObserver.observe(this.fileInput, { attributes: true }); + } + + onClick = (event) => { + this.fileInput.click(); + }; + + // Drag & drop mirrors govuk-frontend's FileUpload: the button is the drop + // target, the whole drop zone shows the dragging state, and enter/leave + // are announced. dragenter/dragleave are on the document so we can tell a + // move between child elements from truly leaving the drop zone. + bindDraggingEvents() { + this.uploadButton.addEventListener("dragover", this.onDragover); + this.uploadButton.addEventListener("drop", this.onDrop); + document.addEventListener("dragenter", this.onDragenter); + document.addEventListener("dragleave", this.onDragleave); + } + + unbindDraggingEvents() { + this.uploadButton?.removeEventListener("dragover", this.onDragover); + this.uploadButton?.removeEventListener("drop", this.onDrop); + document.removeEventListener("dragenter", this.onDragenter); + document.removeEventListener("dragleave", this.onDragleave); + } + + // Prevent the default so the button is a valid drop target. + onDragover = (event) => { + event.preventDefault(); + }; + + onDragenter = (event) => { + this.updateDropzoneVisibility(event); + // A dragenter immediately before a dragleave means the pointer moved to + // another element rather than leaving the window. + this.enteredAnotherElement = true; + }; + + onDragleave = () => { + if (!this.enteredAnotherElement && !this.uploadButton.disabled) { + this.hideDraggingState(); + this.announce(this.i18n.t("leftDropZone")); + } + + this.enteredAnotherElement = false; + }; + + onDrop = (event) => { + event.preventDefault(); + + if (event.dataTransfer && this.canFillInput(event.dataTransfer)) { + this.fileInput.files = event.dataTransfer.files; + this.fileInput.dispatchEvent(new CustomEvent("change")); + this.hideDraggingState(); + } + }; + + updateDropzoneVisibility(event) { + if (this.uploadButton.disabled) return; + if (!(event.target instanceof Node)) return; + + if (this.element.contains(event.target)) { + if (event.dataTransfer && this.canDrop(event.dataTransfer)) { + if (!this.isDragging) { + this.showDraggingState(); + this.announce(this.i18n.t("enteredDropZone")); + } + } + } else if (this.isDragging) { + this.hideDraggingState(); + this.announce(this.i18n.t("leftDropZone")); + } + } + + showDraggingState() { + this.uploadButton.classList.add("govuk-file-upload-button--dragging"); + } + + hideDraggingState() { + this.uploadButton.classList.remove("govuk-file-upload-button--dragging"); + } + + announce(message) { + if (this.announcements) this.announcements.textContent = message; + } + + get announcements() { + return this.element.querySelector(".govuk-file-upload-announcements"); + } + + // Whether a drop of this many files is allowed: any for a multiple input, + // exactly one otherwise. + matchesInputCapacity(numberOfFiles) { + if (this.fileInput.multiple) return numberOfFiles > 0; + + return numberOfFiles === 1; + } + + // During drag the files aren't readable, so count droppable items by kind. + canDrop(dataTransfer) { + if (dataTransfer.items.length) { + return this.matchesInputCapacity(countFileItems(dataTransfer.items)); + } + + if (dataTransfer.types.length) { + return dataTransfer.types.includes("Files"); + } + + return true; + } + + canFillInput(dataTransfer) { + return this.matchesInputCapacity(dataTransfer.files.length); + } + + onChange = () => { + const files = Array.from(this.fileInput.files); + const figures = Array.from(this.element.querySelectorAll("figure")); + + // Re-selection replaces the FileList, so drop unclaimed previews whose + // file is no longer in the input. Claimed figures own their file (an + // upload is running or done) and are unaffected. + figures.forEach((figure) => { + if (figure.file && !files.includes(figure.file)) figure.remove(); + }); + + // Render a figure per new file; each carries its File until a figure + // controller claims it for upload (govuk:upload). Unclaimed files — + // no endpoint, or no controller — stay in the input and submit as + // ordinary multipart. + files.forEach((file) => { + if (figures.some((figure) => figure.file === file)) return; + + const attachment = createAttachment(this.fileInput, file); + this.uploadButton.insertAdjacentElement("beforebegin", attachment); + }); + + this.updateCount(); + }; + + // A figure claimed its file for upload: release it from the FileList so + // the same bytes don't also submit as multipart. + onUpload = ({ detail: { file } }) => { + this.releaseFile(file); + this.updateCount(); + }; + + releaseFile(file) { + const remaining = new DataTransfer(); + + for (const held of this.fileInput.files) { + if (held !== file) remaining.items.add(held); + } + + this.fileInput.files = remaining.files; + } + + onRemove = async (event) => { + // govuk:remove is dispatched before the figure is removed so listeners + // can cancel it; wait until the current task ends, when the removal + // (or the cancellation) is a fact. + await Promise.resolve(); + + if (event.defaultPrevented) return; + + const { name, file } = event.detail; + + // A removed figure that never claimed an upload still owns a file in + // the input; release it so it no longer submits. + if (file) this.releaseFile(file); + + // The removal is an event, so it is announced through the assertive + // announcements region; the polite status region only ever carries + // state (the count), which updates in place. + this.announce(`${name} removed`); + this.updateCount(); + }; + + updateCount() { + const count = this.fileCount; + + if (count === 0) { + this.statusTag.innerText = this.i18n.t("noFileChosen"); + this.uploadButton.classList.add("govuk-file-upload-button--empty"); + } else { + this.statusTag.innerText = this.i18n.t("multipleFilesChosen", { count }); + this.uploadButton.classList.remove("govuk-file-upload-button--empty"); + } + } + + get fileInput() { + return this.element.querySelector("input[type='file']"); + } + + get uploadButton() { + return this.element.querySelector( + "[type='button']:has(+ input[type='file'])", + ); + } + + get isDragging() { + return this.uploadButton.classList.contains( + "govuk-file-upload-button--dragging", + ); + } + + get statusTag() { + return this.element.querySelector("button [aria-live]"); + } + + get fileCount() { + let count = this.fileInput.files.length; + + this.element + .querySelectorAll(`select[name='${this.fileInput.name}']`) + .forEach((select) => { + // A figure still holding its unclaimed File is backed by a FileList + // entry counted above. + if (!select.closest("figure")?.file) count += 1; + }); + + if (!this.fileInput.multiple) count = Math.min(count, 1); + + return count; + } +} + +// Counts DataTransferItems whose kind is "file" (ignoring dragged text etc.). +function countFileItems(items) { + return Array.from(items).filter((item) => item.kind === "file").length; +} + +// A visually-hidden assertive live region for event announcements (drag +// enter/leave, removals), kept separate from the polite status region that +// carries the file count. +function createAnnouncements(brand = "govuk") { + const region = document.createElement("span"); + region.className = `${brand}-file-upload-announcements ${brand}-visually-hidden`; + region.setAttribute("aria-live", "assertive"); + return region; +} + +function createUploadButton(id, i18n, fileInput, brand = "govuk") { + const template = document.createElement("TEMPLATE"); + template.innerHTML = ` + + `; + const button = template.content.firstElementChild; + + // Carry the input's hint/error descriptions onto the button, so the control + // the user actually operates is described the same way the input was. + const describedBy = fileInput.getAttribute("aria-describedby"); + if (describedBy) button.setAttribute("aria-describedby", describedBy); + + return button; +} diff --git a/app/javascript/katalyst/govuk/controllers/index.js b/app/javascript/katalyst/govuk/controllers/index.js index 97dd183..a422546 100644 --- a/app/javascript/katalyst/govuk/controllers/index.js +++ b/app/javascript/katalyst/govuk/controllers/index.js @@ -1,11 +1,21 @@ +import AttachmentController from "./attachment_controller"; import DocumentFieldController from "./document_field_controller"; +import FileUploadController from "./file_upload_controller"; import ImageFieldController from "./image_field_controller"; const Definitions = [ + { + identifier: "govuk-attachment", + controllerConstructor: AttachmentController, + }, { identifier: "govuk-document-field", controllerConstructor: DocumentFieldController, }, + { + identifier: "govuk-file-upload", + controllerConstructor: FileUploadController, + }, { identifier: "govuk-image-field", controllerConstructor: ImageFieldController, diff --git a/spec/builders/document_field_spec.rb b/spec/builders/document_field_spec.rb new file mode 100644 index 0000000..c6a598f --- /dev/null +++ b/spec/builders/document_field_spec.rb @@ -0,0 +1,218 @@ +# frozen_string_literal: true + +require "rails_helper" + +# Server-rendered markup for govuk_document_field backed by Profile's +# has_one_attached :cv: one figure per attached blob (caption, +# keep/remove select) and a direct-upload-ready file input. +# Pending examples pin agreed behaviour that is not implemented yet. +RSpec.describe "govuk_document_field" do + subject(:html) { Capybara.string(builder.govuk_document_field(:cv).to_s) } + + let(:builder) { GOVUKDesignSystemFormBuilder::FormBuilder.new(:profile, object, helper, {}) } + + context "with no attachment" do + let(:object) { Profile.new } + + it "renders no attachment figures" do + expect(html).to have_no_css("figure.govuk-attachment") + end + + it "renders a single file input inside the wrapper" do + expect(html).to have_css(".govuk-file-upload-wrapper input[type=file]", count: 1, visible: :all) + end + + it "does not mark the file input as multiple" do + expect(html).to have_no_css("input[type=file][multiple]", visible: :all) + end + + it "applies the image mime types to the input's accept attribute" do + expect(html).to have_css("input[type=file][accept*='application/pdf']", visible: :all) + end + + # Exclusivity invariant: initAll enhances data-module="govuk-file-upload" + # with govuk-frontend's own FileUpload, so an attachment field emitting it + # would have two implementations fighting over one input. + it "never emits govuk-frontend's file-upload data-module" do + expect(html).to have_no_css("[data-module='govuk-file-upload']", visible: :all) + end + end + + context "with an attached file" do + let(:object) do + create(:profile).tap do |profile| + profile.cv.attach( + io: File.open(file_fixture("cv.pdf")), + filename: "cv.pdf", + content_type: "application/pdf", + ) + end + end + + let(:blob) { object.cv.blob } + + it "renders one figure with caption, and actions, in order" do + expect(html).to have_css("figure.govuk-attachment > figcaption + div.actions", count: 1, visible: :all) + end + + it "groups the select and remove button in the actions container" do + expect(html).to have_css("figure.govuk-attachment .actions > select + button", visible: :all) + end + + it "captions the figure with the filename" do + expect(html).to have_css("figure.govuk-attachment figcaption .filename", text: "cv.pdf") + end + + it "captions the figure with the human file size" do + expect(html).to have_css("figure.govuk-attachment figcaption .size", text: /\A[\d.]+ (Bytes|KB|MB)\z/) + end + + it "selects the current file, labelled with its filename" do + expect(html).to have_css( + "figure.govuk-attachment select option[selected][value='#{blob.signed_id}']", + text: "cv.pdf", + visible: :all, + ) + end + + it "offers a remove option with a blank value naming the file" do + expect(html).to have_css( + "figure.govuk-attachment select option[value='']", + text: "Remove cv.pdf", + visible: :all, + ) + end + + it "names the select for scalar assignment" do + select = html.find("figure.govuk-attachment select", visible: :all) + + expect(select["name"]).to eq("profile[cv]") + end + + it "gives the select a unique per-blob id" do + select = html.find("figure.govuk-attachment select", visible: :all) + + expect(select["id"]).to eq(builder.field_id(:cv, :attachment, blob.id, :input)) + end + + # Hiding the select is CSS-gated on .govuk-frontend-supported so that + # without JavaScript it stays the visible control. + it "does not hide the select in markup" do + expect(html).to have_no_css("figure.govuk-attachment select.govuk-visually-hidden", visible: :all) + end + + it "renders a remove button wired to the attachment controller" do + expect(html).to have_css("figure.govuk-attachment .actions button[data-action='govuk-attachment#destroy']") + end + + it "labels the remove button with the filename" do + button = html.find("figure.govuk-attachment .actions button") + + expect(button["aria-label"]).to eq("Remove cv.pdf") + end + + it "stops the remove button from submitting the form" do + button = html.find("figure.govuk-attachment .actions button") + + expect(button["type"]).to eq("button") + end + + it "labels the figure with its caption" do + figure = html.find("figure.govuk-attachment") + + expect(figure["aria-labelledby"]).to eq(builder.field_id(:cv, :attachment, blob.id, :caption)) + end + + it "makes the caption a polite live region so status changes are announced" do + expect(html.find("figure.govuk-attachment figcaption")["aria-live"]).to eq("polite") + end + + it "announces the whole caption atomically, so announcements name the file" do + expect(html.find("figure.govuk-attachment figcaption")["aria-atomic"]).to eq("true") + end + + it "reserves an empty status span in the caption for upload announcements" do + expect(html.find("figure.govuk-attachment figcaption .status").text).to eq("") + end + + it "gives the select an accessible name that includes the filename" do + select = html.find("figure.govuk-attachment select", visible: :all) + referenced = select["aria-labelledby"].to_s.split.flat_map { |id| html.all("[id='#{id}']").map(&:text) } + label_texts = html.all("label[for='#{select['id']}']", visible: :all).map(&:text) + + expect([select["aria-label"], *referenced, *label_texts].compact.join(" ")).to include("cv.pdf") + end + end + + describe "#direct_upload_url" do + let(:object) { Profile.new } + + it "adds data-direct-upload-url by default" do + input = html.find("input[type=file]", visible: :all) + + expect(input["data-direct-upload-url"]).to eq(helper.rails_direct_uploads_url) + end + + it "respects direct_upload: false" do + html = Capybara.string(builder.govuk_document_field(:cv, direct_upload: false).to_s) + + expect(html).to have_css("input[type=file]:not([data-direct-upload-url])", visible: :all) + end + + context "with an attached file and direct_upload: false" do + let(:object) do + create(:profile).tap do |profile| + profile.cv.attach( + io: File.open(file_fixture("cv.pdf")), + filename: "cv.pdf", + content_type: "application/pdf", + ) + end + end + + # The keep/remove select round-trip works without direct upload; opting + # out of async upload must not degrade the attachment markup. + it "still renders the attachment figure" do + html = Capybara.string(builder.govuk_document_field(:cv, direct_upload: false).to_s) + + expect(html).to have_css("figure.govuk-attachment > figcaption + div.actions", visible: :all) + end + end + + it "uses direct_upload_url when provided" do + html = Capybara.string(builder.govuk_document_field(:cv, direct_upload_url: "/override").to_s) + input = html.find("input[type=file]", visible: :all) + + expect(input["data-direct-upload-url"]).to eq("/override") + end + + it "uses direct_upload_url from form builder when overridden (i.e. Koi admin)" do + builder.instance_eval do + def direct_upload_url + "/extend" + end + end + + input = html.find("input[type=file]", visible: :all) + + expect(input["data-direct-upload-url"]).to eq("/extend") + end + + it "resolves direct-upload-url through main_app" do + allow(helper).to receive(:respond_to?).and_call_original + allow(helper).to receive(:respond_to?).with(:rails_direct_uploads_url).and_return(false) + + input = html.find("input[type=file]", visible: :all) + + expect(input["data-direct-upload-url"]).to eq(helper.main_app.rails_direct_uploads_url) + end + + it "does not set data-direct-upload-url when no direct-upload route is available" do + allow(helper).to receive(:respond_to?).and_call_original + allow(helper).to receive(:respond_to?).with(:rails_direct_uploads_url).and_return(false) + allow(helper).to receive(:main_app).and_return(Object.new) + + expect(html).to have_css("input[type=file]:not([data-direct-upload-url])", visible: :all) + end + end +end diff --git a/spec/builders/gallery_field_spec.rb b/spec/builders/gallery_field_spec.rb new file mode 100644 index 0000000..981345a --- /dev/null +++ b/spec/builders/gallery_field_spec.rb @@ -0,0 +1,75 @@ +# frozen_string_literal: true + +require "rails_helper" + +# The multiple-file counterpart to image_field_spec.rb, backed by Profile's +# has_many_attached :gallery. Covers the array-name and per-blob aspects of +# the figure/select markup: each blob round-trips through its own select. +# Pending examples pin agreed behaviour that is not implemented yet. +RSpec.describe "govuk_image_field (gallery / multiple)" do + subject(:html) { Capybara.string(builder.govuk_image_field(:gallery, multiple: true).to_s) } + + let(:builder) { GOVUKDesignSystemFormBuilder::FormBuilder.new(:profile, object, helper, {}) } + + context "with no attachments" do + let(:object) { Profile.new } + + it "renders no attachment figures" do + expect(html).to have_no_css("figure.govuk-attachment") + end + + it "renders a multiple file input when multiple is passed explicitly" do + expect(html).to have_css("input[type=file][multiple]", visible: :all) + end + + it "infers multiple from the has_many_attached reflection" do + unhinted = Capybara.string(builder.govuk_image_field(:gallery).to_s) + + expect(unhinted).to have_css("input[type=file][multiple]", visible: :all) + end + end + + context "with several attached images" do + let(:object) do + create(:profile).tap do |profile| + %w[first.png second.png].each do |filename| + profile.gallery.attach( + io: File.open(file_fixture("avatar.png")), + filename:, + content_type: "image/png", + ) + end + end + end + + it "renders one figure per attached file, in attachment order" do + expect(html.all("figure.govuk-attachment .filename").map(&:text)).to eq(%w[first.png second.png]) + end + + it "round-trips each attachment via its own array-named select" do + object.gallery.blobs.each do |blob| + expect(html).to have_css( + "select[name='profile[gallery][]'] option[selected][value='#{blob.signed_id}']", + text: blob.filename.to_s, + visible: :all, + ) + end + end + + it "offers a remove option naming each file" do + %w[first.png second.png].each do |filename| + expect(html).to have_css( + "figure.govuk-attachment select option[value='']", + text: "Remove #{filename}", + visible: :all, + ) + end + end + + it "gives each select a unique per-blob id" do + ids = html.all("figure.govuk-attachment select", visible: :all).map { |select| select["id"] } + + expect(ids).to eq(object.gallery.blobs.map { |blob| builder.field_id(:gallery, :attachment, blob.id, :input) }) + end + end +end diff --git a/spec/builders/image_field_spec.rb b/spec/builders/image_field_spec.rb new file mode 100644 index 0000000..cae0fdf --- /dev/null +++ b/spec/builders/image_field_spec.rb @@ -0,0 +1,271 @@ +# frozen_string_literal: true + +require "rails_helper" + +# Server-rendered markup for govuk_image_field backed by Profile's +# has_one_attached :avatar: one figure per attached blob (preview image, +# caption, keep/remove select) and a direct-upload-ready file input. +# Pending examples pin agreed behaviour that is not implemented yet. +RSpec.describe "govuk_image_field" do + subject(:html) { Capybara.string(builder.govuk_image_field(:avatar).to_s) } + + let(:builder) { GOVUKDesignSystemFormBuilder::FormBuilder.new(:profile, object, helper, {}) } + + context "with no attachment" do + let(:object) { Profile.new } + + it "renders no attachment figures" do + expect(html).to have_no_css("figure.govuk-attachment") + end + + it "renders a single file input inside the wrapper" do + expect(html).to have_css(".govuk-file-upload-wrapper input[type=file]", count: 1, visible: :all) + end + + it "does not mark the file input as multiple" do + expect(html).to have_no_css("input[type=file][multiple]", visible: :all) + end + + it "applies the image mime types to the input's accept attribute" do + expect(html).to have_css("input[type=file][accept*='image/png']", visible: :all) + end + + # Exclusivity invariant: initAll enhances data-module="govuk-file-upload" + # with govuk-frontend's own FileUpload, so an attachment field emitting it + # would have two implementations fighting over one input. + it "never emits govuk-frontend's file-upload data-module" do + expect(html).to have_no_css("[data-module='govuk-file-upload']", visible: :all) + end + end + + # govuk_image_field / govuk_document_field delegate to govuk_attachment_field; + # the label, caption, hint and form_group configuration must reach it rather + # than being dropped on the way through. + context "with configuration options" do + let(:object) { Profile.new } + + it "renders the hint" do + html = Capybara.string(builder.govuk_image_field(:avatar, hint: { text: "Max 5MB" }).to_s) + + expect(html).to have_css(".govuk-hint", text: "Max 5MB") + end + + it "describes the input by the hint" do + html = Capybara.string(builder.govuk_image_field(:avatar, hint: { text: "Max 5MB" }).to_s) + hint_id = html.find(".govuk-hint", visible: :all)[:id] + + expect(html.find("input[type=file]", visible: :all)["aria-describedby"]).to eq(hint_id) + end + + it "renders the supplied label text" do + html = Capybara.string(builder.govuk_image_field(:avatar, label: { text: "Your photo" }).to_s) + + expect(html).to have_css("label", text: "Your photo") + end + + it "renders the supplied caption" do + html = Capybara.string(builder.govuk_image_field(:avatar, caption: { text: "Step 1" }).to_s) + + expect(html).to have_css(".govuk-caption-m", text: "Step 1") + end + + it "applies form_group options" do + html = Capybara.string(builder.govuk_image_field(:avatar, form_group: { class: "extra-group" }).to_s) + + expect(html).to have_css(".govuk-form-group.extra-group") + end + end + + context "with an attached image" do + let(:object) { create(:profile) } + let(:blob) { object.avatar.blob } + + it "renders one figure with preview, caption, and actions, in order" do + expect(html).to have_css("figure.govuk-attachment > img + figcaption + div.actions", count: 1, visible: :all) + end + + it "groups the select and remove button in the actions container" do + expect(html).to have_css("figure.govuk-attachment .actions > select + button", visible: :all) + end + + it "captions the figure with the filename" do + expect(html).to have_css("figure.govuk-attachment figcaption .filename", text: "avatar.png") + end + + it "captions the figure with the human file size" do + expect(html).to have_css("figure.govuk-attachment figcaption .size", text: /\A[\d.]+ (Bytes|KB|MB)\z/) + end + + it "selects the current file, labelled with its filename" do + expect(html).to have_css( + "figure.govuk-attachment select option[selected][value='#{blob.signed_id}']", + text: "avatar.png", + visible: :all, + ) + end + + it "offers a remove option with a blank value naming the file" do + expect(html).to have_css( + "figure.govuk-attachment select option[value='']", + text: "Remove avatar.png", + visible: :all, + ) + end + + it "names the select for scalar assignment" do + select = html.find("figure.govuk-attachment select", visible: :all) + + expect(select["name"]).to eq("profile[avatar]") + end + + it "gives the select a unique per-blob id" do + select = html.find("figure.govuk-attachment select", visible: :all) + + expect(select["id"]).to eq(builder.field_id(:avatar, :attachment, blob.id, :input)) + end + + # Hiding the select is CSS-gated on .govuk-frontend-supported so that + # without JavaScript it stays the visible control. + it "does not hide the select in markup" do + expect(html).to have_no_css("figure.govuk-attachment select.govuk-visually-hidden", visible: :all) + end + + it "renders a remove button wired to the attachment controller" do + expect(html).to have_css("figure.govuk-attachment .actions button[data-action='govuk-attachment#destroy']") + end + + it "labels the remove button with the filename" do + button = html.find("figure.govuk-attachment .actions button") + + expect(button["aria-label"]).to eq("Remove avatar.png") + end + + it "stops the remove button from submitting the form" do + button = html.find("figure.govuk-attachment .actions button") + + expect(button["type"]).to eq("button") + end + + it "hides the preview image from assistive technology" do + expect(html.find("figure.govuk-attachment img")["alt"]).to eq("") + end + + it "labels the figure with its caption" do + figure = html.find("figure.govuk-attachment") + + expect(figure["aria-labelledby"]).to eq(builder.field_id(:avatar, :attachment, blob.id, :caption)) + end + + it "makes the caption a polite live region so status changes are announced" do + expect(html.find("figure.govuk-attachment figcaption")["aria-live"]).to eq("polite") + end + + it "announces the whole caption atomically, so announcements name the file" do + expect(html.find("figure.govuk-attachment figcaption")["aria-atomic"]).to eq("true") + end + + it "reserves an empty status span in the caption for upload announcements" do + expect(html.find("figure.govuk-attachment figcaption .status").text).to eq("") + end + + it "gives the select an accessible name that includes the filename" do + select = html.find("figure.govuk-attachment select", visible: :all) + referenced = select["aria-labelledby"].to_s.split.flat_map { |id| html.all("[id='#{id}']").map(&:text) } + label_texts = html.all("label[for='#{select['id']}']", visible: :all).map(&:text) + + expect([select["aria-label"], *referenced, *label_texts].compact.join(" ")).to include("avatar.png") + end + end + + context "with a non-image attachment" do + let(:object) do + create(:profile).tap do |profile| + profile.avatar.attach( + io: StringIO.new("not an image"), + filename: "notes.txt", + content_type: "text/plain", + ) + end + end + + it "renders the figure with caption and actions adjacent" do + expect(html).to have_css("figure.govuk-attachment > figcaption + div.actions", visible: :all) + end + + it "omits the preview image" do + expect(html).to have_no_css("figure.govuk-attachment img") + end + end + + describe "#direct_upload_url" do + let(:object) { Profile.new } + + it "adds data-direct-upload-url by default" do + input = html.find("input[type=file]", visible: :all) + + expect(input["data-direct-upload-url"]).to eq(helper.rails_direct_uploads_url) + end + + it "respects direct_upload: false" do + html = Capybara.string(builder.govuk_image_field(:avatar, direct_upload: false).to_s) + + expect(html).to have_css("input[type=file]:not([data-direct-upload-url])", visible: :all) + end + + context "with an attached image and direct_upload: false" do + let(:object) do + create(:profile).tap do |profile| + profile.avatar.attach( + io: File.open(file_fixture("avatar.png")), + filename: "avatar.png", + content_type: "image/png", + ) + end + end + + # The keep/remove select round-trip works without direct upload; opting + # out of async upload must not degrade the attachment markup. + it "still renders the attachment figure" do + html = Capybara.string(builder.govuk_image_field(:avatar, direct_upload: false).to_s) + + expect(html).to have_css("figure.govuk-attachment > img + figcaption + div.actions", visible: :all) + end + end + + it "uses direct_upload_url when provided" do + html = Capybara.string(builder.govuk_image_field(:avatar, direct_upload_url: "/override").to_s) + input = html.find("input[type=file]", visible: :all) + + expect(input["data-direct-upload-url"]).to eq("/override") + end + + it "uses direct_upload_url from form builder when overridden (i.e. Koi admin)" do + builder.instance_eval do + def direct_upload_url + "/extend" + end + end + + input = html.find("input[type=file]", visible: :all) + + expect(input["data-direct-upload-url"]).to eq("/extend") + end + + it "resolves direct-upload-url through main_app" do + allow(helper).to receive(:respond_to?).and_call_original + allow(helper).to receive(:respond_to?).with(:rails_direct_uploads_url).and_return(false) + + input = html.find("input[type=file]", visible: :all) + + expect(input["data-direct-upload-url"]).to eq(helper.main_app.rails_direct_uploads_url) + end + + it "does not set data-direct-upload-url when no direct-upload route is available" do + allow(helper).to receive(:respond_to?).and_call_original + allow(helper).to receive(:respond_to?).with(:rails_direct_uploads_url).and_return(false) + allow(helper).to receive(:main_app).and_return(Object.new) + + expect(html).to have_css("input[type=file]:not([data-direct-upload-url])", visible: :all) + end + end +end diff --git a/spec/dummy/app/controllers/blocking_direct_uploads_controller.rb b/spec/dummy/app/controllers/blocking_direct_uploads_controller.rb new file mode 100644 index 0000000..9aa473c --- /dev/null +++ b/spec/dummy/app/controllers/blocking_direct_uploads_controller.rb @@ -0,0 +1,25 @@ +# frozen_string_literal: true + +# A direct-uploads endpoint that holds each request until the test releases +# it, so system tests can observe the transient uploading state and decide +# how the request resolves: release with :ok to proceed normally, or with an +# HTTP status (symbol or integer) to respond with that status instead. The +# hold times out rather than hang the suite if a test forgets to release. +class BlockingDirectUploadsController < ActiveStorage::DirectUploadsController + QUEUE = Queue.new + + # Allows one held request to proceed. + def self.release(result = :ok) + QUEUE << result + end + + def create + result = QUEUE.pop(timeout: 5) + + if result.nil? || result == :ok + super + else + head result + end + end +end diff --git a/spec/dummy/app/models/profile.rb b/spec/dummy/app/models/profile.rb index ddf3628..154bead 100644 --- a/spec/dummy/app/models/profile.rb +++ b/spec/dummy/app/models/profile.rb @@ -76,7 +76,7 @@ class Profile < ApplicationRecord COUNTRIES = ["Australia", "New Zealand", "United Kingdom", "Canada", "Ireland"].freeze - validates :name, :email, presence: true + validates :name, :email, :avatar, presence: true def to_s name.presence || "New profile" diff --git a/spec/dummy/app/views/examples/attachment/enhancement.html.erb b/spec/dummy/app/views/examples/attachment/enhancement.html.erb new file mode 100644 index 0000000..611ef7c --- /dev/null +++ b/spec/dummy/app/views/examples/attachment/enhancement.html.erb @@ -0,0 +1,21 @@ +<%# locals: (profile:) %> +<%# + Test-support forms for the attachment field's JavaScript enhancement + (spec/system/attachment/enhancement_spec.rb). These configurations are not + exercised by the profile form: a field with a hint (so the input carries + aria-describedby to pass through to the injected button) and a disabled + field (so the button mirrors the disabled state). +%> +<%= turbo_frame_tag example_frame_id(params[:page], params[:example]) do %> + <%= form_with(model: profile, url: example_path(params[:page], params[:example])) do |f| %> + <%= f.govuk_image_field :avatar, + label: { text: "Profile photo" }, + hint: { text: "Upload a clear colour photograph" } %> + + <%= f.govuk_document_field :cv, + label: { text: "Curriculum vitae" }, + disabled: true %> + + <%= example_submit_buttons(f) %> + <% end %> +<% end %> diff --git a/spec/dummy/app/views/profiles/_form.html.erb b/spec/dummy/app/views/profiles/_form.html.erb index 39836a6..632cce1 100644 --- a/spec/dummy/app/views/profiles/_form.html.erb +++ b/spec/dummy/app/views/profiles/_form.html.erb @@ -13,9 +13,8 @@ <%= f.govuk_combobox :country, Profile::COUNTRIES %> <%= f.govuk_rich_textarea :description %> <%= f.govuk_image_field :avatar, optional: true %> - <%= f.govuk_image_field :gallery, multiple: true, optional: true %> + <%= f.govuk_image_field :gallery, optional: true %> <%= f.govuk_document_field :cv, optional: true %> - <%= f.govuk_file_field :cv, optional: true, javascript: true %> <%= f.govuk_submit %> <% end %> diff --git a/spec/dummy/config/routes.rb b/spec/dummy/config/routes.rb index 539e994..fbd34ea 100644 --- a/spec/dummy/config/routes.rb +++ b/spec/dummy/config/routes.rb @@ -14,5 +14,8 @@ via: %i[get post], constraints: { page: /[a-z_]+/, example: /[a-z_]+/ } + # System tests point fields here to hold direct uploads until released. + post "blocking_direct_uploads", to: "blocking_direct_uploads#create" + root to: "guide#index" end diff --git a/spec/factories/profiles.rb b/spec/factories/profiles.rb index 8cad426..577f4d2 100644 --- a/spec/factories/profiles.rb +++ b/spec/factories/profiles.rb @@ -4,6 +4,7 @@ factory :profile do name { "Ada Lovelace" } email { "ada@example.com" } + avatar { Rack::Test::UploadedFile.new(file_fixture("avatar.png"), "image/png") } bio { "Mathematician and writer." } active { true } born_on { Date.new(1815, 12, 10) } diff --git a/spec/rails_helper.rb b/spec/rails_helper.rb index 4935753..8547def 100644 --- a/spec/rails_helper.rb +++ b/spec/rails_helper.rb @@ -20,12 +20,19 @@ end RSpec.configure do |config| - config.include FactoryBot::Syntax::Methods - config.use_transactional_fixtures = true config.infer_spec_type_from_file_location! config.filter_rails_from_backtrace! # Lets system tests attach fixtures with `file_fixture_upload("avatar.png")`. config.file_fixture_path = File.expand_path("fixtures/files", __dir__) + + config.define_derived_metadata(file_path: %r{spec/builders}) do |metadata| + metadata[:type] ||= :helper + end + + # Remove active-storage uploads + config.after(:suite) do + FileUtils.rm_rf(Rails.application.root.join("tmp", "storage")) + end end diff --git a/spec/requests/profiles/avatar_spec.rb b/spec/requests/profiles/avatar_spec.rb new file mode 100644 index 0000000..f54b700 --- /dev/null +++ b/spec/requests/profiles/avatar_spec.rb @@ -0,0 +1,61 @@ +# frozen_string_literal: true + +require "rails_helper" + +# Updating Profile#avatar (has_one_attached, required — cv is the optional +# counterpart) through the controller: the scalar counterpart to +# gallery_spec.rb. The keep/remove select submits the blob's signed id to +# retain; blank fails the presence validation rather than detaching. +RSpec.describe "Updating a profile's avatar" do + include AttachmentRequestHelpers + + let(:profile) { create(:profile) } + + it "keeps the attachment when its signed id is submitted" do + patch profile_path(profile), params: { profile: { avatar: profile.avatar.signed_id } } + + expect(profile.reload.avatar).to be_attached + end + + it "rejects removal: blank fails the presence validation and keeps the file" do + patch profile_path(profile), params: { profile: { avatar: "" } } + + aggregate_failures do + expect(response).to have_http_status(:unprocessable_content) + expect(profile.reload.avatar).to be_attached + end + end + + it "replaces when a different blob's signed id is submitted" do + blob = uploaded_blob(filename: "replacement.png") + + patch profile_path(profile), params: { profile: { avatar: blob.signed_id } } + + expect(profile.reload.avatar.blob).to eq(blob) + end + + # A submit that fails validation re-renders the submitted signed id as a + # server figure with its keep option preserved — for a scalar field the + # last-submitted blob is the whole assignment, so a replacement supersedes + # the persisted avatar in the re-render. + describe "round-trip on an invalid submit" do + let(:uploaded) { uploaded_blob(filename: "updated.png") } + + def submit_invalid + patch profile_path(profile), + params: { profile: { name: "", email: "ada@example.com", avatar: uploaded.signed_id } } + end + + context "when the fresh upload replaces the persisted avatar" do + before { submit_invalid } + + it "re-renders the replacement" do + expect(figure_for("updated.png")).to be_present + end + + it "does not re-render the superseded avatar" do + expect(figure_for("avatar.png")).to be_nil + end + end + end +end diff --git a/spec/requests/profiles/cv_spec.rb b/spec/requests/profiles/cv_spec.rb new file mode 100644 index 0000000..0740e60 --- /dev/null +++ b/spec/requests/profiles/cv_spec.rb @@ -0,0 +1,51 @@ +# frozen_string_literal: true + +require "rails_helper" + +# Updating Profile#cv (has_one_attached, optional — avatar is the required +# counterpart). Optional scalar semantics live here: blank detaches, and a +# fresh upload round-trips on a profile that has nothing attached yet. +RSpec.describe "Updating a profile's cv" do + include AttachmentRequestHelpers + + let(:profile) { create(:profile) } + + def attach_cv + profile.cv.attach(io: File.open(file_fixture("cv.pdf")), filename: "cv.pdf", content_type: "application/pdf") + end + + it "attaches an uploaded blob's signed id" do + blob = uploaded_blob(filename: "new-cv.png") + + patch profile_path(profile), params: { profile: { cv: blob.signed_id } } + + expect(profile.reload.cv.blob).to eq(blob) + end + + it "detaches when blank is submitted" do + attach_cv + + patch profile_path(profile), params: { profile: { cv: "" } } + + expect(profile.reload.cv).not_to be_attached + end + + describe "round-trip on an invalid submit" do + let(:uploaded) { uploaded_blob(filename: "new-cv.png") } + + # A fresh upload arriving on a profile with no cv — the empty-scalar + # round-trip an always-attached avatar can't express. + before do + patch profile_path(profile), + params: { profile: { name: "", email: "ada@example.com", cv: uploaded.signed_id } } + end + + it "re-renders the just-uploaded blob as a figure" do + expect(figure_for("new-cv.png")).to be_present + end + + it "preserves its signed id as the keep option" do + expect(kept_signed_id(figure_for("new-cv.png"))).to eq(uploaded.signed_id) + end + end +end diff --git a/spec/requests/profile_gallery_spec.rb b/spec/requests/profiles/gallery_spec.rb similarity index 57% rename from spec/requests/profile_gallery_spec.rb rename to spec/requests/profiles/gallery_spec.rb index b1d0ce4..6445e6b 100644 --- a/spec/requests/profile_gallery_spec.rb +++ b/spec/requests/profiles/gallery_spec.rb @@ -15,31 +15,23 @@ # * a signed id that's present is retained; one that's dropped is removed # * a brand-new signed id is added RSpec.describe "Updating a profile's gallery" do - let(:profile) { Profile.create!(name: "Ada Lovelace", email: "ada@example.com") } + include AttachmentRequestHelpers + + let(:profile) { create(:profile) } # Mirrors the submitted field order: blank first, then a signed id per file. def gallery_params(*signed_ids) { profile: { gallery: ["", *signed_ids] } } end - # A blob that exists but isn't attached to the profile yet — stands in for a - # file the browser has already direct-uploaded and injected as a hidden input. - def uploaded_blob(filename) - ActiveStorage::Blob.create_and_upload!( - io: File.open(file_fixture("avatar.png")), - filename:, - content_type: "image/png", - ) - end - # Attaches a file directly, as though it were saved on a previous request. - def attach_existing(filename) - profile.gallery.attach(io: File.open(file_fixture("avatar.png")), filename:, content_type: "image/png") + def attach_existing(filename: "avatar.png", fixture: file_fixture("avatar.png"), content_type: "image/png") + profile.gallery.attach(io: File.open(fixture), filename:, content_type:) profile.gallery.blobs.find { |blob| blob.filename.to_s == filename } end describe "adding a single file (none => 1)" do - let(:blob) { uploaded_blob("added.png") } + let(:blob) { uploaded_blob(filename: "added.png") } it "attaches the newly uploaded blob" do patch profile_path(profile), params: gallery_params(blob.signed_id) @@ -56,7 +48,7 @@ def attach_existing(filename) describe "removing the only file (1 => none)" do it "detaches it when just the blank entry is submitted" do - attach_existing("existing.png") + attach_existing(filename: "existing.png") expect do patch profile_path(profile), params: gallery_params @@ -66,8 +58,8 @@ def attach_existing(filename) describe "adding one and removing another (1 => 1)" do it "replaces the old blob with the new one" do - attach_existing("existing.png") - new_blob = uploaded_blob("added.png") + attach_existing(filename: "existing.png") + new_blob = uploaded_blob(filename: "added.png") # The existing signed id is omitted (removed) and the new one submitted. # contain_exactly is exhaustive: the new blob is the whole gallery. @@ -79,7 +71,7 @@ def attach_existing(filename) describe "submitting an unchanged gallery (1 => 1)" do it "keeps the same blob attached" do - existing = attach_existing("existing.png") + existing = attach_existing(filename: "existing.png") patch profile_path(profile), params: gallery_params(existing.signed_id) @@ -105,4 +97,55 @@ def attach_existing(filename) end.to change { profile.reload.gallery.count }.from(0).to(2) end end + + # A submit that fails validation re-renders every submitted signed id — + # persisted and not-yet-attached alike — as a server figure with its keep + # option preserved, so the user never re-uploads. Blank name fails presence, + # leaving the attachment assignment pending but unsaved. + describe "round-trip on an invalid submit" do + let!(:persisted) { attach_existing(filename: "persisted.png") } + let(:uploaded) { uploaded_blob(filename: "just-uploaded.png") } + + def submit_invalid(*signed_ids) + patch profile_path(profile), + params: { profile: { name: "", email: "ada@example.com", gallery: ["", *signed_ids] } } + end + + describe "keeping the persisted blob and adding a fresh upload" do + before { submit_invalid(persisted.signed_id, uploaded.signed_id) } + + it "responds with unprocessable entity" do + expect(response).to have_http_status(:unprocessable_content) + end + + it "re-renders the persisted attachment as a figure" do + expect(figure_for("persisted.png")).to be_present + end + + it "re-renders the just-uploaded, not-yet-attached blob as a figure" do + expect(figure_for("just-uploaded.png")).to be_present + end + + it "preserves the just-uploaded blob's signed id as its keep option" do + expect(kept_signed_id(figure_for("just-uploaded.png"))).to eq(uploaded.signed_id) + end + + it "leaves the stored gallery untouched (nothing was saved)" do + expect(profile.reload.gallery.blobs.map { |blob| blob.filename.to_s }).to contain_exactly("persisted.png") + end + end + + describe "removing the persisted blob" do + # The persisted id is omitted (removed); only the fresh upload is submitted. + before { submit_invalid(uploaded.signed_id) } + + it "does not re-render the removed attachment" do + expect(figure_for("persisted.png")).to be_nil + end + + it "re-renders the just-uploaded blob that replaced it" do + expect(figure_for("just-uploaded.png")).to be_present + end + end + end end diff --git a/spec/requests/profiles_spec.rb b/spec/requests/profiles_spec.rb index c6e6d68..2d43378 100644 --- a/spec/requests/profiles_spec.rb +++ b/spec/requests/profiles_spec.rb @@ -39,8 +39,33 @@ end end + # A failed submit re-renders the form in standard GOV.UK error style: the + # summary lists the failure and the errored field shows its message. The + # attachment fields share this scaffolding (their round-trip specs live with + # the gallery/avatar request specs). + describe "POST /profiles with invalid params" do + before { post profiles_path, params: { profile: { name: "", email: "ada@example.com" } } } + + it "responds with unprocessable entity" do + expect(response).to have_http_status(:unprocessable_content) + end + + it "renders the validation message in the error summary" do + summary = Nokogiri::HTML(response.body).at_css(".govuk-error-summary") + + expect(summary.text).to match(/can't be blank/i) + end + + it "renders a field-level error message in GOV.UK error style" do + expect(Nokogiri::HTML(response.body).css("p.govuk-error-message")).to be_present + end + end + describe "POST /profiles" do - let(:params) { { profile: { name: "Grace Hopper", email: "grace@example.com", status: "draft" } } } + let(:params) do + { profile: { name: "Grace Hopper", email: "grace@example.com", status: "draft", + avatar: fixture_file_upload("avatar.png", "image/png") } } + end it "creates a profile" do expect { post profiles_path, params: params }.to change(Profile, :count).by(1) diff --git a/spec/support/attachment_field_helpers.rb b/spec/support/attachment_field_helpers.rb new file mode 100644 index 0000000..567e582 --- /dev/null +++ b/spec/support/attachment_field_helpers.rb @@ -0,0 +1,56 @@ +# frozen_string_literal: true + +# Finders and interactions for the profile form's attachment fields, shared +# by the system specs in spec/system/attachment/. +module AttachmentFieldHelpers + # The enhanced field (drop zone) wrapping the file input with this name. + def attachment_field(name) + find("[data-controller='govuk-file-upload']:has(input[type=file][name='#{name}'])") + end + + def gallery_field + attachment_field("profile[gallery][]") + end + + def avatar_field + attachment_field("profile[avatar]") + end + + # The document (non-image) counterpart: cv is optional where avatar is + # required, and its figures render without a preview image. + def cv_field + attachment_field("profile[cv]") + end + + # The upload button's status span is the field's polite live region: it + # carries the field's state — the file count. + def status_region(field) + field.find("button [aria-live]") + end + + # The field's assertive live region: it carries event announcements (drag + # enter/leave, removals). Visually hidden, so found with visible: :all. + def announcements_region(field) + field.find(".govuk-file-upload-announcements", visible: :all) + end + + # The file input is hidden once JavaScript enhances the field; Capybara + # needs it visible to attach a fixture through it. + def choose_file(name, fixture) + within(attachment_field(name)) do + attach_file(name, file_fixture(fixture).to_s, make_visible: true) + end + end + + def choose_gallery_file(fixture) + choose_file("profile[gallery][]", fixture) + end + + def choose_avatar_file(fixture) + choose_file("profile[avatar]", fixture) + end + + def choose_cv_file(fixture) + choose_file("profile[cv]", fixture) + end +end diff --git a/spec/support/attachment_request_helpers.rb b/spec/support/attachment_request_helpers.rb new file mode 100644 index 0000000..3119472 --- /dev/null +++ b/spec/support/attachment_request_helpers.rb @@ -0,0 +1,31 @@ +# frozen_string_literal: true + +require "nokogiri" + +# Helpers for request specs that exercise attachment params and the figures a +# failed submit re-renders. +module AttachmentRequestHelpers + # A blob that exists but isn't attached — stands in for a file the browser + # has already direct-uploaded and injected as a select option, not yet saved. + def uploaded_blob(filename: "avatar.png", fixture: file_fixture("avatar.png"), content_type: "image/png") + ActiveStorage::Blob.create_and_upload!( + io: File.open(fixture), + filename:, + content_type:, + ) + end + + def figures + Nokogiri::HTML(response.body).css("figure.govuk-attachment") + end + + def figure_for(filename) + figures.find { |figure| figure.css(".filename").text.include?(filename) } + end + + # The signed id the figure would submit to keep the attachment: its select's + # selected option value. + def kept_signed_id(figure) + figure.at_css("select option[selected]")&.attr("value") + end +end diff --git a/spec/support/direct_upload_helpers.rb b/spec/support/direct_upload_helpers.rb new file mode 100644 index 0000000..6acd32f --- /dev/null +++ b/spec/support/direct_upload_helpers.rb @@ -0,0 +1,48 @@ +# frozen_string_literal: true + +# Steers where a form's direct uploads go, for system specs that need to +# observe transient upload states or exercise failure paths against real +# (unstubbed) XHRs. Call a helper before visiting the page — the endpoint is +# rendered into the field's markup, not injected with JavaScript. +# +# Including this module restores the default form builder after each example. +module DirectUploadHelpers + def self.included(base) + base.after { ApplicationController.default_form_builder(GOVUKDesignSystemFormBuilder::FormBuilder) } + end + + # Points direct uploads at a route that holds each request until released, + # so transient upload states are observable — the default endpoint can + # settle a small file before Capybara's first poll. + def block_direct_uploads + BlockingDirectUploadsController::QUEUE.clear + use_direct_upload_url("/blocking_direct_uploads") + end + + # Releases one held direct upload: with :ok it proceeds normally, with an + # HTTP status the endpoint responds with that status instead. + def release_direct_uploads(result = :ok) + BlockingDirectUploadsController.release(result) + end + + # Points direct uploads at a missing route so the create XHR fails. + def break_direct_uploads + use_direct_upload_url("/missing-direct-uploads") + end + + # Renders fields without data-direct-upload-url, so the enhanced UI runs + # in no-upload mode: previews without uploads, multipart submission. + def disable_direct_uploads + use_direct_upload_url(nil) + end + + # Swaps in a form builder that overrides direct_upload_url — the same + # extension point engine builders use. + def use_direct_upload_url(url) + builder = Class.new(GOVUKDesignSystemFormBuilder::FormBuilder) do + define_method(:direct_upload_url) { url } + end + + ApplicationController.default_form_builder(builder) + end +end diff --git a/spec/support/factory_bot.rb b/spec/support/factory_bot.rb new file mode 100644 index 0000000..be46c16 --- /dev/null +++ b/spec/support/factory_bot.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +require "factory_bot_rails" + +FactoryBot.definition_file_paths << File.expand_path("../factories", __dir__) +FactoryBot.reload + +RSpec.configure do |config| + config.include FactoryBot::Syntax::Methods + + config.append_after do + FactoryBot.rewind_sequences + Faker::UniqueGenerator.clear + end +end diff --git a/spec/system/attachment/drag_and_drop_spec.rb b/spec/system/attachment/drag_and_drop_spec.rb new file mode 100644 index 0000000..cdd32dc --- /dev/null +++ b/spec/system/attachment/drag_and_drop_spec.rb @@ -0,0 +1,104 @@ +# frozen_string_literal: true + +require "rails_helper" + +# Dropping files onto the attachment field, ported from govuk-frontend's +# FileUpload: the button is the drop target, a valid drag shows the dragging +# state and is announced, and a drop fills the input exactly as choosing files +# does. Drops are simulated with a synthetic DragEvent carrying a DataTransfer, +# which exercises the real controller in the browser (OS-level drag can't be +# driven from Capybara). +RSpec.describe "Dropping files onto an attachment field", :aggregate_failures do + include AttachmentFieldHelpers + include DirectUploadHelpers + + let(:profile) { create(:profile) } + + it "uploads a dropped file like a chosen one" do + visit edit_profile_path(profile) + + drop_files("profile[gallery][]", "avatar.png") + + expect(gallery_field).to have_css("figure.govuk-attachment[data-state=upload-successful]", wait: 10) + expect(gallery_field).to have_css("figure.govuk-attachment .filename", text: "avatar.png") + end + + context "without a direct-upload endpoint" do + before { disable_direct_uploads } + + it "previews a dropped file" do + visit edit_profile_path(profile) + + drop_files("profile[gallery][]", "avatar.png") + + expect(gallery_field).to have_css("figure.govuk-attachment .filename", text: "avatar.png") + end + + it "accepts several files dropped on a multiple field" do + visit edit_profile_path(profile) + + drop_files("profile[gallery][]", "avatar.png", "cv.pdf") + + expect(gallery_field).to have_css("figure.govuk-attachment", count: 2) + end + + it "rejects a multi-file drop on a single-file field" do + visit edit_profile_path(profile) + + # cv: single-file and optional, so the field is verifiably empty after + # the rejected drop (avatar always carries its persisted figure). + drop_files("profile[cv]", "avatar.png", "cv.pdf") + + expect(cv_field).to have_no_css("figure.govuk-attachment") + end + end + + it "shows the dragging state and announces entering and leaving the drop zone" do + visit edit_profile_path(profile) + + dispatch_drag("dragenter", gallery_drop_target) + + expect(gallery_field).to have_css(".govuk-file-upload-button--dragging") + expect(announcements_region(gallery_field)).to have_text("Entered drop zone") + + # A dragenter on an element outside the zone means the drag has left it. + dispatch_drag("dragenter", "body") + + expect(gallery_field).to have_no_css(".govuk-file-upload-button--dragging") + expect(announcements_region(gallery_field)).to have_text("Left drop zone") + end + + def gallery_drop_target + ".govuk-file-upload-wrapper:has(input[name='profile[gallery][]']) .govuk-file-upload-button" + end + + # Dispatch a synthetic drop of `filenames` onto the field's button. The + # File contents are stub bytes — enough for a preview and a direct upload. + def drop_files(input_name, *filenames) + page.execute_script(<<~JS, input_name, filenames) + const [name, names] = arguments; + const input = document.querySelector(`input[type=file][name="${name}"]`); + const button = input + .closest(".govuk-file-upload-wrapper") + .querySelector(".govuk-file-upload-button"); + const data = new DataTransfer(); + names.forEach((n) => data.items.add(new File(["stub"], n, { type: "image/png" }))); + button.dispatchEvent( + new DragEvent("drop", { dataTransfer: data, bubbles: true, cancelable: true }), + ); + JS + end + + # Dispatch a synthetic drag event (carrying one dragged file) on the element + # matching `selector`. + def dispatch_drag(type, selector) + page.execute_script(<<~JS, type, selector) + const [type, selector] = arguments; + const data = new DataTransfer(); + data.items.add(new File(["stub"], "dragged.png", { type: "image/png" })); + document.querySelector(selector).dispatchEvent( + new DragEvent(type, { dataTransfer: data, bubbles: true, cancelable: true }), + ); + JS + end +end diff --git a/spec/system/attachment/enhancement_spec.rb b/spec/system/attachment/enhancement_spec.rb new file mode 100644 index 0000000..13264b1 --- /dev/null +++ b/spec/system/attachment/enhancement_spec.rb @@ -0,0 +1,86 @@ +# frozen_string_literal: true + +require "rails_helper" + +# The attachment field replaces govuk-frontend's FileUpload JavaScript, so its +# accessibility contract is the compatibility bar. When the controller injects +# the pseudo upload button it must, like FileUpload: +# +# * give the field label an id so the button's `aria-labelledby` resolves +# (the label otherwise has only a `for`, and the button references an id +# that would not exist) +# * copy the input's `aria-describedby` (hint / error ids) onto the button, +# so the same descriptions reach the control the user operates +# * mirror the input's `disabled` state onto the button and wrapper, and +# keep them in step when the input's `disabled` attribute changes at +# runtime (a MutationObserver) +# +# These configurations are not present on the profile form, so they are driven +# through a test-support example (examples/attachment/enhancement). +RSpec.describe "Attachment field enhancement", :aggregate_failures do + include AttachmentFieldHelpers + + let(:page_path) { example_path("attachment", "enhancement") } + + it "gives the label an id so the button's accessible name resolves" do + visit page_path + + field = attachment_field("profile[avatar]") + button = field.find(".govuk-file-upload-button", wait: 5) + + # Every id the button is labelled by must resolve to an element on the + # page — no dangling reference. + button["aria-labelledby"].split.each do |id| + expect(page).to have_css("##{id}", visible: :all) + end + + # ...and the first of those is the field's own label, which the enhancement + # has given an id. + expect(page).to have_css("label#profile-avatar-field-label", text: "Profile photo") + end + + it "copies the input's aria-describedby onto the button" do + visit page_path + + field = attachment_field("profile[avatar]") + button = field.find(".govuk-file-upload-button", wait: 5) + input = field.find("input[type=file]", visible: :all) + + described = button["aria-describedby"] + + expect(described).to eq(input["aria-describedby"]) + # The referenced element is the field's hint, so the same description the + # input carried now describes the button. + expect(page).to have_css("##{described}", text: "Upload a clear colour photograph", visible: :all) + end + + it "mirrors the input's disabled state onto the button and wrapper" do + visit page_path + + field = attachment_field("profile[cv]") + + expect(field).to have_css(".govuk-file-upload-button[disabled]", wait: 5) + expect(page).to have_css( + ".govuk-file-upload-wrapper--disabled:has(input[name='profile[cv]'])", + visible: :all, + ) + end + + it "keeps the button in step when the input is enabled at runtime" do + visit page_path + + field = attachment_field("profile[cv]") + expect(field).to have_css(".govuk-file-upload-button[disabled]", wait: 5) + + # A consumer (or a Turbo refresh) re-enables the input; the observer keeps + # the injected button and wrapper in step. + input = field.find("input[type=file]", visible: :all) + page.execute_script("arguments[0].disabled = false", input) + + expect(field).to have_css(".govuk-file-upload-button:not([disabled])") + expect(page).to have_no_css( + ".govuk-file-upload-wrapper--disabled:has(input[name='profile[cv]'])", + visible: :all, + ) + end +end diff --git a/spec/system/attachment/no_upload_mode_spec.rb b/spec/system/attachment/no_upload_mode_spec.rb new file mode 100644 index 0000000..fda5379 --- /dev/null +++ b/spec/system/attachment/no_upload_mode_spec.rb @@ -0,0 +1,87 @@ +# frozen_string_literal: true + +require "rails_helper" + +# The enhanced field without a direct-upload endpoint (direct_upload: false, +# or no route available): selection still previews and counts, but no upload +# starts and nothing claims the files — they stay in the input and submit as +# ordinary multipart. +RSpec.describe "Attachment field without direct upload", :aggregate_failures do + include AttachmentFieldHelpers + include DirectUploadHelpers + + let(:profile) { create(:profile) } + + before do + Capybara.enable_aria_label = true + disable_direct_uploads + end + + after do + Capybara.enable_aria_label = false + end + + it "previews the selection without starting an upload" do + visit edit_profile_path(profile) + + choose_gallery_file("avatar.png") + + expect(gallery_field).to have_css("figure.govuk-attachment .filename", text: "avatar.png") + + # No upload lifecycle: the figure never enters an upload state. + expect(gallery_field).to have_no_css("figure.govuk-attachment[data-state]") + expect(gallery_field).to have_no_css("figure.govuk-attachment progress") + end + + it "counts the selection in the status region" do + visit edit_profile_path(profile) + + choose_gallery_file("avatar.png") + + expect(status_region(gallery_field)).to have_text("1 file chosen") + end + + it "submits the selection as ordinary multipart" do + visit edit_profile_path(profile) + + choose_gallery_file("avatar.png") + click_button "Continue" + + expect(page).to have_current_path(profile_path(profile)) + expect(profile.reload.gallery.blobs.map { |blob| blob.filename.to_s }).to eq(%w[avatar.png]) + end + + it "removes the file from the submission when its preview is removed" do + visit edit_profile_path(profile) + + choose_gallery_file("avatar.png") + # Scoped: the required avatar's own figure offers "Remove avatar.png" too. + within(gallery_field) { click_button "Remove avatar.png" } + + expect(gallery_field).to have_no_css("figure.govuk-attachment") + expect(announcements_region(gallery_field)).to have_text("avatar.png removed") + expect(status_region(gallery_field)).to have_text("No file chosen") + + click_button "Continue" + + expect(page).to have_current_path(profile_path(profile)) + expect(profile.reload.gallery).not_to be_attached + end + + # Browsers replace the FileList on re-selection, so previews of files no + # longer in the input must not linger (they would suggest files that won't + # submit). + it "replaces stale previews when the selection changes" do + visit edit_profile_path(profile) + + choose_gallery_file("avatar.png") + + expect(gallery_field).to have_css("figure.govuk-attachment", count: 1) + + choose_gallery_file("cv.pdf") + + expect(gallery_field).to have_css("figure.govuk-attachment .filename", text: "cv.pdf") + expect(gallery_field).to have_no_css("figure.govuk-attachment .filename", text: "avatar.png") + expect(status_region(gallery_field)).to have_text("1 file chosen") + end +end diff --git a/spec/system/attachment/remove_spec.rb b/spec/system/attachment/remove_spec.rb new file mode 100644 index 0000000..74bc0f7 --- /dev/null +++ b/spec/system/attachment/remove_spec.rb @@ -0,0 +1,98 @@ +# frozen_string_literal: true + +require "rails_helper" + +# Removing an attachment figure with JavaScript. Together the examples pin +# the behaviour of the remove control: +# +# * each figure offers a remove control named after its file +# * removing deletes the figure from the DOM so its signed id no longer +# submits; Rails' auto-blank still clears an emptied has_many +# * focus moves to the next figure's control, else the previous one's, +# else the field's upload button — never lost to +# * removal is announced through the field's announcements region, naming +# the file — the figure's own live region disappears with the figure +RSpec.describe "Removing an attachment", :aggregate_failures do + include AttachmentFieldHelpers + + let(:profile) do + create(:profile).tap do |p| + %w[first.png second.png].each do |filename| + p.gallery.attach(io: File.open(file_fixture("avatar.png")), filename:, content_type: "image/png") + end + end + end + + before do + Capybara.enable_aria_label = true + end + + after do + Capybara.enable_aria_label = false + end + + it "offers a remove control on each figure whose accessible name includes the filename" do + visit edit_profile_path(profile) + + %w[first.png second.png].each do |filename| + within(".govuk-attachment", text: filename) do + expect(page).to have_button("Remove #{filename}") + end + end + end + + it "removes the figure so its value no longer submits" do + visit edit_profile_path(profile) + + click_button "Remove first.png" + + expect(page).to have_no_css(".govuk-attachment", text: "first.png") + expect(page).to have_css(".govuk-attachment", text: "second.png") + + click_button "Continue" + + expect(page).to have_current_path(profile_path(profile)) + expect(profile.reload.gallery.blobs.map { |blob| blob.filename.to_s }).to eq(%w[second.png]) + end + + it "clears the association when every figure is removed" do + visit edit_profile_path(profile) + + click_button "Remove first.png" + click_button "Remove second.png" + + # Scoped: the required avatar keeps its own figure on the page. + expect(gallery_field).to have_no_css(".govuk-attachment") + + click_button "Continue" + + expect(page).to have_current_path(profile_path(profile)) + expect(profile.reload.gallery).not_to be_attached + end + + it "moves focus to the next figure's control, then falls back to the file input" do + visit edit_profile_path(profile) + + click_button "Remove first.png" + + # The next figure's remove control takes focus... + focused = "document.activeElement.getAttribute('aria-label') || document.activeElement.textContent" + expect(page.evaluate_script(focused)).to include("second.png") + + click_button "Remove second.png" + + # ...and with no figures left, this field's file upload button does — never , + # and not some other field's input. + expect( + page.evaluate_script(%(document.activeElement.matches("button#profile-gallery-field"))), + ).to be(true) + end + + it "announces the removal, naming the file" do + visit edit_profile_path(profile) + + click_button "Remove first.png" + + expect(announcements_region(gallery_field)).to have_text("first.png removed") + end +end diff --git a/spec/system/attachment/replace_spec.rb b/spec/system/attachment/replace_spec.rb new file mode 100644 index 0000000..578cd64 --- /dev/null +++ b/spec/system/attachment/replace_spec.rb @@ -0,0 +1,74 @@ +# frozen_string_literal: true + +require "rails_helper" + +# Replacing a has_one_attached file with JavaScript, exercised through the +# avatar field on the profile form. +# +# Every figure's select posts the same scalar param (profile[avatar]) and the +# last one wins, so uploading a replacement appends a new figure rather than +# editing the old one: CSS hides every figure but the last, and the original +# stays in the DOM so removing the replacement reveals it again — reverting +# is free. +RSpec.describe "Replacing an attachment", :aggregate_failures do + include AttachmentFieldHelpers + + let(:profile) do + create(:profile).tap do |p| + p.avatar.attach(io: File.open(file_fixture("avatar.png")), filename: "old-avatar.png", content_type: "image/png") + end + end + + before do + Capybara.enable_aria_label = true + end + + after do + Capybara.enable_aria_label = false + end + + it "supersedes the existing figure, leaving one visible" do + visit edit_profile_path(profile) + + choose_avatar_file("avatar.png") + + expect(avatar_field).to have_css("figure.govuk-attachment[data-state=upload-successful]", wait: 10) + + # Both figures stay in the DOM (the original is what makes revert + # possible), but only the replacement is shown. + expect(avatar_field).to have_css("figure.govuk-attachment", count: 2, visible: :all) + expect(avatar_field).to have_css("figure.govuk-attachment", count: 1) + expect(avatar_field).to have_css("figure.govuk-attachment .filename", exact_text: "avatar.png") + expect(avatar_field).to have_css("figure.govuk-attachment", text: "old-avatar.png", visible: :hidden) + end + + it "saves the replacement" do + visit edit_profile_path(profile) + + choose_avatar_file("avatar.png") + + expect(avatar_field).to have_css("figure.govuk-attachment[data-state=upload-successful]", wait: 10) + + click_button "Continue" + + expect(page).to have_current_path(profile_path(profile)) + expect(profile.reload.avatar.filename.to_s).to eq("avatar.png") + end + + it "reverts to the original when the replacement is removed" do + visit edit_profile_path(profile) + + choose_avatar_file("avatar.png") + + expect(avatar_field).to have_css("figure.govuk-attachment[data-state=upload-successful]", wait: 10) + + click_button "Remove avatar.png" + + expect(avatar_field).to have_css("figure.govuk-attachment .filename", exact_text: "old-avatar.png") + + click_button "Continue" + + expect(page).to have_current_path(profile_path(profile)) + expect(profile.reload.avatar.filename.to_s).to eq("old-avatar.png") + end +end diff --git a/spec/system/attachment/round_trip_spec.rb b/spec/system/attachment/round_trip_spec.rb new file mode 100644 index 0000000..bbcd814 --- /dev/null +++ b/spec/system/attachment/round_trip_spec.rb @@ -0,0 +1,55 @@ +# frozen_string_literal: true + +require "rails_helper" + +# End-to-end round-trip of a direct-uploaded attachment across an invalid +# submit (E1/E2), exercised through the gallery (has_many_attached) field. +# +# A file is direct-uploaded with JavaScript (its figure reaches +# upload-successful, carrying the new blob's signed id), then the form is made +# invalid and submitted. The server re-renders the form with the just-uploaded +# blob as a plain server-rendered figure — no upload state, its signed id +# preserved so it still submits — so the user never re-uploads. The failure +# also renders in the standard GOV.UK error summary. +RSpec.describe "Attachment round-trip across an invalid submit", :aggregate_failures do + include AttachmentFieldHelpers + + let(:profile) { create(:profile) } + + it "re-renders the uploaded blob as a server figure and shows the error summary" do + visit edit_profile_path(profile) + + choose_gallery_file("avatar.png") + + expect(gallery_field).to have_css("figure.govuk-attachment[data-state=upload-successful]", wait: 10) + + signed_id = gallery_field.find( + "figure.govuk-attachment select[name='profile[gallery][]'] option:first-of-type", + visible: :all, + ).value + + # Invalidate the form so the submit fails and re-renders. + fill_in "Name", with: "" + click_button "Continue" + + # The failed submit re-renders the form in place (Turbo keeps the edit URL). + expect(page).to have_css(".govuk-error-summary") + + # The uploaded blob comes back as a server-rendered figure: it names the + # file and carries no upload state (it is not re-uploaded), and its keep + # option still holds the signed id so it submits unchanged. + expect(gallery_field).to have_css("figure.govuk-attachment .filename", text: "avatar.png") + expect(gallery_field).to have_no_css("figure.govuk-attachment[data-state]") + expect(gallery_field).to have_css( + "figure.govuk-attachment select[name='profile[gallery][]'] option[value='#{signed_id}']", + visible: :all, + ) + + # The preserved figure really does submit: fix the form and the blob attaches. + fill_in "Name", with: "Ada Lovelace" + click_button "Continue" + + expect(page).to have_current_path(profile_path(profile)) + expect(profile.reload.gallery.blobs.map { |blob| blob.filename.to_s }).to eq(%w[avatar.png]) + end +end diff --git a/spec/system/attachment/status_spec.rb b/spec/system/attachment/status_spec.rb new file mode 100644 index 0000000..2bbb6f5 --- /dev/null +++ b/spec/system/attachment/status_spec.rb @@ -0,0 +1,65 @@ +# frozen_string_literal: true + +require "rails_helper" + +# The upload button's status region describes the field's contents using +# govuk-frontend's FileUpload i18n strings: "No file chosen" when the field +# is empty, else the count of attachment figures plus any files held in the +# FileList ("2 files chosen"). The region carries only this state — event +# announcements (e.g. removals) go to the assertive announcements region. +RSpec.describe "Attachment field status", :aggregate_failures do + include AttachmentFieldHelpers + + let(:profile) { create(:profile) } + + before do + Capybara.enable_aria_label = true + end + + after do + Capybara.enable_aria_label = false + end + + it "reports an empty field as having no file chosen" do + visit edit_profile_path(profile) + + expect(status_region(gallery_field)).to have_text("No file chosen") + end + + it "reports an empty has_one field as having no file chosen" do + visit edit_profile_path(profile) + + expect(status_region(cv_field)).to have_text("No file chosen") + end + + it "counts existing attachments on load" do + attach_gallery_files + visit edit_profile_path(profile) + + expect(status_region(gallery_field)).to have_text("2 files chosen") + end + + it "counts a newly uploaded file" do + visit edit_profile_path(profile) + + choose_gallery_file("avatar.png") + + expect(gallery_field).to have_css("figure.govuk-attachment[data-state=upload-successful]", wait: 10) + expect(status_region(gallery_field)).to have_text("1 file chosen") + end + + it "recounts after a removal" do + attach_gallery_files + visit edit_profile_path(profile) + + click_button "Remove first.png" + + expect(status_region(gallery_field)).to have_text("1 file chosen") + end + + def attach_gallery_files + %w[first.png second.png].each do |filename| + profile.gallery.attach(io: File.open(file_fixture("avatar.png")), filename:, content_type: "image/png") + end + end +end diff --git a/spec/system/attachment/upload_spec.rb b/spec/system/attachment/upload_spec.rb new file mode 100644 index 0000000..f4bab8d --- /dev/null +++ b/spec/system/attachment/upload_spec.rb @@ -0,0 +1,102 @@ +# frozen_string_literal: true + +require "rails_helper" + +# Direct-upload lifecycle for the attachment field, exercised through the +# gallery (has_many_attached) field on the profile form. +# +# The figure's `data-state` attribute is the observable surface, so the +# implementation is free to change markup/classes without breaking the +# behavioural contract: +# +# * choosing a file immediately inserts a preview figure in an +# `uploading` state +# * on direct-upload success the figure reaches `upload-successful` and +# its select's first option carries the new blob's signed id +# * on failure the figure reaches `upload-failed` with a human-readable +# message and retry/remove; a failed figure never submits a signed id +# +RSpec.describe "Async file upload", :aggregate_failures do + include AttachmentFieldHelpers + include DirectUploadHelpers + + let(:profile) { create(:profile) } + + it "previews the chosen file immediately, in an uploading state" do + block_direct_uploads + visit edit_profile_path(profile) + + expect(gallery_field).to have_field("profile[gallery][]", type: :file, visible: :all) + expect(gallery_field).to have_no_css("figure.govuk-attachment") + + choose_gallery_file("avatar.png") + + expect(gallery_field).to have_css("figure.govuk-attachment[data-state=uploading] .filename", text: "avatar.png") + expect(gallery_field).to have_css("figure.govuk-attachment[data-state=uploading] progress") + + release_direct_uploads + + expect(gallery_field).to have_css("figure.govuk-attachment[data-state=upload-successful]", wait: 10) + + # The progress bar is removed and the caption's live region announces the + # result, so screen readers hear which file finished. + expect(gallery_field).to have_no_css("figure.govuk-attachment progress") + expect(gallery_field).to have_css("figure.govuk-attachment figcaption .status", text: /uploaded/i) + end + + it "reaches upload-successful with the blob's signed id in the select" do + visit edit_profile_path(profile) + + choose_gallery_file("avatar.png") + + # Generous wait: a real XHR to the direct-uploads endpoint plus a blob PUT. + expect(gallery_field).to have_css("figure.govuk-attachment[data-state=upload-successful]", wait: 10) + + signed_id = gallery_field.find( + "figure.govuk-attachment select[name='profile[gallery][]'] option:first-of-type", + visible: :all, + ).value + expect(ActiveStorage::Blob.find_signed(signed_id)&.filename&.to_s).to eq("avatar.png") + end + + # The figure claims the file from the input's FileList when its upload + # starts, so submitting must attach the blob once — not again as multipart. + it "attaches an uploaded file exactly once on submit" do + visit edit_profile_path(profile) + + choose_gallery_file("avatar.png") + + expect(gallery_field).to have_css("figure.govuk-attachment[data-state=upload-successful]", wait: 10) + + click_button "Continue" + + expect(page).to have_current_path(profile_path(profile)) + expect(profile.reload.gallery.blobs.map { |blob| blob.filename.to_s }).to eq(%w[avatar.png]) + end + + it "reaches upload-failed with a human-readable message, and never submits a signed id" do + break_direct_uploads + visit edit_profile_path(profile) + + # Record alert calls: ActiveStorage's dispatchError falls back to window.alert + page.execute_script("window.alertCalls = []; window.alert = (message) => window.alertCalls.push(message)") + + choose_gallery_file("avatar.png") + + figure = gallery_field.find("figure.govuk-attachment[data-state=upload-failed]", wait: 10) + + # A friendly message, not DirectUpload's raw error/alert. + expect(figure.text).to match(/Upload failed/) + expect(figure.text).not_to match(/Error/) + expect(page.evaluate_script("window.alertCalls")).to eq([]) + + # The user can recover from the error + expect(figure).to have_css("button[aria-label*='Remove']") + + click_button "Continue" + + # Submitting without clearing the error does not save the file + expect(page).to have_current_path(profile_path(profile)) + expect(profile.reload.gallery).not_to be_attached + end +end From eab5fe526aa6b9e5847a9d47b17de4c15ab8555b Mon Sep 17 00:00:00 2001 From: Stephen Nelson Date: Thu, 23 Jul 2026 22:55:36 +0930 Subject: [PATCH 05/22] Attachments: round-trip multi-part form uploads --- .../govuk/form_builder/elements/attachment.rb | 4 +- .../govuk/form_builder/traits/attachment.rb | 55 +++++-- spec/builders/image_field_spec.rb | 27 ++++ spec/builders/pending_attachment_spec.rb | 150 ++++++++++++++++++ spec/requests/profiles/avatar_spec.rb | 75 +++++++++ spec/requests/profiles/gallery_spec.rb | 28 ++++ spec/support/attachment_request_helpers.rb | 6 + spec/system/attachment/upload_spec.rb | 4 +- 8 files changed, 334 insertions(+), 15 deletions(-) create mode 100644 spec/builders/pending_attachment_spec.rb diff --git a/app/helpers/katalyst/govuk/form_builder/elements/attachment.rb b/app/helpers/katalyst/govuk/form_builder/elements/attachment.rb index 1269f75..77f16cd 100644 --- a/app/helpers/katalyst/govuk/form_builder/elements/attachment.rb +++ b/app/helpers/katalyst/govuk/form_builder/elements/attachment.rb @@ -29,7 +29,9 @@ def file end def file_with_javascript_markup - tag.div(class: "#{brand}-file-upload-wrapper", data: { controller: "#{brand}-file-upload" }, **i18n_data) { file } + tag.div(class: "#{brand}-file-upload-wrapper", data: { controller: "#{brand}-file-upload" }, **i18n_data) do + file + end end end end diff --git a/app/helpers/katalyst/govuk/form_builder/traits/attachment.rb b/app/helpers/katalyst/govuk/form_builder/traits/attachment.rb index c6f0841..49a45a2 100644 --- a/app/helpers/katalyst/govuk/form_builder/traits/attachment.rb +++ b/app/helpers/katalyst/govuk/form_builder/traits/attachment.rb @@ -30,19 +30,20 @@ def value delegate :attached?, to: :value - # @return [ActiveSupport::SafeBuffer|nil] + # @return [ActiveSupport::SafeBuffer,nil] def attachment return unless attached? - if one? - attachment_for(value.blob) - elsif many? - safe_join(value.blobs.map { |blob| attachment_for(blob) }) - end + # Preserve unsaved multi-part form uploads before rendering + # mimics direct-upload for non-js consumers. + persist_pending_blobs + + blobs = (one? ? [value.blob] : value.blobs).select(&:persisted?) + safe_join(blobs.map { |blob| attachment_for(blob) }) end # @param [ActiveStorage::Blob] blob - # @return [ActiveSupport::SafeBuffer|nil] + # @return [ActiveSupport::SafeBuffer,nil] def attachment_for(blob) tag.figure(class: "#{brand}-attachment", aria: { labelledby: attachment_id_for(blob, :caption) }, @@ -71,7 +72,7 @@ def attachment_actions_for(blob) # A - + `; @@ -222,11 +232,14 @@ export function createAttachment(input, file, brand = "govuk") { const [keep, remove] = figure.querySelectorAll("option"); keep.textContent = file.name; - remove.textContent = `Remove ${file.name}`; + remove.textContent = i18n.t("removeButton", { filename: file.name }); - figure - .querySelector("button") - .setAttribute("aria-label", `Remove ${file.name}`); + const removeButton = figure.querySelector("button"); + removeButton.textContent = i18n.t("removeButtonContent"); + removeButton.setAttribute( + "aria-label", + i18n.t("removeButton", { filename: file.name }), + ); // The figure carries its File until an attachment controller connects and // claims it for upload; while it remains, the file is still in the input's @@ -248,19 +261,19 @@ function humanSize(bytes) { return `${value} ${UNITS[exp]}`; } -function createRetryButton(filename, brand = "govuk") { +function createRetryButton(filename, i18n) { const button = document.createElement("BUTTON"); button.type = "button"; button.className = "retry"; - button.textContent = "Try again"; - button.setAttribute("aria-label", `Try again ${filename}`); - button.dataset.action = `${brand}-attachment#retry`; + button.textContent = i18n.t("retryButton"); + button.setAttribute("aria-label", `${i18n.t("retryButton")} ${filename}`); + button.dataset.action = "govuk-attachment#retry"; return button; } -function createProgressTag(labelId, brand = "govuk") { +function createProgressTag(labelId) { const progress = document.createElement("PROGRESS"); - progress.className = `${brand}-attachment-progress`; + progress.className = `${config.brand}-attachment-progress`; if (labelId) progress.setAttribute("aria-labelledby", labelId); progress.value = 0; progress.max = 100; diff --git a/app/javascript/katalyst/govuk/controllers/file_upload_controller.js b/app/javascript/katalyst/govuk/controllers/file_upload_controller.js index faeeb69..7fcc9c3 100644 --- a/app/javascript/katalyst/govuk/controllers/file_upload_controller.js +++ b/app/javascript/katalyst/govuk/controllers/file_upload_controller.js @@ -1,7 +1,8 @@ import { Controller } from "@hotwired/stimulus"; import { I18n } from "govuk-frontend/dist/govuk/i18n.mjs"; -import { FileUpload } from "govuk-frontend/dist/govuk/all.mjs"; +import { closestAttributeValue } from "govuk-frontend/dist/govuk/common/closest-attribute-value.mjs"; import { createAttachment } from "./attachment_controller"; +import config, { attachmentConfig } from "../config"; export default class FileUploadController extends Controller { connect() { @@ -9,7 +10,10 @@ export default class FileUploadController extends Controller { throw new Error(`Missing file input for ${this.element}`); } - this.i18n = new I18n(FileUpload.defaults.i18n, { locale: "en" }); + this.config = attachmentConfig(this.element); + this.i18n = new I18n(this.config.i18n, { + locale: closestAttributeValue(this.element, "lang"), + }); this.id = this.uploadButton?.id ?? this.fileInput.id; // dragenter/dragleave are on the document so we can tell a move between @@ -122,7 +126,7 @@ export default class FileUploadController extends Controller { this.uploadButton.disabled = disabled; this.element.classList.toggle( - "govuk-file-upload-wrapper--disabled", + `${config.brand}-file-upload-wrapper--disabled`, disabled, ); } @@ -194,11 +198,15 @@ export default class FileUploadController extends Controller { } showDraggingState() { - this.uploadButton.classList.add("govuk-file-upload-button--dragging"); + this.uploadButton.classList.add( + `${config.brand}-file-upload-button--dragging`, + ); } hideDraggingState() { - this.uploadButton.classList.remove("govuk-file-upload-button--dragging"); + this.uploadButton.classList.remove( + `${config.brand}-file-upload-button--dragging`, + ); } announce(message) { @@ -206,7 +214,9 @@ export default class FileUploadController extends Controller { } get announcements() { - return this.element.querySelector(".govuk-file-upload-announcements"); + return this.element.querySelector( + `.${config.brand}-file-upload-announcements`, + ); } // Whether a drop of this many files is allowed: any for a multiple input, @@ -252,7 +262,7 @@ export default class FileUploadController extends Controller { files.forEach((file) => { if (figures.some((figure) => figure.file === file)) return; - const attachment = createAttachment(this.fileInput, file); + const attachment = createAttachment(this.fileInput, file, this.i18n); this.uploadButton.insertAdjacentElement("beforebegin", attachment); }); @@ -293,7 +303,7 @@ export default class FileUploadController extends Controller { // The removal is an event, so it is announced through the assertive // announcements region; the polite status region only ever carries // state (the count), which updates in place. - this.announce(`${name} removed`); + this.announce(this.i18n.t("fileRemoved", { filename: name })); this.updateCount(); }; @@ -302,10 +312,14 @@ export default class FileUploadController extends Controller { if (count === 0) { this.statusTag.innerText = this.i18n.t("noFileChosen"); - this.uploadButton.classList.add("govuk-file-upload-button--empty"); + this.uploadButton.classList.add( + `${config.brand}-file-upload-button--empty`, + ); } else { this.statusTag.innerText = this.i18n.t("multipleFilesChosen", { count }); - this.uploadButton.classList.remove("govuk-file-upload-button--empty"); + this.uploadButton.classList.remove( + `${config.brand}-file-upload-button--empty`, + ); } } @@ -321,7 +335,7 @@ export default class FileUploadController extends Controller { get isDragging() { return this.uploadButton.classList.contains( - "govuk-file-upload-button--dragging", + `${config.brand}-file-upload-button--dragging`, ); } @@ -354,14 +368,15 @@ function countFileItems(items) { // A visually-hidden assertive live region for event announcements (drag // enter/leave, removals), kept separate from the polite status region that // carries the file count. -function createAnnouncements(brand = "govuk") { +function createAnnouncements() { const region = document.createElement("span"); - region.className = `${brand}-file-upload-announcements ${brand}-visually-hidden`; + region.className = `${config.brand}-file-upload-announcements ${config.brand}-visually-hidden`; region.setAttribute("aria-live", "assertive"); return region; } -function createUploadButton(id, i18n, fileInput, brand = "govuk") { +function createUploadButton(id, i18n, fileInput) { + const brand = config.brand; const template = document.createElement("TEMPLATE"); template.innerHTML = ` `; diff --git a/app/javascript/katalyst/govuk/formbuilder.js b/app/javascript/katalyst/govuk/formbuilder.js index 0e7cf1d..5657fbc 100644 --- a/app/javascript/katalyst/govuk/formbuilder.js +++ b/app/javascript/katalyst/govuk/formbuilder.js @@ -114,7 +114,9 @@ function observe(body) { // the element itself, so a Turbo replace render — which swaps in a // new body and re-executes the snippet — disposes and recreates them, while // a morph retains the body and the observers with it. -function init() { +function init(options = {}) { + if (options.brand) config.brand = options.brand; + const body = document.body; if (body.__govukFormbuilderInit) return; @@ -127,6 +129,7 @@ function init() { // stimulus controllers import controllers from "./controllers"; +import config from "./config"; export { controllers as default, diff --git a/config/locales/en.yml b/config/locales/en.yml new file mode 100644 index 0000000..da0d1b1 --- /dev/null +++ b/config/locales/en.yml @@ -0,0 +1,16 @@ +# The attachment field's string defaults — the canonical vocabulary. +# The JS bundle carries a mirror of this table as its offline defaults +# (app/javascript/katalyst/govuk/config.js); keep the two in step. +# Strings that differ from these defaults — a translation, or a host +# app's override in any locale, en included — render onto the field's +# data-i18n.* attributes, where the JS enhancement reads them. +en: + katalyst: + govuk: + attachment: + upload_succeeded: "Uploaded successfully" + upload_failed: "Upload failed — try again" + retry_button: "Try again" + file_removed: "%{filename} removed" + remove_button: "Remove %{filename}" + remove_button_content: "×" diff --git a/lib/katalyst/govuk/form_builder/config.rb b/lib/katalyst/govuk/form_builder/config.rb index 4c4c827..588c159 100644 --- a/lib/katalyst/govuk/form_builder/config.rb +++ b/lib/katalyst/govuk/form_builder/config.rb @@ -27,6 +27,16 @@ def image_mime_types=(value) config.image_mime_types = %w[image/png image/gif image/jpeg image/webp].freeze + def attachment_preview_representation + config.attachment_preview_representation + end + + def attachment_preview_representation=(value) + config.attachment_preview_representation = value + end + + config.attachment_preview_representation = { resize_and_pad: [100, 100, { crop: :centre }] }.freeze + def use_legacy_file_fields? config.use_legacy_file_fields end diff --git a/spec/builders/govuk_design_system_form_builder/form_builder_attachment_field_spec.rb b/spec/builders/govuk_design_system_form_builder/form_builder_attachment_field_spec.rb index 2d8e0d8..3f5740f 100644 --- a/spec/builders/govuk_design_system_form_builder/form_builder_attachment_field_spec.rb +++ b/spec/builders/govuk_design_system_form_builder/form_builder_attachment_field_spec.rb @@ -86,6 +86,169 @@ def govuk_attachment_field(...) end end + # Each text option renders as a data-i18n.* attribute on the wrapper. + # The attribute names are the contract's read surface — the JS + # enhancement configures its strings from these exact names, so a + # rename here breaks localisation without failing anything else. + context "with i18n text options" do + def wrapper(html) + html.find(".govuk-file-upload-wrapper", visible: :all) + end + + { + choose_files_button_text: "data-i18n.choose-files-button", + drop_instruction_text: "data-i18n.drop-instruction", + no_file_chosen_text: "data-i18n.no-file-chosen", + multiple_files_chosen_one_text: "data-i18n.multiple-files-chosen.one", + multiple_files_chosen_other_text: "data-i18n.multiple-files-chosen.other", + entered_drop_zone_text: "data-i18n.entered-drop-zone", + left_drop_zone_text: "data-i18n.left-drop-zone", + upload_succeeded_text: "data-i18n.upload-succeeded", + upload_failed_text: "data-i18n.upload-failed", + retry_button_text: "data-i18n.retry-button", + file_removed_text: "data-i18n.file-removed", + remove_button_text: "data-i18n.remove-button", + remove_button_content_text: "data-i18n.remove-button-content", + }.each do |option, attribute| + it "renders #{option} as #{attribute}" do + html = govuk_attachment_field(:avatar, option => "Custom text") + + expect(wrapper(html)[attribute]).to eq("Custom text") + end + end + + # %{count} is govuk-frontend's interpolation placeholder, passed + # through verbatim — not a Ruby format token. + # rubocop:disable Style/FormatStringToken + it "renders the one form of a multiple_files_chosen_text hash" do + html = govuk_attachment_field(:avatar, multiple_files_chosen_text: { one: "1 file", other: "%{count} files" }) + + expect(wrapper(html)["data-i18n.multiple-files-chosen.one"]).to eq("1 file") + end + + it "renders the other form of a multiple_files_chosen_text hash" do + html = govuk_attachment_field(:avatar, multiple_files_chosen_text: { one: "1 file", other: "%{count} files" }) + + expect(wrapper(html)["data-i18n.multiple-files-chosen.other"]).to eq("%{count} files") + end + # rubocop:enable Style/FormatStringToken + + # With no options the attributes are absent, leaving the strings to + # the enhancement's own defaults. + it "renders no i18n data attributes by default" do + attributes = wrapper(html).native.attribute_nodes.map(&:name) + + expect(attributes.grep(/\Adata-i18n/)).to be_empty + end + end + + # The remove strings also render into the server figures, so the option + # must drive both the data attribute (for client figures) and the + # trait's own markup — string parity between the two figure sources + # depends on the shared option. + # rubocop:disable Style/FormatStringToken + context "with remove control options" do + subject(:html) do + govuk_attachment_field(:avatar, remove_button_text: "Bin %{filename}", remove_button_content_text: "🗑") + end + + it "names the remove option with the interpolated text" do + expect(html).to have_css( + "figure.govuk-attachment select option[value='']", + text: "Bin avatar.png", + visible: :all, + ) + end + + it "labels the remove button with the interpolated text" do + button = html.find("figure.govuk-attachment .actions button") + + expect(button["aria-label"]).to eq("Bin avatar.png") + end + + it "renders the configured remove button content" do + expect(html).to have_css("figure.govuk-attachment .actions button", text: "🗑") + end + end + # rubocop:enable Style/FormatStringToken + + # The attachment strings live in the gem's locale files + # (config/locales), so a consuming app localises by adding Rails + # translations — no JS knowledge needed. A translated string renders + # into the server figures and onto the data-i18n.* attributes (where + # the JS reads it); the en defaults render neither, leaving the JS to + # its bundled mirror of the same table. + context "with a non-default locale" do + around do |example| + # rubocop:disable Style/FormatStringToken -- %{filename} is the i18n placeholder + I18n.backend.store_translations(:fr, { katalyst: { govuk: { attachment: { + upload_succeeded: "Téléversement réussi", + remove_button: "Supprimer %{filename}", + remove_button_content: "Retirer", + } } } }) + # rubocop:enable Style/FormatStringToken + I18n.with_locale(:fr) { example.run } + ensure + I18n.backend.reload! + end + + it "renders the translation onto the data-i18n attribute" do + wrapper = html.find(".govuk-file-upload-wrapper", visible: :all) + + expect(wrapper["data-i18n.upload-succeeded"]).to eq("Téléversement réussi") + end + + it "names the remove option from the translation" do + expect(html).to have_css( + "figure.govuk-attachment select option[value='']", + text: "Supprimer avatar.png", + visible: :all, + ) + end + + it "renders the remove button content from the translation" do + expect(html).to have_css("figure.govuk-attachment .actions button", text: "Retirer") + end + + it "prefers an explicit option over the translation" do + html = govuk_attachment_field(:avatar, upload_succeeded_text: "Custom text") + wrapper = html.find(".govuk-file-upload-wrapper", visible: :all) + + expect(wrapper["data-i18n.upload-succeeded"]).to eq("Custom text") + end + end + + # Overriding the gem's en strings in the app's own locale files is the + # same customisation path as any other locale: the override must reach + # the data-i18n.* attributes, or server-rendered figures show the + # override while JS-created figures and announcements fall back to the + # gem's bundled defaults — a mixed UI. + context "with an app-level en override" do + around do |example| + # The backend loads locale files lazily on first lookup, clobbering + # anything stored beforehand — initialise it first so the override + # merges over the gem's en table, as an app's locale file would. + I18n.backend.translations(do_init: true) + I18n.backend.store_translations(:en, { katalyst: { govuk: { attachment: { + upload_succeeded: "All done!", + remove_button_content: "Bin", + } } } }) + example.run + ensure + I18n.backend.reload! + end + + it "renders the override onto the data-i18n attribute" do + wrapper = html.find(".govuk-file-upload-wrapper", visible: :all) + + expect(wrapper["data-i18n.upload-succeeded"]).to eq("All done!") + end + + it "renders the override into the server figure" do + expect(html).to have_css("figure.govuk-attachment .actions button", text: "Bin") + end + end + context "with an attached image" do it "renders one figure with preview, caption, and actions, in order" do expect(html).to have_css("figure.govuk-attachment > img + figcaption + div.actions", count: 1, visible: :all) @@ -200,6 +363,39 @@ def govuk_attachment_field(...) end end + # Brand follows CSS classes only: a rebranding consumer replaces the + # stylesheet, but the JS ships with the gem and registers fixed govuk + # controller identifiers — behavioural wiring stays govuk whatever the + # brand. + context "with a non-default brand" do + around do |example| + GOVUKDesignSystemFormBuilder.brand = "defra" + example.run + ensure + GOVUKDesignSystemFormBuilder.brand = "govuk" + end + + it "prefixes the figure class with the brand" do + expect(html).to have_css("figure.defra-attachment", visible: :all) + end + + it "prefixes the wrapper class with the brand" do + expect(html).to have_css(".defra-file-upload-wrapper", visible: :all) + end + + it "connects the figure to the attachment controller" do + expect(html).to have_css("figure.defra-attachment[data-controller='govuk-attachment']", visible: :all) + end + + it "connects the wrapper to the file-upload controller" do + expect(html).to have_css(".defra-file-upload-wrapper[data-controller='govuk-file-upload']", visible: :all) + end + + it "keeps the remove action on the govuk identifier" do + expect(html).to have_css("button[data-action='govuk-attachment#destroy']", visible: :all) + end + end + # The preview URL is lazy — the variant is processed when the browser # requests it, so rendering the form never touches the blob's bytes and # a blob that can't be processed costs a broken image, not an error. @@ -305,6 +501,17 @@ def direct_upload_url .to eq(helper.rails_representation_path(representation)) end + it "renders the preview from the configured representation" do + config = GOVUKDesignSystemFormBuilder.config + original = config.attachment_preview_representation + config.attachment_preview_representation = { resize_to_limit: [50, 50] } + + expect(html.find("figure.govuk-attachment img")[:src]) + .to eq(helper.rails_representation_path(blob.representation(resize_to_limit: [50, 50]))) + ensure + config.attachment_preview_representation = original + end + it "resolves the preview URL through main_app" do allow(helper).to receive(:respond_to?).and_call_original allow(helper).to receive(:respond_to?).with(:rails_representation_path).and_return(false) @@ -411,6 +618,26 @@ def pending_blob end end + context "when the pending upload's tempfile has vanished before render" do + before do + profile.avatar = Rack::Test::UploadedFile.new(file_fixture("avatar.png"), "image/png") + File.unlink(profile.attachment_changes["avatar"].attachable.path) + end + + it "drops the figure rather than failing the render" do + expect(html).to have_no_css("figure.govuk-attachment") + end + + it "logs the dropped upload" do + allow(Rails.logger).to receive(:warn) + + html + + expect(Rails.logger).to have_received(:warn) + .with(include("avatar").and(include("Errno::ENOENT"))) + end + end + context "with a mixed gallery of a persisted signed id and a multipart upload" do let(:persisted) do ActiveStorage::Blob.create_and_upload!( diff --git a/spec/dummy/app/views/examples/attachment/i18n.html.erb b/spec/dummy/app/views/examples/attachment/i18n.html.erb new file mode 100644 index 0000000..f789d1d --- /dev/null +++ b/spec/dummy/app/views/examples/attachment/i18n.html.erb @@ -0,0 +1,28 @@ +<%# locals: (profile:) %> +<%# + Test-support form for the attachment field's i18n text options + (spec/system/attachment/i18n_spec.rb). Every option is a sentinel value, + so an assertion that passes cannot have been met by the enhancement's + bundled English defaults. +%> +<%= turbo_frame_tag example_frame_id(params[:page], params[:example]) do %> + <%= form_with(model: profile, url: example_path(params[:page], params[:example])) do |f| %> + <%= f.govuk_attachment_field :gallery, + label: { text: "Gallery" }, + choose_files_button_text: "XX pick files XX", + drop_instruction_text: "XX or drop XX", + no_file_chosen_text: "XX empty XX", + multiple_files_chosen_one_text: "XX one file: %{count} XX", + multiple_files_chosen_other_text: "XX many files: %{count} XX", + entered_drop_zone_text: "XX over zone XX", + left_drop_zone_text: "XX out of zone XX", + upload_succeeded_text: "XX stored XX", + upload_failed_text: "XX broken XX", + retry_button_text: "XX retry XX", + file_removed_text: "XX %{filename} gone XX", + remove_button_text: "XX bin %{filename} XX", + remove_button_content_text: "XX x XX" %> + + <%= example_submit_buttons(f) %> + <% end %> +<% end %> diff --git a/spec/dummy/config/routes.rb b/spec/dummy/config/routes.rb index fbd34ea..a474e59 100644 --- a/spec/dummy/config/routes.rb +++ b/spec/dummy/config/routes.rb @@ -12,7 +12,7 @@ # its own request: a lazy GET to render it and a POST to round-trip it. match "guide/:page/:example", to: "examples#show", as: :example, via: %i[get post], - constraints: { page: /[a-z_]+/, example: /[a-z_]+/ } + constraints: { page: /[a-z_]+/, example: /[a-z0-9_]+/ } # System tests point fields here to hold direct uploads until released. post "blocking_direct_uploads", to: "blocking_direct_uploads#create" diff --git a/spec/system/attachment/i18n_spec.rb b/spec/system/attachment/i18n_spec.rb new file mode 100644 index 0000000..fe56c69 --- /dev/null +++ b/spec/system/attachment/i18n_spec.rb @@ -0,0 +1,137 @@ +# frozen_string_literal: true + +require "rails_helper" + +# The builder's text options render as data-i18n.* attributes on the +# wrapper, and the enhancement reads its strings from them, falling back to +# its bundled defaults when they are absent. Driven through a test-support +# example (examples/attachment/i18n) whose options are all sentinel values, +# so a passing assertion cannot have been met by the English defaults. +# +# Locale (the closest `lang` attribute) feeds plural-form selection, but the +# builder only renders one/other forms — every count resolves to one of them +# in any locale, so locale handling has no observable surface to pin here. +RSpec.describe "Attachment field i18n", :aggregate_failures do + include AttachmentFieldHelpers + include DirectUploadHelpers + + let(:page_path) { example_path("attachment", "i18n") } + + it "builds the upload button from the configured strings" do + visit page_path + + field = attachment_field("profile[gallery][]") + + expect(field).to have_css(".govuk-file-upload-button__pseudo-button", text: "XX pick files XX", wait: 5) + expect(field).to have_css(".govuk-file-upload-button__instruction", text: "XX or drop XX") + expect(field).to have_css(".govuk-file-upload-button__status", text: "XX empty XX") + end + + it "counts a single chosen file with the one form" do + visit page_path + + choose_gallery_files("avatar.png") + + expect(status_region(attachment_field("profile[gallery][]"))).to have_text("XX one file: 1 XX") + end + + it "counts several chosen files with the other form" do + visit page_path + + choose_gallery_files("avatar.png", "cv.pdf") + + expect(status_region(attachment_field("profile[gallery][]"))).to have_text("XX many files: 2 XX") + end + + it "reports upload success with the configured string" do + visit page_path + + choose_gallery_files("avatar.png") + + figure = attachment_field("profile[gallery][]") + .find("figure.govuk-attachment[data-state=upload-successful]", wait: 10) + + expect(figure).to have_css("figcaption .status", text: "XX stored XX") + end + + it "reports upload failure and offers retry with the configured strings" do + break_direct_uploads + visit page_path + + choose_gallery_files("avatar.png") + + figure = attachment_field("profile[gallery][]") + .find("figure.govuk-attachment[data-state=upload-failed]", wait: 10) + + expect(figure).to have_css("figcaption .status", text: "XX broken XX") + expect(figure).to have_css( + "button[type=button][aria-label='XX retry XX avatar.png']", + text: "XX retry XX", + ) + end + + it "builds the figure's remove control from the configured strings" do + visit page_path + + choose_gallery_files("avatar.png") + + figure = attachment_field("profile[gallery][]").find("figure.govuk-attachment", wait: 10) + + expect(figure).to have_css("button[aria-label='XX bin avatar.png XX']", text: "XX x XX") + expect(figure).to have_css("select option[value='']", text: "XX bin avatar.png XX", visible: :all) + end + + it "announces a removal with the configured string, naming the file" do + visit page_path + + choose_gallery_files("avatar.png") + + field = attachment_field("profile[gallery][]") + field.find("figure.govuk-attachment[data-state=upload-successful]", wait: 10) + + field.find("button[aria-label='XX bin avatar.png XX']").click + + expect(announcements_region(field)).to have_text("XX avatar.png gone XX") + end + + it "announces the drop zone with the configured strings" do + visit page_path + + field = attachment_field("profile[gallery][]") + field.find(".govuk-file-upload-button", wait: 5) + + dispatch_drag("dragenter", drop_target) + + expect(announcements_region(field)).to have_text("XX over zone XX") + + # A dragenter outside the zone means the drag has left it. + dispatch_drag("dragenter", "body") + + expect(announcements_region(field)).to have_text("XX out of zone XX") + end + + def drop_target + ".govuk-file-upload-wrapper:has(input[name='profile[gallery][]']) .govuk-file-upload-button" + end + + def choose_gallery_files(*fixtures) + paths = fixtures.map { |fixture| file_fixture(fixture).to_s } + + within(attachment_field("profile[gallery][]")) do + attach_file("profile[gallery][]", paths, make_visible: true) + end + end + + # Dispatch a synthetic drag event (carrying one dragged file) on the + # element matching `selector`. + def dispatch_drag(type, selector) + page.execute_script(<<~JS, type, selector) + const [type, selector] = arguments; + const data = new DataTransfer(); + data.items.add(new File(["stub"], "dragged.png", { type: "image/png" })); + document.querySelector(selector).dispatchEvent( + new DragEvent(type, { dataTransfer: data, bubbles: true, cancelable: true }), + ); + JS + end +end From a87bb322e0fd09e4337cd81a567b6a6e375f2d93 Mon Sep 17 00:00:00 2001 From: Stephen Nelson Date: Fri, 24 Jul 2026 21:16:50 +0930 Subject: [PATCH 14/22] Attachments: basic styling --- .../govuk/components/attachment/_mixin.scss | 43 ++++++++++++++++++- .../govuk/form_builder/traits/attachment.rb | 6 ++- .../controllers/attachment_controller.js | 4 +- 3 files changed, 47 insertions(+), 6 deletions(-) diff --git a/app/assets/stylesheets/katalyst/govuk/components/attachment/_mixin.scss b/app/assets/stylesheets/katalyst/govuk/components/attachment/_mixin.scss index c845ff5..39e6cea 100644 --- a/app/assets/stylesheets/katalyst/govuk/components/attachment/_mixin.scss +++ b/app/assets/stylesheets/katalyst/govuk/components/attachment/_mixin.scss @@ -1,27 +1,66 @@ @use "govuk-frontend/dist/govuk/base"; +$attachment-background-colour: base.govuk-colour("black", $variant: "tint-95"); +$attachment-border-width: 2px; + @mixin styles { :where(.govuk-attachment) { display: grid; grid-template-areas: "preview caption actions"; + grid-template-columns: auto 1fr auto; + margin: 0; + padding: base.govuk-spacing(3) base.govuk-spacing(3); + grid-gap: base.govuk-spacing(2); + background-color: $attachment-background-colour; + border: $attachment-border-width solid + base.govuk-functional-colour("border"); .preview { grid-area: preview; + max-width: 4rem; + aspect-ratio: 1/1; } .caption { + display: flex; + flex-direction: column; grid-area: caption; } + .filename { + @include base.govuk-typography-weight-bold; + } + + .size { + color: base.govuk-functional-colour(secondary-text); + } + .actions { grid-area: actions; } - // The remove button requires JavaScript; without it the select is the - // visible control. + // Buttons require JavaScript .actions button { display: none; } + + &[data-state="upload-successful"] { + .status { + color: base.govuk-functional-colour("success"); + } + } + + &[data-state="upload-failed"] { + border-color: base.govuk-functional-colour("error"); + + .status { + color: base.govuk-functional-colour("error"); + } + } + } + + .govuk-attachment { + margin-bottom: base.govuk-spacing(2); } // With JavaScript running the button is the figure's only interactive diff --git a/app/helpers/katalyst/govuk/form_builder/traits/attachment.rb b/app/helpers/katalyst/govuk/form_builder/traits/attachment.rb index 5a7a107..9919fc2 100644 --- a/app/helpers/katalyst/govuk/form_builder/traits/attachment.rb +++ b/app/helpers/katalyst/govuk/form_builder/traits/attachment.rb @@ -152,7 +152,7 @@ def attachment_preview_for(blob) return if url.nil? # Setting alt to "" as the details already describe the attachment, equivalent to role="presentation" - @builder.image_tag(url, alt: "") + @builder.image_tag(url, alt: "", class: "preview") end # The caption is a polite atomic live region: JS writes upload status @@ -161,7 +161,9 @@ def attachment_preview_for(blob) # @param [ActiveStorage::Blob] blob # @return [ActiveSupport::SafeBuffer,nil] def attachment_caption_for(blob) - tag.figcaption(id: attachment_id_for(blob, :caption), aria: { atomic: true, live: "polite" }) do + tag.figcaption(id: attachment_id_for(blob, :caption), + class: "caption", + aria: { atomic: true, live: "polite" }) do safe_join([ tag.span(blob.filename, class: "filename"), " ", diff --git a/app/javascript/katalyst/govuk/controllers/attachment_controller.js b/app/javascript/katalyst/govuk/controllers/attachment_controller.js index 4bca419..c976513 100644 --- a/app/javascript/katalyst/govuk/controllers/attachment_controller.js +++ b/app/javascript/katalyst/govuk/controllers/attachment_controller.js @@ -209,8 +209,8 @@ export function createAttachment(input, file, i18n) { template.innerHTML = `
- -
+ +
From b5d995c9031692efd4f4a75fc080d9061c03d813 Mon Sep 17 00:00:00 2001 From: Stephen Nelson Date: Tue, 28 Jul 2026 23:59:19 +0930 Subject: [PATCH 15/22] Attachments: aria focus and events for retry/remove --- .../controllers/attachment_controller.js | 10 +++++---- spec/system/attachment/remove_spec.rb | 21 +++++++++---------- spec/system/attachment/upload_spec.rb | 21 +++++++++++++++++++ 3 files changed, 37 insertions(+), 15 deletions(-) diff --git a/app/javascript/katalyst/govuk/controllers/attachment_controller.js b/app/javascript/katalyst/govuk/controllers/attachment_controller.js index c976513..4e25fdd 100644 --- a/app/javascript/katalyst/govuk/controllers/attachment_controller.js +++ b/app/javascript/katalyst/govuk/controllers/attachment_controller.js @@ -105,6 +105,7 @@ export default class AttachmentController extends Controller { retry() { this.performUpload(this.directUpload.file); + this.removeButton?.focus(); } progress = ({ detail }) => { @@ -113,10 +114,7 @@ export default class AttachmentController extends Controller { }; destroy() { - const focusTarget = - this.element.nextElementSibling?.querySelector("button") ?? - this.element.previousElementSibling?.querySelector("button") ?? - this.uploadButton; + const focusTarget = this.uploadButton; const remove = new CustomEvent("govuk:remove", { detail: { @@ -199,6 +197,10 @@ export default class AttachmentController extends Controller { get retryButton() { return this.element.querySelector(".actions button.retry"); } + + get removeButton() { + return this.element.querySelector(".actions button[data-action*='destroy']"); + } } let nextAttachmentId = 0; diff --git a/spec/system/attachment/remove_spec.rb b/spec/system/attachment/remove_spec.rb index 34f422b..4ac3b0f 100644 --- a/spec/system/attachment/remove_spec.rb +++ b/spec/system/attachment/remove_spec.rb @@ -11,8 +11,8 @@ # * removing a has_one's figure still submits the detach — removing with # JavaScript equals choosing the remove option without it, so a required # attachment surfaces its presence error instead of silently surviving -# * focus moves to the next figure's control, else the previous one's, -# else the field's upload button — never lost to +# * focus moves to the field's upload button, whose status summarises +# what remains — never lost to # * removal is announced through the field's announcements region, naming # the file — the figure's own live region disappears with the figure RSpec.describe "Removing an attachment", :aggregate_failures do @@ -95,22 +95,21 @@ expect(profile.reload.cv).not_to be_attached end - it "moves focus to the next figure's control, then falls back to the file input" do + it "moves focus to the field's upload button" do visit edit_profile_path(profile) + on_upload_button = %(document.activeElement.matches("button#profile-gallery-field")) click_button "Remove first.png" - # The next figure's remove control takes focus... - focused = "document.activeElement.getAttribute('aria-label') || document.activeElement.textContent" - expect(page.evaluate_script(focused)).to include("second.png") + # Mid-gallery and last removal alike: the upload button's status is the + # summary of what remains, and its focus reading carries the updated + # count — a sibling figure's reading would not. Never , and not + # some other field's input. + expect(page.evaluate_script(on_upload_button)).to be(true) click_button "Remove second.png" - # ...and with no figures left, this field's file upload button does — never , - # and not some other field's input. - expect( - page.evaluate_script(%(document.activeElement.matches("button#profile-gallery-field"))), - ).to be(true) + expect(page.evaluate_script(on_upload_button)).to be(true) end it "announces the removal, naming the file" do diff --git a/spec/system/attachment/upload_spec.rb b/spec/system/attachment/upload_spec.rb index 0291a64..9d47522 100644 --- a/spec/system/attachment/upload_spec.rb +++ b/spec/system/attachment/upload_spec.rb @@ -129,6 +129,27 @@ expect(ActiveStorage::Blob.find_signed(signed_id)&.filename&.to_s).to eq("avatar.png") end + it "moves focus to the figure's remove button when retry starts" do + block_direct_uploads + visit edit_profile_path(profile) + + choose_gallery_file("avatar.png") + release_direct_uploads(:internal_server_error) + + gallery_field.find("figure.govuk-attachment[data-state=upload-failed]", wait: 10) + + click_button "Try again" + + # Retrying removes the retry control from under the user; focus moves to + # the figure's remove button — the figure's only control while uploading + # — rather than dropping to . + expect( + page.evaluate_script("document.activeElement.getAttribute('aria-label')"), + ).to eq("Remove avatar.png") + + release_direct_uploads(:ok) + end + it "offers a single retry control however many attempts fail" do block_direct_uploads visit edit_profile_path(profile) From 69b61ca169804b23a0fefdceff0459b486809bab Mon Sep 17 00:00:00 2001 From: Stephen Nelson Date: Wed, 29 Jul 2026 00:40:28 +0930 Subject: [PATCH 16/22] Attachments: use filename instead of caption as aria-label Reduces the noise when listening to a form. Consistent with GOVUK. --- .../govuk/form_builder/traits/attachment.rb | 10 ++++----- .../controllers/attachment_controller.js | 21 +++++++++++++------ .../form_builder_attachment_field_spec.rb | 10 +++++++-- spec/system/attachment/upload_spec.rb | 14 +++++++++++++ 4 files changed, 41 insertions(+), 14 deletions(-) diff --git a/app/helpers/katalyst/govuk/form_builder/traits/attachment.rb b/app/helpers/katalyst/govuk/form_builder/traits/attachment.rb index 9919fc2..d84df67 100644 --- a/app/helpers/katalyst/govuk/form_builder/traits/attachment.rb +++ b/app/helpers/katalyst/govuk/form_builder/traits/attachment.rb @@ -57,7 +57,7 @@ def attachment # @return [ActiveSupport::SafeBuffer,nil] def attachment_for(blob) tag.figure(class: "#{brand}-attachment", - aria: { labelledby: attachment_id_for(blob, :caption) }, + aria: { labelledby: attachment_id_for(blob, :filename) }, data: { controller: "govuk-attachment" }) do safe_join([ attachment_preview_for(blob), @@ -92,7 +92,7 @@ def attachment_input_for(blob) { selected: blob.signed_id }, id: attachment_id_for(blob, :input), name: @builder.field_name(@attribute_name, multiple: many?), - aria: { labelledby: attachment_id_for(blob, :caption) }, + aria: { labelledby: attachment_id_for(blob, :filename) }, ) end @@ -161,11 +161,9 @@ def attachment_preview_for(blob) # @param [ActiveStorage::Blob] blob # @return [ActiveSupport::SafeBuffer,nil] def attachment_caption_for(blob) - tag.figcaption(id: attachment_id_for(blob, :caption), - class: "caption", - aria: { atomic: true, live: "polite" }) do + tag.figcaption(class: "caption", aria: { atomic: true, live: "polite" }) do safe_join([ - tag.span(blob.filename, class: "filename"), + tag.span(blob.filename, id: attachment_id_for(blob, :filename), class: "filename"), " ", tag.span(number_to_human_size(blob.byte_size), class: "size"), " ", diff --git a/app/javascript/katalyst/govuk/controllers/attachment_controller.js b/app/javascript/katalyst/govuk/controllers/attachment_controller.js index 4e25fdd..09984d4 100644 --- a/app/javascript/katalyst/govuk/controllers/attachment_controller.js +++ b/app/javascript/katalyst/govuk/controllers/attachment_controller.js @@ -73,7 +73,7 @@ export default class AttachmentController extends Controller { this.element.dataset.state = "uploading"; this.statusText = ""; this.retryButton?.remove(); - const progressTag = createProgressTag(this.captionTag?.id); + const progressTag = createProgressTag(this.filenameTag.id); this.captionTag.appendChild(progressTag); this.input.addEventListener("direct-upload:progress", this.progress); @@ -165,6 +165,13 @@ export default class AttachmentController extends Controller { return this.element.querySelector("figcaption"); } + /** + * @returns {HTMLElement} the caption's filename span, or null + */ + get filenameTag() { + return this.element.querySelector("figcaption .filename"); + } + /** * @returns {HTMLElement} the figure's actions container, or null */ @@ -199,7 +206,9 @@ export default class AttachmentController extends Controller { } get removeButton() { - return this.element.querySelector(".actions button[data-action*='destroy']"); + return this.element.querySelector( + ".actions button[data-action*='destroy']", + ); } } @@ -210,15 +219,15 @@ export function createAttachment(input, file, i18n) { const id = ++nextAttachmentId; template.innerHTML = ` -
+
-
- +
+
- diff --git a/spec/builders/govuk_design_system_form_builder/form_builder_attachment_field_spec.rb b/spec/builders/govuk_design_system_form_builder/form_builder_attachment_field_spec.rb index 3f5740f..718a53f 100644 --- a/spec/builders/govuk_design_system_form_builder/form_builder_attachment_field_spec.rb +++ b/spec/builders/govuk_design_system_form_builder/form_builder_attachment_field_spec.rb @@ -336,10 +336,16 @@ def wrapper(html) expect(html.find("figure.govuk-attachment img")["alt"]).to eq("") end - it "labels the figure with its caption" do + it "labels the figure with its filename" do figure = html.find("figure.govuk-attachment") - expect(figure["aria-labelledby"]).to eq(builder.field_id(:avatar, :attachment, blob.id, :caption)) + expect(figure["aria-labelledby"]).to eq(builder.field_id(:avatar, :attachment, blob.id, :filename)) + end + + it "gives the filename span the id the figure's label references" do + filename_id = builder.field_id(:avatar, :attachment, blob.id, :filename) + + expect(html).to have_css("figure.govuk-attachment figcaption .filename[id='#{filename_id}']") end it "makes the caption a polite live region so status changes are announced" do diff --git a/spec/system/attachment/upload_spec.rb b/spec/system/attachment/upload_spec.rb index 9d47522..1ab2770 100644 --- a/spec/system/attachment/upload_spec.rb +++ b/spec/system/attachment/upload_spec.rb @@ -44,6 +44,20 @@ expect(gallery_field).to have_css("figure.govuk-attachment figcaption .status", text: /uploaded/i) end + it "names the progress bar after the file it reports" do + block_direct_uploads + visit edit_profile_path(profile) + + choose_gallery_file("avatar.png") + + progress = gallery_field.find("figure.govuk-attachment progress") + referenced = progress["aria-labelledby"].to_s.split.map { |id| page.find(id: id, visible: :all).text } + + expect(referenced.join(" ")).to eq("avatar.png") + + release_direct_uploads + end + it "reaches upload-successful with the blob's signed id in the select" do visit edit_profile_path(profile) From b590c6530aed7745403aee5f662c3e4d6078bd9c Mon Sep 17 00:00:00 2001 From: Stephen Nelson Date: Fri, 31 Jul 2026 22:05:54 +0930 Subject: [PATCH 17/22] Attachments: follow GOVUK convention for buttons --- .../katalyst/govuk/form_builder/builder.rb | 2 +- .../govuk/form_builder/traits/attachment.rb | 7 +++--- app/javascript/katalyst/govuk/config.js | 2 +- .../controllers/attachment_controller.js | 7 +++--- config/locales/en.yml | 2 +- .../form_builder_attachment_field_spec.rb | 25 +++++++++++++++++++ spec/system/attachment/upload_spec.rb | 4 +++ 7 files changed, 40 insertions(+), 9 deletions(-) diff --git a/app/helpers/katalyst/govuk/form_builder/builder.rb b/app/helpers/katalyst/govuk/form_builder/builder.rb index 30afa55..6f8ea9e 100644 --- a/app/helpers/katalyst/govuk/form_builder/builder.rb +++ b/app/helpers/katalyst/govuk/form_builder/builder.rb @@ -309,7 +309,7 @@ def govuk_combobox(attribute_name, options_or_src = [], options: {}, label: {}, # no-JavaScript remove option. The component will replace the %{filename} placeholder with the figure's # file name. Default is "Remove %{filename}". # @param remove_button_content_text [String] The visible content of each figure's remove button. Default is - # "×". + # "Remove". # @param & [Block] arbitrary HTML that will be rendered between the hint and the input # # @example A photo upload field with file type specifier and injected content diff --git a/app/helpers/katalyst/govuk/form_builder/traits/attachment.rb b/app/helpers/katalyst/govuk/form_builder/traits/attachment.rb index d84df67..8b423e5 100644 --- a/app/helpers/katalyst/govuk/form_builder/traits/attachment.rb +++ b/app/helpers/katalyst/govuk/form_builder/traits/attachment.rb @@ -101,9 +101,10 @@ def attachment_input_for(blob) # @return [ActiveSupport::SafeBuffer,nil] def attachment_remove_for(blob) tag.button(remove_button_content, - type: "button", - aria: { label: remove_button_label(blob) }, - data: { action: "govuk-attachment#destroy" }) + type: "button", + class: "#{brand}-button #{brand}-button--secondary #{brand}-attachment__remove", + aria: { label: remove_button_label(blob) }, + data: { action: "govuk-attachment#destroy", module: "govuk-button" }) end # The remove strings render here and in the JS figure template, so diff --git a/app/javascript/katalyst/govuk/config.js b/app/javascript/katalyst/govuk/config.js index 4dfd92d..a9f8368 100644 --- a/app/javascript/katalyst/govuk/config.js +++ b/app/javascript/katalyst/govuk/config.js @@ -26,7 +26,7 @@ const Attachment = { retryButton: "Try again", fileRemoved: "%{filename} removed", removeButton: "Remove %{filename}", - removeButtonContent: "×", + removeButtonContent: "Remove", }, }, schema: { properties: { i18n: { type: "object" } } }, diff --git a/app/javascript/katalyst/govuk/controllers/attachment_controller.js b/app/javascript/katalyst/govuk/controllers/attachment_controller.js index 09984d4..794568c 100644 --- a/app/javascript/katalyst/govuk/controllers/attachment_controller.js +++ b/app/javascript/katalyst/govuk/controllers/attachment_controller.js @@ -202,7 +202,7 @@ export default class AttachmentController extends Controller { } get retryButton() { - return this.element.querySelector(".actions button.retry"); + return this.element.querySelector(".actions button[data-action*='retry']"); } get removeButton() { @@ -231,7 +231,7 @@ export function createAttachment(input, file, i18n) { - +
`; @@ -275,10 +275,11 @@ function humanSize(bytes) { function createRetryButton(filename, i18n) { const button = document.createElement("BUTTON"); button.type = "button"; - button.className = "retry"; + button.className = `${config.brand}-button ${config.brand}-button--secondary ${config.brand}-attachment__retry`; button.textContent = i18n.t("retryButton"); button.setAttribute("aria-label", `${i18n.t("retryButton")} ${filename}`); button.dataset.action = "govuk-attachment#retry"; + button.dataset.module = "govuk-button"; return button; } diff --git a/config/locales/en.yml b/config/locales/en.yml index da0d1b1..200d1c5 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -13,4 +13,4 @@ en: retry_button: "Try again" file_removed: "%{filename} removed" remove_button: "Remove %{filename}" - remove_button_content: "×" + remove_button_content: "Remove" diff --git a/spec/builders/govuk_design_system_form_builder/form_builder_attachment_field_spec.rb b/spec/builders/govuk_design_system_form_builder/form_builder_attachment_field_spec.rb index 718a53f..c2187f2 100644 --- a/spec/builders/govuk_design_system_form_builder/form_builder_attachment_field_spec.rb +++ b/spec/builders/govuk_design_system_form_builder/form_builder_attachment_field_spec.rb @@ -332,6 +332,24 @@ def wrapper(html) expect(button["type"]).to eq("button") end + it "styles the remove button as a secondary govuk button" do + button = html.find("figure.govuk-attachment .actions button") + + expect(button[:class].split).to include("govuk-button", "govuk-button--secondary", "govuk-attachment__remove") + end + + it "connects the remove button to govuk-frontend's button behaviour" do + button = html.find("figure.govuk-attachment .actions button") + + expect(button["data-module"]).to eq("govuk-button") + end + + it "renders Remove as the default remove button content" do + button = html.find("figure.govuk-attachment .actions button") + + expect(button.text).to eq("Remove") + end + it "hides the preview image from assistive technology" do expect(html.find("figure.govuk-attachment img")["alt"]).to eq("") end @@ -389,6 +407,13 @@ def wrapper(html) expect(html).to have_css(".defra-file-upload-wrapper", visible: :all) end + it "prefixes the remove button classes with the brand" do + expect(html).to have_css( + ".defra-attachment .actions button.defra-button.defra-button--secondary.defra-attachment__remove", + visible: :all, + ) + end + it "connects the figure to the attachment controller" do expect(html).to have_css("figure.defra-attachment[data-controller='govuk-attachment']", visible: :all) end diff --git a/spec/system/attachment/upload_spec.rb b/spec/system/attachment/upload_spec.rb index 1ab2770..849c76b 100644 --- a/spec/system/attachment/upload_spec.rb +++ b/spec/system/attachment/upload_spec.rb @@ -109,6 +109,10 @@ expect(figure).to have_css("button[aria-label*='Remove']") expect(figure).to have_css("button[type=button][aria-label='Try again avatar.png']", text: "Try again") + # Both figure controls follow govuk-frontend's button conventions (the + # password toggle's markup shape). + expect(figure).to have_css("button.govuk-button.govuk-button--secondary.govuk-attachment__retry") + click_button "Continue" # Submitting without clearing the error does not save the file From 86caf7dadd03ebffe03e9c076b3c4c8d1d3350db Mon Sep 17 00:00:00 2001 From: Stephen Nelson Date: Wed, 22 Jul 2026 12:49:35 +0930 Subject: [PATCH 18/22] Attachments: development spec and future work --- .gitignore | 1 - doc/attachment-field-spec.md | 636 +++++++++++++++++++++++ doc/pitches/components-as-controllers.md | 34 ++ doc/pitches/filename-rename.md | 65 +++ 4 files changed, 735 insertions(+), 1 deletion(-) create mode 100644 doc/attachment-field-spec.md create mode 100644 doc/pitches/components-as-controllers.md create mode 100644 doc/pitches/filename-rename.md diff --git a/.gitignore b/.gitignore index 9ef0c46..1006d2c 100644 --- a/.gitignore +++ b/.gitignore @@ -2,7 +2,6 @@ /.yardoc /_yardoc/ /coverage/ -/doc/ /pkg/ /node_modules/ /spec/reports/ diff --git a/doc/attachment-field-spec.md b/doc/attachment-field-spec.md new file mode 100644 index 0000000..e4d5c1d --- /dev/null +++ b/doc/attachment-field-spec.md @@ -0,0 +1,636 @@ +# Attachment field — specification + +`govuk_attachment_field` (and its `govuk_image_field` / `govuk_document_field` +wrappers) renders an ActiveStorage-backed file upload with previews, async +direct upload, and a full no-JavaScript fallback. + +## Design + +1. **The `` with a *keep* option (value = blob signed id, label = + filename, selected) and a *remove* option (blank value). The form always + round-trips attachments as signed ids — never re-uploaded bytes — and the + same control makes attachments editable without JavaScript: there is no + `_destroy` mechanism, and no hidden input ever carries a signed id (the + only hidden input is §5's blank keeper). +2. **We own the drop zone.** The field replaces govuk-frontend's FileUpload + JS behaviourally (reusing its markup conventions and i18n strings) rather + than running it: that component derives all of its state — status text, + empty styling, drop capacity — from the input's FileList, which this + field clears after dispatching uploads, and it initialises once with no + teardown, which doesn't compose with the Stimulus/Turbo lifecycle. + Parity with `file-upload.mjs` is pinned by the drop-zone parity system + spec, which runs govuk-frontend's own enhancement as the live reference + and diffs the two drop zones in canonical form (intended differences + scrubbed explicitly); purely behavioural differences remain a review + concern on upstream bumps. Accepted differences: plain errors rather + than `ElementError`; count strings even for one file (C10 records why); + our announcements region renders inside the wrapper — scoped finds and + morph re-enhancement keep it — where govuk-frontend's sits after the + drop zone. + govuk-frontend's own component stays supported for plain file fields + (`initAll` initialises `data-module="govuk-file-upload"`), so the + **exclusivity invariant** is critical: every file input is enhanced by + exactly one implementation — attachment fields render the Stimulus + `data-controller` and must never emit `data-module="govuk-file-upload"`; + plain govuk file fields the reverse. +3. **Uploads go through ActiveStorage's `DirectUploadController`** (public + export of `activestorage.esm.js`), subclassed so success writes the signed + id into the figure's select option. This inherits the + `direct-upload:start/progress/end` event lifecycle and error dispatch, + which drive the figure's progress display and error states. + `data-direct-upload-url` lives on the file input, where the controller + reads it. +4. **Enhancement fails open.** The server-rendered form is complete and + functional on its own; JavaScript only takes ownership of a selection + when a figure's controller *claims* it. Selection renders one figure per + file, each carrying its `File` object; when the figure's controller + connects — and only when it can deliver an upload + (`data-direct-upload-url` present on the input) — it claims the file: + it announces `govuk:upload` on the input and starts the upload, and the + file-upload controller releases the claimed file (matched by object + identity — names can collide) from the FileList. Claiming is what + prevents ActiveStorage's submit-time auto-upload from sending the same + bytes again. Unclaimed files stay in the input and submit as ordinary + multipart, so the fallback holds by construction: a controller that is + absent, broken, or never connects claims nothing. Editing the FileList + programmatically (`DataTransfer` reassignment) fires no `change` event, + so releasing a file cannot re-trigger selection handling. +5. **Param shape.** `name[]` for `has_many_attached` (one entry per select), + scalar `name` for `has_one_attached` — in both cases led by a blank + keeper the field renders (hidden input, same name, blank value, no id), + suppressing Rails' auto-blank for `file_field multiple: true` + (`include_hidden: false`) so one input the field owns plays that role + everywhere: the attribute always submits, even when a + removed figure has taken its select away. Scalar last-wins means any + select or multipart part overrides the keeper. Assignment is + replace-on-assign: the + submitted set *is* the resulting set, a lone blank clears, and one array + freely mixes blanks, signed ids, and multipart uploads. +6. **The field makes pending blobs renderable.** A failed save re-renders + with attachment changes still pending, so generating the field persists + any unpersisted blob (record + bytes): its figure renders and its signed + id round-trips exactly like a direct upload — a failed submit never + loses an upload. Persistence is idempotent (already-persisted blobs are + never re-persisted, so controller-level persistence composes and leaves + the field nothing to do), and rendering never touches a blob's bytes: + preview URLs are lazy, so the variant is processed when the browser + requests the image, and a blob whose bytes are missing or unprocessable + (out-of-band purge, storage loss, mirror lag, bad content) costs a + broken image in that figure — never a failed render, keep option intact. + Validating attachment content is the model's responsibility, not the + form's. + Persisted-unattached blobs are the same GC category direct upload + already creates; purging them stays the consumer's existing + responsibility. +7. **JS-injected UI is JS-only.** The pseudo button, its status region, and + the announcements region are injected by the controller and never + server-rendered: without JavaScript the browser-native file input is the + whole experience. A Turbo morph refresh therefore strips the injected UI + and any other JS-set state (including the page's support marker), and it + may either recreate the drop zone (lifecycle events fire) or patch it in + place (no events at all) — so recovery is driven by observing the DOM, + not framework events: losing a marker or the injected UI *is* the morph + signal. After any morph, both levels recover: the support marker is + restored (before paint) and the field re-enhances — injected UI rebuilt, + transient client state discarded (FileList cleared, unclaimed previews + dropped), count matching the server-rendered figures (C11). + +Form group structure (top to bottom): + +``` +form group (data-controller for the drop zone) + label + hint + error message(s) + before_input content + attachment figure(s), one per blob (0..1 for has_one, 0..n for has_many) + pseudo upload button + drop region ("Choose files" / drop instruction) + native file input (hidden when JS is active; data-direct-upload-url) + after_input content + supplemental content (block content, e.g. alt text / caption fields) +``` + +- Without JS the native file input is a plain visible input; with JS it is + hidden and fronted by the pseudo button, which triggers the browser file + picker. The drop target is the pseudo button/drop region, not the whole + form group: a valid drag over it adds `--dragging` to the button and is + announced through a JS-injected visually-hidden assertive region + (`.govuk-file-upload-announcements`), and a drop fills the input exactly + as choosing files does. A drop is accepted only within the input's + capacity (one file unless `multiple`). The announcements region carries + every *event* announcement — drag enter/leave and removals — while the + button's polite status region carries only *state* (the file count). The + button is wired to the field's existing accessibility affordances: it takes + the input's original id (so the label's `for` labels it), the label is given + a matching id for the button's `aria-labelledby`, the input's + `aria-describedby` (hint/error) is copied onto it, and its `disabled` state + is mirrored from the input (F4, F5, C12). +- Each attachment figure: preview (`img`, when representable), `figcaption` + (filename, human size, and an empty status span), then an actions + container (`div.actions`) holding the keep/remove `` per blob, never `select multiple:` (wrong UX and + wrong param semantics). +- Attachment traits must branch on `Attached::One` vs `Attached::Many` + (`value.blob` does not exist on `Many`). +- Resolve focus targets *before* `element.remove()` — `closest()` on a + detached node returns null. +- Live-region writes follow the interaction's last focus move. A focus + change discards pending not-yet-spoken live-region writes; a write + co-arriving in the same task survives only as VoiceOver batching, not a + contract screen readers share. So order focus-first, write-after (as + removal does), or write from handlers that only run after focus settles + (change, drag enter/leave — the only places govuk-frontend's FileUpload + writes). Announcement-before-focus would need focus held back for the + announcement's speech duration — seconds of keystrokes landing on + `` — rejected. +- Accessible names for repeated per-file controls include the filename; + prefer visible or visually-hidden text / `aria-labelledby` to the + filename span over duplicated `aria-label` strings. Thumbnails get `alt=""`; decorative + icons get `aria-hidden="true"` (never a nameless `role="img"`). +- Filenames are user-controlled: assign via `textContent`/attributes, never + interpolate into `innerHTML`. +- The selection hand-off event is cancelable; un-cancelled means unhandled — + leave the FileList intact so the files submit as multipart (Design §4). +- Preview URLs are lazy — render `blob.representation(...)` without + `.processed`, resolved through `attachment_preview_url` (main_app + fallback; nil renders the figure without a preview), so form render never + downloads, transforms, or routes bytes eagerly. + Render-time processing turned missing bytes or a missing image library + into a failed render, and serialized variant generation for every figure + into the request. Missing bytes cost a broken image at request time + (Design §6). (Mid-upload submits never reach the render: assignment + identifies the blob by downloading a chunk, and our JS only writes a + signed id into the select after the byte upload completes.) +- Persist pending blobs upload-first: a blob whose byte upload fails is + simply never saved, and the render's `persisted?` filter drops it — no + purge-on-error step (it would run exactly when the service is failing). + Stranded partial writes are prevented at the service layer (Disk deletes + its partial write before raising `IntegrityError`; S3 PUTs are atomic). + Log dropped uploads — ActiveStorage's own instrumentation logs success + messages even for failed uploads, so nothing else records the drop. +- `app/javascript` is the source; `app/assets/builds` is what browsers run. + Rebuild before trusting a system test. +- Re-enhancement is driven by MutationObservers, never Turbo events: + `turbo:load` doesn't fire on morph renders, `turbo:render` listeners are + registration-order dependent, and a recreated element receives lifecycle + events instead of morph events. Observers attach to the body/drop-zone + *element*, so a replace render disposes them with the node it replaces. + Two enhancement entry points, one registration: `start(application, + config)` — the primary application.js wiring, and the default export — + registers the gem's controllers on the given Stimulus application (a + gem-owned application when omitted; first registration wins) and keeps + enhancement session-durable by watching `documentElement`, surviving + body replacement. `initAll(config)` — rendered per body by + `govuk_formbuilder_init` — is body-scoped with no self-rearming, and + registers the controllers on the gem-owned application for apps not + running Stimulus. The two compose deterministically: head modules run + before the body-end snippet, and registration is first-wins. Legacy + `application.load(govuk)` throws at boot — loud, never a silent + double-registration. (The gem-owned application branch is verified + manually: under first-wins, a host application that registers first — + as the dummy's does — always claims registration, so no runtime test + reaches it.) +- The string vocabulary lives twice by design: `config/locales/en.yml` is + canonical (Rails resolves the strings and renders values onto the + wrapper's `data-i18n.*` attributes whenever they differ from the gem's + bundled defaults), and the JS bundle mirrors the defaults for enhancement + without attributes — keep the two tables in step. The difference check + reads the bundled table straight from the gem's own locale file, never + through I18n resolution: resolution absorbs a host app's overrides, and + an override in any locale — en included — must reach the attributes or + server-rendered and JS-created figures diverge. + Inherited keys keep govuk-frontend's names: the `data-i18n.*` + attribute shapes are the compatibility bar. Nothing diffs the two + default tables — a drifted default only shows on a field rendered with + default strings and no attributes — so review both on any vocabulary + change. +- Brand follows CSS classes only; behavioural wiring is fixed. Stimulus + identifiers, `data-controller`/`data-action` values, and events are + always `govuk-*`, while the classes Ruby renders and JS injects — and the + class-based selectors JS queries — follow the configured brand. Brand + crosses to JS via the entry points' config (`start(application, + { brand })` / `initAll({ brand })`; the snippet omits the argument at + the default brand) and defaults to `govuk`. The gem's compiled CSS stays + govuk-prefixed: a consumer exercising brand brings their own frontend + CSS (upstream govuk-frontend JS likewise only matches fixed module + names, so brand ≠ govuk already presumes a forked frontend). +- Enhancement sweeps overlap by design (marker-restore and arrival sweeps + visit the same roots in one morph batch) — check `isInitialised` before + constructing a component; construct-and-catch logs an error per component + per morph. Re-constructing on morph-retained nodes can duplicate + listeners; govuk components mostly bind injected elements, so this is + tolerable — re-check whenever a component joins the sweep. The arrival + observer runs a full sweep per added element: cheap at the current + component count, worth revisiting if the list grows. +- Interactive controls never live inside a live region: the caption is + atomic, so a control there is re-read with every status update and can + lose focus when the region re-renders. The retry control sits in + `div.actions`; the failure status text ("Upload failed — try again") is + what tells a screen-reader user the affordance exists. +- Stimulus connects controllers asynchronously after DOM insertion — events + dispatched at just-inserted figures must account for this (no bare + synchronous dispatch). + +## Non-goals + +- **Client-side upload errors never register in the GOV.UK error summary.** + The summary is the server-side validation channel (E2); upload failures are + widget-local by design (C3). This is explicitly undesirable, not deferred. +- **Filename normalisation.** The caption names the file exactly as stored + — the same name downloads carry — so percent-encoded or otherwise noisy + stored filenames render (and announce) verbatim. A `%20` in a stored + name is indistinguishable from a legitimate literal, and browsers and + direct upload never produce encoded names — they enter via consumer + ingestion (e.g. URL-derived filenames attached from `io:`). Fix at + ingestion or by data migration in the consuming app; the field renders + the truth. + +## Rabbit holes + +Known traps we are deliberately not entering; each entry names the supported +alternative. Where the alternative is an event we already emit, we do not +build on top of it. + +- **Inferring `optional` from model validations.** Deriving the field's + optionality (e.g. GOV.UK's "(optional)" label convention, remove + affordances) from `validators_on(:attr)` looks like the `multiple` + inference — view behaviour from model metadata — but validation reflection + is unreliable where reflection isn't: presence validators can be + conditional (`if:`/`unless:`, `on:` contexts) or live in attachment + validation gems the reflection can't see, and a wrongly-inferred + "(optional)" label is a content failure, not a cosmetic one. Revisit when + `optional` drives real behaviour again; until then the supported path is + an explicit argument from the caller, who knows the form's context. +- **Client-side file validation (mime type, size, dimensions).** The + `accept` attribute filters the picker as a courtesy and no more: drag/drop + bypasses it, browsers derive a file's type from its extension, and only + the server can verify content. Client-side enforcement with error + messaging would be built on a guess and wrongly reject valid files — the + server's validation is the authority (E2), and a round-trip that re-renders + every kept file (E1) is the correction loop. Consumers who want their own + checks can cancel the selection hand-off event (Design §4) and leave the + file unclaimed. +- **Submit protection for in-flight uploads.** Submitting mid-upload is + allowed: the file simply isn't stored, and if that matters the server's + validation reports it (E2) — the visible `uploading` state and status + announcements give the user enough feedback to understand what happened. + This includes a has_one replacement, where the incomplete figure's blank + supersedes the stored attachment (C5): required fields are protected by + their presence validation; optional fields detach. + Blocking or deferring submission drags in disabled-submit state management, + re-enable-on-error paths, and double-submit interactions; consumers whose + use case demands it can build it from the `direct-upload:start/end` events + on the file input. +- **Non-ActiveStorage attachment values.** Rendering the field for e.g. an + `attr_accessor`-backed form object looks like loose coupling, but + everything load-bearing is built from blob signed ids: direct upload + writes one into the select, error round-trips re-render from them (E1), + and figures need filename, size, content type, and a preview URL. A + non-AS value gets none of that — direct upload would assign signed-id + strings only meaningful if the form object consumes them, the keeper + would assign `""` when nothing is chosen, and a *present* value has no + figure or editing story without a duck-type interface plus a + `polymorphic_url`-style preview mechanism. Possible, but a project of its + own — out of scope. The element raises `ArgumentError` for non-`Attached` + values; plain multipart uploads without previews use the upstream + `govuk_file_field`. +- **Sequential upload queueing.** Figures upload concurrently, each + starting on claim. ActiveStorage's own form-submit flow queues uploads + one at a time — attractive on a slow uplink, where N concurrent uploads + all finish late together while a queue completes the first file at ~1/N + of the total — but queueing drags in queue scope (per field or + page-wide), dequeue-and-abort teardown for removed figures, a queued + lifecycle state touching C1/C8's progress contract, and retry + re-enqueueing. Multiple-file interfaces are rare in practice, and an + early submit keeps completed files under either model (their signed ids + are already in their selects), so concurrency ships; revisit if a real + use case surfaces the slow-uplink problem. +- **Renaming attachments.** `filename` is display metadata on the blob and + the field round-trips signed ids only, so no rename affordance exists. + In-place `blob.update(filename:)` mutates persisted, possibly shared + data outside the form's submit-time-truth model (and public-mode + services bake Content-Disposition into the object at upload, so it lies + there anyway). The viable shape — a copy-on-write rename that the + existing swap semantics absorb — is pitched in + `doc/pitches/filename-rename.md`; until pursued, renaming is out of + scope and rename-before-upload belongs to the client, where the name + rides the blob-creation request. +- **Wrapping govuk-frontend components in Stimulus controllers.** Wrapping + looks like it would unify enhancement under Stimulus's observer, but + Stimulus never re-connects a morph-retained element and govuk components + have no teardown to re-enter through — morph recovery gets worse, not + better — while consumer-authored `data-module` markup silently stops + enhancing, diverging from govuk-frontend's documented conventions. The + supported path is the current `data-module` sweep with our own + observers; the full analysis is pitched in + `doc/pitches/components-as-controllers.md`. +- **Reordering of multiple attachments.** `has_many_attached` order follows + the submitted param order, which invites drag-to-reorder UI, persistence + semantics, and a11y for reordering — none of it here. Figures render in + attachment order; that is the whole contract. diff --git a/doc/pitches/components-as-controllers.md b/doc/pitches/components-as-controllers.md new file mode 100644 index 0000000..b2d6ca3 --- /dev/null +++ b/doc/pitches/components-as-controllers.md @@ -0,0 +1,34 @@ +# Pitch: govuk-frontend components as Stimulus controllers + +Replace `data-module` sweeping entirely: the server emits +`data-controller`, each govuk-frontend component wraps in a thin +controller (`connect()` constructs it, config from the captured option bag +or govuk's own data-attribute config), and Stimulus's observer owns +arrival — including runtime attribute writes, which would gain a public +contract `data-module` never had. Registration unifies under +`application.load(govuk)` and composes with the gem-owned Stimulus +application pitch. Not scheduled. + +## Why the core is small + +A base class plus seven registrations. + +## Why the edges are the real cost + +- **Identifier collision**: the attachment drop zone already owns the + `govuk-file-upload` identifier — upstream's FileUpload needs a rename, + and the exclusivity invariant re-encodes. +- **Morphs get worse without work**: Stimulus never re-connects a + morph-retained element, so stripped injected UI stays broken unless the + base class self-heals à la `file_upload_controller`'s morphObserver. + govuk components have no teardown for `disconnect()` to call — the + original objection to running upstream JS, unresolved by wrapping. +- **Listener leaks become explicit**: FileUpload's document-level drag + listeners leak on every re-construction in both designs, but a wrapper + makes the gap visible, and fixing it means forking components. +- **Convention divergence**: consumer-authored `data-module` markup + silently stops enhancing — a departure from govuk-frontend's documented + conventions carried into every upstream bump review. +- `initAll` shrinks but survives (markSupport + marker restore — Stimulus + knows nothing of the body markers). Parity spec, guide pages, + enhancement/morph specs, and the drop-zone design section all re-encode. diff --git a/doc/pitches/filename-rename.md b/doc/pitches/filename-rename.md new file mode 100644 index 0000000..6f027d1 --- /dev/null +++ b/doc/pitches/filename-rename.md @@ -0,0 +1,65 @@ +# Pitch: attachment filename rename (copy-on-write) + +Let a user rename an attachment from the field — fixing upload-time noise +(camera names, percent-encoded filenames) without re-uploading the file. +Not scheduled; the groundwork below is verified against activestorage +8.1.3. + +## Verified groundwork + +- `filename` is pure blob metadata — the stored object lives at `blob.key` + (random secure token), so a rename never moves bytes and never + invalidates `signed_id` (encodes the blob id only) or variants (keyed by + `key`). Download names resolve at request time on the Disk and + private-S3 paths; **public-mode services bake Content-Disposition into + the object at upload**, so a plain rename leaves stale download names + there. +- No client-writable rename surface exists: ActiveStorage's only writable + routes are `POST /direct_uploads` (create) and the Disk byte `PUT`. Any + JS rename needs an endpoint of ours or the consumer's. +- In-place `blob.update(filename:)` was set aside as an option shape: it + mutates persisted (possibly shared — filename is per-blob, not + per-attachment) data outside the form's submit-time-truth model, on the + strength of a signed id that is not a secret in practice (every rendered + select carries one). + +## Proposed shape: copy-on-write + +`Blob.compose([blob], filename:)` with a single source is +copy-with-rename — new record, new key, new signed id, bytes copied by the +service (Disk file copy; S3 streams through the app, not a server-side +COPY). `blob.open` + `Blob.create_and_upload!` is the alternative that +re-runs checksum and Marcel identification (compose copies the source +`content_type`, so extension edits want `create_and_upload!`). Never point +a second blob record at an existing key: purging either blob deletes the +shared bytes. + +A renamed copy is indistinguishable from a swap upload, so the field's +existing contract absorbs it: JS writes the new signed id into the keep +option (the same write direct-upload success does), submit is swap +semantics with the old attachment's `purge_later` cleaning the old bytes, +an abandoned form leaves only a persisted-unattached orphan (the GC +category direct upload already creates), and an invalid submit round-trips +the new name. The endpoint is creation-only — worst case is orphan copies, +the same risk class as direct-upload spam — though it still wants +rate/authz consideration. + +## Costs + +A full byte copy per rename (double storage until GC, transits the app +server); variants regenerate lazily for the new key; each re-edit mints +another orphan. + +## The client-side freebie + +Fresh (client-held) uploads need none of this: the name rides the +direct-upload blob-creation request, so rename-before-upload is +client-side only. The same hook could normalise percent-encoded filenames +at creation — the case the field's VoiceOver review flagged as encoding +noise. + +## Open if pursued + +The edit affordance in the figure (caption, keep option, remove/retry +labels all regenerate), and which states offer rename (likely +`upload-successful` and server-rendered figures only). From 1a467985b0e381f2fe83e1e3ec8479f9d2b480a3 Mon Sep 17 00:00:00 2001 From: Stephen Nelson Date: Sun, 2 Aug 2026 20:56:51 +0930 Subject: [PATCH 19/22] Attachments: accept drops anywhere in the widget --- .../controllers/file_upload_controller.js | 25 +++++---- doc/attachment-field-spec.md | 10 ++-- spec/system/attachment/drag_and_drop_spec.rb | 52 +++++++++++++------ 3 files changed, 59 insertions(+), 28 deletions(-) diff --git a/app/javascript/katalyst/govuk/controllers/file_upload_controller.js b/app/javascript/katalyst/govuk/controllers/file_upload_controller.js index 7fcc9c3..cc2e069 100644 --- a/app/javascript/katalyst/govuk/controllers/file_upload_controller.js +++ b/app/javascript/katalyst/govuk/controllers/file_upload_controller.js @@ -22,6 +22,11 @@ export default class FileUploadController extends Controller { document.addEventListener("dragenter", this.onDragenter); document.addEventListener("dragleave", this.onDragleave); + // The whole wrapper — figures included — is the drop target: a file + // dragged anywhere over the field can be dropped. + this.element.addEventListener("dragover", this.onDragover); + this.element.addEventListener("drop", this.onDrop); + this.enhance(); // A morph reconciles this element against a server response that has no @@ -37,8 +42,8 @@ export default class FileUploadController extends Controller { this.morphObserver?.disconnect(); this.disabledObserver?.disconnect(); this.uploadButton?.removeEventListener("click", this.onClick); - this.uploadButton?.removeEventListener("dragover", this.onDragover); - this.uploadButton?.removeEventListener("drop", this.onDrop); + this.element.removeEventListener("dragover", this.onDragover); + this.element.removeEventListener("drop", this.onDrop); this.unbindInput(); document.removeEventListener("dragenter", this.onDragenter); document.removeEventListener("dragleave", this.onDragleave); @@ -62,11 +67,9 @@ export default class FileUploadController extends Controller { uploadButton = createUploadButton(this.id, this.i18n, fileInput); fileInput.insertAdjacentElement("beforebegin", uploadButton); - // The button is the drop target; its listeners die with a stripped - // button and rebind with its replacement. + // The click listener dies with a stripped button and rebinds with + // its replacement; the drag listeners live on the wrapper. uploadButton.addEventListener("click", this.onClick); - uploadButton.addEventListener("dragover", this.onDragover); - uploadButton.addEventListener("drop", this.onDrop); } // Appended to the drop zone (not between button and input, whose @@ -146,11 +149,11 @@ export default class FileUploadController extends Controller { this.fileInput.click(); }; - // Drag & drop mirrors govuk-frontend's FileUpload: the button is the drop - // target, the whole drop zone shows the dragging state, and enter/leave - // are announced. - // Prevent the default so the button is a valid drop target. + // Drag & drop: the whole wrapper is the drop target, the button shows + // the dragging state, and enter/leave are announced. onDragover = (event) => { + if (this.fileInput.disabled) return; + event.preventDefault(); }; @@ -171,6 +174,8 @@ export default class FileUploadController extends Controller { }; onDrop = (event) => { + if (this.fileInput.disabled) return; + event.preventDefault(); if (event.dataTransfer && this.canFillInput(event.dataTransfer)) { diff --git a/doc/attachment-field-spec.md b/doc/attachment-field-spec.md index e4d5c1d..d5ba367 100644 --- a/doc/attachment-field-spec.md +++ b/doc/attachment-field-spec.md @@ -27,7 +27,9 @@ direct upload, and a full no-JavaScript fallback. than `ElementError`; count strings even for one file (C10 records why); our announcements region renders inside the wrapper — scoped finds and morph re-enhancement keep it — where govuk-frontend's sits after the - drop zone. + drop zone; and our drop listeners bind to the wrapper where upstream's + bind to its button — equivalent zones, since upstream's button is its + whole drop zone while ours shares the wrapper with figures. govuk-frontend's own component stays supported for plain file fields (`initAll` initialises `data-module="govuk-file-upload"`), so the **exclusivity invariant** is critical: every file input is enhanced by @@ -113,8 +115,10 @@ form group (data-controller for the drop zone) - Without JS the native file input is a plain visible input; with JS it is hidden and fronted by the pseudo button, which triggers the browser file - picker. The drop target is the pseudo button/drop region, not the whole - form group: a valid drag over it adds `--dragging` to the button and is + picker. The drop target is the whole drop zone — the wrapper element, + figures included — not just the pseudo button (a file dragged anywhere + over the field can be dropped), and not the form group around it: a + valid drag over the wrapper adds `--dragging` to the button and is announced through a JS-injected visually-hidden assertive region (`.govuk-file-upload-announcements`), and a drop fills the input exactly as choosing files does. A drop is accepted only within the input's diff --git a/spec/system/attachment/drag_and_drop_spec.rb b/spec/system/attachment/drag_and_drop_spec.rb index cdd32dc..99e6b37 100644 --- a/spec/system/attachment/drag_and_drop_spec.rb +++ b/spec/system/attachment/drag_and_drop_spec.rb @@ -3,11 +3,13 @@ require "rails_helper" # Dropping files onto the attachment field, ported from govuk-frontend's -# FileUpload: the button is the drop target, a valid drag shows the dragging -# state and is announced, and a drop fills the input exactly as choosing files -# does. Drops are simulated with a synthetic DragEvent carrying a DataTransfer, -# which exercises the real controller in the browser (OS-level drag can't be -# driven from Capybara). +# FileUpload with one intended difference: the drop target is the whole +# wrapper, figures included, not just the button (upstream's button is its +# whole zone; ours shares the wrapper with figures). A valid drag shows the +# dragging state and is announced, and a drop fills the input exactly as +# choosing files does. Drops are simulated with a synthetic DragEvent +# carrying a DataTransfer, which exercises the real controller in the +# browser (OS-level drag can't be driven from Capybara). RSpec.describe "Dropping files onto an attachment field", :aggregate_failures do include AttachmentFieldHelpers include DirectUploadHelpers @@ -51,6 +53,25 @@ expect(cv_field).to have_no_css("figure.govuk-attachment") end + + it "accepts a drop on a figure, away from the button" do + visit edit_profile_path(profile) + + # avatar always carries its persisted figure, making the non-button + # surface of the drop zone real. + drop_files("profile[avatar]", "dropped.png", target: "figure.govuk-attachment") + + expect(avatar_field).to have_css("figure.govuk-attachment .filename", text: "dropped.png") + end + + it "ignores a drop on a disabled field" do + visit edit_profile_path(profile) + + page.execute_script("document.querySelector(`input[type=file][name='profile[cv]']`).disabled = true") + drop_files("profile[cv]", "dropped.png") + + expect(cv_field).to have_no_css("figure.govuk-attachment") + end end it "shows the dragging state and announces entering and leaving the drop zone" do @@ -72,18 +93,19 @@ def gallery_drop_target ".govuk-file-upload-wrapper:has(input[name='profile[gallery][]']) .govuk-file-upload-button" end - # Dispatch a synthetic drop of `filenames` onto the field's button. The - # File contents are stub bytes — enough for a preview and a direct upload. - def drop_files(input_name, *filenames) - page.execute_script(<<~JS, input_name, filenames) - const [name, names] = arguments; - const input = document.querySelector(`input[type=file][name="${name}"]`); - const button = input - .closest(".govuk-file-upload-wrapper") - .querySelector(".govuk-file-upload-button"); + # Dispatch a synthetic drop of `filenames` onto the field's wrapper — or + # onto `target`, a selector within it. The File contents are stub bytes — + # enough for a preview and a direct upload. + def drop_files(input_name, *filenames, target: nil) + page.execute_script(<<~JS, input_name, filenames, target) + const [name, names, target] = arguments; + const wrapper = document + .querySelector(`input[type=file][name="${name}"]`) + .closest(".govuk-file-upload-wrapper"); + const element = target ? wrapper.querySelector(target) : wrapper; const data = new DataTransfer(); names.forEach((n) => data.items.add(new File(["stub"], n, { type: "image/png" }))); - button.dispatchEvent( + element.dispatchEvent( new DragEvent("drop", { dataTransfer: data, bubbles: true, cancelable: true }), ); JS From 3f39400d9e4e632015d4ec0c89b4d6dd37170fbd Mon Sep 17 00:00:00 2001 From: Stephen Nelson Date: Wed, 29 Jul 2026 09:22:12 +0930 Subject: [PATCH 20/22] Voiceover automation helper --- script/voiceover/README.md | 85 ++++++++++++++++++++++++++++++++ script/voiceover/capture-ocr.mjs | 58 ++++++++++++++++++++++ script/voiceover/ocr.swift | 46 +++++++++++++++++ 3 files changed, 189 insertions(+) create mode 100644 script/voiceover/README.md create mode 100644 script/voiceover/capture-ocr.mjs create mode 100644 script/voiceover/ocr.swift diff --git a/script/voiceover/README.md b/script/voiceover/README.md new file mode 100644 index 0000000..8f2def2 --- /dev/null +++ b/script/voiceover/README.md @@ -0,0 +1,85 @@ +# VoiceOver caption capture (spike) + +Throwaway tooling (outside the gemspec) to capture what VoiceOver actually +announces during an interaction, so announcement issues can be characterised +from real utterance sequences instead of by ear. + +## Why OCR + +VoiceOver's spoken output is only faithfully readable from its **caption +panel**, and the panel is opaque to scripting: its AppleScript object +exposes only `enabled` (no text), the `last phrase` object holds a single +slot that updates at *queue* time (so it skips past transient announcements +like " removed"), and the VoiceOver process publishes nothing to the +Accessibility API. The panel, however, renders each statement **speech-timed** +— held on screen for the duration it's spoken, then replaced — so screen +**OCR** of the panel region, polled at sub-statement intervals, catches every +statement in turn. That is the only method here that captures the transient +announcements; the AppleScript-slot and guidepup approaches were tried and +dropped because both read that lossy single slot. + +## Setup + +1. Enable VoiceOver AppleScript control: VoiceOver Utility (⌘F5 to start VO, + then VO+F8) → General → **“Allow VoiceOver to be controlled with + AppleScript”**. +2. Grant the terminal **Screen Recording** permission (System Settings → + Privacy & Security → Screen Recording) and restart it — `screencapture` + needs it. +3. Compile the OCR helper (macOS Vision framework): + ``` + swiftc script/voiceover/ocr.swift -o /tmp/vo-ocr + ``` + +## Calibrate the panel region + +The capture crops to the caption panel by screen coordinates (`screencapture +-R x,y,w,h`, in points), which are **display- and position-specific**. Find +yours: capture the full screen and OCR it with coordinates to locate the +caption text, then tighten a crop around just the panel (excluding page +content beside it): + +``` +screencapture -x /tmp/full.png +/tmp/vo-ocr /tmp/full.png --coords # find the caption text's box +screencapture -x -R 905,82,560,98 /tmp/panel.png +/tmp/vo-ocr /tmp/panel.png # adjust R until only the panel shows +``` + +## Capture + +``` +VO_SCRATCH=/tmp VO_REGION=905,82,560,98 node script/voiceover/capture-ocr.mjs 15 +``` + +`VO_SCRATCH` holds the compiled `vo-ocr` binary and scratch frames; +`VO_REGION` is your calibrated crop. It polls the panel for the given +seconds, logging each statement (wrapped lines joined) as it changes: + +``` +region 905,82,560,98, 15s + + 0.24s 🗣 Gallery, No file chosen, Choose file or drop file, button group + 2.60s 🗣 bikes 2.jpg removed +``` + +Drive the interaction by hand with **real keys** (VoiceOver only announces +for real key events — VO+Space to activate), then keep off the keyboard +until the window ends so the panel isn't overwritten by other narration. + +Protocol learnings from the F7 runs: + +- **Suspect network timing before input method.** A missing outcome + announcement means the live-region write landed inside the selection's + announcement traffic — verified both ways: throttled runs voice success + and failure regardless of input method, unthrottled runs lose them. + Mouse steps do change the *focus* narration (no dialog-close or focus + re-read when focus stays in Finder), so keep input consistent across + runs being compared. +- **Statements are held ~3.4s each**, so queued announcements drain slowly: + size the window generously (30s+) or trailing statements are clipped. +- **Devtools network throttling is the timing probe.** Local uploads finish + inside the selection's own announcement traffic (count re-read, + dialog-close, focus re-read), which is exactly where live-region writes + get dropped; throttling moves the upload-outcome write clear of the + traffic so the two effects can be told apart. diff --git a/script/voiceover/capture-ocr.mjs b/script/voiceover/capture-ocr.mjs new file mode 100644 index 0000000..ec2f571 --- /dev/null +++ b/script/voiceover/capture-ocr.mjs @@ -0,0 +1,58 @@ +#!/usr/bin/env node +// Reads VoiceOver's caption panel by OCR — the only faithful source, since +// the panel is opaque to AppleScript/AX. The panel renders each statement +// speech-timed (held for the duration it's spoken, then replaced), so +// polling the region at sub-statement intervals catches every statement in +// turn, including the transient "removed" the AppleScript `last phrase` +// slot drops. +// +// Calibrate REGION (screencapture -R points: x,y,w,h) to your caption panel +// with the calibration captures in the README. Run this, perform a removal +// (real VO+Space so VoiceOver actually announces), then Ctrl-C or wait out +// the window. +// +// node script/voiceover/capture-ocr.mjs [seconds] +import { execFileSync } from "node:child_process"; +import { writeFileSync } from "node:fs"; + +const SCRATCH = process.env.VO_SCRATCH ?? "."; +const OCR = process.env.VO_OCR ?? `${SCRATCH}/vo-ocr`; // swiftc ocr.swift -o $SCRATCH/vo-ocr +const REGION = process.env.VO_REGION ?? "905,82,560,98"; +const FRAME = `${SCRATCH}/vo-frame.png`; +const WINDOW_S = Number(process.argv[2] ?? 20); + +const norm = (s) => s.replace(/\s+/g, " ").trim(); + +// The panel's current statement = its wrapped lines joined. Ignore the +// leading "X"/"x" close-button glyph the OCR sometimes prepends. +function readPanel() { + execFileSync("screencapture", ["-x", "-R", REGION, FRAME]); + const lines = execFileSync(OCR, [FRAME], { encoding: "utf8" }) + .split("\n") + .map(norm) + .filter(Boolean); + return norm(lines.join(" ").replace(/^[Xx]\s+/, "")); +} + +const t0 = Date.now(); +const seq = []; +let last = null; + +while (Date.now() - t0 < WINDOW_S * 1000) { + let statement = ""; + try { + statement = readPanel(); + } catch { + // transient capture/OCR failure; keep polling + } + if (statement && statement !== last) { + seq.push({ at: Date.now() - t0, statement }); + last = statement; + } +} + +console.log(`region ${REGION}, ${WINDOW_S}s\n`); +for (const { at, statement } of seq) { + console.log(`${(at / 1000).toFixed(2).padStart(6)}s 🗣 ${statement}`); +} +writeFileSync(`${SCRATCH}/vo-ocr-transcript.txt`, seq.map((s) => `${(s.at / 1000).toFixed(2)}\t${s.statement}`).join("\n")); diff --git a/script/voiceover/ocr.swift b/script/voiceover/ocr.swift new file mode 100644 index 0000000..067a351 --- /dev/null +++ b/script/voiceover/ocr.swift @@ -0,0 +1,46 @@ +// Vision-framework OCR: prints the text recognised in an image, ordered +// top-to-bottom, one observation per line. Used to read VoiceOver's caption +// panel (which is opaque to AppleScript and the Accessibility API) from a +// screenshot of its region. +// +// swift script/voiceover/ocr.swift +import Foundation +import Vision +import AppKit + +guard CommandLine.arguments.count > 1, + let image = NSImage(contentsOfFile: CommandLine.arguments[1]), + let cgImage = image.cgImage(forProposedRect: nil, context: nil, hints: nil) +else { + FileHandle.standardError.write(Data("usage: ocr.swift \n".utf8)) + exit(1) +} + +let request = VNRecognizeTextRequest() +request.recognitionLevel = .accurate +request.usesLanguageCorrection = false + +let handler = VNImageRequestHandler(cgImage: cgImage, options: [:]) +try? handler.perform([request]) + +let observations = (request.results ?? []) + // Vision's origin is bottom-left; sort descending y for reading order. + .sorted { $0.boundingBox.origin.y > $1.boundingBox.origin.y } + +// With --coords, prefix each line with its pixel bounding box (for locating +// a region on a full-screen capture); otherwise print text only. +let showCoords = CommandLine.arguments.contains("--coords") +let w = CGFloat(cgImage.width) +let h = CGFloat(cgImage.height) + +for observation in observations { + guard let text = observation.topCandidates(1).first?.string else { continue } + if showCoords { + let b = observation.boundingBox + let px = Int(b.origin.x * w) + let py = Int((1 - b.origin.y - b.height) * h) // top-left origin, pixels + print("[x=\(px) y=\(py) w=\(Int(b.width * w)) h=\(Int(b.height * h))] \(text)") + } else { + print(text) + } +} From 68e966e261786af4afcef2cf3d3a25e981181538 Mon Sep 17 00:00:00 2001 From: Stephen Nelson Date: Fri, 31 Jul 2026 16:13:28 +0930 Subject: [PATCH 21/22] Release prep: documentation and examples --- Gemfile.lock | 2 +- README.md | 156 ++++++++++++++++-- .../govuk/components/attachment/_mixin.scss | 1 + .../katalyst/govuk/form_builder/frontend.rb | 13 +- .../govuk/form_builder/traits/attachment.rb | 4 +- app/javascript/katalyst/govuk/config.js | 7 +- .../controllers/attachment_controller.js | 4 +- .../controllers/file_upload_controller.js | 6 +- app/javascript/katalyst/govuk/formbuilder.js | 137 ++++++++++----- doc/attachment-field-spec.md | 9 +- katalyst-govuk-formbuilder.gemspec | 2 +- lib/katalyst/govuk/form_builder/config.rb | 2 +- .../form_builder_attachment_field_spec.rb | 2 +- .../app/controllers/examples_controller.rb | 4 +- .../dummy/app/javascript/controllers/index.js | 9 +- .../examples/attachment/multiple.html.erb | 10 ++ .../views/examples/attachment/single.html.erb | 10 ++ .../dummy/app/views/guide/attachment.html.erb | 14 ++ spec/dummy/app/views/guide/index.html.erb | 5 + spec/fixtures/files/banner.png | Bin 0 -> 3818 bytes spec/requests/profiles/avatar_spec.rb | 13 +- spec/requests/profiles_spec.rb | 20 ++- spec/system/attachment/preview_spec.rb | 42 +++++ spec/system/attachment/round_trip_spec.rb | 2 +- spec/system/attachment/upload_spec.rb | 2 +- spec/system/frontend_enhancements_spec.rb | 60 +++++++ spec/system/frontend_morph_spec.rb | 22 +-- 27 files changed, 462 insertions(+), 96 deletions(-) create mode 100644 spec/dummy/app/views/examples/attachment/multiple.html.erb create mode 100644 spec/dummy/app/views/examples/attachment/single.html.erb create mode 100644 spec/dummy/app/views/guide/attachment.html.erb create mode 100644 spec/fixtures/files/banner.png create mode 100644 spec/system/attachment/preview_spec.rb diff --git a/Gemfile.lock b/Gemfile.lock index 2de2981..c19e68f 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -1,7 +1,7 @@ PATH remote: . specs: - katalyst-govuk-formbuilder (1.30.1) + katalyst-govuk-formbuilder (2.0.0) activestorage (>= 8.0.0) govuk_design_system_formbuilder (>= 6.2.0) diff --git a/README.md b/README.md index 7ce7d3e..668dd1f 100644 --- a/README.md +++ b/README.md @@ -1,7 +1,9 @@ # Katalyst::GOVUK::FormBuilder Repacking of [GOV.UK Frontend](https://frontend.design-system.service.gov.uk) and -[GOV.UK form components](https://govuk-form-builder.netlify.app) for use in Katalyst projects. +[GOV.UK form components](https://govuk-form-builder.netlify.app) for use in Katalyst projects, +extended with ActiveStorage-backed attachment fields: previews, drag-and-drop, async direct +uploads, and a complete no-JavaScript fallback. ## Installation @@ -21,6 +23,14 @@ Or install it yourself as: ## Usage +Use the GOV.UK form builder for your forms, most simply as the application default: + +```ruby +class ApplicationController < ActionController::Base + default_form_builder GOVUKDesignSystemFormBuilder::FormBuilder +end +``` + Add the stylesheet to your default layout: ```erb @@ -33,44 +43,154 @@ You can also add it to your SASS build: @use "katalyst/govuk/formbuilder"; ``` -Some GOVUK components require javascript enhancements -(see [GOVUK docs](https://frontend.design-system.service.gov.uk/get-started/#5-get-the-javascript-working)). +### JavaScript + +The attachment and file upload fields are Stimulus-powered. Load the gem's +controllers into your Stimulus application: + +```js +import { Application } from "@hotwired/stimulus"; -You can use the provided helper to load the formbuilder esm from importmaps and enhance your form: +const application = Application.start(); + +import GOVUK from "@katalyst/govuk-formbuilder"; +GOVUK.start(application); +``` + +If you want to keep GOVUK enhancements separate from your main app's Stimulus +application, you can inject the bootstrap JS module into your body instead: ```erb -<%= form_with ... %> <%= govuk_formbuilder_init %> ``` -You'll need to include the helper to make this method available, which you can add to your `ApplicationController`: +You'll need to include the helper to make this method available, which you can add +to your `ApplicationController`: ```ruby helper Katalyst::GOVUK::FormBuilder::Frontend ``` -### JavaScript dependencies +The snippet marks the page as JavaScript-capable and calls the module's `initAll()`, +but will not survive a new page render (including Turbo navigation). Use this +approach if you are only using GOVUK sparingly. + +#### JavaScript dependencies The formbuilder module imports `@hotwired/stimulus` and `@rails/activestorage`. With -importmaps the gem pins `@rails/activestorage` for you. If you use jsbundling or similar, -you'll need both packages available at runtime. +importmaps the gem pins itself and `@rails/activestorage` for you. It does not pin +`@hotwired/stimulus` — your app provides that (`stimulus-rails` does this in a +standard Rails app). If you use jsbundling or similar, you'll need both packages +available at runtime; the wiring is the same `GOVUK.start(application)` shown above, +from your own bundle. -## Extensions +## Attachment fields -We include some optional extensions for integrating with gems that we (Katalyst) commonly use. +`govuk_image_field` and `govuk_document_field` render an upload field for +`has_one_attached` / `has_many_attached` attributes. With JavaScript, files upload +asynchronously as soon as they are chosen or dropped, each showing a preview figure +with progress, retry on failure, and a remove control. Without JavaScript the same +field is a plain file input plus a keep/remove select per attached file — no +functionality is lost, only polish. -These require additional steps to use. +```erb +<%= f.govuk_image_field :avatar %> +<%= f.govuk_document_field :cv %> +<%= f.govuk_attachment_field :recording, accept: "audio/*" %> +``` + +Both fields delegate to `govuk_attachment_field`; they differ only in the mime types +they accept (`config.image_mime_types` / `config.document_mime_types`). + +- The attribute's value must be an `ActiveStorage::Attached`; anything else raises + `ArgumentError` at render. For plain multipart uploads without ActiveStorage, use + the upstream `govuk_file_field`. +- `multiple` is inferred from the association (`has_many_attached` → multiple), and + an explicit `multiple:` argument is respected. +- Attachments round-trip as blob signed ids: when validation fails, the re-rendered + form retains every attachment — stored, direct-uploaded, or pending multipart — so + a failed submit never loses an upload. +- Removal is always offered. A required attachment should say so with a presence + validation; the form does not hide removal on its behalf. +- Direct uploads post to `rails_direct_uploads_url` by default. Pass + `direct_upload_url:` to use a different endpoint, or `direct_upload: false` to + leave chosen files in the input and submit them as ordinary multipart. +- Fields accept the standard GOV.UK options (`label:`, `hint:`, `caption:`, + `form_group:`, `before_input:`, `after_input:`) and a block for supplemental + content rendered inside the form group. + +Preview thumbnails are generated lazily through ActiveStorage's representation +route. The transformation is configurable: -### File inputs +```ruby +GOVUKDesignSystemFormBuilder.config.attachment_preview_representation = + { resize_to_limit: [100, 100] } # the default +``` -We've added image and document fields designed for use with Koi. These will not be suitable for -every project. If you want to use these, you'll need to import and load their stimulus controllers: +### Strings and internationalisation + +All user-facing strings resolve through Rails i18n under `katalyst.govuk.attachment.*` +(`upload_succeeded`, `upload_failed`, `retry_button`, `file_removed`, `remove_button`, +`remove_button_content`), alongside govuk-frontend's FileUpload strings. Each has a +per-field text option (`upload_succeeded_text`, `upload_failed_text`, +`retry_button_text`, `file_removed_text`, `remove_button_text`, +`remove_button_content_text`, `choose_files_button_text`, `drop_instruction_text`, +`no_file_chosen_text`, `multiple_files_chosen_text`, `entered_drop_zone_text`, +`left_drop_zone_text`). Strings reach the JavaScript enhancement via the field's +`data-i18n.*` attributes, with the locale taken from the closest `lang` attribute. + +## Upgrading from 1.x + +This major version replaces the image/document field implementations with the +attachment field described above. + +- `govuk_image_field` / `govuk_document_field` now require an + `ActiveStorage::Attached` value and raise `ArgumentError` otherwise. The legacy + fields rendered a plain enhanced input for other values (e.g. form objects) — for + those, use `govuk_file_field`. +- The `optional:` argument no longer does anything: removal is always offered, and + submitting the remove option detaches on save. A required attachment must be + guarded by a presence validation. +- Dropped files are no longer filtered by mime type on the client. The `accept` + attribute remains a file-picker courtesy; your model's validations are the + authority on content. +- `application.load(govuk)` no longer works — the default export is no longer the + controller definitions array, and Stimulus raises a `TypeError` at boot. Replace it + with `GOVUK.start(application)` (see JavaScript above), which registers the + controllers and keeps enhancement running across Turbo visits. +- Remove any `turbo:render` / `turbo:frame-load` re-initialisation wiring — + enhancement now observes the DOM and owns re-enhancement; repeated calls are + harmless no-ops. +- Text options on file fields are now honoured by the JavaScript. Previously they + rendered but were never read, so non-English sites got English announcements. +- Brand (`GOVUKDesignSystemFormBuilder.brand`) now affects CSS classes only: + Stimulus identifiers, `data-controller`/`data-action` wiring, and events are + always `govuk-*`. The gem's compiled CSS remains govuk-prefixed — a non-default + brand presumes a consumer-supplied frontend build. + +As a transitional escape hatch, the legacy implementations remain available behind a +flag: -```js -import govuk from "@katalyst/govuk-formbuilder"; -application.load(govuk); +```ruby +GOVUKDesignSystemFormBuilder.config.use_legacy_file_fields = true # default false ``` +The flag flips `govuk_image_field` / `govuk_document_field` back to the legacy +elements. It exists to stage a migration, not to stay on: the flag and the legacy +code will be removed together in a subsequent release. + +## Extensions + +We include some optional extensions for integrating with gems that we (Katalyst) +commonly use. These require additional steps to use. + +### Rich text area + +`govuk_rich_textarea` renders a Trix editor with GOV.UK form conventions. It +requires ActionText to be set up in your application (`rails action_text:install`), +including its JavaScript (`trix` and `@rails/actiontext`) in your bundle or +importmap. + ### Hotwire Combobox [Hotwire Combobox](https://hotwirecombobox.com) is a promising option for adding asynchronous multi-select inputs to diff --git a/app/assets/stylesheets/katalyst/govuk/components/attachment/_mixin.scss b/app/assets/stylesheets/katalyst/govuk/components/attachment/_mixin.scss index 39e6cea..162c13f 100644 --- a/app/assets/stylesheets/katalyst/govuk/components/attachment/_mixin.scss +++ b/app/assets/stylesheets/katalyst/govuk/components/attachment/_mixin.scss @@ -19,6 +19,7 @@ $attachment-border-width: 2px; grid-area: preview; max-width: 4rem; aspect-ratio: 1/1; + object-fit: contain; } .caption { diff --git a/app/helpers/katalyst/govuk/form_builder/frontend.rb b/app/helpers/katalyst/govuk/form_builder/frontend.rb index 2a9cf90..91e91f9 100644 --- a/app/helpers/katalyst/govuk/form_builder/frontend.rb +++ b/app/helpers/katalyst/govuk/form_builder/frontend.rb @@ -13,12 +13,21 @@ module Frontend def govuk_formbuilder_init tag.script type: "module", nonce: request.content_security_policy_nonce do <<~JS.html_safe - import {init} from "@katalyst/govuk-formbuilder"; - init({brand: #{GOVUKDesignSystemFormBuilder.brand.to_json}}); + import {initAll} from "@katalyst/govuk-formbuilder"; + initAll(#{govuk_formbuilder_init_options}); JS end end # rubocop:enable Rails/OutputSafety + + private + + # The bundle's own default is govuk; only a non-default brand renders. + def govuk_formbuilder_init_options + brand = GOVUKDesignSystemFormBuilder.brand + + brand.to_s == "govuk" ? "" : "{brand: #{brand.to_json}}" + end end end end diff --git a/app/helpers/katalyst/govuk/form_builder/traits/attachment.rb b/app/helpers/katalyst/govuk/form_builder/traits/attachment.rb index 8b423e5..e33a1db 100644 --- a/app/helpers/katalyst/govuk/form_builder/traits/attachment.rb +++ b/app/helpers/katalyst/govuk/form_builder/traits/attachment.rb @@ -197,8 +197,8 @@ def persist_pending_blobs def persist_pending_change(change) change.upload change.blob.save! - rescue ActiveStorage::Error => e - # no recovery available + rescue ActiveStorage::Error, Errno::ENOENT => e + # no recovery available (ENOENT: tempfile vanished before render) log_dropped_upload(e) end diff --git a/app/javascript/katalyst/govuk/config.js b/app/javascript/katalyst/govuk/config.js index a9f8368..04a75c1 100644 --- a/app/javascript/katalyst/govuk/config.js +++ b/app/javascript/katalyst/govuk/config.js @@ -7,11 +7,16 @@ import { // CSS classes written or queried by this bundle follow the configured // brand, mirroring the Ruby builder's class prefixes. Behavioural wiring // (Stimulus identifiers, data-actions, events) is always govuk-prefixed. -// Set from the govuk_formbuilder_init snippet via init({ brand }). +// Set from the govuk_formbuilder_init snippet via initAll({ brand }). const config = { brand: "govuk" }; export default config; +// The pseudo upload button, identified structurally — the button fronting +// the (hidden) file input it precedes — so the selector holds under any +// brand. +export const uploadButtonSelector = "[type='button']:has(+ input[type='file'])"; + // The attachment field replaces govuk-frontend's FileUpload, so its // vocabulary is FileUpload's strings plus the attachment additions — one // table for the whole field. The inherited keys keep their upstream names: diff --git a/app/javascript/katalyst/govuk/controllers/attachment_controller.js b/app/javascript/katalyst/govuk/controllers/attachment_controller.js index 794568c..e910cdb 100644 --- a/app/javascript/katalyst/govuk/controllers/attachment_controller.js +++ b/app/javascript/katalyst/govuk/controllers/attachment_controller.js @@ -2,7 +2,7 @@ import { Controller } from "@hotwired/stimulus"; import { DirectUploadController } from "@rails/activestorage"; import { I18n } from "govuk-frontend/dist/govuk/i18n.mjs"; import { closestAttributeValue } from "govuk-frontend/dist/govuk/common/closest-attribute-value.mjs"; -import config, { attachmentConfig } from "../config"; +import config, { attachmentConfig, uploadButtonSelector } from "../config"; class AttachmentUploadController extends DirectUploadController { async start(option) { @@ -149,7 +149,7 @@ export default class AttachmentController extends Controller { get uploadButton() { return this.element .closest(`.${config.brand}-file-upload-wrapper`) - ?.querySelector("[type='button']:has(+ input[type='file'])"); + ?.querySelector(uploadButtonSelector); } set statusText(message) { diff --git a/app/javascript/katalyst/govuk/controllers/file_upload_controller.js b/app/javascript/katalyst/govuk/controllers/file_upload_controller.js index cc2e069..92bff41 100644 --- a/app/javascript/katalyst/govuk/controllers/file_upload_controller.js +++ b/app/javascript/katalyst/govuk/controllers/file_upload_controller.js @@ -2,7 +2,7 @@ import { Controller } from "@hotwired/stimulus"; import { I18n } from "govuk-frontend/dist/govuk/i18n.mjs"; import { closestAttributeValue } from "govuk-frontend/dist/govuk/common/closest-attribute-value.mjs"; import { createAttachment } from "./attachment_controller"; -import config, { attachmentConfig } from "../config"; +import config, { attachmentConfig, uploadButtonSelector } from "../config"; export default class FileUploadController extends Controller { connect() { @@ -333,9 +333,7 @@ export default class FileUploadController extends Controller { } get uploadButton() { - return this.element.querySelector( - "[type='button']:has(+ input[type='file'])", - ); + return this.element.querySelector(uploadButtonSelector); } get isDragging() { diff --git a/app/javascript/katalyst/govuk/formbuilder.js b/app/javascript/katalyst/govuk/formbuilder.js index 5657fbc..889a6b2 100644 --- a/app/javascript/katalyst/govuk/formbuilder.js +++ b/app/javascript/katalyst/govuk/formbuilder.js @@ -13,24 +13,24 @@ import { isSupported, } from "govuk-frontend/dist/govuk/common/index.mjs"; -function initAll(config) { - let _config$scope; - config = typeof config !== "undefined" ? config : {}; +// Component options captured from the initAll call, reused by every +// observer-driven sweep. +let options = {}; + +function enhance($scope = document) { if (!isSupported()) { console.log(new SupportError()); return; } const components = [ - [Button, config.button], - [CharacterCount, config.characterCount], + [Button, options.button], + [CharacterCount, options.characterCount], [Checkboxes], - [ErrorSummary, config.errorSummary], - [FileUpload, config.fileUpload], + [ErrorSummary, options.errorSummary], + [FileUpload, options.fileUpload], [Radios], - [PasswordInput, config.passwordInput], + [PasswordInput, options.passwordInput], ]; - const $scope = - (_config$scope = config.scope) != null ? _config$scope : document; components.forEach(([Component, config]) => { const selector = `[data-module="${Component.moduleName}"]`; // The scope itself can be a component root (an observed insertion is @@ -77,23 +77,21 @@ function supportMarked(body) { } function observe(body) { - // A morph reconciles the live DOM against a server response that carries - // no JS-set state, stripping the body markers, every component's - // data-*-init flag, and all injected UI — with no lifecycle events. Losing - // the markers is therefore the signal that a morph happened: re-mark, then - // sweep. Construction is guarded per component (an already-initialised - // root throws InitError, which initAll swallows), so the sweep only - // (re)enhances roots whose flags were stripped. Re-marking is - // check-then-set, so observing our own write terminates in one bounce. + // The body markers are JS-set, so a morph — which reconciles the live DOM + // against server HTML, with no lifecycle events — strips them, along with + // every component's data-*-init flag. Missing markers signal the morph: + // re-mark, then sweep; the isInitialised guard re-enhances only roots + // whose flags were stripped. Re-marking is check-then-set, so observing + // our own write terminates in one bounce. // - // Registered before the childList observer: callbacks run in creation - // order, so when a strip and insertions land in one batch the markers are - // back before any arrival sweep consults isSupported(). + // Registered before the arrival observer: callbacks run in creation + // order, so markers are back before an arrival sweep consults + // isSupported(). new MutationObserver(() => { if (supportMarked(body)) return; markSupport(body); - initAll(); + enhance(); }).observe(body, { attributes: true, attributeFilter: ["class"] }); // Components can also arrive after load — lazily-loaded turbo frames, @@ -103,37 +101,100 @@ function observe(body) { new MutationObserver((mutations) => { for (const mutation of mutations) { for (const node of mutation.addedNodes) { - if (node instanceof Element) initAll({ scope: node }); + if (node instanceof Element) enhance(node); } } }).observe(body, { childList: true, subtree: true }); } -// Entry point for the govuk_formbuilder_init snippet: mark the page, enhance -// it, and keep both maintained as the DOM changes. The observers attach to -// the element itself, so a Turbo replace render — which swaps in a -// new body and re-executes the snippet — disposes and recreates them, while -// a morph retains the body and the observers with it. -function init(options = {}) { - if (options.brand) config.brand = options.brand; - - const body = document.body; - - if (body.__govukFormbuilderInit) return; +function setup(body) { + if (!body || body.__govukFormbuilderInit) return; body.__govukFormbuilderInit = true; markSupport(body); - initAll(); + enhance(); observe(body); } +// The gem's Stimulus controllers register exactly once, on whichever +// application claims them first: the consumer's (via start) or a gem-owned +// application created on demand (the snippet path, for apps not otherwise +// running Stimulus). Stimulus itself observes the whole document, so +// registration — unlike the body-scoped setup — survives Turbo visits. +let registered = false; + +function register(application = undefined) { + if (registered) return; + registered = true; + + (application ?? Application.start()).load(controllers); +} + +function applyConfig(config) { + if (config.brand) brandConfig.brand = config.brand; + options = config; +} + +/** + * Enhance the current : mark it as JS-capable, enhance its GOV.UK + * components, and observe it for arrivals and morphs. Scoped to the body it + * ran against — it does not survive a body replacement, so render it with + * every page (the govuk_formbuilder_init snippet at the end of ). + * Registers the gem's Stimulus controllers on a gem-owned application + * unless start() has already claimed them. + * + * @param {object} [config] per-component config (button, characterCount, + * errorSummary, fileUpload, passwordInput) + * @param {string} [config.brand] CSS class prefix for injected UI (default "govuk") + */ +function initAll(config = {}) { + applyConfig(config); + register(); + setup(document.body); +} + +let watching = false; + +/** + * Wire the gem into your Stimulus application and keep the page enhanced + * for the life of the session, including across Turbo visits — call once + * from your own bundle: + * + * import GOVUK from "@katalyst/govuk-formbuilder"; + * GOVUK.start(application); + * + * @param {object} [application] Stimulus application to register the gem's + * controllers on (a gem-owned application is created when omitted) + * @param {object} [config] as initAll's config + */ +function start(application = undefined, config = {}) { + applyConfig(config); + register(application); + + if (!watching) { + watching = true; + + // The body-scoped setup dies with each Turbo visit; the documentElement + // survives them, so watch it and set up against every new body. Also + // covers a start() before exists — the body's insertion is + // itself a childList mutation here. + new MutationObserver(() => setup(document.body)).observe( + document.documentElement, + { childList: true }, + ); + } + + setup(document.body); +} + // stimulus controllers +import { Application } from "@hotwired/stimulus"; import controllers from "./controllers"; -import config from "./config"; +import brandConfig from "./config"; + +export default { start }; export { - controllers as default, - init, initAll, Button, CharacterCount, diff --git a/doc/attachment-field-spec.md b/doc/attachment-field-spec.md index d5ba367..259a06f 100644 --- a/doc/attachment-field-spec.md +++ b/doc/attachment-field-spec.md @@ -195,8 +195,13 @@ method) resolves the representation route with the same `main_app` fallback and returns nil when no route is available — the figure then renders without a preview — so engine builders can override preview resolution too. The preview transformation is configurable -(`config.attachment_preview_representation`, default a 100×100 centre-padded -thumbnail). +(`config.attachment_preview_representation`); the default fits the image +within 100×100 preserving aspect, never upscaling +(`resize_to_limit`). Previews are contained, not cropped: the whole image +shows, letterboxed by CSS (`object-fit: contain` in a square preview box) +rather than by baked-in padding — variants carry no background bars, and +client-inserted previews get the same treatment so framing doesn't change +when a figure round-trips. `multiple` is inferred from the attribute's ActiveStorage reflection (`has_many_attached` → true), and an explicit `multiple:` argument is diff --git a/katalyst-govuk-formbuilder.gemspec b/katalyst-govuk-formbuilder.gemspec index 2d4938b..ebb8ef7 100644 --- a/katalyst-govuk-formbuilder.gemspec +++ b/katalyst-govuk-formbuilder.gemspec @@ -2,7 +2,7 @@ Gem::Specification.new do |spec| spec.name = "katalyst-govuk-formbuilder" - spec.version = "1.30.1" + spec.version = "2.0.0" spec.authors = ["Katalyst Interactive"] spec.email = ["developers@katalyst.com.au"] diff --git a/lib/katalyst/govuk/form_builder/config.rb b/lib/katalyst/govuk/form_builder/config.rb index 588c159..06fd8e3 100644 --- a/lib/katalyst/govuk/form_builder/config.rb +++ b/lib/katalyst/govuk/form_builder/config.rb @@ -35,7 +35,7 @@ def attachment_preview_representation=(value) config.attachment_preview_representation = value end - config.attachment_preview_representation = { resize_and_pad: [100, 100, { crop: :centre }] }.freeze + config.attachment_preview_representation = { resize_to_limit: [100, 100] }.freeze def use_legacy_file_fields? config.use_legacy_file_fields diff --git a/spec/builders/govuk_design_system_form_builder/form_builder_attachment_field_spec.rb b/spec/builders/govuk_design_system_form_builder/form_builder_attachment_field_spec.rb index c2187f2..99bfb8d 100644 --- a/spec/builders/govuk_design_system_form_builder/form_builder_attachment_field_spec.rb +++ b/spec/builders/govuk_design_system_form_builder/form_builder_attachment_field_spec.rb @@ -525,7 +525,7 @@ def direct_upload_url # set, so an engine-mounted form must resolve the preview URL through # main_app — the same resolution direct_upload_url uses. describe "preview URL resolution" do - let(:representation) { blob.representation(resize_and_pad: [100, 100, { crop: :centre }]) } + let(:representation) { blob.representation(resize_to_limit: [100, 100]) } it "renders the preview from the representation route" do expect(html.find("figure.govuk-attachment img")[:src]) diff --git a/spec/dummy/app/controllers/examples_controller.rb b/spec/dummy/app/controllers/examples_controller.rb index 96ae873..ba5e1e9 100644 --- a/spec/dummy/app/controllers/examples_controller.rb +++ b/spec/dummy/app/controllers/examples_controller.rb @@ -25,12 +25,14 @@ class ExamplesController < ApplicationController :old_department_id, :old_department_description, :laptop, :other_language, :terms_and_conditions_agreed, :address_one, :address_two, :address_three, :postcode, :profile_photo, + :avatar, :cv, "date_of_birth(1i)", "date_of_birth(2i)", "date_of_birth(3i)", "graduation_month(1i)", "graduation_month(2i)", "graduation_month(3i)", "date_of_trade(1i)", "date_of_trade(2i)", "date_of_trade(3i)", "time_of_birth(1i)", "time_of_birth(2i)", "time_of_birth(3i)", "time_of_birth(4i)", "time_of_birth(5i)", "time_of_birth(6i)", - { department_ids: [], lunch_ids: [], wednesday_lunch_ids: [], languages: [], countries: [] } + { department_ids: [], lunch_ids: [], wednesday_lunch_ids: [], languages: [], countries: [], + gallery: [] } ].freeze def show diff --git a/spec/dummy/app/javascript/controllers/index.js b/spec/dummy/app/javascript/controllers/index.js index 72929fb..a4a3fc3 100644 --- a/spec/dummy/app/javascript/controllers/index.js +++ b/spec/dummy/app/javascript/controllers/index.js @@ -1,9 +1,12 @@ import { application } from "controllers/application"; -// Load the formbuilder stimulus controllers (image/document field previews). -import govuk from "@katalyst/govuk-formbuilder"; +// The README's primary wiring under test: register the gem's controllers +// (the attachment field's whole enhancement plus the legacy image/document +// controllers) on the app's Stimulus application, with session-durable +// page enhancement. +import GOVUK from "@katalyst/govuk-formbuilder"; -application.load(govuk); +GOVUK.start(application); import { eagerLoadControllersFrom } from "@hotwired/stimulus-loading"; eagerLoadControllersFrom("controllers", application); diff --git a/spec/dummy/app/views/examples/attachment/multiple.html.erb b/spec/dummy/app/views/examples/attachment/multiple.html.erb new file mode 100644 index 0000000..f6703ec --- /dev/null +++ b/spec/dummy/app/views/examples/attachment/multiple.html.erb @@ -0,0 +1,10 @@ +<%# locals: (profile:) %> +<%= turbo_frame_tag example_frame_id(params[:page], params[:example]) do %> + <%= form_with(model: profile, url: example_path(params[:page], params[:example])) do |f| %> + <%= f.govuk_image_field :gallery, + label: { text: "Gallery" }, + hint: { text: "Add photographs to the gallery, as many as you like" } %> + + <%= example_submit_buttons(f) %> + <% end %> +<% end %> diff --git a/spec/dummy/app/views/examples/attachment/single.html.erb b/spec/dummy/app/views/examples/attachment/single.html.erb new file mode 100644 index 0000000..ea18941 --- /dev/null +++ b/spec/dummy/app/views/examples/attachment/single.html.erb @@ -0,0 +1,10 @@ +<%# locals: (profile:) %> +<%= turbo_frame_tag example_frame_id(params[:page], params[:example]) do %> + <%= form_with(model: profile, url: example_path(params[:page], params[:example])) do |f| %> + <%= f.govuk_image_field :avatar, + label: { text: "Profile photo" }, + hint: { text: "Upload a clear colour photograph" } %> + + <%= example_submit_buttons(f) %> + <% end %> +<% end %> diff --git a/spec/dummy/app/views/guide/attachment.html.erb b/spec/dummy/app/views/guide/attachment.html.erb new file mode 100644 index 0000000..386ae02 --- /dev/null +++ b/spec/dummy/app/views/guide/attachment.html.erb @@ -0,0 +1,14 @@ +

Attachments

+ +

+ The attachment field is this gem's ActiveStorage extension — there is no + upstream page to mirror. Each example below is a rendered form loaded in its + own turbo frame: choose or drop files to see direct upload, previews, and + per-file keep/remove controls; submitting round-trips the selection as blob + signed ids. +

+ +<%= guide_example("attachment", "single", "Single image attachment") %> +<%= guide_example("attachment", "multiple", "Multiple image attachments") %> + +

<%= link_to("Back to guide", root_path) %>

diff --git a/spec/dummy/app/views/guide/index.html.erb b/spec/dummy/app/views/guide/index.html.erb index 13807fb..c422362 100644 --- a/spec/dummy/app/views/guide/index.html.erb +++ b/spec/dummy/app/views/guide/index.html.erb @@ -20,6 +20,11 @@
  • <%= link_to("Textarea", guide_page_path("textarea")) %>
  • +

    Extensions

    +
      +
    • <%= link_to("Attachments", guide_page_path("attachment")) %>
    • +
    +

    Building blocks

    • <%= link_to("Fieldsets", guide_page_path("fieldsets")) %>
    • diff --git a/spec/fixtures/files/banner.png b/spec/fixtures/files/banner.png new file mode 100644 index 0000000000000000000000000000000000000000..c5184a5658c5eb385aa6a55d08e900a389395b48 GIT binary patch literal 3818 zcmXw6c|6nqAKzv%_uOnPXCrdo>%cJNC^w^1o6JVs0@1Py{D)P;QDW zm1bv6f5(Ldjdq?1WTv?;rx=~On3+Kh5M zuXV(|yeH#ALwZDWi`8|-AW}pb~QXL0( zr5;5iGCVUXR}MCZxGt_1S?j9m+*432Rb}yj?Hw+8W$GC)TGv@|ES=^jIu|MfI{Iqo z@K*Q7Gki1C~7h_lk9EFnskj$ zArA};ja`IP9N|kPS_;&dm2XOX>#f!=`ax3I3bW*KlJvYa)oQUVGCaH-LA$4J7UE)a z$?I9#BZ`h=07Sdi4Dzy|yOEDkn#dBrY&24+hZa<;geD2`wcq5mbCtSR8%g=wFTKVzXOr`t658>TxWocbk|V8)dg79XQC zQjpba1%170Dmrm_fr*n|El_DW>l~b7^?t1uQVh0d3XCk=rIb>7ji~%OwuvB z>v=iO)G2pMXrw&Lq9Vlbf?bm7IPb;iaO%5{DzZ4C4|gyts$bKDZT|GMgYKngnRhuV zK(Tafs9v=(TVxg9J!6txk?tfTiJnt*?l0BAy%`=<5rN1Ldqbv$jrpfi4s?y zTT^O7QN!ylSnA#l1`wn!I#>~1vYZbB`zO-lPu~3pNex#IA-=gw^pGA^idg2cE}Q72 z9-&@HHihXWF`dSuh1&!bJwIftuX>QX1o_Ko{RRzYcj%$VT8hWvu!LXAGxXoGtUBr5 z>$v|Mx&(j))?E!2S&lCsAfm(>|G`yw5?u9Wg~~%a##7OwQ;k~H$H;6~Yeuh_2qN@t z(;!^M5L~PjTJs$LCECVf3IO;iN&Qy3ls(0?fF+p*h(+F;>O3%&!g@Q$x78*sNeoxW zYy-!lI1I3KTKvh3jeSJj_O>v&H;FojGtTQFJ0y3+qkLQendP9-iZkI}hN^Y}%m!0r zBZ1&{)_>Npq>7_39P$XR)z`*7yXmgZzwS+p9J;YPkE+2EU3DG%t4;8mM>+UR-v0g< z=JmMAWs}J~!o!5v@%JIw>%aG`$5c&=IL z7N?lP1d`&-?S*Ibr=LC^eSe9<~^$ZrPhEYHk;ySz^ISJRv-&E!$+%&h}hURIu2D|VN1^^uVrAC-5Blv*qJH!+Vx@RTS-mX%diFj}h6MKiPP zuU4{B9^h0e4sSTG!5|uq-`1U0`>SM=s1dRO;Y-oqhjJWqtMuXQ2PG)7#0C47d!?+E z_7bHQiW_d!)vtB#op|1?#z~E8SYZol=efVjMq~;n(_nd2rjuZxqPpJH31F6FO63h| zw*70OMUGC`JiYjL**#_qVoc3H#D7AH6J>a$Yge?Y%AOTLSpP`l0js+c^oQhg0K|gm zMxIX`w(E5K{1uIqjk2eQTXWHEp|o^?zQa&f23K|RNye6l&qV!NVypSU3%CJHhO zr?E@HD(3ueC>-G*!2dZA5?w=owz^=JuYaR;+i|*Z#W>#%$bx1HQ!(h5g58(gb1PE< zeZ42%eYZg?8x`*bQxfj++~D!9f-5pPv2|vSB=J`f2Jc4oR^Rv1$I-#>`K>zPhc`~D z#wL8)(!5%5V=vD86T)k^!j6*MLhiCh8@;nNk6QmwY_9l2iruS5gaw{!X-9r~<^I#L zkM{|i`<_M`CQo%e8dTm>&kQ=&I4u)7*x}`6QoJrJ@FFg^;y;U4Tuq#}C$Jws2|FC{ zW3OalFVx7m{qT4DEQT61)07H71mq@`E(ASu4z=+1oLg`Wx+~1gZ3B|r09|6lEr#u7 z;e(v`Ghvd`I`_Nh0u2wlzrJBVrGb4{nh^+=2iig70Ca!DmT?>W7 zqCjQi-ooPHy0xX+((gPf<1*^8F%oMWf*1;u@7Jp2Wi@W+cQLn?f<0c!Kl=x*6?^dK ziOo=s3tG**wSMctR-G?B^7YQSBZ(fCzS{LYc4sp+%woj%Fen1x%rC-hqCmxxV|V5R zgMxHpUCannc;imDW$jwVavW_r_+9q&e>>fsUDlF@zM3?m$IC&y_w-F86xlnagi`}C zB9Y1S=UuK4X}Z`Zf2m_uv62uOXu_izh38L z_Q1iJTuMO6cJ_E);+Kk4CmA<=k^a6Pbt|k^wuc^sV+vu87pu`v+~SXh`$j&}zAN+L zv~8fr%V2`{q;4Hy+-G&o*Q(^xo~=97L#S$Wm90mdV>Z5QCiPZLAiC%|GH=v7+l{Q{QQm4D&cm z$pJ6H&@8daF;p0ueesz8eFUq8GMcN$ZBW#f%Q76UB8dX^qbxdencbGXa;P_se&3k& z@c^G3a9(kh704PR&ELc!-s}1Ox-CTl$!q)ZMuP8q=ArfZ2j)JXF*hzhXN_TU-IU{Q5e}z~*+@_mP^p+( zJlpqF3=@V((@uWeYx|=m2Nqs!Y4}@NI-C+R~Bv9$-l81mo1L*Hyq<5;` zgN2-TR_dSqc2b+l6e**9pI$L3&K;TzZ&>Ti#( za!210V+EhxJBD5Zt*vY03@HBn-9k9$Y+_}VF}lU@r);0a0^DP$9<#Ay(B!qCu0cGR zp6&VOeD4bo?~6umSbb*$!S4togHjPn#^>hD`V9tNj4AawRk{PKE$hkBjXLy9)GxnW z?_hCo<8`sX*FH|nYmbhe^H4W}7EnF!0NXZKM}LqY7?5Xx)TJxsB~g}frBP4=M%5#d zgpTuRN`F2d*%~prO%hawr|7Z{11Dx;PJQT+-&@Df$3G0mX{?*l+zIFMx8(;MS~Gk` z?q#og&PPH@Zf+z+w+BY_{C;fT<(Q>LKGBzhYle_rG#CaHXtJo~wV8t$>B$lCi~$qy za>V&yVi3`9HXFTQLUmAE($}}W^X9|vqLI%L7R1owPw&@u@-PZcve2Ig+P}=+ST3T9 zkhO@GvI^~iYBiO0?mlRX)*_UC?ullZgr;57Ndez&y*s*R6P;+cN0_)ZCrQe@4uXD# zn?xaoBT6cK+&aCa<#K(x?{CV`GIaWb^KiaZQKZB9v~ done({ + box: this.clientWidth / this.clientHeight, + natural: this.naturalWidth / this.naturalHeight, + fit: getComputedStyle(this).objectFit, + }); + this.complete && this.naturalWidth + ? measure() + : this.addEventListener("load", measure, { once: true }); + JS + + expect(framing["box"]).to be_within(0.01).of(1) + expect(framing["natural"]).to be_within(0.01).of(3) + expect(framing["fit"]).to eq("contain") + end +end diff --git a/spec/system/attachment/round_trip_spec.rb b/spec/system/attachment/round_trip_spec.rb index bbcd814..a1b44de 100644 --- a/spec/system/attachment/round_trip_spec.rb +++ b/spec/system/attachment/round_trip_spec.rb @@ -3,7 +3,7 @@ require "rails_helper" # End-to-end round-trip of a direct-uploaded attachment across an invalid -# submit (E1/E2), exercised through the gallery (has_many_attached) field. +# submit, exercised through the gallery (has_many_attached) field. # # A file is direct-uploaded with JavaScript (its figure reaches # upload-successful, carrying the new blob's signed id), then the form is made diff --git a/spec/system/attachment/upload_spec.rb b/spec/system/attachment/upload_spec.rb index 849c76b..adf6416 100644 --- a/spec/system/attachment/upload_spec.rb +++ b/spec/system/attachment/upload_spec.rb @@ -111,7 +111,7 @@ # Both figure controls follow govuk-frontend's button conventions (the # password toggle's markup shape). - expect(figure).to have_css("button.govuk-button.govuk-button--secondary.govuk-attachment__retry") + expect(figure).to have_button(class: %w[govuk-button govuk-button--secondary govuk-attachment__retry]) click_button "Continue" diff --git a/spec/system/frontend_enhancements_spec.rb b/spec/system/frontend_enhancements_spec.rb index d1ccb97..2a9b982 100644 --- a/spec/system/frontend_enhancements_spec.rb +++ b/spec/system/frontend_enhancements_spec.rb @@ -48,4 +48,64 @@ expect(page).to have_button("Choose file") end end + + it "stays enhanced across a Turbo visit" do + visit guide_page_path("textarea") + + within("#textarea__max_chars_and_threshold") do + fill_in "profile[education_history]", with: "hello" + expect(page).to have_css(".govuk-character-count__status", text: "5 characters remaining", wait: 5) + end + + # A Turbo Drive visit replaces , discarding the body-scoped + # observers and guard; enhancement must re-establish on the new body + # regardless of whether the init snippet's call is a no-op. + click_link "Back to guide" + click_link "Password input" + + within("#password_input__default") do + toggle = find(".govuk-password-input__toggle", wait: 5) + expect(toggle).to be_visible + end + end + + it "starts from the default export, so bundle consumers need one import" do + visit guide_page_path("password_input") + + within("#password_input__default") do + find(".govuk-password-input__toggle", wait: 5) + end + + # Bundle consumers wire everything through the default export: + # GOVUK.start(application). Called here without an application (and + # after the page's own start), it must be a harmless no-op. + started = page.evaluate_async_script(<<~JS) + const done = arguments[0]; + import("@katalyst/govuk-formbuilder").then((m) => { + if (typeof m.default.start !== "function") return done(false); + m.default.start(); + done(true); + }); + JS + + expect(started).to be(true) + end + + it "tolerates repeated initAll calls without duplicating enhancements" do + visit guide_page_path("password_input") + + within("#password_input__default") do + find(".govuk-password-input__toggle", wait: 5) + end + + # The layout's init snippet already ran; further calls are no-ops. + page.evaluate_async_script(<<~JS) + const done = arguments[0]; + import("@katalyst/govuk-formbuilder").then((m) => { m.initAll(); m.initAll(); done(true); }); + JS + + within("#password_input__default") do + expect(page).to have_css(".govuk-password-input__toggle", count: 1) + end + end end diff --git a/spec/system/frontend_morph_spec.rb b/spec/system/frontend_morph_spec.rb index 5b277db..5bfe93b 100644 --- a/spec/system/frontend_morph_spec.rb +++ b/spec/system/frontend_morph_spec.rb @@ -2,15 +2,15 @@ require "rails_helper" -# The `govuk_formbuilder_init` snippet at the end of marks the page as -# JS-capable (body classes js-enabled / govuk-frontend-supported) and runs -# govuk-frontend's initAll. Turbo re-executes body scripts on a replace -# render, but a morph retains the live script node, so the snippet never -# re-runs: the incoming server body carries no class attribute, the morph -# strips the JS-set markers, and initAll — re-run from the dummy app's -# `turbo:render` handler — declines to initialise anything without the -# support marker, swallowing the error. These examples pin the recovery: -# support markers and component initialisation must survive morphs. +# The `govuk_formbuilder_init` snippet at the end of runs the bundle's +# initAll, which marks the page as JS-capable (body classes js-enabled / +# govuk-frontend-supported), enhances it, and observes . Turbo +# re-executes body scripts on a replace render, but a morph retains the live +# script node, so the snippet never re-runs: the incoming server body carries +# no class attribute, the morph strips the JS-set markers, and no lifecycle +# event fires. The bundle's marker-restore observer is the recovery — missing +# markers signal the morph; re-mark, then re-sweep. These examples pin that +# recovery: support markers and component initialisation must survive morphs. # # A failed profile update is the morph driver: the layout opts into # `turbo-refresh-method: morph`, so the 422 re-render morphs in place. The @@ -46,8 +46,8 @@ # Re-enhancement sweeps overlap by design — the marker-restore sweep and # the arriving-node sweep can visit the same root in one morph — so an # already-enhanced root must be skipped, not constructed-and-caught: - # initAll logs every catch, which would spam the console with InitErrors - # on every morph. + # the sweep logs every catch, which would spam the console with + # InitErrors on every morph. page.execute_script(<<~JS) window.__initErrors = []; const log = console.log.bind(console); From 2ba0ca3ebc0eee877103400588835b396d6326f5 Mon Sep 17 00:00:00 2001 From: Stephen Nelson Date: Mon, 3 Aug 2026 14:08:48 +0930 Subject: [PATCH 22/22] Attachments: design feedback --- README.md | 2 +- .../govuk/components/attachment/_mixin.scss | 4 ++- .../controllers/attachment_controller.js | 5 ++++ doc/attachment-field-spec.md | 21 +++++++++------ lib/katalyst/govuk/form_builder/config.rb | 2 +- .../form_builder_attachment_field_spec.rb | 2 +- spec/system/attachment/preview_spec.rb | 26 ++++++++++++++----- 7 files changed, 43 insertions(+), 19 deletions(-) diff --git a/README.md b/README.md index 668dd1f..a18f123 100644 --- a/README.md +++ b/README.md @@ -124,7 +124,7 @@ route. The transformation is configurable: ```ruby GOVUKDesignSystemFormBuilder.config.attachment_preview_representation = - { resize_to_limit: [100, 100] } # the default + { resize_to_fill: [256, 256] } # the default ``` ### Strings and internationalisation diff --git a/app/assets/stylesheets/katalyst/govuk/components/attachment/_mixin.scss b/app/assets/stylesheets/katalyst/govuk/components/attachment/_mixin.scss index 162c13f..177f9da 100644 --- a/app/assets/stylesheets/katalyst/govuk/components/attachment/_mixin.scss +++ b/app/assets/stylesheets/katalyst/govuk/components/attachment/_mixin.scss @@ -1,4 +1,5 @@ @use "govuk-frontend/dist/govuk/base"; +@use "govuk-frontend/dist/govuk/core/typography"; $attachment-background-colour: base.govuk-colour("black", $variant: "tint-95"); $attachment-border-width: 2px; @@ -19,13 +20,14 @@ $attachment-border-width: 2px; grid-area: preview; max-width: 4rem; aspect-ratio: 1/1; - object-fit: contain; + object-fit: cover; } .caption { display: flex; flex-direction: column; grid-area: caption; + @include base.govuk-font($size: 16, $line-height: 1.25); } .filename { diff --git a/app/javascript/katalyst/govuk/controllers/attachment_controller.js b/app/javascript/katalyst/govuk/controllers/attachment_controller.js index e910cdb..54bcee8 100644 --- a/app/javascript/katalyst/govuk/controllers/attachment_controller.js +++ b/app/javascript/katalyst/govuk/controllers/attachment_controller.js @@ -46,6 +46,11 @@ export default class AttachmentController extends Controller { if (!file) return; + if (!file.type.startsWith("image/")) { + this.imageTag?.remove(); + return; + } + const preview = new FileReader(); preview.onload = this.onPreviewReady; preview.readAsDataURL(file); diff --git a/doc/attachment-field-spec.md b/doc/attachment-field-spec.md index 259a06f..a1f412e 100644 --- a/doc/attachment-field-spec.md +++ b/doc/attachment-field-spec.md @@ -195,13 +195,15 @@ method) resolves the representation route with the same `main_app` fallback and returns nil when no route is available — the figure then renders without a preview — so engine builders can override preview resolution too. The preview transformation is configurable -(`config.attachment_preview_representation`); the default fits the image -within 100×100 preserving aspect, never upscaling -(`resize_to_limit`). Previews are contained, not cropped: the whole image -shows, letterboxed by CSS (`object-fit: contain` in a square preview box) -rather than by baked-in padding — variants carry no background bars, and -client-inserted previews get the same treatment so framing doesn't change -when a figure round-trips. +(`config.attachment_preview_representation`); the default is a crisp +square — `resize_to_fill: [256, 256]`, centre-cropped by vips, sized with +leeway above the preview box for dense displays. Framing is CSS-only: +previews render `object-fit: cover` in a square box, client-inserted +previews get the same treatment so framing doesn't change when a figure +round-trips, and a consumer stylesheet can reframe without touching the +gem — noting the default variant is already square-cropped, so a consumer +wanting whole-image (`contain`) previews overrides the representation +too. `multiple` is inferred from the attribute's ActiveStorage reflection (`has_many_attached` → true), and an explicit `multiple:` argument is @@ -238,7 +240,10 @@ Each criterion names the test type that verifies it. alone — identity, not state: the size and status spans stay visible (and inside the caption's atomic announcements) without entering any name. - **A7** Non-image blobs (e.g. PDF) render figure, caption and select without - an `img` and without error. + an `img` and without error — client-inserted figures for non-image files + likewise carry no preview `img`. The reserved preview space stays empty, + by decision: no placeholder, and a broken image from a failed lazy URL + is left as-is (§6's degradation). - **A8** The field renders exactly one blank hidden input (the keeper), before any figure's select — scalar and `has_many` alike (Rails' auto-blank is suppressed, so there is never a second blank). diff --git a/lib/katalyst/govuk/form_builder/config.rb b/lib/katalyst/govuk/form_builder/config.rb index 06fd8e3..764ec77 100644 --- a/lib/katalyst/govuk/form_builder/config.rb +++ b/lib/katalyst/govuk/form_builder/config.rb @@ -35,7 +35,7 @@ def attachment_preview_representation=(value) config.attachment_preview_representation = value end - config.attachment_preview_representation = { resize_to_limit: [100, 100] }.freeze + config.attachment_preview_representation = { resize_to_fill: [256, 256] }.freeze def use_legacy_file_fields? config.use_legacy_file_fields diff --git a/spec/builders/govuk_design_system_form_builder/form_builder_attachment_field_spec.rb b/spec/builders/govuk_design_system_form_builder/form_builder_attachment_field_spec.rb index 99bfb8d..146a933 100644 --- a/spec/builders/govuk_design_system_form_builder/form_builder_attachment_field_spec.rb +++ b/spec/builders/govuk_design_system_form_builder/form_builder_attachment_field_spec.rb @@ -525,7 +525,7 @@ def direct_upload_url # set, so an engine-mounted form must resolve the preview URL through # main_app — the same resolution direct_upload_url uses. describe "preview URL resolution" do - let(:representation) { blob.representation(resize_to_limit: [100, 100]) } + let(:representation) { blob.representation(resize_to_fill: [256, 256]) } it "renders the preview from the representation route" do expect(html.find("figure.govuk-attachment img")[:src]) diff --git a/spec/system/attachment/preview_spec.rb b/spec/system/attachment/preview_spec.rb index e9836f7..e1167f8 100644 --- a/spec/system/attachment/preview_spec.rb +++ b/spec/system/attachment/preview_spec.rb @@ -2,11 +2,11 @@ require "rails_helper" -# Previews are contained, never cropped or squashed: the preview box is square -# so figures line up down a list, and the image inside keeps its own aspect, -# letterboxed by CSS. Client-inserted previews carry the image's natural -# dimensions (a data URL of the chosen file), so they are the strict case — -# the server variant arrives pre-fitted. +# The preview box is square so figures line up down a list, and the image +# fills it (`object-fit: cover`): centre-cropped, never squashed. +# Client-inserted previews carry the image's natural dimensions (a data URL +# of the chosen file), so they are the strict case — the server variant +# arrives pre-cropped square (`resize_to_fill`). RSpec.describe "Attachment preview framing", :aggregate_failures do include AttachmentFieldHelpers include DirectUploadHelpers @@ -16,7 +16,7 @@ before { disable_direct_uploads } # banner.png is 3:1. - it "shows a non-square preview whole, undistorted" do + it "fills the square preview box from a non-square image, undistorted" do visit edit_profile_path(profile) choose_gallery_file("banner.png") @@ -37,6 +37,18 @@ expect(framing["box"]).to be_within(0.01).of(1) expect(framing["natural"]).to be_within(0.01).of(3) - expect(framing["fit"]).to eq("contain") + expect(framing["fit"]).to eq("cover") + end + + # Only image/* files can render as an img source. A non-image figure + # carries no preview img at all — the same shape the server renders for + # a non-representable blob. + it "renders a non-image figure without a preview img" do + visit edit_profile_path(profile) + + choose_cv_file("cv.pdf") + + expect(cv_field).to have_css("figure.govuk-attachment", count: 1) + expect(cv_field).to have_no_css("figure.govuk-attachment img") end end