Problem/Motivation

#states do not work with 'datetime' form elements.

Proposed resolution

Update logic in the Datetime element to handle the nested elements.

Remaining tasks

Fix test fails.

User interface changes

None, other than #states working.

API changes

None.

Data model changes

None.

Original Report

While trying to create a custom field widget containing a "datetime" form element, I discovered that the #states attribute does not work on it.

Some states can be achieved with a workaround: put the datetime element in a container, and put the #states on the container. But this is obviously no clean fix, and it doesn't work for all states. (Works for "visible" for example, but not for "required".)

I was not able to figure out why this is not working, but I noticed that there have been a lot of issues regarding the #states attribute in the issue tracker. Most of them for were for specific elements like submit buttons, select elements with multiple values, ...

Comments

lucastockmann’s picture

Status: Active » Needs work
FileSize
11.81 KB
1010 bytes
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch drupal_datetime-states_2419131-0.patch. Unable to apply patch. See the log in the details link for more information. View

I could confirm this bug.

We need to parse the #states through to the single form elements from datetime.
Now we only need to hide the label.

Screenshot

bertramakers’s picture

FileSize
9.43 KB
1.83 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch drupal_datetime-states_2419131-2.patch. Unable to apply patch. See the log in the details link for more information. View

I thought about putting the title on either the date element if it's visible, or the time element, which fixed the issue but resulted in a different style then before.

This is because the container element has the class "container-inline" which puts all form elements and their titles on a single line.

Removing this class does not seem like a fix as it will put the date and time elements on separate lines.

Title on date element.

Formatting of the code in my patch could also use some love but I'm in a hurry at the moment :)

mpdonadio’s picture

Status: Needs work » Needs review

Just setting Needs Review so the attached patch gets tested.

The last submitted patch, 1: drupal_datetime-states_2419131-0.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 2: drupal_datetime-states_2419131-2.patch, failed testing.

mpdonadio’s picture

Issue tags: +Needs reroll
andileco’s picture

I'm going to re-roll this.

andileco’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
2.08 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 98,929 pass(es), 0 fail(s), and 3,414 exception(s). View

Patch attached.

Status: Needs review » Needs work

The last submitted patch, 8: drupal_datetime-states_2419131-8.patch, failed testing.

Sharique’s picture

+++ b/core/lib/Drupal/Core/Datetime/Element/Datetime.php
@@ -261,14 +261,14 @@ public static function processDatetime(&$element, FormStateInterface $form_state
+        '#states' => $element['#states'],

@@ -292,15 +292,22 @@ public static function processDatetime(&$element, FormStateInterface $form_state
+        '#states' => $element['#states'],

"Notice : Undefined index: #states" is the cause.

andileco’s picture

Hey @Sharique, are you recommending that the above code (post #10) be added to the patch to help it pass? Thanks!

bertramakers’s picture

I think what he means is that the tests are failing because the lines he mentioned do not check that $element['#states'] is set before it is accessed.

All failing tests are complaining about an "undefined index #states", so that makes sense.

Sharique’s picture

No, I'm saying this line of code has issues. It seems like $element does not have '#states' element, hence giving warning. Changing it to

'#states' => $element['#states']?'#states' => $element['#states']:[],

might fix the issue but we need to investigate why it is not there.

bertramakers’s picture

#states is always optional on elements, so that's why it's not always set.

kevin.dutra’s picture

Status: Needs work » Needs review
FileSize
1 KB

I've updated the patch to fix the PHP notice. I've also removed all the title changes that were added in more recent patches. Things may have been working a bit differently 6 months ago, but I don't believe those changes are necessary or the right approach. The title displays are hidden, so they shouldn't be showing up anyways. If there are still problems with that, I'd suggest opening a separate issue. This simple patch at least gets the states working.

mpdonadio’s picture

Status: Needs review » Needs work

Thanks for the patch. Looks good, but can we get a quick test to demonstrate the bug in HEAD and that this fixes it?

pcambra’s picture

Status: Needs work » Needs review
FileSize
3.73 KB

I was discussing about this on IRC and we don't have a way to test states atm.

What I've done is to work in the issue of the label display, here's a patch that takes into account the element as a whole.

Status: Needs review » Needs work

The last submitted patch, 17: datetime-element-states-2419131-17.patch, failed testing.

mpdonadio’s picture

Issue tags: +Needs manual testing
FileSize
4.48 KB

Here's the interdiff.

kevin.dutra’s picture

Haven't looked too deeply into the test failures, but with some manual testing, I can confirm that the patch from #17 is not quite there. To see the issue, configure a content type with a date field and try to add a new node of that type. The date field renders without any label. (#states doesn't even come into play in this example)

mpdonadio’s picture

Since this needs manual testing, would it be possible to add a test module to datetime.module to show off how #states work for it?

kevin.dutra’s picture

I like the idea for an example module to aid in testing. In creating an example, it helped illustrate where there were still some rough spots.

Here is an updated patch with the following changes since the previous patch:

  1. Restored the title to the datetime wrapper.
  2. Added an actual <div> to contain the the wrapper contents.
  3. Added some styling that was missing for disabled date/time elements from a couple themes.
  4. Divided the states that are applied to the datetime element as a whole. Based on the way the #states JS code works, some states need to operate on the wrapper while others need to operate on the fields themselves. I was trying for the least amount of modification to the existing JS code, but maybe that's the wrong way to go about it.
  5. Adjusted the #states JS code to account for the fact that certain labels aren't implemented as actual <label> elements.
  6. Added a testing module with an example.

@mpdonadio: Is this what you meant by a testing module? The one thing I couldn't figure out was how you can enable a module in the Drupal\tests namespace. They all appear to be hidden, which would be unhelpful if it's intended for manual testing. Is there a way I'm unaware of? Or were you thinking it would actually be outside the Drupal\tests namespace so that it was publicly listed as any other core module?

Status: Needs review » Needs work

The last submitted patch, 22: datetime-element-states-2419131-22.patch, failed testing.

mpdonadio’s picture

#23, I'll look at the patch later, but

$settings['extension_discovery_scan_tests'] = TRUE;

should allow you to enable test modules from the UI. See sites/example.settings.local.php for more info.

mpdonadio’s picture

#23, yeah, that was what I was thinking for the example module for manual testing. However, it gives me

InvalidArgumentException: Class "\Drupal\datetime_states\Form\StateForm" does not exist. in Drupal\Core\DependencyInjection\ClassResolver->getInstanceFromDefinition() (line 29 of core/lib/Drupal/Core/DependencyInjection/ClassResolver.php).

when go goto the path defined by the module.

kevin.dutra’s picture

Whoops, typo on the directory name. This one should actually work.

Status: Needs review » Needs work

The last submitted patch, 26: datetime-element-states-2419131-24.patch, failed testing.

kevin.dutra’s picture

Hmm, that first failure may actually be valid. It looks like something goofy is happening when the element is used in the field widget for a single valued field. (Everything is inline, at least, which isn't supposed to happen.)

The second failure is probably just the test needing adjustment, although I haven't delved too deeply into either yet.

mpdonadio’s picture

Awesome. I get a

Notice: Undefined index: #default_value in Drupal\Core\Datetime\Element\Datetime::valueCallback() (line 103 of core/lib/Drupal/Core/Datetime/Element/Datetime.php).

when I have E_ALL set for warnings when I visit the URL. Just add

$form['date'] = array(
      '#default_value' => '',
 );

in the StateForm.

If we fix the tests, and this warning, I think we are good. Pining one of the theme maintainers to double check the front end changes.

mpdonadio’s picture

Issue summary: View changes
jhedstrom’s picture

Confirmed that the small tweak from #29 resolves the php notice. I also manually tested and verified that the 'Required' state now works, but I couldn't get the 'Disabled' or 'Invisible' checkboxes on the demo page to do anything (for instance, invisible did nothing, and disabled would appear to disable at first, but I could still use the date picker to change the value).

+++ b/core/modules/datetime/tests/modules/datetime_states/src/Form/StateForm.php
@@ -0,0 +1,75 @@
+ * Defines a settings page for the File Hosting module.

Small nit, this isn't the File Hosting module :)

jhedstrom’s picture

Re #31 after reloading the page both invisible and disabled are working--not sure what I had done before.

davidhernandez’s picture

+++ b/core/misc/states.js
@@ -609,13 +609,21 @@
         var $label = $(e.target).attr({'required': 'required', 'aria-required': 'aria-required'}).closest('.js-form-item, .js-form-wrapper').find(label);
+        // A complex form element like 'datetime' (one made up of multiple
+        // sub-elements) may have a label for the element as a whole, which will
+        // not be a true <label>.
+        var $complexLabel = $(e.target).closest('.js-form-wrapper').find('.label');

Is this .label logic in the states JS being added just for this? If so, I'd want that to use a js- prefixed class and not a simple css class name.

+++ b/core/themes/seven/css/components/form.css
@@ -52,7 +52,8 @@ label[for] {
+.form-disabled label,
+.form-disabled .label {

What is the use case exactly where this structure can't use a label element? Can someone provide an example?

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

The last submitted patch, 26: datetime-element-states-2419131-24.patch, failed testing.

DuaelFr’s picture

kevin.dutra’s picture

Assigned: Unassigned » kevin.dutra

Picking this back up.

kevin.dutra’s picture

Okay, here's the latest patch. This includes the following changes:

  1. Some misc. cleanup of comments 'n such while I was already in there.
  2. The Datetime element no longer barfs if a #default_value is not explicitly defined.
  3. Fixed another issue I noticed, with the label not receiving 'disabled' CSS treatment when you just use a normal '#disabled' => TRUE
  4. Switched from using an <h4> to an actual <label>, per @davidhernandez's suggestion. (Returned the CSS changes back to normal.)
  5. Added changes to templates I had missed in core & Stable.
  6. For the Classy template, tossed in a few of the classes that would normally be included if form_element was used as the theme wrapper so that you can actually identify it as a datetime type item.
  7. Updated the DateTimeFieldTest to correct the Xpath expression that was failing due to the changes in the template.
  8. Removed the 'required' attribute that was getting applied to the wrapper, which was causing the FormTest to fail.
  9. Removed a duplicate 'container-inline' that the field widget was adding, which was causing the field label to also become inline.
kevin.dutra’s picture

Status: Needs work » Needs review
kevin.dutra’s picture

Assigned: kevin.dutra » Unassigned
mpdonadio’s picture

Status: Needs review » Needs work

Looks like #38 has a lot of out of scope changes for the issue at hand. What is needed, compared to #26 to address the #states problem?

  1. +++ b/core/lib/Drupal/Core/Datetime/Element/Datetime.php
    @@ -28,11 +30,11 @@ public function getInfo() {
    -        /** @var $date_format_entity \Drupal\Core\Datetime\DateFormatInterface */
    +        /* @var $date_format_entity \Drupal\Core\Datetime\DateFormatInterface */
             $date_format = $date_format_entity->getPattern();
    

    Out of scope, which would remove type hinting.

  2. +++ b/core/lib/Drupal/Core/Datetime/Element/Datetime.php
    @@ -28,11 +30,11 @@ public function getInfo() {
    -        /** @var $time_format_entity \Drupal\Core\Datetime\DateFormatInterface */
    +        /* @var $time_format_entity \Drupal\Core\Datetime\DateFormatInterface */
             $time_format = $time_format_entity->getPattern();
    

    Out of scope, which would remove type hinting.

  3. +++ b/core/lib/Drupal/Core/Datetime/Element/Datetime.php
    @@ -137,17 +139,17 @@ public static function valueCallback(&$element, $input, FormStateInterface $form
    -   *     timezone. Converting a date stored in the database from UTC to the local
    -   *     zone and converting it back to UTC before storing it is not handled here.
    -   *     This element accepts a date as the default value, and then converts the
    -   *     user input strings back into a new date object on submission. No timezone
    -   *     adjustment is performed.
    +   *     timezone. Converting a date stored in the database from UTC to the
    +   *     local zone and converting it back to UTC before storing it is not
    +   *     handled here. This element accepts a date as the default value, and
    +   *     then converts the user input strings back into a new date object on
    +   *     submission. No timezone adjustment is performed.
        * Optional properties include:
    

    Out of scope for this issue?

  4. +++ b/core/lib/Drupal/Core/Datetime/Element/Datetime.php
    @@ -137,17 +139,17 @@ public static function valueCallback(&$element, $input, FormStateInterface $form
    -   *     element, no other format will work. See the format_date() function for a
    -   *     list of the possible formats and HTML5 standards for the HTML5
    +   *     element, no other format will work. See the format_date() function for
    +   *     a list of the possible formats and HTML5 standards for the HTML5
        *     requirements. Defaults to the right HTML5 format for the chosen element
    

    out of scope for this issue?

  5. +++ b/core/lib/Drupal/Core/Datetime/Element/Datetime.php
    @@ -172,23 +174,24 @@ public static function valueCallback(&$element, $input, FormStateInterface $form
    -   *     element. Can be used to add a jQuery timepicker or an 'All day' checkbox.
    +   *     element. Can be used to add a jQuery timepicker or an 'All day'
    +   *     checkbox.
        *   - #date_year_range: A description of the range of years to allow, like
    

    Out of scope for the issue?

  6. +++ b/core/lib/Drupal/Core/Datetime/Element/Datetime.php
    @@ -172,23 +174,24 @@ public static function valueCallback(&$element, $input, FormStateInterface $form
    -   *     year at the time the form is displayed. Used in jQueryUI datepicker year
    -   *     range and HTML5 min/max date settings. Defaults to '1900:2050'.
    +   *     year at the time the form is displayed. Used in jQueryUI datepicker
    +   *     year range and HTML5 min/max date settings. Defaults to '1900:2050'.
        *   - #date_increment: The increment to use for minutes and seconds, i.e.
    

    Out of scope for the issue?

  7. +++ b/core/lib/Drupal/Core/Datetime/Element/Datetime.php
    @@ -172,23 +174,24 @@ public static function valueCallback(&$element, $input, FormStateInterface $form
    -   *    '15' would show only :00, :15, :30 and :45. Used for HTML5 step values and
    -   *     jQueryUI datepicker settings. Defaults to 1 to show every minute.
    -   *   - #date_timezone: The local timezone to use when creating dates. Generally
    -   *     this should be left empty and it will be set correctly for the user using
    -   *     the form. Useful if the default value is empty to designate a desired
    -   *     timezone for dates created in form processing. If a default date is
    -   *     provided, this value will be ignored, the timezone in the default date
    -   *     takes precedence. Defaults to the value returned by
    +   *    '15' would show only :00, :15, :30 and :45. Used for HTML5 step values
    +   *     and jQueryUI datepicker settings. Defaults to 1 to show every minute.
    +   *   - #date_timezone: The local timezone to use when creating dates.
    +   *     Generally this should be left empty and it will be set correctly for
    +   *     the user using the form. Useful if the default value is empty to
    +   *     designate a desired timezone for dates created in form processing. If a
    +   *     default date is provided, this value will be ignored, the timezone in
    +   *     the default date takes precedence. Defaults to the value returned by
        *     drupal_get_user_timezone().
    

    Out of scope for the issue?

  8. +++ b/core/lib/Drupal/Core/Datetime/Element/Datetime.php
    @@ -366,8 +384,10 @@ public static function validateDatetime(&$element, FormStateInterface $form_stat
    +   *   A format acceptable by PHP's date() function.
        *
    

    Out of scope for the issue?

  9. +++ b/core/lib/Drupal/Core/Datetime/Element/Datetime.php
    @@ -366,8 +384,10 @@ public static function validateDatetime(&$element, FormStateInterface $form_stat
    +   *   A formatted example date.
        */
    

    Out of scope for the issue?

  10. +++ b/core/modules/system/templates/datetime-wrapper.html.twig
    @@ -16,18 +16,25 @@
    diff --git a/core/themes/classy/templates/form/datetime-wrapper.html.twig b/core/themes/classy/templates/form/datetime-wrapper.html.twig
    

    Missing endline.

  11. +++ b/core/themes/stable/templates/form/datetime-wrapper.html.twig
    @@ -14,18 +14,25 @@
    \ No newline at end of file
     
    

    Missing endline.

kevin.dutra’s picture

Status: Needs work » Needs review
FileSize
9.81 KB
13.81 KB

Fair enough -- cleanup stuff removed. This interdiff is based of #26, for easier comparison.

jhedstrom’s picture

I'm removing the manual testing tag, as this can now be automatically tested since there's already test module here in the patch. It just needs a quick test added (see #2702233: Add Javascript tests for Form API's #states for examples).

davidhernandez’s picture

Status: Needs review » Needs work
+++ b/core/themes/stable/templates/form/datetime-wrapper.html.twig
@@ -14,18 +14,25 @@
+<div{{ attributes.addClass(container_classes) }}>

Sorry, should have pointed this out. The Stable templates can't be changed. At least not significantly enough to add a wrapper div. The same goes for Classy. They both require backwards compatibility. The System and Datetime templates are fair game.

kevin.dutra’s picture

@davidhernandez: Ah, I guess that makes sense. I don't think this bug is possible to fix without something to wrap the markup, to be able to give the context of what the label applies to. (Right now, datetime element pieces just hang out in the middle of nowhere.) I'm not totally clear here on how much change is acceptable for a patch release -- would it be viable to add a second wrapper to the datetime element, where this wrapper would just have the containing div?

mpdonadio’s picture

#45, would it make sense to postpone this on #1918994: Improve Datetime and Daterange Widget accessibility, and then use the fieldset as the wrapper?

kevin.dutra’s picture

I'm not sure -- it would depend on where that issue goes. Right now, it looks like it's only addressing the datetime field widget, which won't help here where we're looking at the datetime form element.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

kevin.dutra’s picture

Status: Needs work » Needs review
FileSize
13.85 KB

Re-roll, in case anyone was relying on this for 8.2.

Status: Needs review » Needs work

The last submitted patch, 49: datetime-element-states-2419131-48.patch, failed testing.

kevin.dutra’s picture

Status: Needs work » Needs review
FileSize
15.75 KB
1.76 KB

Oops, forgot that they added that date range module -- a couple test adjustments required.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

jhedstrom’s picture

Status: Needs review » Postponed
Related issues: +#1918994: Improve Datetime and Daterange Widget accessibility

Postponing as mentioned above since #1918994: Improve Datetime and Daterange Widget accessibility will impact this patch.

mpdonadio’s picture

Version: 8.3.x-dev » 8.4.x-dev
Status: Postponed » Needs work
Issue tags: +Needs reroll
Jo Fitzgerald’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
13.55 KB

Re-rolled.

mpdonadio’s picture

Component: datetime.module » theme system
Status: Needs review » Needs work
  1. +++ b/core/misc/states.js
    @@ -621,13 +624,20 @@
    diff --git a/core/modules/datetime/tests/modules/datetime_states/datetime_states.info.yml b/core/modules/datetime/tests/modules/datetime_states/datetime_states.info.yml
    
    diff --git a/core/modules/datetime/tests/modules/datetime_states/datetime_states.info.yml b/core/modules/datetime/tests/modules/datetime_states/datetime_states.info.yml
    new file mode 100644
    
    new file mode 100644
    index 0000000..75e139f
    
    index 0000000..75e139f
    --- /dev/null
    
    --- /dev/null
    +++ b/core/modules/datetime/tests/modules/datetime_states/datetime_states.info.yml
    
    +++ b/core/modules/datetime/tests/modules/datetime_states/datetime_states.info.yml
    +++ b/core/modules/datetime/tests/modules/datetime_states/datetime_states.info.yml
    @@ -0,0 +1,8 @@
    
    @@ -0,0 +1,8 @@
    +name: 'Datetime #states test'
    +type: module
    +description: 'Provides an example demonstrating how form #states affect datetime elements.'
    +package: Testing
    +version: VERSION
    +core: 8.x
    +dependencies:
    +  - datetime
    

    This change is to the datetime element, which isn't part of the datetime.module. So, this should live with the element tests, most(?) of which are in system.module. NW for this part.

  2. +++ b/core/modules/datetime/tests/src/Functional/DateTimeFieldTest.php
    @@ -48,7 +48,7 @@ public function testDateField() {
    -      $this->assertFieldByXPath('//*[@id="edit-' . $field_name . '-wrapper"]//label[contains(@class,"js-form-required")]', TRUE, 'Required markup found');
    +      $this->assertFieldByXPath('//*[@id="edit-' . $field_name . '-wrapper"]/div/label[contains(@class,"js-form-required")]', TRUE, 'Required markup found');
           $this->assertNoFieldByName("{$field_name}[0][value][time]", '', 'Time element not found.');
    

    Not sure if this is a legit change. It is checking that the a div is there, but not necessarily the right one. The middle div probably needs a class on it.

The reroll looks good, and no datetime.module tests broke, but this has markup changes, so I am moving this to the theme system queue so the FE people can look at this.

There are several date / time / datetime element related bugs right now, and we pretty much have no test coverage of these. The tests on the issues should be coordinated to give these a common home.

gordon’s picture

FileSize
67.42 KB

Here is a version of the patch which works on 8.3.1.

I have been making some changes to date_recur and I need this patch to get the new form working.

gordon’s picture

I found another problem to do with states and the datetime field. I am not sure if this should be added to this issue or a new issue added.

Basically I have done the following.

$form['date'] = [
  '#type' => 'datetime',
  '#title' => t('Date'),
];

$form['checkbox'] = [
  '#type' => 'checkbox',
  '#title' => t('check me'),
  '#states' => [
    'disabled' => [ ':input['name="date"]' => [ 'empty' => TRUE ] ]
  ],
];

So basically in this case the when you enter a date via the datepicker the checkbox is not enabled. however if you just type in the date the checkbox is enabled.

I managed to find a work around and this is basically because of how the value condition is checked. Using the following

$form['checkbox'] = [
  '#type' => 'checkbox',
  '#title' => t('check me'),
  '#states' => [
    'disabled' => [ ':input['name="date"]' => [ 'value' =>'' ] ]
  ],
];

Thanks.

realityloop’s picture

@gordon patch at #57 doesn't apply against 8.3.2, patch at #55 does..

borisson_’s picture