Problem/Motivation

The 'datetime_wrapper' template is applied to the render array in DateTimeWidgetBase (which then gets used in DateTimeDefaultWidget and DateTimeDatelistWidget). When DateTimeDefaultWidget is used, the template gets applied multiple times: once for each #datetime item representing a field item (which are Datetime form elements), and then for again for the widget. This wrapper should only be used for the Datetime form elements.

Proposed resolution

Rework DateTimeWidgetBase to not use the 'datetime_wrapper' template. There are two options

- introduce a new template for the widget
- use an existing template for the widget

Remaining tasks

TBD.

User interface changes

TBD.

API changes

TBD.

Data model changes

None.

Comments

mpdonadio created an issue. See original summary.

mpdonadio’s picture

Issue tags: +blocker

Major because this is blocking #2161337: Add a Date Range field type with support for end date. Test demonstrating the issue to follow sometime tonight. To see it in action

- turn on twig debug
- add a datetime field to a node
- goto node/add/page
- view source, and you should see nested datetime_wrapper debug statements

mpdonadio’s picture

Status: Active » Needs review
Related issues: +#2161337: Add a Date Range field type with support for end date
StatusFileSize
new4.07 KB

Rather contrived test that shouldn't be in final version, but demonstrates the point. Best applied and run locally to see what is going on.

mpdonadio’s picture

Assigned: mpdonadio » Unassigned
StatusFileSize
new1.15 KB

Didn't adjust the tests, or add a new assertion to prevent a regression, but here is a simple fix that works when I test manually.

Feedback appreciated.

mpdonadio’s picture

Issue summary: View changes
Issue tags: +Usability
StatusFileSize
new71.01 KB

Added usability tag to see if we actually regress the usability here (not trying to improve it, just not make things worse). Screenshot show nothing really changes:

The last submitted patch, 3: 2788359-03.patch, failed testing.

mpdonadio’s picture

StatusFileSize
new52.09 KB

And this is what 2161337-293 looks like with this patch applied:

Status: Needs review » Needs work

The last submitted patch, 4: 2788359-04.patch, failed testing.

mpdonadio’s picture

Issue tags: +Needs usability review
yoroy’s picture

There's an incomplete sentence in the issue summary: "This means that the widget." I'm not sure what's being fixed here. What's the before/after?

mpdonadio’s picture

Issue summary: View changes

I'll post before/after markup when I am back on my main dev machine tonight. Essentially, the after is that the field items get the `datetime_wrapper` markup, while the widget itself just gets the standard `form_element` markup. This avoids the situation where `datetime_wrapper` markup is nested inside iteself, and each of which gets preprocessed.

yoroy’s picture

Thansk, lets do this then :)

mpdonadio’s picture

Issue summary: View changes
StatusFileSize
new224.29 KB
new263.97 KB
new241.35 KB
new256.76 KB

OK, with Twig debug on you can see the problem easily:

In this picture note that datetime_wrapper is being rendered twice, and nested, on the node/edit page for a single datetime field. The outer one is wrapping the whole widget, the inner one is wrapping the field inputs.

This shows the standard form element wrapping the widget, and a single datetime_wrapper wrapping the field inputs.

The before markup. Note the h4 masquerading as a label.

The after markup. Not the happy label for everything.

Posting the mess that you get with daterange next.

Note that this issue isn't trying to tackle the fieldset issue; that is over at #1918994: Improve Datetime and Daterange Widget accessibility. However, the work there is really messy because of the nested templates.

mpdonadio’s picture

StatusFileSize
new310.22 KB
new347.13 KB
new452.72 KB
new460.19 KB

Here is a daterange field.

Blerg. A datetime_wrapper for the widget itself, which has two datetime_wrappers inside it, for the start and end dates.

The datetime_wrappers for start and end are inside the form_element.

Note the two H4 next to each other: one is the widget title and the other is the start label. This is what is causing the misalignment.

Happier.

So, in short, the latest patch may not be the proper or ideal fix, but the nest datetime wrappers are definitely causing problems.

mpdonadio’s picture

Status: Needs work » Needs review
StatusFileSize
new4 KB
new2.85 KB

In case anyone was holding off on reviewing this because it was NW, these should be passing tests.

lauriii’s picture

+1 for this change. The markup and the change from themers perspective looks good.

I would still like to have an accessibility review for this.

mpdonadio’s picture

Assigned: Unassigned » andrewmacpherson

Let's try assigning instead of the tag to try to break the logjam here.

@andrewmacpherson, we are looking for a quick accessibility review of this. Personally, I think we just want to make sure we are not regressing accessibility. Improvements can be handled in the related issue, #1918994: Improve Datetime and Daterange Widget accessibility.

Thanks.

benjy’s picture

+++ b/core/modules/datetime/src/Plugin/Field/FieldWidget/DateTimeWidgetBase.php
@@ -17,13 +17,9 @@ class DateTimeWidgetBase extends WidgetBase {
+    // Someone give me a good comment here.

I think the patch is RTBC but we should fix the comment first.

mpdonadio’s picture

StatusFileSize
new4.13 KB
new911 bytes

Here is a stab at the comment. This applies cleanly to 8.3.x and 8.4.x.

benjy’s picture

Assigned: andrewmacpherson » Unassigned
Status: Needs review » Reviewed & tested by the community

Looks good to me.

mgifford’s picture

Status: Reviewed & tested by the community » Needs work
StatusFileSize
new72.48 KB

I'm pretty sure this patch leaves an orphaned label.

This was found by just using the WAVE Toolbar. It is an easy extension that all web developers should be familiar with.

orphaned label

As per #1918994: Improve Datetime and Daterange Widget accessibility the answer is about using fieldsets properly.

mpdonadio’s picture

@mgifford, are you suggesting we close this and do it all in #1918994: Improve Datetime and Daterange Widget accessibility instead?

mgifford’s picture

I don't care where it is done. Keeping a styled H4 would be fine for this issue. Making the H4 a label is creating an accessibility regression.

mpdonadio’s picture

Thanks @mgifford. I'll think about how we should handle this. The two main issues are the h4 is fine with datetime, but causes usability problems with daterange. The other, is that this and the other issue are critical path for daterange staying in core.

mpdonadio’s picture

Status: Needs work » Closed (duplicate)

Since we have looped to accessibility issues, I think it is counterproductive to work on this and then work on them again #1918994: Improve Datetime and Daterange Widget accessibility, so I am closing this.