Datetime and Datelist elements should render as fieldsets

Thanks for filing this @tormi.

The way we use H4 here is a bit awkward, sure. We don't use headings for fields or form elements in general. It makes these form elements seem more important than other ones.

I think it's worth revisiting how we label our compound date and time inputs, to see if we can improve semantics in a reliable way.

However, just replacing the <h4> with <label> here isn't ideal. It results in a label which isn't programmatically linked to any input. It may as well be a span.

In the case of the node "authored on" info, the individual date and time inputs already have <label for>. For assistive tech, these are programatically labelled as "date" and "time". However there's no programmatically-determinable association between these inputs and the phrase "authored on" (either before or after the patch here). The "authored on" group would be improved by treating it as a fieldset with at legend, instead of a h4.

In the case of a single datetime field (Field API, datetime module) we already get a fieldset, using the the field name as a legend. The individual date and time inputs have <label for>, so screen reader users skipping through form elements will hear "preferred date, date" (for example). That's great for a single-value datetime field.

In the case of a datetime range field (datetime_range module), the field name is treated as a legend, but the start and end each have a h4 from the datetime-wrapper. This might be better with nested fieldsets (field_name (legend) > start date (legend) > date (label). Nested fieldsets are sometimes confusing for screen reader users though, so we'd want to tread carefully here.

Review of patch #3:

This only changes the template in the stable theme. so it has no effect when using Seven. Whatever changes we make here should be repeated for each of the datetime-wrapper.html.twig templates: in system module, stable theme, and classy theme.

Working on this at Drupalcon Amsterdam

Working on this at Drupalcon Amsterdam

Helping out with this today!

To reproduce the issue, you can use a vanilla installation:

Visit /node/add/article

And take a look in the "authoring information" section and you'll see the incorrect behavior.

How to find the 4 files

PHP Storm: Navigate --> File --> Search for "datetime-wrapper.html.twig"

* core/modules/system/templates/datetime-wrapper.html.twig
* core/themes/claro/templates/datetime-wrapper.html.twig
* core/themes/classy/templates/form/datetime-wrapper.html.twig
* core/themes/stable/templates/form/datetime-wrapper.html.twig

*Banana skin warning*

Seven admin theme inherits from Classy theme. So by editing the file in core/themes/classy/templates/form/datetime-wrapper.html.twig it should you should be able to replicate and fix the issue.

Before

After

I've edited the 4 files. See attached patch.

Thanks to @Waliur, @joshk, and @michaellenahan for working on this.

I've tested this and the the results match the screen shots by @Waliur in comment #17.

As this touches the stable theme, this will need to have the "Needs frontend framework manager review" tag when it's ready for RTCB.

@andrewmacpherson in comment #8 states that "just replacing the <h4> with <label> here isn't ideal" and "this might be better with nested fieldsets". The current patch doesn't wrap the fields in a fieldset. Setting back to "Needs work" because of this.

Yes, this is broken. However, we already need to fix this as part of #2419131: [PP-1] #states attribute does not work on #type datetime which is a much older issue, with a bunch more people following it, and there's already working patches and tests for this change. There's also word in there from @lauriii (Front end framework manager) that we're not allowed to fix this bug in Classy for BC reasons...

Ah, lots of new comments. Thanks for working on this at DrupalCon Amsterdam. Tagging for another look at this once the 8.8.0 beta/RC rush has died down.

Meanwhile, @dww told me about another issue with some overlap with this one. #2419131: [PP-1] #states attribute does not work on #type datetime. We should look at these carefully together, and maybe merge them.

Upon further discussion in Slack, @andrewmacpherson and I agreed to re-open this and postpone #2419131: [PP-1] #states attribute does not work on #type datetime on getting this fixed, first.

Initial stab at a real issue summary.

Thanks @dww!

Re: fieldsets:

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. :/

More updates to the summary, including pasting in large chunks of #8 into the proposed resolution section.

Instead of nested fieldsets, is this a possibility?
Approach 2: Associating related controls with WAI-ARIA:

WAI-ARIA provides a grouping role that functions similarly to fieldset and legend. In this example, the div element has role=group to indicate that the contained elements are members of a group and the aria-labelledby attribute references the id for text that will serve as the label for the group.

This technique provides additional styling possibilities.
...
Because WAI-ARIA not fully supported in all web browser and screen reader combinations, a group identifier should be added to the first form control in the group.

It suggests code that looks like this:

<div role="group" aria-labelledby="shipping_head">
	<div id="shipping_head">Shipping Address:</div>
	<div>
		<label for="shipping_name">
      <span class="visuallyhidden">Shipping </span>Name:
    </label><br>
		<input type="text" name="shipping_name" id="shipping_name">
	</div>
	[…]
</div>
<div role="group" aria-labelledby="billing_head">
	<div id="billing_head">Billing Address:</div>
	<div>
		<label for="billing_name">
      <span class="visuallyhidden">Billing </span>Name:
    </label><br>
		<input type="text" name="billing_name" id="billing_name">
	</div>
	[…]
</div>

Adopting the markup from Bartik's Authored on and adjusting for the above, I get something like this:

<div role="group" aria-labelledby="edit-created-header" class="field--type-created field--name-created field--widget-datetime-timestamp js-form-wrapper form-wrapper" data-drupal-selector="edit-created-wrapper" id="edit-created-wrapper">
  <label id="edit-created-header">Authored on</label>
  <div id="edit-created-0-value" data-drupal-field-elements="date-time" class="container-inline">
    <div class="js-form-item form-item js-form-type-date form-type-date js-form-item-created-0-value-date form-item-created-0-value-date form-no-label">
      <label for="edit-created-0-value-date" class="visually-hidden">Date</label>
      <input data-drupal-selector="edit-created-0-value-date" aria-describedby="edit-created-0-value--description" title="Date (e.g. 2019-11-06)" type="date" min="1902-01-01" max="2037-12-31" data-drupal-date-format="Y-m-d" placeholder="YYYY-MM-DD" data-help="Enter the date using the format YYYY-MM-DD (e.g., 2019-11-06)." id="edit-created-0-value-date" name="created[0][value][date]" value="2019-11-06" size="12" class="form-date" />
    </div>
    <div class="js-form-item form-item js-form-type-date form-type-date js-form-item-created-0-value-time form-item-created-0-value-time form-no-label">
      <label for="edit-created-0-value-time" class="visually-hidden">Time</label>
      <input data-drupal-selector="edit-created-0-value-time" aria-describedby="edit-created-0-value--description" title="Time (e.g. 21:52:36)" type="time" step="1" placeholder="hh:mm:ss" data-help="Enter the time using the format hh:mm:ss (e.g., 21:52:37)." id="edit-created-0-value-time" name="created[0][value][time]" value="21:52:36" size="12" class="form-time" />
    </div>
  </div>
</div>

Could that save us the visual clutter of nested fieldsets, but still be semantically meaningful and fully accessible with assistive tech?
https://caniuse.com/#feat=wai-aria seems pretty solid across browsers that D8+ core is supporting, no?

A quick grep indicates we're already using aria-labelledby in some parts of core.

Pardon my ignorance, but is this worth exploring? I put it into the summary as option B (and clarified/updated option A for fieldsets).

Thanks!
-Derek

Removing visual clutter from the example proposed markup in option B. ;) Oh the irony. Want to make it easier to make sense of what's being proposed. Leaving the full details in #27 if someone really wants to see everything.

In Slack, @andrewmacpherson told me:

Google "first rule of ARIA"

https://www.w3.org/TR/using-aria/#firstrule

If you can use a native HTML element [HTML51] or attribute with the semantics and behavior you require already built in, instead of re-purposing an element and adding an ARIA role, state or property to make it accessible, then do so.

So, that's probably going to be a strong vote for option A - nested fieldsets.

Here's a very rough and still quite broken patch that starts down that road. This is somewhat based on the changes I wrote for #2419131: [PP-1] #states attribute does not work on #type datetime. Lots of @todo and incomplete, but it's a proof-of-concept on the approach. Do not queue this for testing, not even close to being ready. But enough to get initial screenshots on Seven.

Before

Clean 8.8.x dev site, with an 'Event' node type with 3 fields: date-only, datetime, and datetime_range, and 'Authored on' visible:

After

Patch applied, caches cleared, re-load the node/add/event form:

TODO

1. Obviously, we'd need to fix things so we don't have the extra fieldset with a Field API datetime-only formatter ("DATE / TIME" field label).
2. Probably don't need the fieldset for date-only fields at all, either ("DATE" field label).
3. We might want some CSS so that the legend for datetime fieldset wrappers is NOT ALL CAPS.
4. The implementation is gross. We should either re-use fieldset.html.twig or refactor things so it's easier to share. E.g. a #theme => 'fieldset_legend' pipeline or something (akin to #theme => 'form_element_label').

All that said, I'm still not sold this is better than ARIA group + described by. ;) This is turning into a much more disruptive change (both markup/code, and visually). Yes, I hear the First Rule and I understand it. But this might be a situation to re-consider. If we "only" added the wrapper div with group + described_by, we'd have a lot less to do here, we wouldn't introduce so much visual change, but we could still make all this much more accessible than it is now.

#27:

Pardon my ignorance, but is this worth exploring? I put it into the summary as option B

Not really. Options A and B are semantically equivalent as far as assistive tech is concerned; try inspecting that page with the Firefox accessibility tree inspector to see what the browser sends to the OS-level accessibility API.

What the WAI article doesn't make clear, is that option B is really just a fallback for where you can't use HTML fieldset/legend for some reason. See the First Rule of ARIA Use. Situations where you might use the ARIA faux-fieldset are:

• When you don't have control of the HTML templates (e.g. the HTML markup is hardcoded in some ASP.NET assembly)
• You have built a UI component from SVG rather than HTML, and need to add form semantics.

There's little point in option B, because Drupal already has all the theming infrastructure for the HTML fieldset/legend (render element, theme_wrapper, preprocess, twig, and Classy CSS).

Could that save us the visual clutter of nested fieldsets,

Fieldsets don't have to be visual clutter. It's fine to style a fieldset with border: none;. The legend can be visually-hidden - template_preprocess_fieldset() already handles '#title_display' => 'invisible' for fieldset legends.

I recall reading that there are some bugs using CSS grids to arrange content inside of fieldset elements, but we can probably avoid that for our date+time use case.

#29: I skim-read the patch to see how you did it.

It might be simpler. Can you re-use the existing code for fieldsets, with something like:

$element['#theme_wrappers'][] = 'fieldset';

There's an example of this in DateTimeDateTimeWidget, which uses a fieldset wrapper.
There's also a similar example in FileWidget, but that uses a details wrapper.

I haven't completely grokked how #theme_wrappers works, and there aren't many instances of it used in core, but I looked around the preprocess function for the fieldset and it already handles invisible legends.

Re: #31 -- yeah, I noticed #theme_wrappers, too. I was going to give that a spin. As I said, #29 was a super quick/dirty PoC to start to see how this would behave as nested fieldsets.

I *think* we might be able to avoid the double fieldset on the datetime (not datetime_range) field widget by conditionally checking if something upstream already set a fieldset #theme_wrapper, and if so, leaving it alone instead of adding our own.

Alternatively, we might have to change the upstream field widget to not wrap this in a fieldset, which might have more BC implications.

Re: visual clutter -- yeah, we can do some CSS tricks to make this look better, but not in classy itself, only seven, claro, bartik. At least that's my understanding of our front end BC policies. I think we need to see the legend in all these cases, not just hear it, so I don't think #title_display => 'invisible' is our friend here. But maybe some targeted CSS to NOT VISUALLY SHOUT it would be good (and acceptable to @lauriii et al). ;)

Anyway, I'll see if I can find time to push this forward in the near future. I'd love to see this fixed before 8.8.0 (among other reasons, since it's blocking a lot of other issues I care about), but I worry it's not going to be a candidate after beta1 (which is happening basically now). I'm hopeful since it's an #accessibility bug that it'll be acceptable during beta.

Thanks,
-Derek

(better title to more accurately convey the bug we're trying to fix)

Still needs work, but putting this up for review on the new approach.

Once we're inside template_preprocess_datetime_wrapper(), it's too late for #theme_wrappers to do any good. We've already decided what template we're using at that point.

So, here's an attempt at solving the problem at a different layer of the chain. Now, the underlying Datetime (and I discovered, Datelist) elements completely avoid using datetime_wrapper at all. They take on the smarts about if they have multiple form elements, and if so, using a fieldset for their #theme_wrappers, not datetime_wrapper.

This also means that the Field API widget plugins from Datetime module no longer have to worry about the conditional fieldset stuff -- that's handled directly by using '#type' => 'datetime' or '#type' => 'datelist' render arrays.

So here's the minimum patch that accomplishes all this. However, it really means that *all* of the datetime_wrapper plumbing is now dead code. We should probably rip it completely out. But let's do that as a separate patch. I don't fully understand the ramifications of this for BC, but I don't think they're pretty. ;) I suspect @Lauriii will say we can't go this far.

But it does seem pointless to take our existing datetime_wrapper template + preprocess world and have it duplicate the fieldset world, so I like the idea of ripping out custom (broken) handling for this and using existing functionality.

I don't know how we deprecate twig templates for removal. Maybe we need a @trigger_error() inside template_preprocess_datetime_wrapper() that says it's no longer supported, not invoked by core, and if you're calling it directly for some reason, you should stop?

I also didn't start trying to do theme-specific CSS to make this any prettier.

Anyway, would love feedback on this approach before I/we go any further.

Before (now with a Datelist)

After

Thanks!
-Derek

p.s. Radically different patch, so no interdiff from #29.

Here's how much code we can remove if we do this. ;)

Again, I'm guessing the BC considerations makes this un-doable. But it's a nice thought...

Curious what the bot thinks of this.

That's less failing than I feared. ;)

In related news, I took my new test module and JS test from #2419131-136: [PP-1] #states attribute does not work on #type datetime (but no other changes from that issue), applied it on top of #36, and #states basically All Just Works(tm). The only part that doesn't is the "required" myth, but a) that should be easy to fix and b) I believe it's completely stupid that #states lets you visually toggle that since it has no bearing on anything. ;) But whatever.

Just had a Slack chat w/ @lauriii about this. Pasting his replies with permission:

lauriii 7 minutes ago
we need a change record for the render array change and we should figure out how to deprecate the template but other than that it should be fine

lauriii 6 minutes ago
and we also need a11y and UX sign off on that :slightly_smiling_face:

lauriii 6 minutes ago
render arrays can be changed in minor releases

So yay! In principle, we can proceed down the path of #35. Huzzah!

Updating the summary to clarify we're committed to option A (nested fieldsets), and to update remaining tasks accordingly.

@dww would you say the patch in #35 is good to go for 8.9? Or would you have another recommendation? The problem I'm currently facing is for a government mandated content type where certain fields need to be made required if the value of a dropdown is A or B.

@smustgrave: No, #35 is still broken, as the test results show. :( Just haven't had time to move this forward again, and no one else has stepped up to help.

Note: Using #states to control required-ness is a total joke. All it does is toggle the little red asterix thingy. If you actually want the fields to be conditionally required, you need to implement custom validation. It's extra wonky if you're dealing with an entity form, since then there's entity constraints to deal with, too. It's a mess. If you have a budget and want to hire me for a few hours to consult you on it, let me know.

Thanks/sorry,
-Derek

Widening the scope of the title to match where we've ended up.

Opened #3157353: Provide a mechanism to mark entire twig templates as deprecated to address point 3 in remaining tasks.

Created a reroll.

Moving to NW for failing tests and MR re-roll.

Created merge request for 9.4.x https://git.drupalcode.org/project/drupal/-/merge_requests/2730
Created merge request for 9.5.x https://git.drupalcode.org/project/drupal/-/merge_requests/2731

Test needs revision:

There were 3 errors:

1) Drupal\Tests\datetime\Functional\DateTimeFieldTest::testDateField
Behat\Mink\Exception\ElementNotFoundException: Element matching xpath "//input[@aria-describedby="edit-qtyi1yyk-0-value--description"]" not found.

/var/www/html/vendor/behat/mink/src/WebAssert.php:418
/var/www/html/core/tests/Drupal/Tests/WebAssert.php:968
/var/www/html/core/modules/datetime/tests/src/Functional/DateTimeFieldTest.php:61
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:726

2) Drupal\Tests\datetime\Functional\DateTimeFieldTest::testDatetimeField
Behat\Mink\Exception\ElementNotFoundException: Element matching xpath "//fieldset[@id="edit-tuuxtpnc-0"]/legend" not found.

/var/www/html/vendor/behat/mink/src/WebAssert.php:418
/var/www/html/core/tests/Drupal/Tests/WebAssert.php:968
/var/www/html/vendor/behat/mink/src/WebAssert.php:457
/var/www/html/core/tests/Drupal/Tests/WebAssert.php:1005
/var/www/html/core/modules/datetime/tests/src/Functional/DateTimeFieldTest.php:253
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:726

3) Drupal\Tests\datetime\Functional\DateTimeFieldTest::testDatelistWidget
Behat\Mink\Exception\ElementNotFoundException: Element matching xpath "//fieldset[@id="edit-huqg2sid-0"]/legend" not found.

/var/www/html/vendor/behat/mink/src/WebAssert.php:418
/var/www/html/core/tests/Drupal/Tests/WebAssert.php:968
/var/www/html/vendor/behat/mink/src/WebAssert.php:457
/var/www/html/core/tests/Drupal/Tests/WebAssert.php:1005
/var/www/html/core/modules/datetime/tests/src/Functional/DateTimeFieldTest.php:417
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:726

Appears to cause a regression UX wise.

Also adding a new for a CR since a new template will be needed to removed by themes.

Should the fieldsets be getting an aria-describedby attribute? Reading the failed tests and on
Drupal 10.1.x standard install
I added a datetime field to the basic content type
I don't see the attribute.

Tagging for accessibility review to make sure this is going in the right track.

Fixed up the tests a bit from the 9.5/9.4 MR and updated form.inc to add that aria-describedby

  $description_id = $element['#attributes']['id'] . '--description';
  // Add the description's id to the fieldset aria attributes.
  $variables['attributes']['aria-describedby'] = $description_id;

But under this scenario. If the date field storage is set to just date and not datetime. Should it be wrapped? Does it need aria-describedby? Commented that one assertion out to see what else fails.

This issue is being worked on in the MR

Please do not reroll a patch

Only uploading this because the MR needs fixing.

I've applied the MR3221 to a Drupal 10.1.0-dev install with PHP 8.2.0 and quickly tested with voiceover on macos 12.6.1 two or three days ago - but haven't had the time and capacity to write up a comment until now. when i've updated the dependencies to the install today, before writing up the comment, the patch failed to apply now (but not sure if there was another commit since my recording). I've noticed a few details in the linked videos. I went into an existing node created for an anonymous user with devel and wanted to change the date to 2023-05-12 and change the hour value to 19. i've tested in safari 16.1 and the latest version of firefox and edge:

Safari (16.1)

• The label authored on isn't announced.
• The description The time that the node was created. is announced twice, once for the date field and once for the time field
• The date field shows 01/05/2023 but in contrast voiceover announces YYYY-MM-D. The actual value isn't announced and based on the announced format of YYYY-MM-D i've entered 2023 as the first value which ends up to be 03 then. i tab once and enter 05 for the month and the value ends up to be 05 (which is correct). i tab once again and enter 12 for the day and so the year is set to 0012 now.
• The time format of the time field is set to the 12h time format (am/pm) but voiceover only announces hh-mm-ss. as an european i am used to a 24h time format (that is also what i am using in my operating system). with the announcement of hh-mm-ss simply relying on the aural presentation i would expect it to be a 24h time format. if i enter 19 as the hour value the actual value is set to 12

Firefox (Latest version)

• The label authored on is correctly announced.
• The description isn't announced at all.
• The date field is announced correctly and it doesn't have the example date format announcement like in edge later on.
• The time field is displayed in the 12h time format which isn't directly announced. but when i enter 19 the value gets automatically changed correctly to 07.

Edge (Latest version)

• The label authored on is correctly announced.
• The description isn't announced at all.
• For the date field the individual month, day year value is correctly announced as well as styled differently than in safari and firefox. ( but it might be easier cognitively if the date example (e.g. 2023-01-14 is in the same date format as the field currently have if it would be possible dynamically - but since safari and firefox arent announcing that example it is something edge is adding?)
• The time field is automatically in the 24h time format. i am able to enter 19 for the hour directly.

Sounds like got some work to do! Thanks for the detailed tests.

Also sounds like the tests can be updated as those aria described by fields may change

Can we re-base the MR on 11.x?

Don't have time to actually rebase but updated target.

I rebased the MR. It was not really possible at first because there were merge commits in the branch. Luckily it was pretty straight forward to get rid of the merged commits because there were no merge commits before the MR commits. I rangit reset --hard a9d6facb4e, where a9d6facb4e was the last commit before the commits that were introduced by the merge commit. After that I could run git rebase 11.x without any difficult conflicts to resolve.

Thanks for the review @rkoller! I'm wondering if you've tested the experience before this patch? It would be helpful if we could figure out how the experience changes as the result of this issue, so that we know which issues needs to be tackles here, and which issues should be moved into their own bug reports.

It looks like I mentioned this already in #38, but putting it again here for visibility. Instead of just removing the datetime_wrapper from drupal_common_theme(), we need to deprecate it. Unfortunately we haven't defined the steps for this #3157353: Provide a mechanism to mark entire twig templates as deprecated so this is something we'd need to come up with.

Here's the current state of the MR as a patch file for stable use in composer builds. This applies for me on 10.2.4.

Adding note on "current" behaviour for Drupal 10.2.5 with Claro admin theme.

I have a custom entity with 2 fields:
- BaseFieldDefinition::create('timestamp')
- BaseFieldDefinition::create('timestamp')

When rendering by Drupal\Core\Entity\ContentEntityForm, they appear as such:

However, changin the definition to BaseFieldDefinition::create('datetime') and clearing the cache:

Not sure if current MR/patch updates 'timestamp' too, but I would expect them to both display the same.

I've updated the patch for Drupal 10.3.1

I was trying to rebase with 11.x. Some functions are deprecated. Not sure which way should I go to fix this.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

I've merged 11.x into this branch and resolved all conflicts. I've reverted some of the code that removed datetime_wrapper theme hooks and preprocessing and I think we need to add the templates back as well and deprecate that properly.

Has the datetime-local element been considered as replacement for some of these scenarios? If there's a time element, this option is a much better UX to eliminate wrappers. Theres a few related patches that fix a few datetime-local issues.

There are a few browser compatibility issues for datetime-local with safari & firefox not showing the time picker but time can still be entered. Is that a deal breaker?
Internet Explorer doesn't support it all. Do we still care about IE?

For just dates, a custom element that extends Datetime would work that just removes the theme wrappers and replaces the date title with the element title. The only one that should get a fieldset wrapper IMO is the date list widget if we could adopt datetime-local.

I've reverted some of the code that removed datetime_wrapper theme hooks and preprocessing and I think we need to add the templates back as well and deprecate that properly.

I added the templates back and added a deprecation.

And I drafted a change record: https://www.drupal.org/node/3530128

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

I merged the latest 11.x and applied suggestions.

Think we need screenshots of what this going to look like after in claro and olivero. May need UX sign off depending on how severe the change is.

Added screenshots, there's at least small padding issue caused by css rule

.fieldset__legend--visible ~ .fieldset__wrapper {
  margin-block-start: 0;
}