Files: 
CommentFileSizeAuthor
#106 datetime-aria-1918994-106.patch9.38 KBTim Bozeman
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,850 pass(es).
[ View ]
#99 datetime-aria-1918994-99.patch7.84 KBmgifford
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,160 pass(es), 13 fail(s), and 3 exception(s).
[ View ]
#98 interdiff.txt330 bytesTim Bozeman
#98 datetime-aria-1918994-98.patch9.37 KBTim Bozeman
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch datetime-aria-1918994-98.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#97 Screen Shot 2014-08-10 at 11.57.31 AM.png125.04 KBmgifford
#95 Selection_040.png39.53 KBTim Bozeman
#95 Selection_039.png48.93 KBTim Bozeman
#95 interdiff.txt5.62 KBTim Bozeman
#95 datetime-aria-1918994-95.patch9.05 KBTim Bozeman
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 74,972 pass(es).
[ View ]
#92 Screen Shot 2014-06-20 at 9.04.01 AM.png151.55 KBmgifford
#92 1918994-psr4-reroll-2_1-92.patch4.83 KBmgifford
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,041 pass(es).
[ View ]
#84 1918994-psr4-reroll-2.patch5.83 KBmgifford
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,999 pass(es).
[ View ]
#81 1918994-psr4-reroll.patch5.86 KBxjm
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 1918994-psr4-reroll.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#80 Date-ARIA.png129.04 KBmgifford
#80 DateTime-Fieldset-ARIA.png138.32 KBmgifford
#79 interdiff.txt712 bytesTim Bozeman
#79 datetime.fieldset.79.patch5.99 KBTim Bozeman
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,614 pass(es).
[ View ]
#78 Selection_024.png29.5 KBTim Bozeman
#76 interdiff.txt872 bytesTim Bozeman
#76 datetime.fieldset.76.patch5.98 KBTim Bozeman
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,689 pass(es).
[ View ]
#72 datetime.fieldset.72.patch5.13 KBTim Bozeman
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,686 pass(es).
[ View ]
#67 interdiff.txt1.32 KBTim Bozeman
#67 datetime.fieldset.67.patch6.13 KBTim Bozeman
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,467 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#65 datetime.fieldset.65.patch5.61 KBmgifford
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,319 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#63 interdiff.txt2.8 KBsun
#63 datetime.fieldset.63.patch2.24 KBsun
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,393 pass(es).
[ View ]
#62 date_fieldset-1918994-62.patch3.71 KBmgifford
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,319 pass(es).
[ View ]
#59 date_fieldset-1918994-59.patch3.65 KBTim Bozeman
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,414 pass(es).
[ View ]
#59 Selection_017.png79.38 KBTim Bozeman
#58 Selection_016.png50.78 KBTim Bozeman
#56 date_fieldset-1918994-56.patch4.07 KBTim Bozeman
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,308 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#54 date_fieldset-1918994-54.patch3.12 KBmgifford
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 25,211 pass(es), 3,267 fail(s), and 3,822 exception(s).
[ View ]
#52 date_fieldset-1918994-51.patch2.93 KBTim Bozeman
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 1,686 pass(es), 1,074 fail(s), and 4,260 exception(s).
[ View ]
#49 date_fieldset-1918994-49.patch2.13 KBTim Bozeman
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,271 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#48 date_fieldset-1918994-48.patch1.89 KBTim Bozeman
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,402 pass(es).
[ View ]
#47 double-date.png117.57 KBmgifford
#46 authoring_information_fieldset.png68.32 KBTim Bozeman
#41 date_fieldset-1918994-41.patch2.21 KBTim Bozeman
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,242 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#40 date_fieldset-1918994-40.patch2.4 KBTim Bozeman
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,064 pass(es).
[ View ]
#39 date_fieldset-1918994-39.patch914 bytesmgifford
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,165 pass(es).
[ View ]
#39 date_fieldset.png78.35 KBmgifford
#38 legend-match-label.png82.17 KBmgifford
#37 date_fieldset-1918994-37.patch1.38 KBmgifford
PASSED: [[SimpleTest]]: [MySQL] 59,787 pass(es).
[ View ]
#34 Screen Shot 2013-09-21 at 11.36.04 AM.png122.76 KBmgifford
#33 date_fieldset-33.patch1.37 KBmgifford
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch date_fieldset-33.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#30 Date-Without-Patch.png160.35 KBmgifford
#30 Date-With-Patch.png131.86 KBmgifford
#29 date_fieldset-29.patch870 bytesfalcon03
PASSED: [[SimpleTest]]: [MySQL] 58,701 pass(es).
[ View ]
#22 fieldsets-with-date-1918994-22.patch1.33 KBmgifford
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch fieldsets-with-date-1918994-22.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#15 DateField with Date and Time Widget Before Patch.PNG47.42 KBcwarsaw
#15 DateField with Select List Widget Before Patch.PNG47.05 KBcwarsaw
#15 DateField with Date and Time Widget After Patch.PNG45.73 KBcwarsaw
#15 DateField with Select List Widget After Patch.PNG39.6 KBcwarsaw
#13 FieldSetNotWorking.png249.01 KBmgifford
#12 fieldsets-with-date-1918994-12.patch1.17 KBmgifford
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch fieldsets-with-date-1918994-12.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#8 datetimefieldsetbefore1.png74.76 KBandymartha
#8 datetimefieldsetafter1.png72.47 KBandymartha
#1 fieldsets-with-date-1918994-1.patch1.14 KBmgifford
PASSED: [[SimpleTest]]: [MySQL] 56,160 pass(es).
[ View ]
#1 fieldset-with-date-needs-css.png157.43 KBmgifford

Comments

mgifford’s picture

Title:Add Fieldsets to Date» Improve Accessibility with Date field by Using Fieldsets
Status:Active» Needs review
StatusFileSize
new157.43 KB
new1.14 KB
PASSED: [[SimpleTest]]: [MySQL] 56,160 pass(es).
[ View ]

The CSS definitely needs work, but it's built against Core (now) using the example I provided in comment #141 of the parent issue.

Oh ya, there is another issue looking to remove fieldsets and I'm looking for clarification on that #1467712-97: Make it possible to disable fieldset for date field

So far all that's been described is formatting problems (which can be fixed with CSS).

Status:Needs review» Needs work

The last submitted patch, fieldsets-with-date-1918994-1.patch, failed testing.

swentel’s picture

Component:field system» datetime.module

Moving to right component

mgifford’s picture

Status:Needs work» Needs review

#1: fieldsets-with-date-1918994-1.patch queued for re-testing.

mgifford’s picture

Seems to work fine in simplytest.me.

mgifford’s picture

Issue tags:-date, -accessibility

#1: fieldsets-with-date-1918994-1.patch queued for re-testing.

mgifford’s picture

Issue tags:+date, +accessibility

#1: fieldsets-with-date-1918994-1.patch queued for re-testing.

andymartha’s picture

StatusFileSize
new72.47 KB
new74.76 KB

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.
datetimefieldsetbefore1.pngdatetimefieldsetafter1.png

mgifford’s picture

Thanks @andymartha - that looks good to me, but I wrote the patch.

Not sure what else would be involved for it to be RTBC. Seems like it's already a strong pattern now in D8 to wrap elements like this in fieldsets.

andymartha’s picture

Status:Needs review» Reviewed & tested by the community

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

Bojhan’s picture

Status:Reviewed & tested by the community» Needs work

Can we use the invisible fieldset styling here?

mgifford’s picture

StatusFileSize
new1.17 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch fieldsets-with-date-1918994-12.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Until this patch gets in Core, this will need to be applied with http://drupal.org/files/cardinality-label-1932074.38.patch

Or, this would need to be added:

+/* Fieldset variant */
+fieldset.fieldset-no-border {
+  border: none;
+  padding: 0;
+}
+fieldset.fieldset-no-border legend {
+  text-transform: none;
+}
mgifford’s picture

Status:Needs work» Needs review
StatusFileSize
new249.01 KB

Well, this should work. I get this output:

<fieldset class="fieldset-no-border">
<legend>DAte</legend>
<div id="edit-field-date-und-0-value" class="container-inline">
</fieldset>

And the CSS is there and it's working fine in the related patch.

Image of Field Set Not Working

Unless core/modules/field_ui/field_ui.admin.css isn't loaded when not administering a field....

In which case this CSS should be moved somewhere else.

Bojhan’s picture

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

cwarsaw’s picture

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.

mgifford’s picture

That CSS is going to be provided by #504962: Provide a compound form element with accessible labels - so I don't see any reason not to mark this RTBC.

@cwarsaw want to mark this RTBC?

mgifford’s picture

Issue tags:-date, -accessibility

#12: fieldsets-with-date-1918994-12.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+date, +accessibility

The last submitted patch, fieldsets-with-date-1918994-12.patch, failed testing.

mgifford’s picture

Issue tags:+fieldset

tagging

mgifford’s picture

Status:Needs work» Needs review
Issue tags:-date, -accessibility, -fieldset

#12: fieldsets-with-date-1918994-12.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+date, +accessibility, +fieldset

The last submitted patch, fieldsets-with-date-1918994-12.patch, failed testing.

mgifford’s picture

Status:Needs work» Needs review
StatusFileSize
new1.33 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch fieldsets-with-date-1918994-22.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

I've rerolled the patch, but is there only one date widget now? Could only see one here:
admin/structure/types/manage/article/fields/node.article.field_date/field

mgifford’s picture

#22: fieldsets-with-date-1918994-22.patch queued for re-testing.

bowersox’s picture

Issue tags:+TwinCities
mgifford’s picture

Status:Needs review» Needs work
Issue tags:+date, +accessibility, +fieldset, +TwinCities

The last submitted patch, fieldsets-with-date-1918994-22.patch, failed testing.

jessebeach’s picture

nod_’s picture

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

falcon03’s picture

Status:Needs work» Needs review
StatusFileSize
new870 bytes
PASSED: [[SimpleTest]]: [MySQL] 58,701 pass(es).
[ View ]

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’s picture

Status:Needs review» Needs work
StatusFileSize
new131.86 KB
new160.35 KB

Unfortunately this patch changes the styling.

Without the patch the date/time are right next to each other:
Date Without Patch

But with the patch it's clearly in a fieldset.
Date With Patch

That still needs #1932074: Tags Includes Label which isn't Associated with an Input Form to pull in the CSS to see that this isn't styled.

falcon03’s picture

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

aaronbauman’s picture

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’s picture

StatusFileSize
new1.37 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch date_fieldset-33.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

@aaronbauman - Yes, #22 did check for multiple fields before applying the fieldset. I do think this is a lot harder to do though with twig. Not that this doesn't mean it shouldn't be done with twig.

It might just be cleaner if rather than trying to do the logic in twig, we simply have datetime-single-wrapper-html.twig & datetime-multiple-wrapper-html.twig.

I'm not familiar with how to do this. $element['#date_time_element'] & $element['#date_date_element'] are the only elements we need to consider. If they are both there, it should be wrapped in a fieldset.

I think #date_date_element = 'datetime'; produces more than one field mind you.

@falcon03 - I think so. I added it here and am testing on STM. Next post should include a screenshot.

Latest patch just includes the problematic code from #29 with <fieldset class="fieldset-no-border"> plus the CSS to style it.

mgifford’s picture

StatusFileSize
new122.76 KB

I didn't notice this before, but this patch also produces a fieldsets inside of fieldsets. That's obviously not what we want.

<div id="edit-field-date-wrapper" class="field-type-datetime field-name-field-date field-widget-datetime-default form-wrapper">
<div id="field-date-add-more-wrapper">
<fieldset class="fieldset-no-border">
<legend class="label"> Date </legend>
<fieldset class="fieldset-no-border">
<div id="edit-field-date-0-value" class="container-inline">
<div class="form-item form-type-date form-item-field-date-0-value-date">
<label class="visually-hidden" for="edit-field-date-0-value-date">Date</label>
<input id="edit-field-date-0-value-date" class="form-date" type="date" size="12" value="" name="field_date[0][value][date]" title="Date (i.e. 2013-09-21)">
</div>
<div class="form-item form-type-date form-item-field-date-0-value-time">
</div>
</fieldset>
<div class="description">This is where you enter the date.</div>
</fieldset>
</div>
</div>

That being said, I think that with the CSS applied, it looks right:

Screen Shot 2013-09-21 at 11.36.04 AM.png

mgifford’s picture

Issue summary:View changes
Status:Needs work» Needs review

Bot didn't seem to run earlier.

Status:Needs review» Needs work

The last submitted patch, 33: date_fieldset-33.patch, failed testing.

mgifford’s picture

Status:Needs work» Needs review
StatusFileSize
new1.38 KB
PASSED: [[SimpleTest]]: [MySQL] 59,787 pass(es).
[ View ]

re-roll.

mgifford’s picture

Status:Needs review» Needs work
Issue tags:+Novice
StatusFileSize
new82.17 KB

Legend needs to match labels <legend class="label"> date </legend>

The legend doesn't match the styling of the labels:
Non-bold legend

mgifford’s picture

Status:Needs work» Needs review
Related issues:+#2192419: Use a WCAG-compliant fieldset (fieldgroup) for #type radios/checkboxes
StatusFileSize
new78.35 KB
new914 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,165 pass(es).
[ View ]

Much simpler...
Properly emphasized date legend.

Tim Bozeman’s picture

StatusFileSize
new2.4 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,064 pass(es).
[ View ]

@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
Tim Bozeman’s picture

StatusFileSize
new2.21 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,242 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

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

Status:Needs review» Needs work

The last submitted patch, 41: date_fieldset-1918994-41.patch, failed testing.

mgifford’s picture

Getting closer! It's giving me some confusing results though. Fieldsets within fieldsets.

No aria-describedby="" in the first one, but there in the second.

<fieldset id="edit-field-date-0--description" class="fieldgroup form-composite">
<legend class="label"> date </legend>
<fieldset class="fieldgroup form-composite" aria-describedby="edit-field-date-0-value--description">
<div id="edit-field-date-0-value" class="container-inline">
<div class="form-item form-type-date form-item-field-date-0-value-date">
<label class="visually-hidden" for="edit-field-date-0-value-date">Date</label>
<input id="edit-field-date-0-value-date" class="form-date" type="date" size="12" value="" name="field_date[0][value][date]" title="Date (i.e. 2014-04-22)">
</div>
<div class="form-item form-type-date form-item-field-date-0-value-time">
</div>
</fieldset>
<div id="edit-field-date-0--description" class="container-inline description">Well, what do we have here?</div>
</fieldset>

I didn't see that duplicate previously, but could have missed it....

Tim Bozeman’s picture

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

mgifford’s picture

That makes sense...

We can dig into this a bit further. If we can't get the duplicate resolved we can move the aria-describedby issue back to #2244923: The help text for datetime field needs an ID for screen readers. and reopen it.

Tim Bozeman’s picture

StatusFileSize
new68.32 KB

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?

mgifford’s picture

StatusFileSize
new117.57 KB

@Tim Bozeman I'm guessing that the authoring information doesn't leverage the datetime.module

It would make sense that it does, but apparently........

Also interesting, set up a date field with 2 or more date options in it. It then has the duplicate fieldsets for each row of date fields (4 or more) but only a single description field.

With multiple values the aria-describedby needs to be in the table field-date-values table or maybe <h4 class="label">Date</h4>

Definitely have to dig into the datetime.module more though.

EDIT: The description for multi-part forms comes out in core/modules/system/templates/field-multiple-value-form.html.twig

Tim Bozeman’s picture

StatusFileSize
new1.89 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,402 pass(es).
[ View ]

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.

Tim Bozeman’s picture

Status:Needs work» Needs review
StatusFileSize
new2.13 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,271 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

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

Status:Needs review» Needs work

The last submitted patch, 49: date_fieldset-1918994-49.patch, failed testing.

mgifford’s picture

Looking great for a single date element. It gracefully degrades for multiple date elements. I think you've got it! I think if we can work through the test this should be good. I think we should deal with more complicated multiple date options in a different issue. It's an edge case for the most part.

EDIT: Pretty neat patch btw. Oh ya, we should eliminate the boundary. Also, we should be using the CSS class fieldgroup so that it doesn't have the border.

Tim Bozeman’s picture

Assigned:Unassigned» Tim Bozeman
Status:Needs work» Needs review
StatusFileSize
new2.93 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 1,686 pass(es), 1,074 fail(s), and 4,260 exception(s).
[ View ]

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

Status:Needs review» Needs work

The last submitted patch, 52: date_fieldset-1918994-51.patch, failed testing.

mgifford’s picture

Status:Needs work» Needs review
StatusFileSize
new3.12 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 25,211 pass(es), 3,267 fail(s), and 3,822 exception(s).
[ View ]

This patch doesn't actually add the fieldgroup with '#attributes' => array('class' => array('fieldgroup')), it really should.

I'm hoping this fixes the bot's concerns by changing where the tests are looking:
$this->assertFieldByXPath('//*[@id="edit-' . $field_name . '-wrapper"]/fieldset/legend/span', '*', 'Required markup found');

Status:Needs review» Needs work

The last submitted patch, 54: date_fieldset-1918994-54.patch, failed testing.

Tim Bozeman’s picture

Status:Needs work» Needs review
StatusFileSize
new4.07 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,308 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

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

Status:Needs review» Needs work

The last submitted patch, 56: date_fieldset-1918994-56.patch, failed testing.

Tim Bozeman’s picture

StatusFileSize
new50.78 KB

hmm...
should be working
looks like it should be working to me :(

Tim Bozeman’s picture

Status:Needs work» Needs review
StatusFileSize
new79.38 KB
new3.65 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,414 pass(es).
[ View ]

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');
mgifford’s picture

That's great progress! The tests pass.

Now how do we deal with the styling so that there's no box around it?

fieldgroup removes it, but it also makes the title ALL CAPS.

It's also not clear to me how to pass this attribute through.

Tim Bozeman’s picture

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.

mgifford’s picture

StatusFileSize
new3.71 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,319 pass(es).
[ View ]

Ok, this works for datetime, but if you choose just date in the Field settings, there shouldn't be a fieldset admin/structure/types/manage/article/fields/node.article.field_date/field

Fieldsets shouldn't be wrapping a single element.

sun’s picture

StatusFileSize
new2.24 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,393 pass(es).
[ View ]
new2.8 KB

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

Tim Bozeman’s picture

Assigned:Tim Bozeman» Unassigned
Status:Needs review» Needs work

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’s picture

Status:Needs work» Needs review
StatusFileSize
new5.61 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,319 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

So we've got two use cases here. One needs fieldsets (date & time) one doesn't (date only). Both need to address the aria-describedby issue of linking the help text to the Label or Legend. With the existing patches we've been working with, I think this combination works.

Status:Needs review» Needs work

The last submitted patch, 65: datetime.fieldset.65.patch, failed testing.

Tim Bozeman’s picture

StatusFileSize
new6.13 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,467 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new1.32 KB

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

mgifford’s picture

Yes.. We'd want the "add another date only field to DateTimeFieldTest.php"

Can you take that on?

Glad you found the template_preprocess_container() to add the aria-describedby.

mgifford’s picture

Status:Needs work» Needs review
Tim Bozeman’s picture

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

Status:Needs review» Needs work

The last submitted patch, 67: datetime.fieldset.67.patch, failed testing.

Tim Bozeman’s picture

Issue summary:View changes
StatusFileSize
new5.13 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,686 pass(es).
[ View ]

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

Tim Bozeman’s picture

Status:Needs work» Needs review
mgifford’s picture

It's looking good from an output perspective.. I do think it's worth adding the display: block; so that the help text starts on a new line:

.form-composite > .fieldset-wrapper > .description, .form-item .description {
    display: block;
    font-size: 0.85em;
}

The aria-describedby looks great and it's all wrapped properly in a fieldset. I wonder who we could get to help improve the tests....

Tim Bozeman’s picture

Assigned:Unassigned» Tim Bozeman

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

Tim Bozeman’s picture

StatusFileSize
new5.98 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,689 pass(es).
[ View ]
new872 bytes

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.

mgifford’s picture

Status:Needs review» Needs work

So close... Unfortunately, the aria-describedby property in the Date only implementation (of a date field) doesn't line up.

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

<div id="edit-field-date2-0--description" class="container-inline description">helpful date</div>

Tim Bozeman’s picture

StatusFileSize
new29.5 KB

Yes, that's not right... Hmm

Tim Bozeman’s picture

Status:Needs work» Needs review
StatusFileSize
new5.99 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,614 pass(es).
[ View ]
new712 bytes

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>
mgifford’s picture

Status:Needs review» Reviewed & tested by the community
StatusFileSize
new138.32 KB
new129.04 KB

Looks good. I do think we need to address when we have multiple items in a separate issue. For now though, dates are good!

EDIT: Followup issue added here #2273671: Allowed number of values more than 1 needs aria-describedby Support

DateTime-Fieldset-ARIADate-ARIA

xjm’s picture

StatusFileSize
new5.86 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 1918994-psr4-reroll.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 81: 1918994-psr4-reroll.patch, failed testing.

mgifford’s picture

Status:Needs work» Needs review
mgifford’s picture

StatusFileSize
new5.83 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,999 pass(es).
[ View ]
mgifford’s picture

I was having trouble posting this today.. I also re-rolled it on the plane. Hopefully the bot likes it better.

Tim Bozeman’s picture

Status:Needs review» Reviewed & tested by the community

Patch applies and the new attributes seem honky dory still.

alexpott’s picture

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.

Tim Bozeman’s picture

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

Tim Bozeman’s picture

Assigned:Tim Bozeman» Unassigned
Status:Reviewed & tested by the community» Needs work
Tim Bozeman’s picture

Assigned:Unassigned» Tim Bozeman

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

Tim Bozeman’s picture

Assigned:Tim Bozeman» Unassigned
mgifford’s picture

Status:Needs work» Needs review
StatusFileSize
new4.83 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,041 pass(es).
[ View ]
new151.55 KB

@Tim Bozeman - to try to move this issue along, I've taken out the essential aria stuff that was showing up where it shouldn't have been.

This is a slightly simpler patch that is really just building on your work. It will leave us in a good place to insert ARIA when we know where we want to insert it. Possibly even in a separate issue.

Here's a screenshot:

Tim Bozeman’s picture

Status:Needs review» Reviewed & tested by the community

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

alexpott’s picture

Status:Reviewed & tested by the community» Needs work
+++ 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.

Tim Bozeman’s picture

Issue summary:View changes
Status:Needs work» Needs review
Issue tags:-Novice
StatusFileSize
new9.05 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 74,972 pass(es).
[ View ]
new5.62 KB
new48.93 KB
new39.53 KB

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

mgifford’s picture

mgifford’s picture

Status:Needs review» Needs work
StatusFileSize
new125.04 KB

I've got this up here for the sprint - http://se46427db0c1bf5b.s2.simplytest.me/

The date field here - http://se46427db0c1bf5b.s2.simplytest.me/node/add/article - shows the outline from the fieldset. We don't want to add this visual change.

Other than that it is working well.

Tim Bozeman’s picture

Status:Needs work» Needs review
StatusFileSize
new9.37 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch datetime-aria-1918994-98.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new330 bytes

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

.field-type-datetime .form-item {
  border: none;
}
mgifford’s picture

StatusFileSize
new7.84 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,160 pass(es), 13 fail(s), and 3 exception(s).
[ View ]

That didn't apply for me.. Just rerolling with your css.

The last submitted patch, 98: datetime-aria-1918994-98.patch, failed testing.

The last submitted patch, 98: datetime-aria-1918994-98.patch, failed testing.

Status:Needs review» Needs work

The last submitted patch, 99: datetime-aria-1918994-99.patch, failed testing.

Status:Needs review» Needs work

The last submitted patch, 99: datetime-aria-1918994-99.patch, failed testing.

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 99: datetime-aria-1918994-99.patch, failed testing.

Tim Bozeman’s picture

Status:Needs work» Needs review
StatusFileSize
new9.38 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,850 pass(es).
[ View ]

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

Status:Needs review» Needs work

The last submitted patch, 106: datetime-aria-1918994-106.patch, failed testing.

Tim Bozeman’s picture

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

Status:Needs work» Needs review
Tim Bozeman’s picture

Issue summary:View changes
Tim Bozeman’s picture

Issue summary:View changes

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

Tim Bozeman’s picture

Issue summary:View changes
Status:Needs review» Needs work

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.

mgifford’s picture

Adding a div to the templates isn't the same as adding a template as far as I'm concerned.

What would it look like if we put in another div in datetime.wrapper.html.twig?

These are crappy examples, but thought I'd put it here to see where it goes:

{{ fieldset-aria-describedby-start }}<fieldset{{ attributes }}>
  {% if legend.title is not empty or required -%}
    {#  Always wrap fieldset legends in a SPAN for CSS positioning. #}
    <legend{{ legend.attributes }}><span class="{{ legend_span.attributes.class }}">{{ legend.title }}{{ required }}</span></legend>
  {%- endif %}
  <div class="fieldset-wrapper">
    {{ content }}
    {% if description.content %}
      <div{{ description.attributes }}>{{ description.content }}</div>
    {% endif %}
  </div>
</fieldset>{{ fieldset-aria-describedby-end }}

In the scheme of things, not having the aria-describedby is much less important than not having the fieldset for the dates.

Tim Bozeman’s picture

Assigned:Unassigned» Tim Bozeman

Makes sense to me!

Tim Bozeman’s picture

Assigned:Tim Bozeman» Unassigned
Issue tags:-TCDrupal 2014

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.