From 3b1976a5aae58a3ef6ef2f115265415cd6828371 Mon Sep 17 00:00:00 2001 From: ulferts Date: Thu, 30 Jun 2016 20:00:00 +0200 Subject: [PATCH] Fix/accessible pre and suffix (#4601) * accessible form pre- and suffix Both pre- and suffix are attributes with aria-hidden="true". This should lead to them no longer being read out of context. In order to have them read when appropriate, the prefix is turned into a part of the label which is hidden for sighted users. The suffix is referenced in the form element via aria-describedby. * extract method refactoring --- app/helpers/settings_helper.rb | 18 +++++-- lib/tabular_form_builder.rb | 73 ++++++++++++++++++------- spec/lib/tabular_form_builder_spec.rb | 77 ++++++++++++++++++++++----- 3 files changed, 133 insertions(+), 35 deletions(-) diff --git a/app/helpers/settings_helper.rb b/app/helpers/settings_helper.rb index 385cb78cd7..3ecb811018 100644 --- a/app/helpers/settings_helper.rb +++ b/app/helpers/settings_helper.rb @@ -27,6 +27,8 @@ # See doc/COPYRIGHT.rdoc for more details. #++ +require 'securerandom' + module SettingsHelper include OpenProject::FormTagHelper @@ -56,9 +58,6 @@ module SettingsHelper end def setting_multiselect(setting, choices, options = {}) - setting_values = Setting.send(setting) - setting_values = [] unless setting_values.is_a?(Array) - setting_label(setting, options) + content_tag(:span, class: 'form--field-container -vertical') { hidden_field_tag("settings[#{setting}][]", '') + @@ -83,11 +82,22 @@ module SettingsHelper def setting_text_field(setting, options = {}) unit = options.delete(:unit) + unit_html = '' + + if unit + unit_id = SecureRandom.uuid + options[:'aria-describedby'] = unit_id + unit_html = content_tag(:span, + unit, + class: 'form--field-affix', + :'aria-hidden' => true, + id: unit_id) + end setting_label(setting, options) + wrap_field_outer(options) { styled_text_field_tag("settings[#{setting}]", Setting.send(setting), options) + - (unit ? content_tag(:span, unit, class: 'form--field-affix') : '') + unit_html } end diff --git a/lib/tabular_form_builder.rb b/lib/tabular_form_builder.rb index 6077ad84f6..b817798325 100644 --- a/lib/tabular_form_builder.rb +++ b/lib/tabular_form_builder.rb @@ -28,6 +28,7 @@ #++ require 'action_view/helpers/form_helper' +require 'securerandom' class TabularFormBuilder < ActionView::Helpers::FormBuilder include Redmine::I18n @@ -119,16 +120,28 @@ class TabularFormBuilder < ActionView::Helpers::FormBuilder prefix, suffix = options.values_at(:prefix, :suffix) - # TODO (Rails 4): switch to SafeBuffer#prepend - ret = content_tag(:span, prefix.html_safe, class: 'form--field-affix').concat(ret) if prefix - ret.concat content_tag(:span, suffix.html_safe, class: 'form--field-affix') if suffix + if prefix + ret.prepend content_tag(:span, + prefix.html_safe, + class: 'form--field-affix', + id: options[:prefix_id], + :'aria-hidden' => true) + end + + if suffix + ret.concat content_tag(:span, + suffix.html_safe, + class: 'form--field-affix', + id: options[:suffix_id], + :'aria-hidden' => true) + end field_container_wrap_field(ret, options) end - def merge_required_attributes(required, options=nil) + def merge_required_attributes(required, options = nil) if required - options.merge!({ required: true, :'aria-required' => 'true' }) + options.merge!(required: true, :'aria-required' => 'true') end end @@ -184,41 +197,56 @@ class TabularFormBuilder < ActionView::Helpers::FormBuilder # Returns a label tag for the given field def label_for_field(field, options = {}, translation_form = nil) - options = options.dup - return ''.html_safe if options.delete(:no_label) + return ''.html_safe if options[:no_label] text = get_localized_field(field, options[:label]) - label_options = { class: '', title: text } - id = element_id(translation_form) if translation_form + label_options = { class: 'form--label', title: text } - label_options[:class] << 'form--label' + content = h(text) + label_for_field_errors(content, label_options, field) + label_for_field_required(content, label_options, options[:required]) + label_for_field_for(options, label_options, translation_form, field) + label_for_field_prefix(content, options) + label_options[:lang] = options[:lang] + label_options.reject! do |_k, v| v.nil? end - content = h(text) + @template.label(@object_name, field, content, label_options) + end + + def label_for_field_errors(content, options, field) if @object.try(:errors) && @object.errors.include?(field) - label_options[:class] << ' -error' + options[:class] << ' -error' error_label = I18n.t('errors.field_erroneous_label', full_errors: @object.errors.full_messages_for(field).join(' ')) - content << content_tag('p', error_label, class: 'hidden-for-sighted') + content << content_tag('p', error_label, class: 'hidden-for-sighted') end + end - if options.delete(:required) - label_options[:class] << ' -required' + def label_for_field_required(content, options, is_required) + if is_required + options[:class] << ' -required' content << content_tag('span', '*', class: 'form--label-required', :'aria-hidden' => true) end + end + + def label_for_field_for(options, label_options, translation_form, field) + id = element_id(translation_form) if translation_form label_options[:for] = if options[:for] options[:for] elsif options[:multi_locale] && id id.sub(/\_id$/, "_#{field}") end - label_options[:lang] = options[:lang] - label_options.reject! do |_k, v| v.nil? end + end - @template.label(@object_name, field, content, label_options) + def label_for_field_prefix(content, options) + if options[:prefix] + content << content_tag(:span, options[:prefix].html_safe, class: 'hidden-for-sighted') + end end def get_localized_field(field, label) @@ -324,6 +352,15 @@ class TabularFormBuilder < ActionView::Helpers::FormBuilder label_options = options.dup input_options = options.dup.except(:for, :label, :no_label, :prefix, :suffix) + if options[:suffix] + options[:suffix_id] ||= SecureRandom.uuid + + input_options[:'aria-describedby'] ||= options[:suffix_id] + end + if options[:prefix] + options[:prefix_id] ||= SecureRandom.uuid + end + [input_options, label_options] end diff --git a/spec/lib/tabular_form_builder_spec.rb b/spec/lib/tabular_form_builder_spec.rb index dfe2f5287f..1774cca9d6 100644 --- a/spec/lib/tabular_form_builder_spec.rb +++ b/spec/lib/tabular_form_builder_spec.rb @@ -41,7 +41,7 @@ describe TabularFormBuilder do mail: 'jj@lost-mail.com', failed_login_count: 45) } - let(:builder) { TabularFormBuilder.new(:user, resource, helper, {}) } + let(:builder) { TabularFormBuilder.new(:user, resource, helper, {}) } describe '#text_field' do let(:options) { { title: 'Name', class: 'custom-class' } } @@ -112,19 +112,42 @@ describe TabularFormBuilder do end context 'with affixes' do + let(:random_id) { 'random_id' } + + before do + allow(SecureRandom) + .to receive(:uuid) + .and_return(random_id) + end + context 'with a prefix' do let(:options) { { title: 'Name', prefix: %{Prefix} } } it 'should output elements' do expect(output).to be_html_eql(%{ - Prefix + + id="user_name" + name="user[name]" + title="Name" + type="text" + value="JJ Abrams" /> }).within_path('span.form--field-container') end + + it 'includes the prefix hidden in the label' do + expect(output).to be_html_eql(%{ + + Prefix + + }).within_path('label.form--label') + end end context 'with a suffix' do @@ -134,10 +157,18 @@ describe TabularFormBuilder do expect(output).to be_html_eql(%{ + id="user_name" + name="user[name]" + title="Name" + type="text" + aria-describedby="#{random_id}" + value="JJ Abrams" /> + + - Suffix }).within_path('span.form--field-container') end end @@ -148,19 +179,40 @@ describe TabularFormBuilder do title: 'Name', prefix: %{PREFIX}, suffix: %{SUFFIX} - } } + } + } it 'should output elements' do expect(output).to be_html_eql(%{ - PREFIX + + id="user_name" + name="user[name]" + title="Name" + type="text" + aria-describedby="#{random_id}" + value="JJ Abrams" /> + + - SUFFIX }).within_path('span.form--field-container') end + + it 'includes the prefix hidden in the label' do + expect(output).to be_html_eql(%{ + + PREFIX + + }).within_path('label.form--label') + end end end end @@ -561,7 +613,6 @@ JJ Abrams }).at_path('label') end - context 'with a label specified as string' do let(:text) { 'My own label' }