From 211b6f6ce0ed4d6596ccee896493582cc9aff05f Mon Sep 17 00:00:00 2001 From: Peter Yates Date: Tue, 21 Mar 2023 18:43:40 +0000 Subject: [PATCH 1/2] Use the accordion id to prefix its section ids The GOV.UK Design System accordion component saves the state of open/closed accordions sections in localStorage, but it only saves the section's ID. If there are components on different pages that share IDs, the accordion could incorrectly render in the incorrect state (with the state of the _other_ matching accordion section on another page.) While ensuring global uniqueness is beyond the scope of this gem, we can increase the chances of uniqueness by incorporating the outer accordion's ID into each section ID. --- .../govuk_component/accordion_component.rb | 4 ++- .../accordion_component/section_component.rb | 11 +++++--- .../accordion_component_spec.rb | 28 +++++++++++++------ 3 files changed, 29 insertions(+), 14 deletions(-) diff --git a/app/components/govuk_component/accordion_component.rb b/app/components/govuk_component/accordion_component.rb index 5377ddf3..ab7532d4 100644 --- a/app/components/govuk_component/accordion_component.rb +++ b/app/components/govuk_component/accordion_component.rb @@ -7,6 +7,7 @@ class GovukComponent::AccordionComponent < GovukComponent::Base html_attributes: html_attributes, summary_text: summary_text, heading_text: heading_text, + accordion_id: id, &block ) end @@ -15,6 +16,7 @@ class GovukComponent::AccordionComponent < GovukComponent::Base def initialize(heading_level: 2, classes: [], html_attributes: {}) @heading_level = heading_tag(heading_level) + @id = html_attributes[:id] super(classes: classes, html_attributes: html_attributes) end @@ -22,7 +24,7 @@ def initialize(heading_level: 2, classes: [], html_attributes: {}) private def default_attributes - { class: %w(govuk-accordion), data: { module: 'govuk-accordion' } } + { id: id, class: %w(govuk-accordion), data: { module: 'govuk-accordion' } }.compact end def heading_tag(level) diff --git a/app/components/govuk_component/accordion_component/section_component.rb b/app/components/govuk_component/accordion_component/section_component.rb index a76cc122..d73d8e8f 100644 --- a/app/components/govuk_component/accordion_component/section_component.rb +++ b/app/components/govuk_component/accordion_component/section_component.rb @@ -1,26 +1,29 @@ class GovukComponent::AccordionComponent::SectionComponent < GovukComponent::Base - attr_reader :heading_text, :summary_text, :expanded, :heading_level + attr_reader :heading_text, :summary_text, :expanded, :heading_level, :accordion_id renders_one :heading_html renders_one :summary_html alias_method :expanded?, :expanded - def initialize(heading_text:, summary_text:, expanded:, heading_level:, classes: [], html_attributes: {}) + def initialize(heading_text:, summary_text:, expanded:, heading_level:, accordion_id: nil, classes: [], html_attributes: {}) @heading_text = heading_text @summary_text = summary_text @expanded = expanded @heading_level = heading_level + @accordion_id = accordion_id super(classes: classes, html_attributes: html_attributes) end def id(suffix: nil) + prefix = @accordion_id + # generate a random number if we don't have heading_text to avoid attempting # to parameterize a potentially-huge chunk of HTML - @prefix ||= heading_text&.parameterize || SecureRandom.hex(4) + @unique_identifier ||= heading_text&.parameterize || SecureRandom.hex(4) - [@prefix, suffix].compact.join('-') + [prefix, @unique_identifier, suffix].compact.join('-') end def heading_content diff --git a/spec/components/govuk_component/accordion_component_spec.rb b/spec/components/govuk_component/accordion_component_spec.rb index 663f6662..6e8deb0c 100644 --- a/spec/components/govuk_component/accordion_component_spec.rb +++ b/spec/components/govuk_component/accordion_component_spec.rb @@ -43,31 +43,41 @@ describe 'for each section' do specify 'the heading text and content is present' do sections.each do |heading_text, content| - expect(rendered_content).to have_tag('div', with: { class: 'govuk-accordion__section', id: %(#{heading_text.parameterize}-section) }) do + expect(rendered_content).to have_tag('div', with: { class: 'govuk-accordion__section', id: %(#{id}-#{heading_text.parameterize}-section) }) do with_tag('h2', class: 'govuk-accordion__section-heading') do - with_tag('span', text: heading_text, with: { id: heading_text.parameterize, class: 'govuk-accordion__section-button' }) + with_tag('span', text: heading_text, with: { id: %(#{id}-#{heading_text.parameterize}), class: 'govuk-accordion__section-button' }) end - with_tag('div', with: { id: %(#{heading_text.parameterize}-content), class: 'govuk-accordion__section-content' }, text: content) + with_tag('div', with: { id: %(#{id}-#{heading_text.parameterize}-content), class: 'govuk-accordion__section-content' }, text: content) end end end specify 'each section ID matches the content aria-labelledby' do sections.each_key do |heading_text| - id = heading_text.parameterize + expected_id = %(#{id}-#{heading_text.parameterize}) - expect(rendered_content).to have_tag('span', with: { id: id, class: 'govuk-accordion__section-button' }) - expect(rendered_content).to have_tag('div', with: { 'aria-labelledby' => id }) + expect(rendered_content).to have_tag('span', with: { id: expected_id, class: 'govuk-accordion__section-button' }) + expect(rendered_content).to have_tag('div', with: { 'aria-labelledby' => expected_id }) end end specify 'each section ID matches the button aria-controls' do sections.each_key do |heading_text| - id = heading_text.parameterize + expected_id = %(#{id}-#{heading_text.parameterize}-content) - expect(rendered_content).to have_tag('div', with: { id: %(#{id}-content) }) - expect(rendered_content).to have_tag('span', with: { 'aria-controls' => %(#{id}-content) }) + expect(rendered_content).to have_tag('div', with: { id: expected_id }) + expect(rendered_content).to have_tag('span', with: { 'aria-controls' => expected_id }) + end + end + end + + describe 'building unique section ids' do + context 'when the accordion has an ID' do + let(:kwargs) { { html_attributes: { id: id } } } + + specify 'the accordion id is prefixes the section id' do + expect(html.css('.govuk-accordion__section').map { |e| e[:id] }).to all(start_with(id)) end end end From b2e1928260ac02979233fd008e3dfc199b96f56a Mon Sep 17 00:00:00 2001 From: Peter Yates Date: Wed, 22 Mar 2023 10:48:48 +0000 Subject: [PATCH 2/2] Remove id from default attrs and improve naming --- app/components/govuk_component/accordion_component.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/app/components/govuk_component/accordion_component.rb b/app/components/govuk_component/accordion_component.rb index ab7532d4..536467e9 100644 --- a/app/components/govuk_component/accordion_component.rb +++ b/app/components/govuk_component/accordion_component.rb @@ -7,16 +7,16 @@ class GovukComponent::AccordionComponent < GovukComponent::Base html_attributes: html_attributes, summary_text: summary_text, heading_text: heading_text, - accordion_id: id, + accordion_id: accordion_id, &block ) end - attr_reader :id, :heading_level + attr_reader :accordion_id, :heading_level def initialize(heading_level: 2, classes: [], html_attributes: {}) @heading_level = heading_tag(heading_level) - @id = html_attributes[:id] + @accordion_id = html_attributes[:id] super(classes: classes, html_attributes: html_attributes) end @@ -24,7 +24,7 @@ def initialize(heading_level: 2, classes: [], html_attributes: {}) private def default_attributes - { id: id, class: %w(govuk-accordion), data: { module: 'govuk-accordion' } }.compact + { class: %w(govuk-accordion), data: { module: 'govuk-accordion' } }.compact end def heading_tag(level)