[PP-1] #states attribute does not work on #type datetime

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.

Just setting Needs Review so the attached patch gets tested.

I'm going to re-roll this.

Patch attached.

+++ 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.

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

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.

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.

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

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.

Here's the interdiff.

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)

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?

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?

#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.

#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.

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

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.

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.

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 :)

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

+++ 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?

Picking this back up.

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.

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.

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

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: [backport] Add JavaScript tests for Form API #states: required, visible, invisible, expanded, checked, unchecked for examples).

+++ 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.

@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?

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

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.

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

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

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

Missed reopening this when #1918994: Improve Datetime and Daterange Widget accessibility landed.

Re-rolled.

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.

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.

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.

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

This is blocking #2866563: [Ideas] New interactive widget.

this is a reroll against 8.4.x

adding newline at the end of file...

Did not play with this yet, but ...

1. +++ b/core/lib/Drupal/Core/Datetime/Element/Datetime.php
   @@ -296,6 +310,7 @@ public static function processDatetime(&$element, FormStateInterface $form_state
   diff --git a/core/misc/states.js b/core/misc/states.js
   
   diff --git a/core/misc/states.js b/core/misc/states.js
   index 61bbf46..9e86515 100644
   
   index 61bbf46..9e86515 100644
   --- a/core/misc/states.js
   
   --- a/core/misc/states.js
   +++ b/core/misc/states.js
   

   JS changes need to be made to the ES6 versions of the file. NW for this.

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');
   

   Was this actually needed? The // should find the child?

This also needs markup review as we are changing templates.

This doesn't seem to work with state 'required': the primary label never gets the required class, I guess because it doesn't have the "for" attribute.

Have the same problem as #66 for visible too, the label never gets hidden correctly.

I had some issues with visible state when the operator is exposed on an exposed datetime filter in a view!.
The min/max and the value fields wouldnt become visible properly when the operator is between/not between and vice versa and the label also wouldnt get hidden correctly.

Submitting a patch for the same!

Patch in 64 wouldnt apply cleanly on 8.7 , so rolled patch to 8.7 (not shown in interdiff).

Thanks,
Sukanya

1. diff --git a/.idea/workspace.xml b/.idea/workspace.xml
   new file mode 100644
   index 0000000000..07504417a1
   --- /dev/null
   +++ b/.idea/workspace.xml
   @@ -0,0 +1,181 @@
   

   You've added your workspace.xml

2. +++ b/core/lib/Drupal/Core/Datetime/Element/Datetime.php
   @@ -301,6 +302,7 @@ public static function processDatetime(&$element, FormStateInterface $form_state
   diff --git a/core/misc/states.js b/core/misc/states.js
   

   The javascript changes should be made in core/misc/states.es6.js, and then compiled into core/misc/states.js

Thanks Erik Frèrejean for the recommendations!

Submitting updated patch. Previous patch in 64 did not have required changes in the the es6 file. Added them in this patch.
Also Submitting interdiff between 64-72.

As mentioned earlier, some changes are for reroll to 8.7

Thanks,
Sukanya

Oops, missed to add theme.inc changes in patch in 72,

Updating corrected patch and interdiff between 64 and 74 excluding roll up changes to 8.7

Thanks
Sukanya

Uploading interdiff for 64-74

Thanks,
Sukanya

@sukanya.ramakrishnan and @kevin.dutra thanks for the work so far.
yarn run lint:core-js-passing
Is not passing for the es6 changes because prettier has not been run on the code

See: https://www.drupal.org/node/2986680

Once yarn run prettier is run I think all errors will be fixed.

Thanks @tedbow,

uploading a 'prettier' patch !!

Thanks,
Sukanya

Uploading interdiff between 74 and 79 with the right file extension.

Thanks,
Sukanya

Apologies , the patch in #79 is not needed. I was trying to make states work with https://www.drupal.org/node/2648950 and https://www.drupal.org/node/2625136 and thats when i found this issue.

It can be fixed only after https://www.drupal.org/node/2648950 and https://www.drupal.org/node/2625136 are fixed!

Reverting all of my patches!

Sorry for the noise!!

Thanks,
Sukanya

Have this bug on datelist form element.

Almost 4 years? No kidding, maybe it's time to open the gates a bit?

The problem isn't the gates! It is that no one has reviewed the latest patch. Also the intediff that was submitted with the patch should not be named .patch so that it doesn't get tested. This issue should be in needs review, but because of that patch, it's in "needs work."

Patch #79 worked for me

Is it possible to write an automated test?

I'm here because of #3026456: [PP-1] Remove #states hacks on end date once #2419131 is in core and #2845081: Provide a datetime_range widget to define end time via a duration offset.

Re: #88: Yes, via a FunctionalJavascript test. In fact, a previous contributor (sorry, I haven't closely read every comment and patch in here) already started that by adding core/modules/datetime/tests/modules/datetime_states. Thanks!

#79 is working okay with some limited local testing. However, no test classes are actually enabling the datetime_states test module and testing that any of this works as intended, covers all the cases, etc.

So, +1 to both "Needs tests" and "Contributed project blocker" tags...

I know this fix is wider than datetime.module itself, but that seems like a more appropriate component than the generic "theme system".

Bah, I might as well figure out how to get FunctionalJavascript tests working. ;) Stay tuned.

It's not obvious the right way to assert for the field label and the required status or not. So I left those as @todo for now. But this all passes locally, and now we at least assert that visible/invislble and enabled/disabled works. Mostly. ;)

Also fixed some CS and comment bugs in datetime_states/src/Form/StateForm.php.

NR to let the bot chew on this and see what it thinks.
Really NW for finishing the test. Pointers on the clean/recommended way to assert field labels and "requiredness" are most welcome. ;)

p.s. there seems to be some noise at the top of the interdiff. I didn't change *anything* in the JS files. *shrug*

Bah. ;) All kinds of weirdness there. Sorry.

I had the wrong test-only patch from a local mixup. This should apply and fail (to both 8.6.x and 8.7.x). ;)

Second, I was developing all this on 8.6.x branch. Turns out #2672950: Notice: Undefined index: #default_value in Drupal\Core\Datetime\Element\Datetime::valueCallback() (line 103 includes one of the fixes to core/lib/Drupal/Core/Datetime/Element/Datetime.php already. But that's only in 8.7.x, not (yet?) in 8.6.x. So, we actually need two slightly different patches here for the two branches.

Let's try that again...

Note:
2419131.91_93.interdiff.txt is between 2419131-91.full_.patch and 2419131-93.full_.8_6_x.patch
2419131-93.interdiff.86x_87x.txt is between 2419131-93.full_.8_6_x.patch and 2419131-93.full_.8_7_x.patch

Yay, bot is fully happy, as expected. Yee haw. Fairly close to RTBC, but there are a few lingering @todo's in the test.

I asked in Slack about nice ways to try to write test assertions for the datetime label itself. I've tried elementExists('css')and elementExists('xpath') to absolutely no avail. WTF is going on here?

Here's the raw markup from the form from datetime_states:

<div data-drupal-selector="edit-datetime" aria-describedby="edit-datetime--description" data-drupal-states="{&quot;invisible&quot;:{&quot;:input[name=\u0022toggle_invisible\u0022]&quot;:{&quot;checked&quot;:true}},&quot;disabled&quot;:{&quot;:input[name=\u0022toggle_disabled\u0022]&quot;:{&quot;checked&quot;:true}},&quot;required&quot;:{&quot;:input[name=\u0022toggle_required\u0022]&quot;:{&quot;checked&quot;:true}}}" class="js-form-item form-item js-form-type-datetime form-type-datetime js-form-item-datetime form-item-datetime js-complex-form-item">
      <label>Datetime</label>
    <div id="edit-datetime" class="container-inline">
  <div class="js-form-item form-item js-form-type-date form-type-date js-form-item-datetime-date form-item-datetime-date form-no-label">
      <label for="edit-datetime-date" class="visually-hidden">Date</label>
        <input data-drupal-selector="edit-datetime-date" aria-describedby="edit-datetime--description" title="Date (e.g. 2019-01-17)" type="date" data-drupal-date-format="Y-m-d" id="edit-datetime-date" name="datetime[date]" value="" size="12" class="form-date" data-drupal-states="{&quot;invisible&quot;:{&quot;:input[name=\u0022toggle_invisible\u0022]&quot;:{&quot;checked&quot;:true}},&quot;disabled&quot;:{&quot;:input[name=\u0022toggle_disabled\u0022]&quot;:{&quot;checked&quot;:true}},&quot;required&quot;:{&quot;:input[name=\u0022toggle_required\u0022]&quot;:{&quot;checked&quot;:true}}}" />

        </div>
<div class="js-form-item form-item js-form-type-date form-type-date js-form-item-datetime-time form-item-datetime-time form-no-label">
      <label for="edit-datetime-time" class="visually-hidden">Time</label>
        <input data-drupal-selector="edit-datetime-time" aria-describedby="edit-datetime--description" title="Time (e.g. 23:34:08)" type="time" step="1" id="edit-datetime-time" name="datetime[time]" value="" size="12" class="form-time" data-drupal-states="{&quot;invisible&quot;:{&quot;:input[name=\u0022toggle_invisible\u0022]&quot;:{&quot;checked&quot;:true}},&quot;disabled&quot;:{&quot;:input[name=\u0022toggle_disabled\u0022]&quot;:{&quot;checked&quot;:true}},&quot;required&quot;:{&quot;:input[name=\u0022toggle_required\u0022]&quot;:{&quot;checked&quot;:true}}}" />

        </div>

</div>

        <div id="edit-datetime--description" class="description">
      A datetime form element.
    </div>
  </div>

(Apologies for the indenting, that's how core sent it to my browser).

The part I actually care about is line #2:

<div data-drupal-selector="edit-datetime" ...
      <label>Datetime</label>   # This beast right here.
...

A few notes:

1. Why doesn't that have a for attribute? Isn't that an accessibility bug?
2. Why doesn't the wrapper div have an ID? The only way to uniquely target the wrapper seems to be via data-drupal-selector="edit-datetime" Is that intended?

Meanwhile, although this works fine:

$this->assertSession()->elementExists('xpath', '//div[@data-drupal-selector="edit-datetime"]');

none of these assertions work at all:

$this->assertSession()->elementExists('xpath', '//div[@data-drupal-selector="edit-datetime"]/label');
$this->assertSession()->elementExists('xpath', '//div[@data-drupal-selector="edit-datetime"]/child::label');
$this->assertSession()->elementExists('xpath', '//div[@data-drupal-selector="edit-datetime"]/label[1]');

All of them give a variant of this:

ElementNotFoundException: Element matching xpath
    "//div[@data-drupal-selector="edit-datetime"]/label"
    not found.

In the markup above, isn't the "raw" <label>Datetime</label> I care about the first child of the wrapper <div> that the working xpath for the wrapper found?

If I use //, then I get an element, but it's the wrong label!

$this->assertSession()->elementExists('xpath', '//div[@data-drupal-selector="edit-datetime"]//label');

If I call getText() on this, I get "Date" since it seems to be matching this:

      <label for="edit-datetime-date" class="visually-hidden">Date</label>

So // is successfully looking into grandchildren, but it's missing the obvious first child of the wrapper div.

Re-re-reading the xpath docs, I'm stumped why the first set of assertions isn't working. Wasting time trying to sort out cryptic crap like this makes me resent trying to be responsible and write tests for things. :( Does anyone spot what I'm doing wrong here?

Thanks!
-Derek

OMFG, what a pain. Turns out my test assertions were failing because the markup is broken. If I utterly hack things locally so that the markup is no longer this:

      <label>Datetime</label>

But actually looks like this:

      <label for="edit-datetime">Datetime</label>

then lo, all my test assertions start working.

So, the actual code in here is still broken. And lo, it's all kinds of messed up and incomplete with <label> vs <h4> in the various wrapper templates. core/themes/classy/templates/form/datetime-wrapper.html.twig uses a label. Everything else still uses an h4.

I'm going to attempt to fix this now. Stay tuned.

Wow, what a mess. ;)

I think I'm doing this right. Instead of hard-coding <h4> and <label> and all kinds of other hackery in here, if we're going to use <label> for this, we should use the existing form label templates and other plumbing.

Here's the working test-only. Should fail. Removing 'Needs tests'.

I'll post patches for 8.6.x and 8.7.x next with the interdiffs.

Patches for 8.7.x and 8.6.x, interdiff relative to #93, and between these two patches.

Meanwhile, definitely "Needs markup review", and another look from someone who groks D8's theme stuff more than I do.

Notes for the markup review:

A) What's up with the inconsistencies in rendering errors?
Edit: out of scope, see comment #102, please ignore.
8.7.x HEAD has this
core/themes/classy/templates/form/datetime-wrapper.html.twig:

{% if errors %}
  <div class="form-item--error-message">
    <strong>{{ errors }}</strong>
  </div>
{% endif %}

core/themes/stable/templates/form/datetime-wrapper.html.twig

{% if errors %}
  <div>
    {{ errors }}
  </div>
{% endif %}

and core/modules/system/templates/datetime-wrapper.html.twig has this:

{% if errors %}
  <div>
    {{ errors }}
  </div>

I probably screwed this up "trying to be consistent". Do we really want a raw div in the 2nd two vs. using both the class and <strong> (wtf?) in classy?

B) Properly using the form label template takes care of setting these classes for us, and leaving them in the wrapper template is just pointless bloat, right?

{%
  set title_classes = [
    required ? 'js-form-required',
    required ? 'form-required',
  ]
%}

That's what I'm seeing when I test locally. Please confirm this is a legit change.

Thanks!
-Derek

whoops, forgot this crucial step. ;)

p.s. for fun, here's an interdiff between #79 and #98.

Upon further reflection...

Re: #96: Actually, it wasn't a question of <label> vs. <label for="something"> that explained why the assertions weren't working. It was really <label> vs. <h4>. In hindsight, duh! My raw markup from #95 was because I was foolishly testing "live" in the site (after enabling the datetime_states test module), not looking at the actual HTML from the test itself. Had I done that earlier, I would have immediately seen that the xpath was failing because the thing I was looking for wasn't inside a <label> at all. The reason I thought I saw it working was because I had temporarily changed my test class to use 'standard' not 'minimal', and then all of a sudden we started using the one template (from classy) that actually had <label> in it. That was the test-only I accidentally uploaded in #97.

Re: #99.A: Out of scope and unrelated. I'm reverting my "fixes" that touch {{ errors }} since that has nothing to do with this issue, and only produces more possible markup changes that might piss someone off. ;)

I'm also removing the (obviously bogus) doc comment about supporting 'attribute' style labels in these templates, which I had copy/pasted without really thinking while trying to "reuse core's label plumbing". I am leaving in the after vs. before/invisible option for label_display, although a) that might be scope creep and b) I'm not really sure why anyone would want a datetime label after the elements. ;) Perhaps that should be ripped out, too.

Here's a shiny clean test-only, and new patches for 8.6.x and 8.7.x that undo the changes for {{ errors }}.

Fixed the summary to reflect the current state.
Also, tagging for 'needs JavaScript review' since I'd love some other eyes on that part of this.

Finally made time to actually read the whole history here.

Thanks @kevin.dutra for datetime_states.module. ;)

Re: #29: That error is fixed in #2672950: Notice: Undefined index: #default_value in Drupal\Core\Datetime\Element\Datetime::valueCallback() (line 103 which is already in 8.7.x but not in 8.6.x. I'm going to stop trying to include that fix in here at all. Instead, this patch (and all the 87x patches I've uploaded) apply cleanly to 8.6.x branch, too. If you want that error to go away on 8.6.x, use #2672950. I'm trying to get that backported. See also #2992580: Custom callbacks doesn't work.

Re: #56.1: Good point. Yeah, it seemed weird to have the tests in datetime.module. Moved to system at @mpdonadio's request, fixed the module name to be "datetime_states_test" to be consistent with everything else in there, updated the route name and path, class name, etc.

Re: #56.2: Now I understand why this was in the "theme system" component. Sure, I guess that's okay for now. Perhaps "forms system" would be a more appropriate spot for the final home for this issue once the theme folks sign-off on the markup changes.

New clean test-only. I'll upload the full patch next.

p.s. I'm hiding a bunch of old files that are no longer relevant.

Forgot the interdiffs.
102 to 106 (recent fixes).
79 to 106 (everything I've touched since I started on this).

Ugh, I left one more of the bogus 'attribute' docblocks in.

Just took a quick look at the test, looks great, just some nits.

1. +++ b/core/modules/system/tests/src/FunctionalJavascript/DatetimeStatesTest.php
   @@ -0,0 +1,119 @@
   +  /**
   +   * The profile to install as a basis for testing.
   +   *
   +   * This test uses the minimal profile since it's just twiddling a single form.
   +   *
   +   * @var string
   +   */
   +  protected $profile = 'minimal';
   

   If you leave this out, it will use the testing profile, which is even more minimal then minimal, tried it locally and works fine.

2. +++ b/core/modules/system/tests/src/FunctionalJavascript/DatetimeStatesTest.php
   @@ -0,0 +1,119 @@
   +  public static $modules = ['datetime_states_test'];
   

   this needs to be protected

Re: #110: Thanks! Both fixed.

I think if we are making the change to the Classy template, the we should also mirror the change in the system one (but we don't touch stable).

And, changing markup in Classy requires frontend manager review and a CR.

Also tested this manually. In a default install, the label isn't visible.

I don't recall why we didn't tackle the h4 vs label in #1918994: Improve Datetime and Daterange Widget accessibility.

*sigh* So we leave "stable" broken? That's a win for people using stable? Weird. I don't understand this "policy". The markup is broken. Why don't we fix it? *shrug*

Meanwhile, what do you mean the label isn't visible in a default install? Here's before and after with #111 applied to a clean 8.7.x dev site. Both a date and daterange field:

Before:

After:

I submitted an initial draft CR

Before/after screenshots with a date-only field, too. I think something else is fubar w/ mpdonadio's test site for the screenshot in #112. ;)

I don't know what "work" this needs. I agree we need a frontend framework manager review...

I really don't think we should leave "stable" broken. That makes absolutely no sense to me.

I retract my problem. Re-applied patch and retested and it works.

While we're fixing this template, we should also fix #2982187: datetime-wrapper.html.twig ignores #description_display parameter. I believe we'd only need a single CR for both issues. I don't know the best way to coordinate. These two patches conflict for now. This one here seems like the bigger bug, but I'm open to suggestions on the best way forward. Should we merge those fixes and tests into here? Should we leave them separate and try to coordinate?

Thanks,
-Derek

Probably not worth trying to fix this until we get some clarity at #3031198: [META] Add classy_dev and minor-branch theme snapshots to allow fixing markup bugs within minor releases without front-end disruption.

Hi, just to report that small change is necessary for date list element if someone using this patch at #111

For #states to work one `datelist` field type, it should be added one small line at
\Drupal\Core\Datetime\Element\Datelist::processDatelist at line 272
'#states' => empty($element['#states']) ? [] : $element['#states'],

Sorry for the noise, but #3031198: [META] Add classy_dev and minor-branch theme snapshots to allow fixing markup bugs within minor releases without front-end disruption has evolved, and now I see we have #2352949: Deprecate using Classy as the default theme for the 'testing' profile as the immediate solution to our bugfix deadlock.

p.s. the <strong> WTF from #99 is fixed here: #3010558: Unnecessary <strong> element in Umami's form-element template may produce invalid markup

I recommend that we fix this bug in core without fixing it in Classy. Classy is potentially getting removed from Drupal Core prior to Drupal 9 on this issue: #3050378: [meta] Replace Classy with a starterkit theme. We could provide a change record with instruction for users to override Classy template with a version that fixes this bug. What comes to the test coverage, I recommend that you use Stark theme for building the test coverage, and add a @todo for that to be removed as part of #2352949: Deprecate using Classy as the default theme for the 'testing' profile.

I'm planning to provide this issue with a more thorough review later to remove the tag.

#111 doesn't apply for me (8.7.6) because I am also applying https://www.drupal.org/project/drupal/issues/994360#comment-12489118

Re: #122 sounds like a plan, thanks!

I didn't have time to do all of that, but I needed a re-roll of this for composer and Getting Things Done(tm) on real sites. Here are patches for All The Branches(tm). Hopefully I can find some time to do what you suggest (change the tests to use stark, leave classy broken, write a CR, etc). This is blocking not just contrib, but also #2648950: [PP-2] Use form element of type date instead textfield when selecting a date in an exposed filter so I'd love to see a viable solution land soon.

Cheers,
-Derek

p.s. Interdiff is empty, these are straight re-rolls.

A new day, a little sleep, and a firm desire to finally put this bug to rest. ;) I'm going to take a stab at this now. Stay tuned.

Amazing! That was significantly easier than I feared. ;) The test required 0 changes beyond this:

  /**
    * {@inheritdoc}
    */
  protected $defaultTheme = 'stark';

Still passes locally, still has very complete coverage of this fix.

The only other changes here are reverting the fixes for the datetime-wrapper.html.twig template from classy and stable.

While the testbot chews on this, I'll update the CR: https://www.drupal.org/node/3030265 to clarify that classy and stable are not fixed, providing instructions on how to define your own datetime-wrapper.html.twig to benefit from the fixes.

p.s. Same patch applies cleanly to 8.9.x, 8.8.x and and 8.7.x, so I'll only upload 1 copy. I'll queue it for testing on all 3 branches, though.

CR updated: https://www.drupal.org/node/3030265/revisions/view/11283996/11619669

Meanwhile, I had a thought -- are we allowed to fix Seven, Bartik and Claro? They all extend classy, so they're all broken. But I don't believe they're still subjected to the same BC rules, right? If so, we'll need different patches for 8.9.x / 8.8.x vs. 8.7.x, since Claro is new in 8.8.x

Claro already overrides datetime-wrapper.html.twig and adds a wrapper <div>. Smart theme! ;) It would only need some minor updates, or maybe already Just Works(tm). Needs testing.

Bartik and Seven are still fully broken, relying on Classy.

Thoughts?

Almost there! :)

YAY!!
-Derek

Whoops, right, #2352949: Deprecate using Classy as the default theme for the 'testing' profile wasn't backported to 8.7.x. ;) I doubt this is going to be, either, so maybe I won't care. I'll ignore 8.7.x for the testbot for now.

Next, bot is complaining about 13 CS violations, but I don't understand them, since none of it looks like it's touched by my patch:

13 coding standards messages
✗ 13 more than branch result

/var/lib/drupalci/workspace/jenkins-drupal_patches-14570/source/core/misc/states.es6.js ✗ 1 more
line 0	File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
/var/lib/drupalci/workspace/jenkins-drupal_patches-14570/source/core/misc/states.js ✗ 1 more
0	File ignored because of a matching ignore pattern. Use "--no-ignore" to override.
/var/lib/drupalci/workspace/jenkins-drupal_patches-14570/source/core/themes/seven/css/components/form.css ✗ 11 more
57	Element (tr.odd) is overqualified, just use .odd without element name.
58	Element (tr.even) is overqualified, just use .even without element name.
103	Element (select.form-select) is overqualified, just use .form-select without element name.
274	Background image '../../../../misc/icons/333333/caret-down.svg' was used multiple times, first declared at line 258, col 5.
286	Don't use IDs in selectors.
289	Don't use IDs in selectors.
337	Element (div.password-suggestions) is overqualified, just use .password-suggestions without element name.
340	Don't use IDs in selectors.
344	Don't use IDs in selectors.
354	Don't use IDs in selectors.
355	Element (div.filter-options) is overqualified, just use .filter-options without element name.

Preemptively hoping we can fix the other core themes, here's a patch that at least fixes Bartik and Seven. Other than copying the templates to the right places, it also adds an admin route to datetime_states_test so you can play with the test form via an admin theme at /admin/datetime-states-test/example. I'm not sure if/how we should try to run this test N times for all the core themes, that seems out of scope and a bigger problem to solve than a 1-off solution for this one JS test.

I started trying to fix Claro. Since there's already a datetime-wrapper.html.twig template, as I feared, it requires more work. I'm really not clear on the right solutions. A few issues:

1. Claro uses form-item__description not description for the class on the form description. states.es6.js doesn't know about this, and only targets description. Seems reasonable to have states.es6.js target both, but I'm not sure.
2. Claro's template is using the <h4> with title_classes and this: <h4{{ title_attributes.addClass(title_classes) }}>{{ title }}</h4>. Once again, states.es6.js doesn't know about this, and is specifically targeting <label>, not whatever happens to have the form-item__label class. Not sure if it's better to change Claro's template to use <label> or to try to expand states.es6.js to handle both cases.
3. Also not clear about the status of Claro, if we can still make changes without worrying about BC, etc. E.g. if we *do* move to <label>, we probably want to change the title_classes stuff to do something like this:
   

   {%
     set label_classes = [
       required ? 'js-form-required',
       required ? 'form-required',
       errors ? 'has-error',
     ]
   %}
   {%
     set label_attributes = label_attributes.addClass(label_classes)
   %}
   

   Right? Is that legit?

Sorry I'm not much of a front-ender, and not versed in the inner workings of Claro. Any help/directory would be lovely!

Thanks,
-Derek

For fun, here's how I think we're supposed to get a specific test to use stark in 8.7.x. Seems to work locally. Hope the bot agrees. Still not clear if this is viable for an 8.7.x backport, but if so, this should work.

Patch looks really good to me. I'm not sure what's the policy around HTML structure change in minor/patch release.

+++ b/core/misc/states.js
index a14da89..81f0060 100644
--- a/core/modules/system/templates/datetime-wrapper.html.twig

--- a/core/modules/system/templates/datetime-wrapper.html.twig
+++ b/core/modules/system/templates/datetime-wrapper.html.twig

OMG, yes to bringing this template inline like other fapi element.

+++ b/core/modules/system/tests/src/FunctionalJavascript/DatetimeStatesTest.php
@@ -0,0 +1,116 @@
+    $this->assertTrue($this->container->get('theme_installer')->install(['stark']));
+    $this->container->get('config.factory')
+      ->getEditable('system.theme')
+      ->set('default', 'stark')
+      ->save();

We should be able to just set the the theme property on class.

Re: #132:

We should be able to just set the the theme property on class.

You mean like #129 does?

+++ b/core/modules/system/tests/src/FunctionalJavascript/DatetimeStatesTest.php
@@ -0,0 +1,115 @@
+  /**
+   * {@inheritdoc}
+   */
+  protected $defaultTheme = 'stark';

That's new in 8.8.x branch. I don't think there's any other way to do it in the 8.7.x branch, which is why I posted #130 as an 8.7.x-only patch.

Re: #133: Yeah you are correct. I confused \Drupal\Tests\BrowserTestBase::$profile with the theme as well.

Still haven't heard anything from anyone in Slack that knows anything about Claro. I hope we can fix Claro, too. 🤞

If so, this seems to do it. Would still love feedback from someone who knows. ;)

Here are my answers to my questions from #129:

1. I taught states.es6.js to target both div.description, div.form-item__description.
2. Converted it to a <label>.
3. Actually, that mostly seemed like cruft, given claro_preprocess_datetime_wrapper(). So I fixed the preprocess and removed all the manual class setting in the template. Note that by solving #2, we're already getting form-required and js-form-required from themes/classy/templates/form/form-element-label.html.twig.

Also note: I added the validateForm() method on the datetime_states_test form to play with it and make sure the has-error class is getting set properly on the label. Writing more automated tests for this across all browsers seems out of scope, but at least this simplifies manual testing in the meanwhile. ;)

Well, poo. The changes to states.es6.js in here are weird, don't fully work, and seem needlessly complex.

Instead of all the mojo for js-complex-form-item trying to find the right sub-elements (label, description, etc), let's just toggle the visibility of the entire thing with the js-complex-form-item class. When I tested manually with stark, I realized the description wasn't getting toggled. Same problem as #129.3 only there is *no* class at all. ;)

So here's better test coverage of the problem, and a fix to states(.es6)?.js that seems to work well on all the core themes.

Updating summary that all the core themes (except classy and stable) are fixed in #136, but we still need to review / sign-off that we're allowed to make these changes.

Sorry about the 8.7.x fail. I was hasty and pulled in the 8.8.x $defaultTheme stuff into the wrong branch. This will pass.

Meanwhile, I was looking more at the states.es6.js changes, and was wondering why bother with all this:

      $(e.target)
        .closest('.js-form-item, .js-form-submit, .js-form-wrapper')
        .toggle(e.value);
      // For a complex form element like 'datetime' (one made up of multiple
      // sub-elements) we want to toggle visiblity on the entire element
      // (including sub-elements, label, description, etc.
      const $complexElement = $(e.target)
        .closest('.js-complex-form-item');
      if ($complexElement.length) {
        $complexElement.toggle(e.value);
      }

Why not just this?

      $(e.target)
        .closest('.js-complex-form-item, .js-form-item, .js-form-submit, .js-form-wrapper')
        .toggle(e.value);

For reasons I can't explain, that doesn't work. The automated test fails, and testing manually confirms. Attaching here for comparison / reference. #136 is still the right patch for 8.8.x and above branches.

I'll put the now-working 8.7.x patch last here, so hopefully the bot doesn't mark this NW because of the WILL-FAIL patch.

FYI. The date capability in forms doesn't work for me using a container. I've attached the code.

My configuration:
Drupal core 8.7.8
PHP 7.3.11 (cli) (built: Oct 22 2019 08:11:04) ( NTS )
mysql Ver 15.1 Distrib 5.5.64-MariaDB, for Linux (x86_64) using readline 5.1

resend
FYI. #STATES in containers does not appear to work with dates in forms API. I've uploaded the code excerpt in StatesDateFail.txt. My configuration appears below.

Drupal 8.7.8
PHP 7.3.11
mysql Ver 15.1 Distrib 5.5.64-MariaDB, for Linux (x86_64) using readline 5.1

@othdvlpr: Sorry, it's really hard to make sense of your code, since there's a lot of stuff commented out, it's not indented properly, and you're embedding <table> markup all over the place using #prefix and #suffix. It's also not clear if you've applied the latest patch in here or not.

More to the point, the patch in this issue is about getting #states to work properly directly on form elements of #type 'datetime'. It kinda looks like your code is trying to get a given container to appear/disappear based on if a form element of type 'date' has a value or not. If so, that'd be more appropriately sorted out in a support request in another issue, since that's not what this issue is trying to solve.

tl;dr, I don't think this is the right issue to try to help you with your form. ;)

Thanks,
-Derek

Yay. In the #admin-ui Slack meeting today (2019-11-06) I asked about fixing this bug in Claro, to which @lauriii replied:
"We can make changes to Claro 🎉"
Updating summary accordingly. ;)

Also from Slack, @andrewmacpherson writes:

The <h4> isn't ideal, but replacing it with <label> isn't straightforward, because it's a compound field (date and time).

Fieldset/legend is better, but then we get nested fieldsets when it's a multi-value field. Those work well in a sense, but they are sometimes confusing for screen reader users. Currently screen readers typically announce when you enter a fieldset, but not when you leave one. So when moving from one nested fieldset to another, users sometimes get confused about the nesting depth. The screen readers are handling the machine-readable structure well, it's just that the way they convey it to users isn't great.

To which I'll reply here:

A fieldset + legend would also be problematic when using a datetime element in an exposed filter on a view. That's (part of) why I'm trying to fix this bug, to unblock:
#2648950: [PP-2] Use form element of type date instead textfield when selecting a date in an exposed filter (one of the top-20 most followed issues in the core queue).
Especially in combo with #2625136: Fix label visibility and add wrapper container for exposed numeric/date filters with multiple form elements -- we'd have nested, ugly (and potentially confusing) fieldsets/wrappers/etc. Ooof. :/

Also, got a link to #3078334: Datetime and Datelist elements should render as fieldsets which I believe is duplicate with this issue, so I marked it as such. Technically, maybe we could fix that first, then take advantage of that here, but there's already so much prior discussion and work here, it seems counter-productive to fork the discussion. If @lauriii and @andrewmacpherson believe otherwise, I'm happy to be wrong, and we can re-open that one, postpone this, and get all hands on deck over there. *shrug*
Edit: @andrewmacpherson and I discussed in Slack and agreed to re-open #3078334: Datetime and Datelist elements should render as fieldsets.

Upon further discussion, @andrewmacpherson and I decided to postpone this on getting #3078334: Datetime and Datelist elements should render as fieldsets fixed, first. That's a more serious accessibility bug, that potentially is fixable in All The Templates(tm), even Classy + stable. TBD. Regardless, it's a smaller scope, so hopefully more easy to deal with separately. Then, the work in here can focus on the lack of a wrapper + the states.js updates, the JS tests, etc.

Meanwhile, they pointed out that the summary in here was somewhat lacking, didn't actually describe the bug, etc. So I tried to flesh that out a bit more. Also updated remaining tasks and other sections to indicate that the <h4> vs. <label> parts are happening at #3078334, not here.

#136 needs a re-roll on top of #3078334: Datetime and Datelist elements should render as fieldsets. As datetime-wrapper.html.twig gets removed in that issue in later patches.

Hello just following up on the current status of this issue? I'm running Drupal 8.8 and we need to make some date fields conditionally required. Any suggestion or help would be appreciated!

@smustgrave re: #149: If you want minimal disruption that works, you can use #136 (or a re-roll of that) and it should be fine. However, I don't think that's the direction core will (eventually) go. #3078334: Datetime and Datelist elements should render as fieldsets needs to happen for other reasons, and when it does, this bug will magically go away (since there will no longer be the broken custom "datetime wrapper" that doesn't actually include a wrapper). #3078334 is where the energy needs to go for fixing this in core, but I ran out of time/steam for that, and (as usual) I needed to simply apply this patch to the project and move on. Huzzah for technical debt! :/ For a working D8.8 site, the project where I care about this functionality needs 13 core patches in composer.json. :(

Added relevant tag

Reroll and removes core version per #3072702: Core extensions should not need to specify the new core_version_requirement in *.info.yml files so that 9.0.x can be installed

Reroll of #135 with the same fix as #152 - no generated interdiff as it's the same as #152

Recompiled states.es6.js for reroll as its changed again in 9.5.x, no change to patch (for use until the referenced issue in #150 is resolved).

Rerolled patch against 9.5.x

Disregard the comment above :) Was writing multiple patches and uploaded the wrong one.

This should be the correct reroll of this patch to 9.5.x

Here is a reroll for 10.0.