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!

Took a stab at the CR.

Change record looks good to go! I think this is ready!

I agree with removing the "needs usability review" tag ;-)
Looking at the screenshots: is "date-only" (with the dash between the two words) correct English?

#176, those are my field labels that were configured from the UI so I could keep everything straight. That string is not in the patch as user facing. The term 'date-only' does appear in the patch as a code comment, but it is consistent with how we document that particular setting (it appears 17 times in 8.4.x HEAD).

Ah ok, thank you. Lets get this in :)

I don't know why #146 got re-triggered by someone, and I can't cancel the run.

The RTBC version of the patch is in #167 (labeled 1918994-163.patch b/c some cross-posting).

I put in a retest of #167, which should get RTBC retesting back into place.

RTBC retesting’s choice of what to retest is a bit naive - for each issue, it looks for the most-recently-updated test, and requeues if updated over 24 hours ago. There must have been a DrupalCI disturbance that got some jobs running out of order around February 8.

Ok, 8.3.0-beta1 is out, and this is still RTBC. This patch is insanely important to datetime_range staying in core (it solves one of the two remaining core gates) and also blocks much of the work on datetime and daterange, so I am requesting consideration for 8.3.0-RC

Thanks @mpdonadio. Today the committers agreed that we are no longer going to do an RC target triage process, and instead just going to use our best discretion about what we backport when. However, I was already thinking about whether this issue was backportable during RC. I think this issue got missed for two reasons in our release management for the past month:

1. This issue should not have been moved back to 8.3.x after the automatic migration; that actually led to committers missing it when we triaged the RTBC queue prior to beta. :(
2. It needs a frontend framework manager review, but those resources have been limited. In particular, we are breaking BC for stable base themes in order to address the accessibility bug, and that needs signoff. The lower-risk option would be to change the module output and Bartik/Seven templates, but leave Classy/Stable alone or discuss them in a separate followup.

So moving to 8.4.x for now actually. :( I'll ping some theme system maintainers to see if they have time to evaluate this issue and whether we could safely backport it to a release candidate given the importance.

To be brutally honest, I don't care anymore if this gets into 8.3.0. We just need to get this in so we can unblock work and complete the action plan to keep datetime_range in core and also keep moving forward on other work. This is a triaged D8 major that has been RTBC for nearly a month, so I don't see why we can't do that.

This is essentially the same problem as #2788359: datetime_wrapper improperly applied to DateTimeWidgetBase that sank #2161337: Add a Date Range field type with support for end date in the first place. The clock has been ticking on #2788253: Plan for DateTime Range experimental module, but because of the very limited bandwidth of the UX/A11Y/Theme/Etc framework managers, patch review on the core gates (which have also been blocking other work) have sat for a very long time. That combined with the perpetual randoms in Nov/Dec have severely impacted progress.

I am extremely worried that further delays are going to mean that #2788253: Plan for DateTime Range experimental module won't be completed. Yes, I know that deadline is 2017/08/14, but given weeks between reviews, that time is going to go quickly.

@mpdonadio, I'm definitely concerned also. I agree the most important thing is to just get it committed when possible.

This is a triaged D8 major that has been RTBC for nearly a month, so I don't see why we can't do that.

Maybe your question is rhetorical, but I'm going to answer it just in case, since you've done great work on this and other DateTime issues that absolutely deserved prompt feedback they did not get. Unfortunately, committers also have limited capacity. :( There has been really unprecedented RTBC workload this past month, nothing like it since 8.0.0 I think, and in addition to that we have lost around 20% of our sponsored committer hours recently.

I've tried to fill some of the RTBC queue resource gap myself by spending a lot more time on the RTBC queue than I usually do (and let some other high-priority stuff slip because of it), but I also have a minor to ship and etc. And other committers are not paid full-time. We're also trying to add new resources, but it takes time to do that too. Plus, all those random test failures that have messed with so many issues, committers also have to try to manage that problem as well. And so on.

Speaking for myself, this patch is totally outside my expertise, and on top of that contains the one kind of change I mostly know I'm not supposed to commit for theme stuff without knowing what I'm doing which is Classy and Stable changes.

When committers can't keep up with the RTBC queue, the stuff that gets backlogged is stuff that's hard. Stuff that's taken a whole lot of work, like this issue. Stuff that's just simply a lot of code, like the Views issue. Stuff that we're less comfortable with. (I'm sure I don't have to tell you that datetime stuff is complex.) So something like a simple refactoring, I can handle quickly to reduce the overall burden of the queue, because code is code. And I try to help with things like that in the hope that someone else will have the capacity to help with things like this since I'm a theme-challenged. Hopefully someone else can soon.

Thanks @xjm; sorry if that came off as a rant. It was more that I don't want to hold this up on debate vs 8.3.0 or not. I know the RTBC queue got backlogged quite a bit.

In overall, the latest patch looks great and is a great improvement for the accessibility!

+++ b/core/includes/theme.inc
@@ -576,8 +576,14 @@ function template_preprocess_datetime_wrapper(&$variables) {
+  $variables['description'] = NULL;
...
-    $variables['description'] = $element['#description'];
...
+    $variables['description']['attributes'] = new Attribute($description_attributes);
+    $variables['description']['content'] = $element['#description'];

However, this is a BC breaking change, which would break any custom implementation of this template. I propose that instead of making description an array, we create a new variable called description_attributes.

OK, who needs to review this again when I post a patch later and adjust the CR?

I'm happy to do some more reviews on this :)

+++ b/core/modules/system/templates/datetime-wrapper.html.twig
@@ -30,4 +30,6 @@
+{% if description.content %}
+  {{ description.content }}
+{% endif %}

I just noticed that we should also add the wrapper to the template in the system module. That ensures themes not extending classy can benefit from this accessibility fix.

OK, #188 and #190 should be addressed. Adjusted CR. Spot checked in WAVE, both with the fieldset and w/o it. Think this is good to go.