From cdc03602edde1ff35dccfbe8402db7c7a7081771 Mon Sep 17 00:00:00 2001 From: Simon Gow Date: Fri, 19 May 2017 15:47:51 +1200 Subject: [PATCH] CheckboxField creates invalid HTML when required #2939 - Updated CheckboxField, CheckboxSetField, DropdownField, OptionsetField to validate with HTML5 attributes & aria-required. https://www.w3.org/TR/wai-aria/states_and_properties#aria-required --- src/Forms/CheckboxField.php | 12 +++++++++--- src/Forms/CheckboxSetField.php | 14 ++++++++++++++ src/Forms/DropdownField.php | 11 +++++++++++ src/Forms/OptionsetField.php | 8 +++++--- tests/php/Forms/CheckboxFieldTest.php | 20 ++++++++++++++++++++ tests/php/Forms/CheckboxSetFieldTest.php | 19 +++++++++++++++++++ tests/php/Forms/DropdownFieldTest.php | 16 ++++++++++++++++ tests/php/Forms/OptionsetFieldTest.php | 20 ++++++++++++++++++++ 8 files changed, 114 insertions(+), 6 deletions(-) diff --git a/src/Forms/CheckboxField.php b/src/Forms/CheckboxField.php index 5050f5a79..a7a1a937c 100644 --- a/src/Forms/CheckboxField.php +++ b/src/Forms/CheckboxField.php @@ -28,10 +28,16 @@ class CheckboxField extends FormField public function getAttributes() { - $attrs = parent::getAttributes(); - $attrs['value'] = 1; + $attributes = parent::getAttributes(); + $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', diff --git a/src/Forms/CheckboxSetField.php b/src/Forms/CheckboxSetField.php index 1e27d7a05..764e3659e 100644 --- a/src/Forms/CheckboxSetField.php +++ b/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; + } } diff --git a/src/Forms/DropdownField.php b/src/Forms/DropdownField.php index 51f212bc8..b7f29c41f 100644 --- a/src/Forms/DropdownField.php +++ b/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 diff --git a/src/Forms/OptionsetField.php b/src/Forms/OptionsetField.php index 25bd2973f..3587a6b64 100644 --- a/src/Forms/OptionsetField.php +++ b/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; } } diff --git a/tests/php/Forms/CheckboxFieldTest.php b/tests/php/Forms/CheckboxFieldTest.php index 5cb2f8bfb..70375afee 100644 --- a/tests/php/Forms/CheckboxFieldTest.php +++ b/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)); + } } diff --git a/tests/php/Forms/CheckboxSetFieldTest.php b/tests/php/Forms/CheckboxSetFieldTest.php index 953963bd4..0c2ca5b29 100644 --- a/tests/php/Forms/CheckboxSetFieldTest.php +++ b/tests/php/Forms/CheckboxSetFieldTest.php @@ -370,4 +370,23 @@ class CheckboxSetFieldTest extends SapphireTest $this->assertContains('<firstname>', $fieldHTML); $this->assertNotContains('', $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)); + } } diff --git a/tests/php/Forms/DropdownFieldTest.php b/tests/php/Forms/DropdownFieldTest.php index 622141778..d2bf202c3 100644 --- a/tests/php/Forms/DropdownFieldTest.php +++ b/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()); + } } diff --git a/tests/php/Forms/OptionsetFieldTest.php b/tests/php/Forms/OptionsetFieldTest.php index 98914013a..08911b60f 100644 --- a/tests/php/Forms/OptionsetFieldTest.php +++ b/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)); + } }