Merge pull request #6937 from caffeineinc/2930-checkboxfield-invalid-html

CheckboxField creates invalid HTML when required #2939

src/Forms/CheckboxField.php

@@ -28,10 +28,16 @@ class CheckboxField extends FormField
 
     public function getAttributes()
     {
-        $attrs = parent::getAttributes();
+        $attributes = parent::getAttributes();
-        $attrs['value'] = 1;
+        $attributes['value'] = 1;
+        if ($this->Required()) {
+            // Semantically, it doesn't make sense to have a required attribute
+            // on a field in which both checked and unchecked are allowable.
+            unset($attributes['aria-required']);
+        }
+
         return array_merge(
-            $attrs,
+            $attributes,
             array(
                 'checked' => ($this->Value()) ? 'checked' : null,
                 'type' => 'checkbox',

src/Forms/CheckboxSetField.php

@@ -104,4 +104,18 @@ class CheckboxSetField extends MultiSelectField
     {
         return 'optionset checkboxset';
     }
+
+    public function getAttributes()
+    {
+        $attributes = array_merge(
+            parent::getAttributes(),
+            array('role' => 'listbox')
+        );
+
+        // Remove invalid attributes from wrapper.
+        unset($attributes['name']);
+        unset($attributes['required']);
+        unset($attributes['aria-required']);
+        return $attributes;
+    }
 }

src/Forms/DropdownField.php

@@ -113,6 +113,17 @@ class DropdownField extends SingleSelectField
         ));
     }
 
+    /**
+     * A required DropdownField must have a user selected attribute,
+     * so require an empty default for a required field
+     *
+     * @return bool
+     */
+    public function getHasEmptyDefault()
+    {
+        return parent::getHasEmptyDefault() || $this->Required();
+    }
+
     /**
      * @param array $properties
      * @return string

src/Forms/OptionsetField.php

@@ -148,11 +148,13 @@ class OptionsetField extends SingleSelectField
 
     public function getAttributes()
     {
-        $attributes = parent::getAttributes();
+        $attributes = array_merge(
+            parent::getAttributes(),
+            array('role' => 'listbox')
+        );
+
         unset($attributes['name']);
         unset($attributes['required']);
-        unset($attributes['role']);
-
         return $attributes;
     }
 }

tests/php/Forms/CheckboxFieldTest.php

@@ -2,6 +2,9 @@
 
 namespace SilverStripe\Forms\Tests;
 
+use SilverStripe\Control\Controller;
+use SilverStripe\Forms\FieldList;
+use SilverStripe\Forms\Form;
 use SilverStripe\Forms\Tests\CheckboxFieldtest\Article;
 use SilverStripe\ORM\DB;
 use SilverStripe\Dev\SapphireTest;
@@ -185,4 +188,21 @@ class CheckboxFieldTest extends SapphireTest
             'Field correct validates null as allowed'
         );
     }
+
+    /**
+     * #2939 CheckboxField creates invalid HTML when required
+     */
+    public function testNoAriaRequired()
+    {
+        $field = new CheckboxField('RequiredField', 'myRequiredField');
+
+        $form = new Form(
+            Controller::curr(), "form", new FieldList($field), new FieldList(),
+            new RequiredFields(["RequiredField"])
+        );
+        $this->assertTrue($field->Required());
+
+        $attributes = $field->getAttributes();
+        $this->assertFalse(array_key_exists("aria-required", $attributes));
+    }
 }

tests/php/Forms/CheckboxSetFieldTest.php

@@ -370,4 +370,23 @@ class CheckboxSetFieldTest extends SapphireTest
         $this->assertContains('<firstname>', $fieldHTML);
         $this->assertNotContains('<firstname>', $fieldHTML);
     }
+
+    /**
+     * #2939 CheckboxSetField creates invalid HTML when required
+     */
+    public function testNoAriaRequired()
+    {
+        $field = new CheckboxSetField('RequiredField', 'myRequiredField');
+
+        $form = new Form(
+            Controller::curr(), "form", new FieldList($field), new FieldList(),
+            new RequiredFields(["RequiredField"])
+        );
+        $this->assertTrue($field->Required());
+
+        $attributes = $field->getAttributes();
+        $this->assertFalse(array_key_exists("aria-required", $attributes));
+        $this->assertFalse(array_key_exists("name", $attributes));
+        $this->assertFalse(array_key_exists("required", $attributes));
+    }
 }

tests/php/Forms/DropdownFieldTest.php

@@ -2,6 +2,7 @@
 
 namespace SilverStripe\Forms\Tests;
 
+use SilverStripe\Control\Controller;
 use SilverStripe\ORM\ArrayList;
 use SilverStripe\Dev\CSSContentParser;
 use SilverStripe\Dev\SapphireTest;
@@ -498,4 +499,19 @@ class DropdownFieldTest extends SapphireTest
         $field->setValue('Five');
         $this->assertFalse($field->validate($validator));
     }
+
+    /**
+     * #2939 DropdownField creates invalid HTML when required
+     */
+    public function testRequiredDropdownHasEmptyDefault()
+    {
+        $field = new \SilverStripe\Forms\DropdownField("RequiredField", "dropdown", ["item 1", "item 2"]);
+
+        $form = new Form(
+            Controller::curr(), "form", new FieldList($field), new FieldList(),
+            new RequiredFields(["RequiredField"])
+        );
+
+        $this->assertTrue($field->getHasEmptyDefault());
+    }
 }

tests/php/Forms/OptionsetFieldTest.php

@@ -2,6 +2,7 @@
 
 namespace SilverStripe\Forms\Tests;
 
+use SilverStripe\Control\Controller;
 use SilverStripe\ORM\FieldType\DBField;
 use SilverStripe\Dev\CSSContentParser;
 use SilverStripe\Dev\SapphireTest;
@@ -100,4 +101,23 @@ class OptionsetFieldTest extends SapphireTest
         $this->assertContains('Four & Five & Six', $fieldHTML);
         $this->assertNotContains('Four & Five & Six', $fieldHTML);
     }
+
+    /**
+     * #2939 OptionSetField creates invalid HTML when required
+     */
+    public function testNoAriaRequired()
+    {
+        $field = new OptionsetField('RequiredField', 'myRequiredField');
+
+        $form = new Form(
+            Controller::curr(), "form", new FieldList($field), new FieldList(),
+            new RequiredFields(["RequiredField"])
+        );
+        $this->assertTrue($field->Required());
+
+        $attributes = $field->getAttributes();
+        $this->assertFalse(array_key_exists("name", $attributes));
+        $this->assertFalse(array_key_exists("required", $attributes));
+        $this->assertTrue(array_key_exists("role", $attributes));
+    }
 }