bug #17787 [Form] Fix choice placeholder edge cases (Tobion)
This PR was merged into the 2.7 branch.
Discussion
----------
[Form] Fix choice placeholder edge cases
| Q | A
| ------------- | ---
| Bug fix? | yes
| New feature? | no
| BC breaks? | no
| Deprecations? | no
| Tests pass? | yes
| Fixed tickets | -
| License | MIT
| Doc PR | -
Fixing several problems with choice placeholder that enhances #9030 for more edge cases:
- A choice with an empty value manually added in the choices array should only be considered a placeholder when it is the first element in the final choice select.
This is part of the HTML spec and how browsers also behave. If you select a choice with an empty value that is not the first option, it will still pass the "required" check
and thus submit the empty value. So it's not a placeholder.
If in the example below you move the empty option to the first place, the browsers will error on submit that you
must select a value. So only then it is a placeholder to show as initial value.
```html
<select id="form_timezone" name="form[timezone]" required="required">
<option value="Africa/Abidjan">Abidjan</option>
<option value="">Empty</option>
</select>
```
Also the validator https://validator.w3.org/nu/ will mark the above code as error:
> The first child option element of a select element with a required attribute, and without a multiple attribute, and without a size attribute whose value is greater than 1, must have either an empty value attribute, or must have no text content. Consider either adding a placeholder option label, or adding a size attribute with a value equal to the number of option elements.
This is fixed by replacing`0 !== count($choiceList->getChoicesForValues(array('')))` with `$view->vars['placeholder_in_choices'] = $choiceListView->hasPlaceholder()`.
Which means, the required attribute is removed automatically because the select form element is required implicitly anyway due to the nature of the choice UI.
- As the above quote mentions, the `size` attribute also has impact. Namely for a select with size > 1 it can be possible to have a required attribute even without placeholder.
This is because when the size > 1, there is no default choice selected (similar to select with "multiple").
- A placeholder for required radio buttons or a select with size > 1 does not make sense as it would just be fake data that can be submitted (similar to the ignored placeholder for multi-select and checkboxes).
Commits
-------
0efbc30
[Form] fix edge cases with choice placeholder
This commit is contained in:
commit
d3c55cb3f1
@ -52,7 +52,7 @@
|
||||
{%- endblock choice_widget_expanded -%}
|
||||
|
||||
{%- block choice_widget_collapsed -%}
|
||||
{%- if required and placeholder is none and not placeholder_in_choices and not multiple -%}
|
||||
{%- if required and placeholder is none and not placeholder_in_choices and not multiple and (attr.size is not defined or attr.size <= 1) -%}
|
||||
{% set required = false %}
|
||||
{%- endif -%}
|
||||
<select {{ block('widget_attributes') }}{% if multiple %} multiple="multiple"{% endif %}>
|
||||
|
@ -1,5 +1,5 @@
|
||||
<select
|
||||
<?php if ($required && null === $placeholder && $placeholder_in_choices === false && $multiple === false):
|
||||
<?php if ($required && null === $placeholder && $placeholder_in_choices === false && $multiple === false && (!isset($attr['size']) || $attr['size'] <= 1)):
|
||||
$required = false;
|
||||
endif; ?>
|
||||
<?php echo $view['form']->block($form, 'widget_attributes', array(
|
||||
|
@ -246,7 +246,7 @@ class DefaultChoiceListFactory implements ChoiceListFactoryInterface
|
||||
|
||||
$groupLabel = (string) $groupLabel;
|
||||
|
||||
// Initialize the group views if necessary. Unnnecessarily built group
|
||||
// Initialize the group views if necessary. Unnecessarily built group
|
||||
// views will be cleaned up at the end of createView()
|
||||
if (!isset($preferredViews[$groupLabel])) {
|
||||
$preferredViews[$groupLabel] = new ChoiceGroupView($groupLabel);
|
||||
|
@ -48,4 +48,24 @@ class ChoiceListView
|
||||
$this->choices = $choices;
|
||||
$this->preferredChoices = $preferredChoices;
|
||||
}
|
||||
|
||||
/**
|
||||
* Returns whether a placeholder is in the choices.
|
||||
*
|
||||
* A placeholder must be the first child element, not be in a group and have an empty value.
|
||||
*
|
||||
* @return bool
|
||||
*/
|
||||
public function hasPlaceholder()
|
||||
{
|
||||
if ($this->preferredChoices) {
|
||||
$firstChoice = reset($this->preferredChoices);
|
||||
|
||||
return $firstChoice instanceof ChoiceView && '' === $firstChoice->value;
|
||||
}
|
||||
|
||||
$firstChoice = reset($this->choices);
|
||||
|
||||
return $firstChoice instanceof ChoiceView && '' === $firstChoice->value;
|
||||
}
|
||||
}
|
||||
|
@ -199,7 +199,7 @@ class ChoiceType extends AbstractType
|
||||
}
|
||||
|
||||
// Check if the choices already contain the empty value
|
||||
$view->vars['placeholder_in_choices'] = 0 !== count($options['choice_list']->getChoicesForValues(array('')));
|
||||
$view->vars['placeholder_in_choices'] = $choiceListView->hasPlaceholder();
|
||||
|
||||
// Only add the empty value option if this is not the case
|
||||
if (null !== $options['placeholder'] && !$view->vars['placeholder_in_choices']) {
|
||||
@ -343,6 +343,9 @@ class ChoiceType extends AbstractType
|
||||
if ($options['multiple']) {
|
||||
// never use an empty value for this case
|
||||
return;
|
||||
} elseif ($options['required'] && ($options['expanded'] || isset($options['attr']['size']) && $options['attr']['size'] > 1)) {
|
||||
// placeholder for required radio buttons or a select with size > 1 does not make sense
|
||||
return;
|
||||
} elseif (false === $placeholder) {
|
||||
// an empty value should be added but the user decided otherwise
|
||||
return;
|
||||
|
@ -232,6 +232,26 @@ abstract class AbstractBootstrap3LayoutTest extends AbstractLayoutTest
|
||||
);
|
||||
}
|
||||
|
||||
public function testSelectWithSizeBiggerThanOneCanBeRequired()
|
||||
{
|
||||
$form = $this->factory->createNamed('name', 'choice', null, array(
|
||||
'choices' => array('a', 'b'),
|
||||
'choices_as_values' => true,
|
||||
'multiple' => false,
|
||||
'expanded' => false,
|
||||
'attr' => array('size' => 2),
|
||||
));
|
||||
|
||||
$this->assertWidgetMatchesXpath($form->createView(), array('attr' => array('class' => '')),
|
||||
'/select
|
||||
[@name="name"]
|
||||
[@required="required"]
|
||||
[@size="2"]
|
||||
[count(./option)=2]
|
||||
'
|
||||
);
|
||||
}
|
||||
|
||||
public function testSingleChoiceWithoutTranslation()
|
||||
{
|
||||
$form = $this->factory->createNamed('name', 'choice', '&a', array(
|
||||
@ -754,6 +774,7 @@ abstract class AbstractBootstrap3LayoutTest extends AbstractLayoutTest
|
||||
'multiple' => false,
|
||||
'expanded' => true,
|
||||
'placeholder' => 'Test&Me',
|
||||
'required' => false,
|
||||
));
|
||||
|
||||
$this->assertWidgetMatchesXpath($form->createView(), array(),
|
||||
|
@ -528,6 +528,26 @@ abstract class AbstractLayoutTest extends \Symfony\Component\Form\Test\FormInteg
|
||||
);
|
||||
}
|
||||
|
||||
public function testSelectWithSizeBiggerThanOneCanBeRequired()
|
||||
{
|
||||
$form = $this->factory->createNamed('name', 'choice', null, array(
|
||||
'choices' => array('a', 'b'),
|
||||
'choices_as_values' => true,
|
||||
'multiple' => false,
|
||||
'expanded' => false,
|
||||
'attr' => array('size' => 2),
|
||||
));
|
||||
|
||||
$this->assertWidgetMatchesXpath($form->createView(), array(),
|
||||
'/select
|
||||
[@name="name"]
|
||||
[@required="required"]
|
||||
[@size="2"]
|
||||
[count(./option)=2]
|
||||
'
|
||||
);
|
||||
}
|
||||
|
||||
public function testSingleChoiceWithoutTranslation()
|
||||
{
|
||||
$form = $this->factory->createNamed('name', 'choice', '&a', array(
|
||||
@ -1001,6 +1021,7 @@ abstract class AbstractLayoutTest extends \Symfony\Component\Form\Test\FormInteg
|
||||
'multiple' => false,
|
||||
'expanded' => true,
|
||||
'placeholder' => 'Test&Me',
|
||||
'required' => false,
|
||||
));
|
||||
|
||||
$this->assertWidgetMatchesXpath($form->createView(), array(),
|
||||
|
@ -1689,7 +1689,7 @@ class ChoiceTypeTest extends \Symfony\Component\Form\Test\TypeTestCase
|
||||
'expanded' => $expanded,
|
||||
'required' => $required,
|
||||
'placeholder' => $placeholder,
|
||||
'choices' => array('A' => 'a', 'Empty' => ''),
|
||||
'choices' => array('Empty' => '', 'A' => 'a'),
|
||||
'choices_as_values' => true,
|
||||
));
|
||||
$view = $form->createView();
|
||||
@ -1716,9 +1716,9 @@ class ChoiceTypeTest extends \Symfony\Component\Form\Test\TypeTestCase
|
||||
array(false, true, false, '', 'None'),
|
||||
array(false, true, false, null, null),
|
||||
array(false, true, false, false, null),
|
||||
array(false, true, true, 'foobar', 'foobar'),
|
||||
// radios should never have an empty label
|
||||
array(false, true, true, '', 'None'),
|
||||
// required radios should never have a placeholder
|
||||
array(false, true, true, 'foobar', null),
|
||||
array(false, true, true, '', null),
|
||||
array(false, true, true, null, null),
|
||||
array(false, true, true, false, null),
|
||||
// multiple non-expanded
|
||||
|
Reference in New Issue
Block a user