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

@@ -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
 

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
 

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: %{<span style="color:red">Prefix</span>} } }
 
         it 'should output elements' do
           expect(output).to be_html_eql(%{
-            <span class="form--field-affix"><span style="color:red">Prefix</span></span>
+            <span class="form--field-affix"
+                  id="#{random_id}"
+                  aria-hidden="true">
+              <span style="color:red">Prefix</span>
+            </span>
             <span class="form--text-field-container">
               <input class="form--text-field"
-                id="user_name" name="user[name]" title="Name" type="text"
-                value="JJ Abrams" />
+                     id="user_name"
+                     name="user[name]"
+                     title="Name"
+                     type="text"
+                     value="JJ Abrams" />
             </span>
           }).within_path('span.form--field-container')
         end
+
+        it 'includes the prefix hidden in the label' do
+          expect(output).to be_html_eql(%{
+            <span class="hidden-for-sighted">
+              <span style="color:red">Prefix</span>
+            </span>
+          }).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(%{
             <span class="form--text-field-container">
               <input class="form--text-field"
-                id="user_name" name="user[name]" title="Name" type="text"
-                value="JJ Abrams" />
+                     id="user_name"
+                     name="user[name]"
+                     title="Name"
+                     type="text"
+                     aria-describedby="#{random_id}"
+                     value="JJ Abrams" />
+            </span>
+            <span class="form--field-affix"
+                  aria-hidden="true"
+                  id="#{random_id}">
+              <span style="color:blue">Suffix</span>
             </span>
-            <span class="form--field-affix"><span style="color:blue">Suffix</span></span>
           }).within_path('span.form--field-container')
         end
       end
@@ -148,19 +179,40 @@ describe TabularFormBuilder do
             title: 'Name',
             prefix: %{<span style="color:yellow">PREFIX</span>},
             suffix: %{<span style="color:green">SUFFIX</span>}
-          } }
+          }
+        }
 
         it 'should output elements' do
           expect(output).to be_html_eql(%{
-            <span class="form--field-affix"><span style="color:yellow">PREFIX</span></span>
+            <span class="form--field-affix"
+                  id="#{random_id}"
+                  aria-hidden="true">
+              <span style="color:yellow">PREFIX</span>
+            </span>
             <span class="form--text-field-container">
               <input class="form--text-field"
-                id="user_name" name="user[name]" title="Name" type="text"
-                value="JJ Abrams" />
+                     id="user_name"
+                     name="user[name]"
+                     title="Name"
+                     type="text"
+                     aria-describedby="#{random_id}"
+                     value="JJ Abrams" />
+            </span>
+            <span class="form--field-affix"
+                  aria-hidden="true"
+                  id="#{random_id}">
+              <span style="color:green">SUFFIX</span>
             </span>
-            <span class="form--field-affix"><span style="color:green">SUFFIX</span></span>
           }).within_path('span.form--field-container')
         end
+
+        it 'includes the prefix hidden in the label' do
+          expect(output).to be_html_eql(%{
+            <span class="hidden-for-sighted">
+              <span style="color:yellow">PREFIX</span>
+            </span>
+          }).within_path('label.form--label')
+        end
       end
     end
   end
@@ -561,7 +613,6 @@ JJ Abrams</textarea>
         }).at_path('label')
       end
 
-
       context 'with a label specified as string' do
         let(:text) { 'My own label' }