Improve Datetime and Daterange Widget accessibility

Moving to right component

Ok, so this patch does nothing for the ui (?) but is for accessibility. After applying patch fieldsets-with-date-1918994-1.patch from mgifford in #1 on a fresh install of Drupal 8, it applies a fieldset and legend to date select lists on a content create/edit screen. Correct me if I'm wrong but it seems like it works well. See screenshots below for markup.

I am switching the status to reviewed if the patch is supposed to change the markup for fieldsets, which I confirm it does.

Can we use the invisible fieldset styling here?

Strange, I have no idea why system would overwrite it there. Perhaps system should provide a class for this too?

I have reviewed this patch. I verified that before the patch, the fieldset is not there. I verified after the patch is applied, the fieldset is there correctly. I verified this patch with both the Date and Time widget and the Select List widget. See attached images.

I have also confirmed that the border is visible. If the intention is for there to be no border, then this is a separate CSS issue.

Since that doesn't impact the accessibility of this control, I don't believe that should prevent this patch from being committed to core.

See #2084599: The 'authored on' vertical tab form items should be wrapped in a fieldset for an additional example.

Why all the selects to begin with? #1496652: Add new HTML5 FAPI Element : datetime-local

Ok, maybe this issue could be fixed with a patch as simple as this one.

I've tested it in Voiceover and it improves accessibility for both the standard widget and the select list widget.

However, we have a problem when multiple values can be entered in the field. The uix for a screen reader user in that case is pretty bad. But, maybe fixing that is out of scope for this issue (the problem is not introduced by this patch).

@mgifford, would leveraging the css class eventually introduced by #2002336: Introduce a CSS class to hide borders of fieldset elements be enough to solve the styling issues?

Adding concerns via #1467712: Make it possible to disable fieldset for date field:
If the input is actually one field, it should not be wrapped in a fieldset.

#22 accomplishes this, i think, but #29 always forces the input to be wrapped by a fieldset.

@mgifford I brought the changes from #2244923: The help text for datetime field needs an ID for screen readers. and closed the issue. Thanks again for walking me through #2126761: The body field summary textarea indicates it has a description with aria-describedby attribute, but the DOM id value points to a non-existent node!

The additions to this patch:

• add the aria-described by attribute to the datefield input
• associate the description with with the input by adding an ID to to the description

This change adds the aria-describedby attribute to the fieldset <fieldset class="fieldgroup form-composite"{{ attributes }}> with an else on line 249 of datetime.module, but I'm not sure its the best way to do it.

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

Hmm, I went back to patch #39 and it looks like there was double fieldsets there too. My guess is that both the date and time field are hitting datetime-wrapper.html.twig twice as rendered elements (content).

The date in the Authoring Information section on the right isn't in a double fieldset. Why is that one different from the content type field?

There doesn't seem to be a way to delete a row of dates...

I added the aria-describedby to the table field-date-values. I'm not sure what to do about the fieldset. Cottser suggested using a general form field wrapper instead of a date-specific wrapper.

Cottser said to use a more general #theme_wrapper in DateTimeDefaultWidget.php The results are pretty magical IMO.

I googled what this line of DateTimeFieldTest.php:101 is doing.
$this->assertFieldByXPath('//*[@id="edit-' . $field_name . '-wrapper"]/h4/span', '*', 'Required markup found');
It seems that Xpath is some xml element grabbing thing and its looking for the associated h4 or span? I removed that line from the test and it seems to pass simple test. I wonder if it's important...

Should I wrap the field-group in an if type == datetime ?

hmm...

looks like it should be working to me :(

The test originally said

$this->assertFieldByXPath('//*[@id="edit-' . $field_name . '-wrapper"]/h4/span', '*', 'Required markup found');

It had an extra span tag so I added an extra span too and it passes simple test locally.

$this->assertFieldByXPath('//*[@id="edit-' . $field_name . '-wrapper"]/fieldset/legend/span/span', '*', 'Required markup found');

I added the field-group class to the fieldset tag in DateTimeDefaultWidget.php:59 the CSS acting on it is

fieldset {
border: 1px solid #c0c0c0;
...
}

in normalize.css. If you disable it a thicker border shows up. I don't see a good attribute to grab to over-ride the rule.

The theme hook for fieldsets is registered by System module already, and we should be able to re-use the #pre_render callback of #type radios/checkboxes here.

At least I hope so. :-)

I don't see how to re-use the #pre_render callback of #type radios/checkboxes.

I read through ElementInfoInterface::getInfo and I see that form_pre_render_conditional_form_element adds the fieldset like we want.

How do we only wrap datetime and not just date by itself? I studied how radios and checkboxes work in system_element_info(), but throw in DateTimeDefaultWidget.php and I can't seem to connect the dots.

I want to add $types['date'] to datetime_element_info(), but it starts making duplicate fields.

@mgifford nice!!
The date only field was missing the aria-describedby attribute so I added it to template_preprocess_container()

  // Add the aria-describedby attribute for date fields.
  if (isset($element['widget']['#attributes']['aria-describedby'])) {
    $element['#attributes']['aria-describedby'] = $element['widget']['#attributes']['aria-describedby'];
  }

Should we add another date only field to DateTimeFieldTest.php?

Yep, I'll try adding the test field :)

The patch didn't apply so I rerolled it.

I think this patch does what we need it too and passes simpletest, but the test doesn't reflect the changes.

DateTimeFieldTest.php:65 sets the type as 'settings' => array('datetime_type' => 'date'), which I think hits the switch statement DateTimeDefaultWidget.php:61 case DateTimeItem::DATETIME_TYPE_DATE: which sets the theme_wrapper to datetime_wrapper. That is allowing the test:101 line $this->assertFieldByXPath('//*[@id="edit-' . $field_name . '-wrapper"]/h4[contains(@class, "form-required")]', TRUE, 'Required markup found'); to find the label under the h4. Since this test is testing date + time it should be looking under /fieldset/legend/span, but again because the datetime_type is set to date it's hitting the wrong wrapper.

I tried to change datetime_type to datetime, but I get AJAX errors when I run the test.

An AJAX HTTP error occurred.
HTTP Result Code: 500
Debugging information follows.
Path: /batch?id=22&op=do_nojs&op=do
StatusText: Internal Server Error
ResponseText:

I tried adding case DateTimeItem::DATETIME_TYPE_DATETIME: to the widget. Doesn't help...

When in doubt Cottser out. I brought it up during office hours and Cottser walked me through simpletesting.

Okay, I guess the test was closer than I thought. ¯\_(ツ)_/¯

I added this to DateTimeFieldTest.php:173 and I think that it covers all the changes from the patch.

$this->assertFieldByXPath('//*[@id="edit-' . $field_name . '-wrapper"]/fieldset/legend/span[contains(@class, "form-required")]', TRUE, 'Required markup found');

EDIT: @mgifford The help text seems to be showing up on a new line. I had to clear caches before to get them to show up right.

Yes, that's not right... Hmm

For a single date field the output matches now.

<div class="field-type-datetime field-name-field-date field-widget-datetime-default form-wrapper" id="edit-field-date-wrapper" aria-describedby="edit-field-date-0--description">

<div class="container-inline description" id="edit-field-date-0--description">well, what have we here?</div>

and for date + time

<fieldset class="container-inline fieldgroup form-composite form-wrapper form-item" aria-describedby="edit-field-date-0--description" id="edit-field-date-0">

<div class="description" id="edit-field-date-0--description">well, what have we here?</div>

Reroll for #2247991: [May 27] Move all module code from …/lib/Drupal/… to …/src/… for PSR-4.

Patch applies and the new attributes seem honky dory still.

The changes here seem to be more far reaching than just date - can we confirm that we are only affecting what we think we are.

The line if (isset($element['widget'][0]['#attributes']['aria-describedby'])) { is true for the title and tags field as well.

I don't think the container is the right place to put the ARIA

Applies and sounds good to me. The patch changes the wrapper template to use a fieldset.

+++ b/core/modules/datetime/src/Plugin/Field/FieldWidget/DateTimeDefaultWidget.php
@@ -69,6 +69,7 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
+        $element['#pre_render'][] = 'form_pre_render_conditional_form_element';

But this not a conditional form element.

I removed the form_pre_render_conditional_form_element and added an extra template and preprocess function for date + time.
Date

Date + Time

I added a rule to seven's style.css to hide the fieldset border.

.field-type-datetime .form-item {
  border: none;
}

Poor testbot.
reroll of #98. I opened up the patch and added /css to the style path. Passes simple test locally.

I don't get it... PDO exceptions and stuff? The fails seem like they are unrelated to this patch.

The aria-describedby still needs to go on the date only field. Can we add it to the h4 or the div .container-inline? If we add it to #edit-field-date-time-wrapper div we have to go into template_preprocess_container() and, as alexpott pointed out, touch things that we don't intend to.

Or we could (dare I say?) add an extra wrapping div in datetime.wrapper.html.twig

I brought this up at core mentoring. No one likes the idea of adding templates. There's got to be a way to use a more general wrapper in the widget for date + time.

Makes sense to me!

Are we simply changing datetime-wrapper.html.twig to out put fieldsets or are we still trying to conditionally wrap date+time in fieldset and have date in a div like it is now?

It is still difficult to implement conditionally. The template it hit twice for every field rendered.

1. The Date and time fields hit
2. Then the title and description

I guess that's the wrapping part? Double hitting the tpl for one element confuses me. I don't see how to get the title and description inside the fieldset with the content.

Probably. All of these old issues need to be gone through and reevaluated. On my todo list.

This may need to be postponed on #2161337: Add a Date Range field type with support for end date and #2632040: [PP-1] Add ability to select a timezone for datetime field.

If it doesn't, I would love to get this going again. Those two patches are going to make the field widget a mess.

Rerolled patch https://www.drupal.org/node/1918994#comment-9060061

Looks like a bad re-roll. The patch went from about 9k to 49k.

Also changing this to a task, as there is no real bug to fix here.

This is an attempt to re-roll this patch. I think it still needs some work. There are things that seem to be wrong but this is the best I can do right now.

Patch looks good. Just get it to pass tests and we'll be gtg.

Hello guys,
diff & patch is ready

looks there's no BC possible

+++ b/core/includes/theme.inc
@@ -1721,6 +1767,10 @@ function drupal_common_theme() {
+    'datetime_fieldset_wrapper' => array(

+++ b/core/modules/system/templates/datetime-wrapper.html.twig
@@ -8,6 +8,7 @@
+ * - attributes: Attributes for the description field.

@@ -30,4 +31,4 @@
-{{ description }}
+<div{{ attributes }}>{{ description }}</div>

+++ b/core/modules/system/templates/fieldset.html.twig
@@ -16,6 +16,7 @@
+ * - content: The form element to be output.

@@ -52,6 +53,7 @@
+    {{ content }}

that requires change record

checking error

Failed tests

Ensures that Stable overrides all relevant core templates.

+++ b/core/includes/theme.inc
@@ -576,6 +583,45 @@ function template_preprocess_datetime_wrapper(&$variables) {
+ * Prepares variables for datetime fieldset form wrapper templates.
...
+ * Default template: datetime-fieldset-wrapper.html.twig.

this should be copied to Stable theme

ok, will do that

done

I find the fieldset legend + the label on the input weird, but will defer to our UX/A11Y crew.

Also, patch doesn't seem to work with date-only versions of datetime fields.

Diff looks like it should produced the desired HTML. Could you just post a chunk in a comment for an example fieldset?

My head isn't in the nitty-gritty of Drupal theming code. Can someone who is more familiar with this bit just 'lgtm' on the PHP/Twig changes?

#139 screenshot is from Seven on node add page, from a fresh install of 8.2.x.

From #138: "Also, patch doesn't seem to work with date-only versions of datetime fields."

Needs to be investigated.

The issue summary is lacking a reason why. Linking to long overview articles elsewhere doesn't count :-)

Screenshot in context:

I did make the typo in that description but did not enter it twice. That doesn't happen in my d8 install without this patch so needs work there too.

Similar to #138, I think that fieldset legend + form label duplicating eachother looks bad. Even more when the field is required:

This is actually an accessibility bug.

Bumping this to Major because it now blocks #2161337: Add a Date Range field type with support for end date.

Reroll b/c #2546248: Use consistent style to mention HTML tags in code comments. Minor merge conflict in core/modules/system/templates/fieldset.html.twig

Here is a start.

@yoroy, I totally agree with your comment in #142. That double labelling is not ideal! There's a way around this. The output HTML will look something like this (in pseudocode):

<fieldset>
  <legend id="js_1">Event start</legend>
  <input id="js_2" aria-labelledby="js_1 js_2" aria-label="date" type="date" />
  <input id="js_3" aria-labelledby="js_1 js_3" aria-label="time" type="datetime" />
</fieldset>

We use aria-labelledby on the individual inputs to composite a label, referencing the text of the legend and the input itself (a self-referencing label), yielding an accessible label like: "Event start, date, edit text"

Can somebody please update the issue summary with the exact fix needed? As @yoroy mentioned in #142, linking off to articles doesn't make clear what the proposed solution is.

Why remove the label elements for the inputs all together instead of having them invisible (like it currently is)? All of the links in the IS reference labels as being important.

Few more related questions. Are we fixing datetime.module field widgets (the default one and/or the select list) and/or the datetime form element (what DateTimeDefaultWidget builds upon and can also be used in forms in non-field contexts) and/or the datelist form element (what DateTimeDatelistWidget builds upon)?

Are we not doing anything with date-only fields?

With the field widgets, how will these work with multi-valued fields?

IS still needs some TLC: we have a proposed solution but we don't have a clear explanation of what is wrong (why isn't the current implementation accessible?). Not trying to be a pain here, but this issue is hard and soft blocking a lot of other work right now.

This is also mentioned in #2788359: datetime_wrapper improperly applied to DateTimeWidgetBase; I am fine with scoping this issue to also address that if we can address this in a timely manner. Review is against #146

1. +++ b/core/modules/datetime/src/Plugin/Field/FieldWidget/DateTimeDefaultWidget.php
   @@ -64,6 +64,7 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
          case DateTimeItem::DATETIME_TYPE_DATE:
            $date_type = 'date';
            $time_type = 'none';
   +        $element['#theme_wrappers'][] = 'datetime_wrapper';
            $date_format = $this->dateStorage->load('html_date')->getPattern();
            $time_format = '';
            break;
   @@ -71,6 +72,7 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
   
   @@ -71,6 +72,7 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
          default:
            $date_type = 'date';
            $time_type = 'time';
   +        $element['#theme_wrappers'][] = 'datetime_fieldset_wrapper';
            $date_format = $this->dateStorage->load('html_date')->getPattern();
            $time_format = $this->dateStorage->load('html_time')->getPattern();
            break;
   

   These hunks show the basic problem with the current patch; a lot of the other code in the patch is working around this: 'datetime_wrapper' is being applied to both the widgets and the form elements.

   In this HEAD, DateTimeWidgetBase adds 'datetime_wrapper' to the $element. The in the patch, either 'datetime_wrapper' or 'datetime_fieldset_wrapper' will also be added by DateTimeDefaultWidget. Then the form elements themselves will also get a 'datetime_wrapper' from '#type' => 'datetime' (part of HEAD). Then there is code to try to attempt to fix up the problems introduced by all of these wrappers. This get really ugly when the field is configured to allow multiple values.

   In short 'datetime_wrapper' and 'datelist_wrapper' are meant for the form elements and not the widgets. But, I am starting to think that a proper solution means taking care of this and needing proper handlesMultipleValues() support. I would love a Field API maintainer to weigh in here.

2. +++ b/core/includes/theme.inc
   @@ -1736,6 +1782,10 @@ function drupal_common_theme() {
   +    'datetime_fieldset_wrapper' => array(
   +      'template' => 'datetime-fieldset-wrapper',
   +      'render element' => 'element',
   +    ),
   

   We are introducing a theme implementation system wide and only using it inside a particular module. If we need a new theme implementation to address this problem, then it should be defined by the module.

Hey,

I've tried the most recent patch and am having issues modifying the labels on the date field. I thought this patch would have made the labels accessible and modifiable via hook_form_alter but I can't seem to find exactly where in the form object it would be in.

I've tried changing various title within my field to the label that I want it to be but no luck. Any help would be appreciated.

Closed #2788359: datetime_wrapper improperly applied to DateTimeWidgetBase as dup of this, since that has been identified as having accessibility issues and it would be counterproductive to work on that and then again on this. Going to post a fresh-start patch later tonight that we can evaluate next steps on.

This is my proposal for a fresh-start.

Datetime:

Daterange:

It gets rid of the double wrapper problem I described earlier. It uses a fieldset around multi-element fields, and no wrapper around datetime/date-only.

And @mgifford reminded me that I can run WAVE myself :) No WAVE errors or alerts on these elements.

So, right now the patch leverages all of the existing theme elements, has minimal impact, and we don't have any WAVE errors. What else do we need?

Does someone want to take a stab at the IS update to properly describe what the accessibility problems with these in HEAD are? HEAD doesn't have WAVE errors or alerts on these either.

DateTimeFieldTest and DateRangeFieldTest come up green locally, so I expect this to pass. But, I am leaving the Needs Tests tag as we probably want to add some additional coverage to prevent regressions.

Note, I already rolled the patch and spent a while doing the screenshots and comment here. If we like this approach, the patch needs to be tweaked to wrap datetime/date-only+datelist widget in a fieldset. It doesn't currently.

Rearranged things a bit to avoid undoing work in base classes. datetime/date-only/datelist should be in a fieldset now.

When I ran WAVE this morning, it detected an error on datetime/date-only/datedefaut. I had to adjust the datetime-wrapper Twig to do this. @davidhernandez did a quick eyeball on the change, and thought it was OK b/c there is no removed functionality but suggested a small CR to announce the change to Classy.

Working on tests next.

Interdiff goodness.

OK, test coverage for the fieldsets and the ARIA. I think this is ready for review. If this approach is OK with everyone, I will draft a short CR to announce the Classy addition.

We are triaging this issue @cilefen

• We've discussed it this with @mpdonadio. It stays Major because it is an issue that has been identified as a core-gate for the datetime_range module staying in core.
• It does need an issue summary update (this has been tagged).

Let's post the right patch this time.

Per agreement in #160, here are some IS updates to outline the latest patch.

The code is looking really good. This is still tagged for both usability and accessibility review though. @mgifford are you signed off on the latest patch for accessibility?

Since the aim of this issue was accessibility, and we are leveraging existing templates, I am not sure we need anything else. I will draft the CR tomorrow to announce the the change that primarily impacts Classy. Someone can then set RTBC if they desire.

Once the change record is added, I think this is RTBC. Thanks everybody!