From ca87b8b79496a78454d63c225f1f4f38fa55a150 Mon Sep 17 00:00:00 2001 From: Will Rossiter Date: Sun, 26 May 2013 11:09:03 +1200 Subject: [PATCH] API: Form Field ID attribute should follow HTML specification Fixes: http://open.silverstripe.org/ticket/4431. Changes Form and Form Field classes to make use of Convert::raw2htmlid() which follows http://www.w3.org/TR/REC-html40/types.html#type-cdata. Introduces a FormTemplateHelper class to assist in these sort of updates in the future. --- docs/en/changelogs/rc/3.2.0.md | 76 +++++++++++++++++ forms/Form.php | 99 +++++++++++++++++----- forms/FormField.php | 37 +++++++-- forms/FormTemplateHelper.php | 122 ++++++++++++++++++++++++++++ templates/forms/FormField_holder.ss | 2 +- tests/forms/FieldListTest.php | 41 ++++++---- tests/forms/FormTest.php | 20 ++++- 7 files changed, 354 insertions(+), 43 deletions(-) create mode 100644 forms/FormTemplateHelper.php diff --git a/docs/en/changelogs/rc/3.2.0.md b/docs/en/changelogs/rc/3.2.0.md index 44698bbed..afb542352 100644 --- a/docs/en/changelogs/rc/3.2.0.md +++ b/docs/en/changelogs/rc/3.2.0.md @@ -14,6 +14,7 @@ Otherwise, you'll need to include the module yourself * API: Removed URL routing by controller name * Security: The multiple authenticator login page should now be styled manually - i.e. without the default jQuery UI layout. A new template, Security_MultiAuthenticatorLogin.ss is available. * Security: This controller's templates can be customised by overriding the `getTemplate` function. + * API: Form and FormField ID attributes rewritten. ## Details @@ -63,3 +64,78 @@ you can reinstate the old behaviour through a director rule: Director: rules: '$Controller//$Action/$ID/$OtherID': '*' + +### API: Default Form and FormField ID attributes rewritten. + +Previously the automatic generation of ID attributes throughout the Form API +could generate invalid ID values such as Password[ConfirmedPassword] as well +as duplicate ID values between forms on the same page. For example, if you +created a field called `Email` on more than one form on the page, the resulting +HTML would have multiple instances of `#Email`. ID should be a unique +identifier for a single element within the document. + +This rewrite has several angles, each of which is described below. If you rely +on ID values in your CSS files, Javascript code or application unit tests *you +will need to update your code*. + +#### Conversion of invalid form ID values + +ID attributes on Form and Form Fields will now follow the +[HTML specification](http://www.w3.org/TR/REC-html40/types.html#type-cdata). +Generating ID attributes is now handled by the new `FormTemplateHelper` class. + +Please test each of your existing site forms to ensure that they work +correctly in particular, javascript and css styles which rely on specific ID +values. + +#### Invalid ID attributes stripped + +ID attributes will now be run through `Convert::raw2htmlid`. Invalid characters +are replaced with a single underscore character. Duplicate, leading and trailing +underscores are removed. Custom ID attributes (set through `setHTMLID`) will not +be altered. + + Before: +
+ + Now: + +
+ +#### Namespaced FormField ID's + +Form Field ID values will now be namespaced with the parent form ID. + + Before: +
+ + Now: +
+ +#### FormField wrapper containers suffixed with `_Holder` + +Previously both the container div and FormField tag shared the same ID in +certain cases. Now, the wrapper div in the default `FormField` template will be +suffixed with `_Holder`. + + Before: +
+ + + After: +
+ +#### Reverting to the old specification + +If upgrading existing forms is not feasible, developers can opt out of the new +specifications by using the `FormTemplateHelper_Pre32` class rules instead of +the default ones. + + :::yaml + # mysite/config/_config.yml + + Injector: + FormTemplateHelper: + class: FormTemplateHelper_Pre32 diff --git a/forms/Form.php b/forms/Form.php index 19fabdddc..32679b16e 100644 --- a/forms/Form.php +++ b/forms/Form.php @@ -149,6 +149,21 @@ class Form extends RequestHandler { */ protected $attributes = array(); + /** + * @var FormTemplateHelper + */ + private $templateHelper = null; + + /** + * @ignore + */ + private $htmlID = null; + + /** + * @ignore + */ + private $formActionPath = false; + /** * Create a new form, with the given fields an action buttons. * @@ -642,13 +657,14 @@ class Form extends RequestHandler { public function getAttributes() { $attrs = array( - 'id' => $this->FormName(), + 'id' => $this->getTemplateHelper()->generateFormID($this), 'action' => $this->FormAction(), 'method' => $this->FormMethod(), 'enctype' => $this->getEncType(), 'target' => $this->target, 'class' => $this->extraClass(), ); + if($this->validator && $this->validator->getErrors()) { if(!isset($attrs['class'])) $attrs['class'] = ''; $attrs['class'] .= ' validationerror'; @@ -671,6 +687,7 @@ class Form extends RequestHandler { if(!$attrs || is_string($attrs)) $attrs = $this->getAttributes(); + // Figure out if we can cache this form // - forms with validation shouldn't be cached, cos their error messages won't be shown // - forms with security tokens shouldn't be cached because security tokens expire @@ -706,13 +723,43 @@ class Form extends RequestHandler { } /** - * Set the target of this form to any value - useful for opening the form contents in a new window or refreshing - * another frame - * - * @param target The value of the target - */ + * Set the {@link FormTemplateHelper} + * + * @param string|FormTemplateHelper + */ + public function setTemplateHelper($helper) { + $this->templateHelper = $helper; + } + + /** + * Return a {@link FormTemplateHelper} for this form. If one has not been + * set, return the default helper. + * + * @return FormTemplateHelper + */ + public function getTemplateHelper() { + if($this->templateHelper) { + if(is_string($this->templateHelper)) { + return Injector::inst()->get($this->templateHelper); + } + + return $this->templateHelper; + } + + return Injector::inst()->get('FormTemplateHelper'); + } + + /** + * Set the target of this form to any value - useful for opening the form + * contents in a new window or refreshing another frame. + * + * @param target $target The value of the target + * + * @return FormField + */ public function setTarget($target) { $this->target = $target; + return $this; } @@ -862,38 +909,50 @@ class Form extends RequestHandler { } } - /** @ignore */ - private $formActionPath = false; - /** * Set the form action attribute to a custom URL. * - * Note: For "normal" forms, you shouldn't need to use this method. It is recommended only for situations where - * you have two relatively distinct parts of the system trying to communicate via a form post. + * Note: For "normal" forms, you shouldn't need to use this method. It is + * recommended only for situations where you have two relatively distinct + * parts of the system trying to communicate via a form post. */ public function setFormAction($path) { $this->formActionPath = $path; + return $this; } /** - * @ignore - */ - private $htmlID = null; - - /** - * Returns the name of the form + * Returns the name of the form. + * + * @return string */ public function FormName() { - if($this->htmlID) return $this->htmlID; - else return $this->class . '_' . str_replace(array('.', '/'), '', $this->name); + if($this->htmlID) { + return $this->htmlID; + } else { + return $this->class . '_' . str_replace(array('.', '/'), '', $this->name); + } } /** - * Set the HTML ID attribute of the form + * Set the HTML ID attribute of the form. + * + * @param string $id + * + * @return FormField */ public function setHTMLID($id) { $this->htmlID = $id; + + return $this; + } + + /** + * @return string + */ + public function getHTMLID() { + return $this->htmlID; } /** diff --git a/forms/FormField.php b/forms/FormField.php index 0496e7d92..55b67bb70 100644 --- a/forms/FormField.php +++ b/forms/FormField.php @@ -115,6 +115,7 @@ class FormField extends RequestHandler { } else { $label = $fieldName; } + $label = preg_replace("/([a-z]+)([A-Z])/","$1 $2", $label); return $label; @@ -177,17 +178,44 @@ class FormField extends RequestHandler { /** * Returns the HTML ID of the field - used in the template by label tags. + * * The ID is generated as FormName_FieldName. All Field functions should ensure * that this ID is included in the field. + * + * @return string */ public function ID() { - $name = preg_replace('/(^-)|(-$)/', '', preg_replace('/[^A-Za-z0-9_-]+/', '-', $this->name)); - if($this->form) return $this->form->FormName() . '_' . $name; - else return $name; + return $this->getTemplateHelper()->generateFieldID($this); } /** - * Returns the field name - used by templates. + * Returns the HTML ID for the form field holder element. + * + * @return string + */ + public function HolderID() { + return $this->getTemplateHelper()->generateFieldHolderID($this); + } + + /** + * Returns the current {@link FormTemplateHelper} on either the parent + * Form or the global helper set through the {@link Injector} layout. + * + * To customize a single {@link FormField}, use {@link setTemplate} and + * provide a custom template name. + * + * @return FormTemplateHelper + */ + public function getTemplateHelper() { + if($this->form) { + return $this->form->getTemplateHelper(); + } + + return Injector::inst()->get('FormTemplateHelper'); + } + + /** + * Returns the raw field name. * * @return string */ @@ -552,7 +580,6 @@ class FormField extends RequestHandler { * message has not been defined then just return blank. The default * error is defined on {@link Validator}. * - * @todo Should the default error message be stored here instead * @return string */ public function getCustomValidationMessage() { diff --git a/forms/FormTemplateHelper.php b/forms/FormTemplateHelper.php new file mode 100644 index 000000000..9fbe9090b --- /dev/null +++ b/forms/FormTemplateHelper.php @@ -0,0 +1,122 @@ + + * $form->setTemplateHelper('ClassName'); + * + * + * Globally, the FormTemplateHelper can be set via the {@link Injector} API. + * + * For backwards compatibility, with < 3.2 use the {@link FormTemplateHelper_Pre32} + * class which will preserve the old style form field attributes. + * + * + * Injector: + * FormTemplateHelper: + * class: FormTemplateHelper_Pre32 + * + * + * @package framework + * @subpackage forms + */ +class FormTemplateHelper { + + /** + * @param Form $form + * + * @return string + */ + public function generateFormID($form) { + if($id = $form->getHTMLID()) { + return Convert::raw2htmlid($id); + } + + return Convert::raw2htmlid($form->FormName()); + } + + /** + * @param FormField $field + * + * @return string + */ + public function generateFieldHolderID($field) { + return $this->generateFieldID($field) . '_Holder'; + } + + /** + * Generate the field ID value + * + * @param FormField + * + * @return string + */ + public function generateFieldID($field) { + if($form = $field->getForm()) { + return sprintf("%s_%s", + $form->getHTMLID(), + Convert::raw2htmlid($field->getName()) + ); + } + + return Convert::raw2htmlid($field->getName()); + } + +} + +/** + * Note that this will cause duplicate and invalid ID attributes. + * + * @deprecated 4.0 + * + * @package framework + * @subpackage forms + */ +class FormTemplateHelper_Pre32 extends FormTemplateHelper { + + /** + * @param Form + * + * @return string + */ + public function generateFormID($form) { + if($id = $form->getHTMLID()) { + return $id; + } + + return $form->class . '_' . str_replace(array('.', '/'), '', $form->getName()); + } + + /** + * @param FormField + * + * @return string + */ + public function generateFieldHolderID($field) { + return $field->getName(); + } + + /** + * @param FormField + * + * @return string + */ + public function generateFieldID($field) { + $name = preg_replace('/(^-)|(-$)/', '', preg_replace('/[^A-Za-z0-9_-]+/', '-', $field->getName())); + + if($form = $field->getForm()) { + return $form->FormName() . '_' . $name; + } + + return $name; + } +} diff --git a/templates/forms/FormField_holder.ss b/templates/forms/FormField_holder.ss index fa23b8595..ae6f5353b 100644 --- a/templates/forms/FormField_holder.ss +++ b/templates/forms/FormField_holder.ss @@ -1,4 +1,4 @@ -
+
<% if Title %><% end_if %>
$Field diff --git a/tests/forms/FieldListTest.php b/tests/forms/FieldListTest.php index 9047623a8..6f2525801 100644 --- a/tests/forms/FieldListTest.php +++ b/tests/forms/FieldListTest.php @@ -126,24 +126,28 @@ class FieldListTest extends SapphireTest { /* We have no fields in the tab now */ $this->assertEquals(0, $tab->Fields()->Count()); } - - /** - * Test removing a field from a set by it's name. - */ + public function testRemoveFieldByName() { $fields = new FieldList(); - - /* First of all, we add a field into our FieldList object */ $fields->push(new TextField('Name', 'Your name')); - /* We have 1 field in our set now */ $this->assertEquals(1, $fields->Count()); - - /* Then, we call up removeByName() to take it out again */ $fields->removeByName('Name'); - - /* We have 0 fields in our set now, as we've just removed the one we added */ $this->assertEquals(0, $fields->Count()); + + $fields->push(new TextField('Name[Field]', 'Your name')); + $this->assertEquals(1, $fields->Count()); + $fields->removeByName('Name[Field]'); + $this->assertEquals(0, $fields->Count()); + } + + public function testDataFieldByName() { + $fields = new FieldList(); + $fields->push($basic = new TextField('Name', 'Your name')); + $fields->push($brack = new TextField('Name[Field]', 'Your name')); + + $this->assertEquals($basic, $fields->dataFieldByName('Name')); + $this->assertEquals($brack, $fields->dataFieldByName('Name[Field]')); } /** @@ -157,14 +161,19 @@ class FieldListTest extends SapphireTest { /* A field gets added to the set */ $fields->addFieldToTab('Root', new TextField('Country')); - /* We have the same object as the one we pushed */ $this->assertSame($fields->dataFieldByName('Country'), $tab->fieldByName('Country')); - /* The field called Country is replaced by the field called Email */ $fields->replaceField('Country', new EmailField('Email')); - - /* We have 1 field inside our tab */ - $this->assertEquals(1, $tab->Fields()->Count()); + $this->assertEquals(1, $tab->Fields()->Count()); + + $fields = new FieldList(); + $fields->push(new TextField('Name', 'Your name')); + $brack = new TextField('Name[Field]', 'Your name'); + + $fields->replaceField('Name', $brack); + $this->assertEquals(1, $fields->Count()); + + $this->assertEquals('Name[Field]', $fields->first()->getName()); } public function testRenameField() { diff --git a/tests/forms/FormTest.php b/tests/forms/FormTest.php index 35a38dd28..db98158e3 100644 --- a/tests/forms/FormTest.php +++ b/tests/forms/FormTest.php @@ -1,4 +1,5 @@ update('Injector', 'FormTemplateHelper', array('class' => 'FormTemplateHelper_Pre32')); + $this->get('FormTest_Controller'); $response = $this->post( @@ -251,7 +254,6 @@ class FormTest extends FunctionalTest { ), 'Required fields show a notification on field when left blank' ); - } public function testSessionSuccessMessage() { @@ -433,6 +435,10 @@ class FormTest extends FunctionalTest { } +/** + * @package framework + * @subpackage tests + */ class FormTest_Player extends DataObject implements TestOnly { private static $db = array( 'Name' => 'Varchar', @@ -454,6 +460,10 @@ class FormTest_Player extends DataObject implements TestOnly { } +/** + * @package framework + * @subpackage tests + */ class FormTest_Team extends DataObject implements TestOnly { private static $db = array( 'Name' => 'Varchar', @@ -465,6 +475,10 @@ class FormTest_Team extends DataObject implements TestOnly { ); } +/** + * @package framework + * @subpackage tests + */ class FormTest_Controller extends Controller implements TestOnly { private static $url_handlers = array( '$Action//$ID/$OtherID' => "handleAction", @@ -510,6 +524,10 @@ class FormTest_Controller extends Controller implements TestOnly { } +/** + * @package framework + * @subpackage tests + */ class FormTest_ControllerWithSecurityToken extends Controller implements TestOnly { private static $url_handlers = array( '$Action//$ID/$OtherID' => "handleAction",