Skip to content

Commit

Permalink
bug #17787 [Form] Fix choice placeholder edge cases (Tobion)
Browse files Browse the repository at this point in the history
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
  • Loading branch information
fabpot committed Feb 18, 2016
2 parents de5c737 + 0efbc30 commit d3c55cb
Show file tree
Hide file tree
Showing 8 changed files with 73 additions and 8 deletions.
Expand Up @@ -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 %}>
Expand Down
@@ -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(
Expand Down
Expand Up @@ -246,7 +246,7 @@ private static function addChoiceViewGroupedBy($groupBy, $choice, $value, $label

$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);
Expand Down
20 changes: 20 additions & 0 deletions src/Symfony/Component/Form/ChoiceList/View/ChoiceListView.php
Expand Up @@ -48,4 +48,24 @@ public function __construct(array $choices = array(), array $preferredChoices =
$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;
}
}
Expand Up @@ -199,7 +199,7 @@ public function buildView(FormView $view, FormInterface $form, array $options)
}

// 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']) {
Expand Down Expand Up @@ -343,6 +343,9 @@ public function configureOptions(OptionsResolver $resolver)
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;
Expand Down
21 changes: 21 additions & 0 deletions src/Symfony/Component/Form/Tests/AbstractBootstrap3LayoutTest.php
Expand Up @@ -232,6 +232,26 @@ public function testSingleChoice()
);
}

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(
Expand Down Expand Up @@ -754,6 +774,7 @@ public function testSingleChoiceExpandedWithPlaceholder()
'multiple' => false,
'expanded' => true,
'placeholder' => 'Test&Me',
'required' => false,
));

$this->assertWidgetMatchesXpath($form->createView(), array(),
Expand Down
21 changes: 21 additions & 0 deletions src/Symfony/Component/Form/Tests/AbstractLayoutTest.php
Expand Up @@ -528,6 +528,26 @@ public function testSingleChoice()
);
}

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(
Expand Down Expand Up @@ -1001,6 +1021,7 @@ public function testSingleChoiceExpandedWithPlaceholder()
'multiple' => false,
'expanded' => true,
'placeholder' => 'Test&Me',
'required' => false,
));

$this->assertWidgetMatchesXpath($form->createView(), array(),
Expand Down
Expand Up @@ -1689,7 +1689,7 @@ public function testDontPassPlaceholderIfContainedInChoices($multiple, $expanded
'expanded' => $expanded,
'required' => $required,
'placeholder' => $placeholder,
'choices' => array('A' => 'a', 'Empty' => ''),
'choices' => array('Empty' => '', 'A' => 'a'),
'choices_as_values' => true,
));
$view = $form->createView();
Expand All @@ -1716,9 +1716,9 @@ public function getOptionsWithPlaceholder()
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
Expand Down

0 comments on commit d3c55cb

Please sign in to comment.