Updated:

Comment #278

Problem/Motivation

To help screen readers navigate complex forms, it is important to group related form elements in a manner that makes the form more semantic. The most established way of doing this is grouping it in a fieldset as per this WebAim Article. All WCAG 2.0 requires is a way to get the necessary information into the DOM in a way that the User Agent can pass it through to the accessibility API.

A challenge with adding fieldsets is that each fieldset added to a form adds a visual element, which changes the UI. The UI changes introduced by a fieldset may not improve the usability of complex forms for sighted users; additional CSS may be required to style out undesirable visual elements entailed by the addition of fieldsets.

Since #1168246: Freedom For Fieldsets! Long Live The DETAILS. fieldsets are no longer used as collapsible or uncollapsible containers, which actually have little in common with the original purpose of HTML fieldsets. (The FIELDSET element allows authors to group thematically related controls and labels.)

Core's complex form elements:

  • radios
  • checkboxes
  • dates

Most of the patches that have been written to address these problems have provided different ways to add fieldsets (or making it easier for users to do so), without altering the visual presentation of the form.

Unfortunately, there are problems with this approach that have not been easy to resolve:

  • Present solutions require extensive CSS overrides to restyle automatic fieldset styling
  • Redundant fieldsets are very probable (This is no longer a problem with the addition of DETAILS, see patch #272)
  • The automatic method presently just duplicates the #title rather than adding an explicit #property on the form elements

Proposed resolution

Provide a distinction between visual & logical ways to group form elements:
1) Logical FIELDSETS that group a set of composite form elements like checkboxes, radios, or other fields (such as a date's month, day and year);
2) Visual DETAILS that can collapse and expand and show a visual border.

Remaining tasks

There are no remaining tasks for this issue, however there is still a problem with inconsistency of styling. See #2002336: Introduce a CSS class to hide borders of fieldset elements.

User interface changes

N/A

API changes

Patch #232 changes the form pre-render function call from form_pre_render_conditional_form_element to form_pre_render_composite_form_element.

Patch #272 includes a new theme_details function call, in addition to theme_fieldset function call, to indicate a visual display of grouped fields, as opposed to a logical organization of fields into a fieldset.

Original report by @BWPanda

---

Original Summary

To help screen readers navigate complex forms it is important to group related form elements in a manner that makes it more semantic. The most established way of doing this is grouping it in a fieldset as per this WebAim Article. All WCAG 2.0 requires is a way to get the necessary information into the DOM in a way that the User Agent can pass it through to the accessibility API.

A challenge with adding fieldsets is that it adds a visual element presently which changes the UI and which may not improve the usability of complex forms for sighted users. Since #1168246: Freedom For Fieldsets! Long Live The DETAILS. fieldsets are no longer used as lame collapsible or uncollapsible containers, which actually have little in common with the original purpose of HTML fieldsets (The FIELDSET element allows authors to group thematically related controls and labels).

There is now a distinction between visual & logical ways to group form elements:
1) Visual DETAILS that can collapse and expand and show a visual border;
2) Logical FIELDSETS that group a set of composite form elements like checkboxes, radios, or other fields (such as a date's month, day and year).

Most of the patches have been looking at different ways to add fieldsets (or making it easier for users to do so) without altering the look of the site.

Core's complex form elements:

  • radios
  • checkboxes
  • dates

Unfortunately, there are problems with this that aren't easy to resolve:

  • Redundant fieldsets are very probable (This is no longer a problem with the switch to DETAILS)
  • Present solutions require an awkward mess of lengthy CSS overrides
  • Nobody wants to add extra HTML, CSS & Javascript if there is any way it can be avoided
  • The automatic method presently just duplicates the #title rather than adding an explicit #property on the form elements

---

When trying to validate my site, I got a warning saying I had a "reference to non-existent ID". This was because the date fields didn't have an ID set on their container div, hence the preceding label didn't match anything.

This patch adds the ID to the date container.

CommentFileSizeAuthor
#288 compound-element-504962.288.patch8.41 KBmgifford
#284 compound-element-504962.282.patch8.44 KBmgifford
#277 compound-element-504962.277.patch8.69 KBmgifford
#272 compound-element-504962.272.patch8.97 KBmgifford
#270 compound-element-504962.269.patch8.8 KBlarowlan
#270 interdiff.txt4.43 KBlarowlan
#269 Selection_006.png516.36 KBmgifford
#266 Screenshot 2013-12-06 09.14.39.png31.41 KBlarowlan
#266 compound-element-504962.266ish.patch5.18 KBlarowlan
#266 interdiff.txt1.15 KBlarowlan
#264 theme_form_element-fieldset-504962-264.patch5.25 KBmgifford
#258 theme_form_element-fieldset-504962-258.patch6.01 KBmgifford
#256 theme_form_element-fieldset-504962-256.patch5.96 KBmgifford
#252 theme_form_element-fieldset-504962-252.patch6.31 KBmgifford
#248 theme_form_element-fieldset-504962-248.patch5.43 KBmgifford
#242 theme_form_element-fieldset-504962-242.patch5.4 KBeffulgentsia
#233 theme_form_element-fieldset-504962-233.patch5.38 KBeffulgentsia
#233 interdiff.txt915 byteseffulgentsia
#232 theme_form_element-fieldset-504962-232.patch5.34 KBeffulgentsia
#231 theme_form_element-fieldset-504962-231.patch4.82 KBeffulgentsia
#224 FieldSetsAndRadios.png134.37 KBmgifford
#224 FieldSetsAndCheckboxes.png113.1 KBmgifford
#223 interdiff-214-219.txt5.06 KBmgifford
#219 theme_form_element-fieldset-504962-219.patch5.15 KBLiam Morland
#214 theme_form_element-fieldset-504962-214.patch5.15 KBmgifford
#214 interdiff-196-214.txt5.87 KBmgifford
#212 interdiff-196-207.txt6.21 KBmgifford
#207 theme_form_element-fieldset-504962-207.patch5.52 KBmgifford
#203 theme_form_element-fieldset-504962-203.patch5.55 KBmgifford
#202 FieldsetsAndRadios.png231.78 KBmgifford
#202 FieldsetsAndCheckboxes.png226.29 KBmgifford
#196 theme_form_element-fieldset-504962-196.patch4.94 KBLiam Morland
#191 theme_form_element-fieldset-504962-191.patch4.89 KBmgifford
#186 theme_form_element-fieldset-504962-186.patch4.52 KBmgifford
#186 AggregatorCheckboxesWithFieldset.png246.38 KBmgifford
#186 UserRadiosWithFieldset.png226.51 KBmgifford
#178 theme_form_element-fieldset-504962-178.patch4.17 KBmgifford
#177 theme_form_element-fieldset-504962-177.patch6.12 KBmgifford
#177 Fieldset-Radios-Invisible.png289.04 KBmgifford
#177 Radios-Fieldset-Visible.png295.54 KBmgifford
#177 Invisilbe-Checkboxes-Visible-Radios.png261.48 KBmgifford
#174 theme_form_element-fieldset-504962-174.patch5.19 KBmgifford
#174 FieldsetsAndDetails.png394.25 KBmgifford
#167 theme_form_element-fieldset-504962-167.patch5.19 KBLiam Morland
#163 installpage-error.jpg30.69 KBjthorson
#162 UsernamePassword.png515.7 KBmgifford
#160 theme_form_element-fieldset-504962-160.patch5.19 KBLiam Morland
#143 theme_form_element-fieldset-504962-143.patch5.2 KBmgifford
#137 theme_form_element-fieldset.patch5.23 KBLiam Morland
#135 theme_form_element-fieldset.patch4.67 KBLiam Morland
#122 theme_form_element-fieldset.patch5.58 KBLiam Morland
#119 Fieldset-invisible-on-preview-before.png128.75 KBmgifford
#117 theme_form_element-fieldset.patch5.51 KBLiam Morland
#115 theme_form_element-fieldset.patch5.5 KBLiam Morland
#113 theme_form_element-fieldset.patch5.5 KBLiam Morland
#103 theme_form_element-fieldset.patch5.42 KBLiam Morland
#100 theme_form_element-fieldset.patch5.43 KBLiam Morland
#93 theme_form_element-fieldset.patch4.51 KBLiam Morland
#88 theme_form_element-fieldset.patch4.5 KBLiam Morland
#85 theme_form_element-fieldset.patch3.61 KBLiam Morland
#60 fieldsetPatchProb.png115.54 KBYaxBalamAhaw
#44 fieldsets_legends_504962-take2-1.patch1.48 KBmgifford
#41 fieldsets_legends_504962-4.patch10.72 KBYaxBalamAhaw
#40 DifferencesInSeven.png73.73 KBmgifford
#38 fieldsets_legends_504962-3.patch10.25 KBYaxBalamAhaw
#38 my_module.zip1.35 KBYaxBalamAhaw
#35 screen-capture-23.png47.09 KBmgifford
#35 screen-capture-24.png109.5 KBmgifford
#35 screen-capture-25.png46.8 KBmgifford
#35 screen-capture-26.png96.49 KBmgifford
#35 screen-capture-27.png57.52 KBmgifford
#34 fieldsets_legends_504962-2.patch9.08 KBYaxBalamAhaw
#31 screen-capture-14.png54.26 KBmgifford
#28 fieldsets_legends_504962.patch5.97 KBYaxBalamAhaw
#26 fieldset_n_legends_v1.patch2.44 KBmgifford
#10 date-fieldset-legend.patch1.49 KBLiam Morland
#8 date-fieldset-legend.patch1.49 KBLiam Morland
#6 date-fieldset-legend.patch1.41 KBLiam Morland
form-date_id.patch483 bytesBWPanda
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Liam Morland’s picture

It would be better if the date fields were wrapped in a fieldset element and the label was replaced with the fieldset's legend. From an accessibility standpoint, that would clearly indicate that the three date fields are associated.

Everett Zufelt’s picture

Title: Add ID to date element » Use fieldset and legend for date field
Version: 6.x-dev » 7.x-dev
Category: task » bug

Changed to 7.x-dev as this is an issue in Drupal 7. It is possible that this could be backported, depending on the theming that would be involved.

Comment 1 is correct, it is not desirable, or possible, to associate a label for attribute with a div or more than one input field. This will have to be solved using a fieldset and legend. I have not looked at the date field in drupal recently. If someone can give me an example URL where the standard date field is being used I can provide some more detailed comments and role a patch.

Resources:
theme_date() - http://api.drupal.org/api/function/theme_update_page/7

H82: Grouping form controls with FIELDSET and LEGEND | Techniques for WCAG 2.0 - http://www.w3.org/TR/2008/WD-WCAG20-TECHS-20080430/H82.html

Everett Zufelt’s picture

Issue tags: +Accessibility

tagging

Status: Needs review » Needs work

The last submitted patch failed testing.

Liam Morland’s picture

Status: Needs work » Needs review
FileSize
1.41 KB

Attached is a patch that patches theme_form_element() to use a fieldset and legend instead of a label for date fields. I don't have a D7 install, so this is rolled against 6.13, but it could easily be ported to D7.

It is probably desirable to change the CSS so that the date fieldset does not have a border.

As requested, here is an example of the date field.

Status: Needs review » Needs work

The last submitted patch failed testing.

Liam Morland’s picture

Status: Needs work » Needs review
FileSize
1.49 KB

Here is a D7 version of my patch.

Status: Needs review » Needs work

The last submitted patch failed testing.

Liam Morland’s picture

Status: Needs work » Needs review
FileSize
1.49 KB

Fix typo in patch.

lilou’s picture

http://drupal.org/coding-standards#concat :

$output .= ' <fieldset class="date"><legend>'. $title ."</legend>\n";

should be :

$output .= ' <fieldset class="date"><legend>' . $title . "</legend>\n";
mgifford’s picture

Would be great to have this in core. Proper use of fieldsets is important.

Everett Zufelt’s picture

The patch in #10 is functionally correct, but I think we can have the same behavior without having to hack theme_form_element.

As I understand it, the date element has children which are having their labels themed correctly. So, all we need to do here is modify theme_date() so that it wraps the children in a fieldset (similar to theme_fieldset, and make sure that date element types aren't passed through theme_form_element.

Everett Zufelt’s picture

The default date element is defined in modules/system/system.module : system_elements()

  $type['date'] = array(
    '#input' => TRUE,
    '#element_validate' => array('date_validate'),
    '#process' => array('form_process_date'),
    '#theme' => 'date',
    '#theme_wrappers' => array('form_element'),
  );

#theme = date, which means that when this element is rendered it is passed to theme_date

#theme_wrappers = form_element means that the output from theme_date is wrapped by theme_form_element.

I am not sure what best practice is here, but we could omit #theme_wrappers and then make sure that theme_date() is properly wrapping the date child elements (not forgetting to include description, etc.

Everett Zufelt’s picture

Status: Needs review » Needs work
Liam Morland’s picture

If dates don't go through theme_form_element(), then, as you say, the description, etc. would have to be added separately. I don't think it is a good idea to duplicate that code, which would have to be kept in sync.

Perhaps the fieldset should be added by form_process_date(), which is the function that creates the three inputs for year, month, and day. If this function also set #form_element_skip_title, then theme_form_element() wouldn't add the label.

What do you think of this approach?

Everett Zufelt’s picture

@Liam

If the date element goes through theme_date and then theme_fieldset (as the theme_wrapper) then description and legend will be taken care of automatically.

Note: I believe that this approach is also desirable for checkboxes and radios element types, as date, radios and checkboxes are all containers for different elements, and not elements in their own right.

mgifford’s picture

Everett Zufelt’s picture

Status: Needs work » Postponed
geerlingguy’s picture

Title: Use fieldset and legend for theme_date theme_checkboxes and theme_radios » Use fieldset and legend for date field
Version: 8.x-dev » 7.x-dev
Status: Needs review » Postponed

Is there a way of adding a css ID or class selector to the <fieldset> on the node add/edit form? I would like to theme the fieldset, but it currently only shows <fieldset>, whereas most of the other fieldsets show something like <fieldset id="id-goes-here">

mgifford’s picture

What version of Drupal are you working with?

geerlingguy’s picture

6.x - I was trying to use hook_form_alter(), but no matter what I tried, I couldn't either change the title of the date fieldset (just to try), nor could I add an id to that fieldset.

Instead I implemented a drupal_add_js() (with the help of another drupaller) to add the css ID in after the page loads.

mgifford’s picture

Glad you found a solution. This issue is for Drupal 7 though.

bowersox’s picture

sub

Everett Zufelt’s picture

Title: Use fieldset and legend for date field » Use fieldset and legend for theme_date theme_checkboxes and theme_radios

Broadening the issue, as this affects the date, checkboxes and radios form elements. Each is a complex form element (an element that consists of more than one children elements). A label can only be associated with one form element. Therefore, a fieldset / legend is required to group multiple form elements into one complex element.

mgifford’s picture

Status: Postponed » Needs review
FileSize
2.44 KB

I probably should have just passed everything through theme_fieldset() to do the theming... However, he's a patch that addresses these inevitably grouped items.

If there's interest I can try to rework this.

Status: Needs review » Needs work

The last submitted patch, fieldset_n_legends_v1.patch, failed testing.

YaxBalamAhaw’s picture

Status: Needs work » Needs review
FileSize
5.97 KB

Here's a patch that sends checkboxes, radios, and date to theme_fieldset. I'm not sure if a few of the things I did to get this to work are acceptable though:

1) For some reason, theme_fieldset checks for a #value attribute, and if there is one, it'll print below #children without any tags enclosing it. So before I sent the $variables to theme_fieldset, I unset $variables['element']['#values'].

2) There was previously no way to mark a fieldset "required" the Drupal-way, so I added a few lines to theme_fieldset to address the issue. This could be considered a violation of WCAG 2 3.3.2. This kinda changes the "interface" to that function, since it now checks for the #required attribute when it didn't before.

3) I noticed that the date form-control did not label or add titles to the the drop-down boxes, so I added a few lines to address that issue. WCAG 2 3.3.2 again.

4) I'm sure there are places in Drupal where these things were already enclosed in fieldsets before (but with orphaned labels, and controls with no labels, etc...), so modifications will be required elsewhere too. I know the date field you can use with the profile module is one.

4) I'm not sure If I followed Drupal's coding style, but I'm sure I'll find out :)

Status: Needs review » Needs work

The last submitted patch, fieldsets_legends_504962.patch, failed testing.

mgifford’s picture

The patch applies nicely, however I'm not sure how it relates to the patch in 26. I'm not convinced it's better by providing a more flexible solution.

Adding support for date is good. Adding a required element is good.

Both patch #26 & #28 need tests to be written to have them get by the bot.

In #26 the problem is with form.test line 381

The patch in #28 has problems with that test and also:
form.test line 384 & 97

How do we best keep this moving forward?

mgifford’s picture

FileSize
54.26 KB

Oh ya, display issues with the label.

We've got to get the css right. I think we'd be safest to hide the label at this stage so it doesn't look like this.

YaxBalamAhaw’s picture

Yeah, the display issue with the legend is caused by Seven using "position: absolute" for the legends. I'm not really sure why Seven does this...

I like calling theme_fieldset for styling, because it already takes care of the attributes, most importantly "title" and "description." Also, if someone wanted to change the styling of all fieldsets (including checkboxes, radios, date) they would just have to change theme_fieldset. I think this helps with consistency. Though, that's just my preference.

Question about the automated tests: Are those the same tests in HEAD? If I wrote a patch for a test in HEAD, would that be the revision of the test it would run; or do the tests have to be modified by someone with proper access?

Everett Zufelt’s picture

@YaxBalamAhaw

When your patch contains a modified test that test is uesed for testing the patch.

YaxBalamAhaw’s picture

Status: Needs work » Needs review
FileSize
9.08 KB

Here's another patch that fixes the CSS and test issues.

mgifford’s picture

Status: Needs review » Needs work
FileSize
57.52 KB
96.49 KB
46.8 KB
109.5 KB
47.09 KB

There are still issues with the CSS. The fieldset-legend class still brings all the text to upper case:

fieldset .fieldset-legend {
  margin-top: 0.5em;
  padding-left: 15px;
  position: absolute;
  text-transform: uppercase;
}

There's still a border around the images where it shouldn't be.

Some nice generic places to test this patch:
/admin/config/development/logging
/admin/config/people/accounts
/admin/people/permissions
/admin/config/content/formats/2

and especially:
/admin/structure/types/manage/article

So still more CSS issues to resolve.

YaxBalamAhaw how are you with CSS?

YaxBalamAhaw’s picture

Holy crap. I did not realize there was so much context specific CSS relating to fieldsets in Seven. I'm ok at CSS I suppose. I understand how it works at least.

Do you mean there's a border around fieldsets when there shouldn't be? Shouldn't fieldsets have a visual queues for where they begin and end?

All the other fieldsets in Seven have uppercase legends, so shouldn't checkbox, radio, and date fieldsets have the same? Or no?

The problem at the bottom of /admin/people/permissions is definitely... interesting.

mgifford’s picture

Ya, sorry to point that out. Have a few pages there for testing purposes too.

I have no idea where to test the date specific info. We might need to build something to test that. Not sure it occurs in core.

Yes, in some cases there is a border and the field set that shouldn't be there. In the case of checkboxes & radioboxes there aren't boxes around them because often they are inside another box which is set aside for logical reasons of presentation.

It's a change in the UI. Without the patch everything is mixed case. With the patch every legend is upper case.

I'm not sure that the extra lines on the bottom of the permissions is tied to this patch. I didn't test that.

This is going to take a bit of documentation too for the themes.

YaxBalamAhaw’s picture

Hmm. I'm still not following you completely.

Should all fieldsets have borders around them, including the fieldsets made from the checkboxes, radios, and date FAPI elements? Should fieldsets nested in another fieldset have borders around them?

Currently in the Seven theme, all fieldset legends are in uppercase, but the labels for checkboxes, radios, and date are not. With the patch, what was previously a label is now a legend, and what was previously a div is now a fieldset. So, even though checkboxes, radios, and date are now fieldsets, should they look differently than the other fieldsets in Seven?

This patch fixes some of the obvious problems with the previous one. I also noticed a minor issue with theme_date, and fixed that as well. Checkboxes, radios, and date FAPI containers are styled in exactly the same way as all other fieldsets (may want to change this though?).

I've also attached a little module I was using to check out the styling of these particular elements. Install the module, and just go to /my_module/form to see the elements rendered. I've also tested this patch on Garland, Corolla, and Bartik and the elements seem to look good on all of them without modifications (well, all fieldsets in Bartik look a little funky).

mgifford’s picture

Sorry for any confusion.

Ultimately, the goal is to keep things looking like it does now unless we want to involve the design/usability folks. Don't get me wrong, they are great, but it'll slow down the process.

1) Should all fieldsets have borders around them, including the fieldsets made from the checkboxes, radios, and date FAPI elements?

No. Any fieldsets added by using checkboxes, radios, and date FAPI elements should not include a border.

I don't know of any instances of fieldsets within fieldsets that should have borders. I wasn't looking for this mind you.

I was simply noting those places where I saw a change in appearance.

2) So, even though checkboxes, radios, and date are now fieldsets, should they look differently than the other fieldsets in Seven?

I believe this would be more visually consistent with the design team's original vision of Seven. I think we want to keep the look of the interface the same. That is if we want to have these changes brought into D7.

Having consistent fieldset displays isn't critical. They are logically broken up into groups bigger than checkboxes and radios in most cases. Adding the upper case css makes it look inconsistent.

3) Checkboxes, radios, and date FAPI containers are styled in exactly the same way as all other fieldsets (may want to change this though?).

Yes, this will need to change I expect.

Thanks for the my_module file. Nice to have a way to look at these beyond Seven too. I'll take a look.

mgifford’s picture

FileSize
73.73 KB

Oh ya, the vertical tabs in all of the create content pages seem to display differently than all of the other checkboxes & radios I played with.

I think the test case for these forms either needs to be built into your my_module or tested by visiting /admin/structure/types/manage/article

Some pages have both checkboxes & radios on them which helps.

Using your module, just looking at Seven, here are some differences (screenshots attached).

On Seven these are the issues I see:
- boxes around radios test, date test & checkboxes text that shouldn't be there.
- decreased spacing between items of radios & checkboxes
- description is placed ahead of form for the date test
- Month, Day, Year should be invisible
- Radios Test, Date Test & Checkboxes test should be mixed case

YaxBalamAhaw’s picture

Ok, this patch results in checkboxes, radios, and date elements that look very much like they did before even though they're now fieldsets. The only thing that is completely different, is that the description precedes date's child elements instead of following them.

mgifford’s picture

Status: Needs work » Needs review

Setting to needs review for the bot.

There are some potential problems with this approach as there may be instances where a manually created fieldset just has the radios or checkboxes in it. In this case you've got fully redundant fieldsets.

I looked into whether it's possible to manually create sub-fieldsets with fapi, but it didn't seem to work for me. There's a lot of potential work for D8 in this.

Status: Needs review » Needs work

The last submitted patch, fieldsets_legends_504962-4.patch, failed testing.

mgifford’s picture

Status: Needs work » Needs review
FileSize
1.48 KB

After talking with @Jeff Burnz it seems like there are a few problems with this approach to implementing fieldsets in D7. #41 would be way faster to implement & involve way less code, but there isn't as much control in it's use. I do think this will need to be considered in more detail with D8 when FAPI gets re-written.

However, in the mean time, there are several places where I'd like to see better use of fieldsets around checkboxes, radios & date fields. this is an example of how to do this with the user/1/edit page

  $form['account']['set_status'] = array(
    '#type' => 'fieldset',
    '#attributes' => array('style' => 'border-width:0; margin:0; padding:0; top:0; -moz-border-radiu
s:0;'),
 );

  $form['account']['set_status']['status'] = array(
    '#type' => 'radios',
    '#title' => t('Status'),
    '#default_value' => $status,
    '#options' => array(t('Blocked'), t('Active')),
    '#access' => $admin,
  );
  $form['account']['set_roles'] = array(
    '#type' => 'fieldset',
    '#attributes' => array('style' => 'border-width:0; margin:0; padding:0; top:0; -moz-border-radiu
s:0;'),
  ); 
  $form['account']['set_roles']['roles'] = array(
    '#type' => 'checkboxes',
    '#title' => t('Roles'),
    '#default_value' => (!$register && isset($account->roles) ? array_keys($account->roles) : array(
)),

Sorry for just injecting the css inline there, but it was the fastest way to demonstrate this.

It outputs the following:

<fieldset style="border-width:0; margin:0; padding:0; top:0; -moz-border-radius:0;" class="form-wrapper" id="edit-set-status"><div class="fieldset-wrapper"><div class="form-item form-type-radios form-item-status">
  <label for="edit-status">Status </label>

 <div id="edit-status" class="form-radios"><div class="form-item form-type-radio form-item-status">
 <input type="radio" id="edit-status-0" name="status" value="0"   class="form-radio" />  <label class="option" for="edit-status-0">Blocked </label>

</div>
<div class="form-item form-type-radio form-item-status">
 <input type="radio" id="edit-status-1" name="status" value="1"  checked="checked"  class="form-radio" />  <label class="option" for="edit-status-1">Active </label>

</div>
</div>
</div>

</div></fieldset>
<fieldset style="border-width:0; margin:0; padding:0; top:0; -moz-border-radius:0;" class="form-wrapper" id="edit-set-roles"><div class="fieldset-wrapper"><div class="form-item form-type-checkboxes form-item-roles">
  <label for="edit-roles">Roles </label>
 <div id="edit-roles" class="form-checkboxes"><div class="form-item form-type-checkbox form-item-roles-2">
 <input type="checkbox" name="roles[2]" id="edit-roles-2" value="1" checked="checked"  disabled="disabled" class="form-checkbox" />  <label class="option" for="edit-roles-2">authenticated user </label>

</div>
<div class="form-item form-type-checkbox form-item-roles-3">
 <input type="checkbox" name="roles[3]" id="edit-roles-3" value="3" checked="checked"  class="form-checkbox" />  <label class="option" for="edit-roles-3">administrator </label>

</div>
</div>
</div>
</div></fieldset>

Which is more accessible for those using assistive technology.

Everett Zufelt’s picture

@mgifford

What were some of the problems you discussed with Jeff?

mgifford’s picture

Going through the comments in Skype. His concerns were:

- using fieldsets, especially in such a pervasive manner (in the actual theme function) is a problem, modules and themes can achieve this simply using FAPI - I dont see where the bug is that needs to be fixed.

- isnt this patch going to result in unnecessary nested fieldsets?

- say I already create a fieldset to place my form controls in, then the theme function now wraps another fieldset around it - also what is the legend generated from? If my fielset ONLY has 5 checkboxes in it and title and description, this patch will write another unnecessary and potentially confusion fieldset

- I would like to see some examples and hear from @chx on this, I'm not really convinced on the approach since we have the capability in FAPI already, its not a bug imo, and can be addressed on a case by case basis in core forms

- It's not that it isn't an issue, I'm saying I don't agree with the approach

I roughly edited these but do think it's a very valid argument to make.

Perhaps if there was a way to optionally disable fieldsets...

Everett Zufelt’s picture

I agree that the same can be done through fapi. However, the date, checkboxes and radios elements are containers. The containers contain elements that need to be in fieldsets (controls that make up a date, multiple checkboxes, multiple radios). So, why not have it automatic instead of requiring each of those element types to be manually wrapped in a fieldset?

mgifford’s picture

However, what if there is a fieldset that has just a date, checkboxes or radios. Then it would be good to be able to at least disable it.

I do think at the very least we'd need to add a $variables['element']['no-fieldset'] option so that we can disable it when required.

Everett Zufelt’s picture

I'm not sure I foolow. Can you give a use case?

mgifford’s picture

Jeff did that very well I think.

You've got a set of options (radios, checkboxes or dates) that you want in a collapsible fieldset that stands apart from the rest of the form.

With the patch presented in #41 there is no way that you could avoid having redundant fieldsets.

Everett Zufelt’s picture

I'm not sure what is meant by 'stands apart', but I will assume this means is styled differently than all other fieldsets.

So, perhaps the best approach is for date, checkboxes and radios to implement their own fieldsets and not to use theme_fieldset as a wrapper. This way the developer can add a class to date, checkboxes or radios when developing the form. Would this solve the problem?

mgifford’s picture

The main purpose of adding fieldsets is to group together input forms.. So say on the Account page of the admin account, when you are editing it here /user/1/edit you have all of the input fields associated with Account information together and separate from Picture, Contact settings & Locale settings. The input forms in Picture, Contact settings & Locale settings are all set apart from the others because it is beneficial to have them 'stand apart' from each other rather than being one long list of input forms.

Having date, checkboxes or radios have their own fieldsets rather than using the default theme_fieldset will not get us anywhere in my opinion. We'd still be in a position of possibly needing to have redundant fieldsets.

I think we can probably just find a means to disable fieldsets for radios, checkboxes & dates.

YaxBalamAhaw’s picture

Fieldsets can be styled separately as it is in #41. Radios fieldsets get assigned a "form-radios" class attribute. Checkbox fieldsets get assigned a "form-checkboxes" class attribute. And date fieldsets get assigned a "form-date" class attribute. A theme can also override a radio, date, or checkbox theme function if they want the HTML handled differently. Or a theme can override the fieldset theme function to change the HTML of all of them.

With patch #41, date, radios, and checkboxes aren't the only theme functions that modify variables, and then send them to other theme functions for further processing in form.inc. theme_tableselect and theme_submit do as well.

Yes, this patch will probably result in unnecessary nested fieldsets. I seem to remember seeing one somewhere. I guess I should have took notes :) Nested fieldsets aren't necessarily invalid or bad design though. It is perfectly acceptable to nest a group of fieldsets in one fieldset if the nested fieldsets are related in some way. A fieldset that only contains another fieldset is redundant and bad design though.

The legend is generated from the #title array key. This issue cannot be fixed on a case-by-case basis because the checkboxes, date, and radios containers themselves are broken (in my opinion). Without the patch, the #title array key generates a label that points to a non-existent form element which is a WCAG violation itself (may not be valid HTML either?). Like Everett said, checkboxes, radios, and date are containers for multiple, related elements. In HTML, form element containers are fieldsets, and WCAG states that related from items should be contained in fieldsets to semantically represent this.

YaxBalamAhaw’s picture

If a #disable_fieldset attribute is added, how should the #title element be rendered when it is true? A label element probably shouldn't be used because it cannot be "for" multiple elements correct? Is it valid to have a label's for attribute to point to a div id, or must it point to a form element? Maybe a high level heading tag should be used?

Everett Zufelt’s picture

@mgifford

I still don't seem to understand.

If I have a form with:

First name
Last name
Date of birth
Address
City
state
country
Phone

I might use one fieldset for the personal info and a second for the address info. But, within the personal info it is required to have a nested fieldset for the fields that compose date of birth, as these are a separate grouping within the personal info group.

Jeff Burnz’s picture

Mike asked me to comment here so I will, maybe its not exactly what you want to hear:

1. Potential developer wtf here and no resolution to redundant fieldsets - if I'm taking care to do things right using just FAPI I get screwed over by the theme function.

2. Given the current stage of the D7 cycle it seems very late for such a major change - is this D8 material / wait for a better implementation in D8 FAPI?

3. Anyone actually tested these patches to any extent? Re point 2 - this is late guys, very late, we're bearing down on beta1...

3. No comments from any of the Form team here (fago, sun, chx, effulgentsia, Frando) or webchick/Dries as yet, I think their input is crucial before getting too excited about a patch or approach.

Everett Zufelt’s picture

@Jeff

1. Potential developer wtf here and no resolution to redundant fieldsets - if I'm taking care to do things right using just FAPI I get screwed over by the
theme function.

* If I understand you correctly you are saying that if you put in the required fieldset for a set of checkboxes yourself then there will be redundancy. Wouldn't you only need to look at the page source / theme function and remove your fieldset? fapi automates lots of things, I don't think that this is a valid argument.

2. Given the current stage of the D7 cycle it seems very late for such a major change - is this D8 material / wait for a better implementation in D8 FAPI?

* I think that this is a valuable accessibility improvement and haven't been told that I should stop trying to come up with ways to improve D7 accessibility.

3. Anyone actually tested these patches to any extent? Re point 2 - this is late guys, very late, we're bearing down on beta1...

* Same response as 2.

4. No comments from any of the Form team here (fago, sun, chx, effulgentsia, Frando) or webchick/Dries as yet, I think their input is crucial before getting
too excited about a patch or approach.

* I don't think that not having received input from anyone on your list is a good idea to stop working on improving core. If that were the case Drupal would evolve far less quickly.

Jeff Burnz’s picture

My concern in 1 is "what is the expected DX"? I'm not saying there isn't an issue - I'm asking if this is right approach, I think some feedback from the core forms team could bring some light to this. I don't think its bad to slow down a bit and ask questions - this is a major change to both the output and DX.

YaxBalamAhaw’s picture

#41: fieldsets_legends_504962-4.patch queued for re-testing.

YaxBalamAhaw’s picture

FileSize
115.54 KB

#41 fails because checkboxes, radios, and date fieldset tags are given a " disabled="disabled" " attribute for some reason. It's really weird because the container divs without the patch didn't have this problem, and if you set a normal fiedset's disabled attribute to true, there isn't a problem either; but if theme_checkboxes, theme_radios, or theme_date calls theme_fieldset, a disabled attribute given to the fieldset tag. I don't exactly understand how FAPI determines that only children of containers should get disabled attribute applied, and not the containers themselves, so I'm stumped on this one.

I also noticed CSS bug with patch #41, collapsible fieldsets, and Chrome. See attached image.

mgifford’s picture

For D7 can we not just use theme_radios, theme_checkboxes & theme_date & propose the changes we've made as part of an accessible theme.

It would be great to have this managed right in core, but I don't think we're going to resolve this issue until D8 when the Forms API gets a full review.

I'm open to other suggestions, but.

Everett Zufelt’s picture

@mgifford

Why would we stop improving core? IMO this seems like a pretty simple improvement if we had a bit of assistance from someone familiar with fapi.

mgifford’s picture

We need to get D7 out the door!

Please contact folks intimately familiar with FAPI, to see if there is interest, but so far we've running into some concerns here where we haven't found the easy solutions we were looking for.

bowersox’s picture

Regarding D7/8, I feel that this is a D8 feature. Changing the API behavior of these FAPI methods could throw off contrib developers who have already been counting on a "frozen" D7 API.

Regarding multiple fieldsets as mentioned in #52 and #55, I think it is helpful to distinguish between 2 types of fieldsets: 1) visual fieldsets that can collapse and expand and show a visual border; 2) logical fieldsets that group a set of checkboxes, radios, or other fields (such as a date's month, day and year).

Currently I believe there is no limit on nesting fieldsets to any depth.

sun’s picture

Version: 7.x-dev » 8.x-dev

I can see how this may improve accessibility, but this is D8 material. As of now, as of D7, fieldsets are used to perform visual/wanted grouping of otherwise separate form elements and widgets. The idea of this issue is to automate the output of fieldsets for certain element types/widgets, because they are expanded into multiple elements. However, those elements are otherwise not separate, they are technically the same element.

A solution for this issue would simply replace the existing wrapping conditional form element with a fieldset. However, a fieldset that is equally invisible like the current wrapping container. To achieve this, we would have to introduce new, separate fieldset classes (to gain separate "types" of fieldsets) for the existing and these new fieldsets. All of which would require very extensive changes to Drupal core's stylesheets as well as CSS of core themes. Plus JavaScript.

In addition, we'd have to setup clear rules for why and when to use which type of fieldset (although the type of fieldset proposed here is used internally by theme functions only). In particular, because theme_date() is entirely different to theme_checkboxes() and theme_radios(). The latter may be logically understood to some extent, but the former is like translating this proposal into "any #process function that expands an element into multiple sub-elements has to inject a fieldset for accessibility", which would be the point, where this entire proposal could start to get wonky, because #type 'text_format' and other element types are also expanded into multiple elements, and those would have to be grouped in such a fieldset, too.

However, aside from that, I'd agree that an additional, different usage of fieldsets would make sense, purely from a markup perspective even. Because, as of now, all fieldsets in Drupal are considered to be those lame collapsible or uncollapsible containers, which actually have little in common with the original purpose of HTML fieldsets. Even more so, the stylesheets for vertical tabs contain an awkward mess of lengthy CSS overrides for those lame collapsible containers, just to revert each and everything.

casey’s picture

Subscribe.

mgifford’s picture

Can this be brought into D7 using the Accessible Helper Module?

YaxBalamAhaw’s picture

It might be possible to fix this in the Accessibility Helper Module. It shouldn't be a problem to override the theme functions, but I'm not sure about the stuff in system.module. I'll try to make some time to check it out this weekend.

bowersox’s picture

Advanced Search will need to use fieldset-legend for the group of checkboxes called "Only of the type(s)". When we get checkboxes properly working with a fieldset-legend, we'll need to upgrade the Advanced Search (/search/node/) form to take advantage of this.

Here is a note about where this problem was discovered:
http://groups.drupal.org/node/100014#comment-318719

Jeff Burnz’s picture

@brandonojc - could you take a look at my implementation of advanced search in Adaptivetheme (D7 version).

This is done with a simple hook_form_alter in the theme (yes we have hooks to play with in themes now), this would be very trivial to put into the accessibility helper module, certainly that effort can take the code from my theme.

EDIT: I updated the link to my theme, since the original live demo is now dead.

bowersox’s picture

@Jeff, the implementation looks great to me. I think this improves the usability of the Advanced Search form regardless of whether you are a screenreader user or not. As a sighted user, it is clear to me that I use the checkboxes to select type. In the markup it is good that you have semantically identified what the Type checkboxes mean.

One interesting aspect is that we have a nested fieldset. Advanced Search is the outer fieldset. The type checkboxes are the inner fieldset. On the group discussion thread you've asked for testers and hopefully they'll confirm whether assistive technology tools handle this well.

Thanks for your work on this!

Everett Zufelt’s picture

I tested http://sitespring.eu/at7-dom_test/search/node/

Methodology
1. Click on link to Show advanced search
2. Navigate by form field ('f' in JAWS / NVDA, 'control + option + command + j' in VoiceOver)

Results

JAWS 11 / FF 3.6.10
JAWS only reads the most recent legend for the form fields, it does not read every nested legend (IMO expected behavior).

NVDA 2010.1 / FF 3.6.10
NVDA reads none of the legends

VoiceOver SL / Safari 5
VO actually ignores the Advanced search form fields. Likely a bug in VoiceOver / Safari.

bowersox’s picture

@Everett, I get a different behavior with VoiceOver. I am running Mac OS 10.6.4 and Safari 5.0.2.

Here is what I get:

  1. On the link "show advanced search" I clicked the link using VoiceOver-Enter key sequence.
  2. It did not automatically read any further down the page, so I navigate forwards with VoiceOver-Right Arrow key.
  3. It read all the labels, fields and legends in order. So it says "Types" before "checkbox article, checkbox basic page" etc.

I would consider that a success, but I might be unclear about the intended VoiceOver behavior.

Everett Zufelt’s picture

@brandon

You are getting the correct behavior from VoiceOver if you are navigating the page one element at a time. Many users, especially on familiar pages, navigate by form field, skipping the rest of the elements on the page. In this case using (control + option + command + j) VoiceOver is ignoring the elements that were previously hidden.

This isn't our problem, it is a VoiceOver bug, and since the fields are accessible using the alternate interaction model we are not going to accommodate a VO/Safari bug.

Jeff Burnz’s picture

@brandon, @Everett so that seems to be a success then - with fieldsets wrapping each thematic group of form elements. I wasn't sure about wrapping the keyword text fields, since they're first in the source order and no one has mentioned wrapping text fields, however they all work together as a group - would you concur this is the right way to go, or at least no less accessible than if the keyword fields were not wrapped?

Everett Zufelt’s picture

@Jeff

Seems pretty subjective to me. They are semantically related fields. I don't think it will make too much difference either way. What are you thinking of using as legend text?

Jeff Burnz’s picture

@Everett, at the moment the legend texts are:

Keywords: this fieldset wraps the 3 keyword fields.

Types: this fieldset wraps the content type check-boxes.

Languages: this fieldset wraps the language check-boxes.

Everett Zufelt’s picture

@Jeff

This seems good to me.

Liam Morland’s picture

What is the status of this?

I have written a version of theme_form_element which checks to see of the element has element children, and if so, wraps them in a fieldset, putting the title in a legend instead of in a label. This avoids the need to deal individually with dates or other compound elements.

If there is some way I can help, please let me know.

Jeff Burnz’s picture

From post #70 onwards this somewhat got subverted by us discussing some proof of concept stuff I was doing for accessibility in my theme.

Before that see suns comment #65 is a pretty good overview of the wider issues.

Everett Zufelt’s picture

Title: Use fieldset and legend for date field » Use fieldset and legend for theme_date theme_checkboxes and theme_radios
Version: 7.x-dev » 8.x-dev
Status: Postponed » Needs review

The purpose of fieldset is to group like form controls. These might be a set of checkboxes or radios, but also may be controls to control placement of a node in a menu.

IMO, most of the uses of fieldset (expandable and vertical tabs), is likely proper in Core, although I really havent reviewed it.

If there is a vertical tab for authoring information, then that should be a fieldset, and the controls related to authoring information should be grouped within. If within that grouping there is a set of radios for a selection these should be grouped in a fieldset as well.

sun’s picture

mgifford’s picture

Issue tags: -Accessibility

Status: Needs review » Needs work
Issue tags: +Accessibility

The last submitted patch, fieldsets_legends_504962-take2-1.patch, failed testing.

Liam Morland’s picture

Status: Needs work » Needs review
FileSize
3.61 KB

Attached is a patch to theme_form_element(). It now checks to see of the element has element children, and if so, wraps them in a fieldset, putting the title in a legend instead of in a label.

Making this have no visual effect requires a bunch of CSS to reset the rules for fieldset and legend; there is probably a tidier way of doing this by not assigning all the styles to fieldset and legend in the first place, but those are applied in individual themes while my changes could be done in the core CSS.

Status: Needs review » Needs work

The last submitted patch, theme_form_element-fieldset.patch, failed testing.

mgifford’s picture

Looks like the test case is going to have to be revised:
Required 'checkboxes' field is marked as required Other form.test 98 FormsTestCase->testRequiredFields()

Liam Morland’s picture

Status: Needs work » Needs review
FileSize
4.5 KB

Updated patch.

Unfortunately, when I run simpletest on my clean D8 install, I get a big pile of errors, so it's hard for me to know if I have something that ought to work.

Status: Needs review » Needs work
Issue tags: -Accessibility

The last submitted patch, theme_form_element-fieldset.patch, failed testing.

Liam Morland’s picture

Status: Needs work » Needs review

#88: theme_form_element-fieldset.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Accessibility

The last submitted patch, theme_form_element-fieldset.patch, failed testing.

Everett Zufelt’s picture

I think that this functionality would best be implemented as a theme wrapper.

function theme_?()

This would allow for it to be more easily overriden in a theme, theme_form_element() is a bit bulky and confusing, and it would remove the logic to where it is required.

You would then need to update the element types in system_element_info() to use the new theme wrapper.

AFAIK an element can have only one theme function, but multiple wrappers. So for date as an example, it would be themed by theme_date(), then theme_?() to add the fieldset, and finally by theme_form_element()

Liam Morland’s picture

Status: Needs work » Needs review
FileSize
4.51 KB

I got simpletest working.

Liam Morland’s picture

@Everett Unless I misunderstand how wrappers work, I don't see how it could be implemented there. theme_form_element() will still want to add the #title as a label, but it needs to be a legend inside the fieldset. I makes logical sense to me that theme_form_element() should do this: If a form element is composite, it needs a fieldset and legend, otherwise just a label. Putting it in theme_form_element() ensures that all composite elements, even ones yet to be defined, will get the fieldset and legend.

Everett Zufelt’s picture

@liam

You are correct, if we moved the logic for fieldset out of theme_form_element we would have to do the same for label, not something we want to do IMO.

Everett Zufelt’s picture

sun’s picture

Hold on - this patch wraps every single form element having children into an invisible fieldset... (?!)

Also, what's the point of duplicating the element #title once more into the fieldset's legend?

I don't think this "automated" approach will work out. Setting an explicit custom #property on the form element types that should be wrapped would sound like a more sane idea to me.

---
btw, that $has_children code probably makes sense to move as a nice general element_has_children() helper function into common.inc, next to the other element_*() functions.

Liam Morland’s picture

@sun Yes, it wraps any element that has children. As far as I can tell, the only Drupal form elements that have children are composite elements made up of more than one HTML form control element. They should be grouped with a fieldset.

The title is not duplicated in the legend. The function generates either a label or a legend, depending on whether or not it is generating a fieldset.

We could have a custom property, but that would mean that every composite form element would need it set. As it is, it just detects when a fieldset is needed and adds it to all fields that require it.

I agree that making a element_has_children() function would be a good idea. I will update the patch.

Everett Zufelt’s picture

I do like @sun's suggestion to use a #property. IMO this should be set to true for date, checkboxes and radios by default, but would allow developers to easily override the fieldset wrapping the element.

If the property is true then use the fieldset, if false then them as it is now.

Liam Morland’s picture

I have updated the patch. It now checks for a property called #composite. If true, it generates a fieldset and legend, otherwise it does what it does now. The #composite property is set for composite dates, checkboxes, and radio buttons.

Liam Morland’s picture

Jeff Burnz’s picture

+++ b/modules/system/system.theme.css
@@ -185,6 +185,48 @@ html.js fieldset.collapsed .fieldset-legend {
+  ¶

white spaces

Powered by Dreditor.

Liam Morland’s picture

Updated patch, rolled against latest D8.

Jacine’s picture

Component: forms system » markup
Jacine’s picture

I would just like to say that I am not happy that this and other issues like that dramatically affect markup and CSS are being completely hidden from us. It's not cool to find out about things like this 100 comments in, or worse after they've been committed.

When I come across issues that affect accessibility I tag them. I would appreciate if you guys would please give us this same courtesy going forward. If there is any confusion over component to use, it's understandable, but in the future please use the Markup or CSS component where the main result of the issue is to change Markup or CSS.

Thank you.

Everett Zufelt’s picture

@Jacine

That was likely my bad, apologies.

I sent an e-mail to the WebAIM mailing list to see if anyone can suggest an alternative to fieldset / description, as I understand that style (if not semantics) are difficult with this issue.

WCAG 2.0 does not require fieldset / legend, it does require a way to get the necessary information into the DOM in a way that the UA can pass it through to the accessibility API.

http://webaim.org/discussion/mail_message?id=18508

Everett Zufelt’s picture

As fieldset / legend is difficult to theme (ask a themer) I have been looking at alternatives. Note: I believe that from a semantics, but especially an accessibility perspective, that f / l is the best markup.

However, I am happy to post this potential alternative:

<div id="legend">Are you happy?</div>
<label for="h1" id="l1">Yes</label><input type="radio" id="h1" aria-labelledby="legend l1" />
<label for="h2" id="l2">No</label><input type="radio" id="h2" aria-labelledby="legend l2" />

This allows the radios to be labelled by both the legend and the label. The label alone is fallback for UA/AT not supporting ARIA. For the above FF 6.0.2 returns "Are you happy? Yes" as the accessibleName of the radio control through MSAA/IA2.

JAWS 12/FF 6.0.2 reports "Are you happy? Yes Yes", but I am told that this bug is already fixed in JAWS 13 (public beta coming soon).

This would read the legend text for each control in the group. Most, if not all, UA/AT currently read the legend text only upon entering the group. We could apply legend in aria-labelledby only to the first item in the group, but that would not cover reverse tabbing through the form (come to think of it I don't know if using f/l currently covers this).
* Edit: I am wrong, using JAWS / FF 6 definitely reads legend for each item in the group, so above point is somewhat moot.

* Edit: The above example does not allow for the application of aria-describedby to the div with id=legend. We would have to apply aria-descrbiedby to each control separately. This is possible, but would seriously complicate #405360: Use aria-describedby to link form elements with form element descriptions.

Why is fieldset / legend better?

1. Fieldset / legend are the semantic elements that html5 defines for use in grouping and providing a name to a group of controls.
2. UA / AT currently supports applying legend text to form controls, in a consistent and tested way, and has for a long time.
3. No WAI-ARIA support is required by UA/AT.

Everett Zufelt’s picture

Title: Use fieldset and legend for theme_date theme_checkboxes and theme_radios » Provide accessible labels (was "Use fieldset and legend") for theme_date theme_checkboxes and theme_radios
Everett Zufelt’s picture

Title: Provide accessible labels (was "Use fieldset and legend") for theme_date theme_checkboxes and theme_radios » Provide accessible labels (was "Use fieldset and legend") for compound form elements
mgifford’s picture

Issue tags: +aria

Tagging.

xjm’s picture

Status: Needs review » Needs work

Thanks for your work on this patch. I have two minor notes about the code comments:

+++ b/includes/form.incundefined
@@ -3865,36 +3868,67 @@ function theme_form_element($variables) {
+  // Composite elements consist of more than one HTML form control. These must be grouped by a fieldset.

This comment needs to be wrapped onto a second line so that each line is less than 80 characters.

+++ b/includes/form.incundefined
@@ -3865,36 +3868,67 @@ function theme_form_element($variables) {
+  // Generate the title, either a legend or a label, only if the title will be used

This comment needs similar rewrapping, and also should end in a period.

Note also that the Drupal 8.x patch will need to be rerolled, because the core directory structure for Drupal 8 has now changed. (For more information, see #22336: Move all core Drupal files under a /core folder to improve usability and upgrades). When the patch has been rerolled, please set the issue back to "Needs Review."

Tagging as novice for the task of rerolling the Drupal 8.x patch and making the above changes.

If you need help rerolling this patch, you can come to core office hours or ask in #drupal-gitsupport on IRC.

xjm’s picture

Issue tags: +Novice

Actually adding the tag this time.

Liam Morland’s picture

Status: Needs work » Needs review
FileSize
5.5 KB

Thanks, xjm. Updated patch attached.

mgifford’s picture

#113: theme_form_element-fieldset.patch queued for re-testing.

Liam Morland’s picture

Patch rerolled against latest D8.

mgifford’s picture

This applies nicely in git. Thanks Liam!

What's the easiest way to test this in D8? I don't think that the "Authored on" field really demonstrates it. I can't just add a date field to core.

Would be good to get a screenshot & examples of the output before marking it RTBC.

Liam Morland’s picture

To test, visit admin/structure/types/manage/article. At the bottom in the vertical tabs are several composite fields, such as "Preview before submitting", which is several radio buttons. Inspect with Firebug to observe the use of the fieldset element with class fieldset-invisible and the accompanying legend element.

I don't know what the point of screenshots is. This patch doesn't change the appearance at all.

Re-roll attached.

xjm’s picture

Well, presumably one can also tab to the element to see the label? Or am I mistaken?

mgifford’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
128.75 KB

@xjm - You can't see the label when you tab over it, but don't think that was one of the goals.

Looks good Liam. Thanks. I've attached a screenshot showing the code in place.

sun’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/includes/form.inc
@@ -2869,6 +2869,7 @@ function form_process_date($element) {
+  $element['#composite'] = TRUE;

@@ -2961,6 +2962,7 @@ function weight_value(&$form) {
+    $element['#composite'] = TRUE;

@@ -3097,6 +3099,7 @@ function form_process_checkboxes($element) {
+    $element['#composite'] = TRUE;

These should be set in hook_element_info().

+++ b/core/includes/form.inc
@@ -3986,36 +3989,69 @@ function theme_form_element($variables) {
+  if ($composite) {
+    $output .= '<fieldset' . drupal_attributes(array('class' => 'fieldset-invisible')) . '>';
+  }
...
+      $title = '<legend' . drupal_attributes($attributes) . '>' . $element['#title'];
+      if (!empty($element['#required'])) {
+        $title .= theme('form_required_marker', array('element' => $element));
+      }
+      $title .= '</legend>';

1) Why do we hard-code the output of theme_fieldset() here?

2) I also don't see where a "for" attribute is set for the legend is output?

+++ b/core/modules/system/system.theme.css
@@ -188,6 +188,48 @@ abbr.form-required, abbr.tabledrag-changed, abbr.ajax-changed {
+fieldset.fieldset-invisible > legend {
+  margin: 0;
+  padding: 0;
+  border: none;
+  outline: 0;
+  font-style: inherit;
+  font-size: 100%;
+  font-family: inherit;
+  vertical-align: baseline;
+
+  border-radius: 0;
+  color: inherit;
+  background-color: transparent;
+  background-image: none;
+
+  display: block;
+  font-weight: bold;
+
+  position: static;
+  left: 0;
+  right: 0;
+  top: 0;
+  bottom: 0;
+
+  text-indent: 0;
+  text-shadow: none;
+  line-height: normal;
+  width: auto;
+  height: auto;
+}

euhm, seriously? That's almost a complete browser style reset.

Doing something like this is sorta ok-ish in a theme, but in system.theme.css, this looks like overkill to me.

Knowing fieldset rendering differences in browsers, I kinda understand that this is required to make this idea work, but if this change proposal requires so many style overrides, it'll be hard to get a general buy-in from others (especially themers).

mgifford’s picture

Thanks @sun - very thorough feedback. Always appreciated!

Liam Morland’s picture

Thanks @sun.

1) Why do we hard-code the output of theme_fieldset() here?

The purpose is to add a fieldset, thus semantically grouping the form controls, without changing the appearance. If someone wants to override the default theme for manually-added fieldsets, they probably don't want to also change the appearance of the invisible, semantic-only fieldsets.

2) I also don't see where a "for" attribute is set for the legend is output?

@for is used in label elements. legend elements must appear as the child of a fieldset element and are automatically associated with their parent fieldset.

The CSS is designed to remove all impact on appearance from the new fieldsets. We're not proposing a visual redesign, so it should look the same, but we need to add the fieldset and legend since that is the only way in HTML to group related form controls, which is needed for accessibility.

This updated patch sets #composite in system_element_info().

Liam Morland’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: -Novice, -Accessibility, -aria

The last submitted patch, theme_form_element-fieldset.patch, failed testing.

mgifford’s picture

Status: Needs work » Needs review
Issue tags: +Novice, +Accessibility, +aria

#122: theme_form_element-fieldset.patch queued for re-testing.

Jacine’s picture

Status: Needs review » Needs work
Issue tags: -Novice

I agree with @sun in #120. This is not going to fly.

Also, removing the novice tag.

mgifford’s picture

@Jacine - There's a new patch in #122 trying to get address @sun's points. The main point of concern is the number of theme changes to system.theme.css, right?

Is there a better way you can think of to hide the fieldset-invisible & legend?

Jacine’s picture

@mgifford my comment is based on that updated patch.

I don't like the concept and the CSS code is not going to fly AT ALL. To be completely honest, I cannot even stand the thought of this issue. I don't think this is an improvement for anyone. Making elements invisible is not the answer to everything, and this patch is taking it to a whole new level of barfing out crap markup. And only to ultimately make it invisibile? Ugh. If it's really needed, then why on earth would we make it invisible? This pattern of making things invisible to solve accessibility problems is getting out of control.

I hate the fact that there is a random fieldset thrown in theme_form_element() when we have a theme_fieldset(). To me, this proposal is equivalent to hijacking the forms API, and forcing people into authoring forms a certain way instead of allowing them to do it themselves, and I don't agree with that. I think this issue is going about things the wrong way, and should just be changing the structure of elements that are currently problematic. This patch doesn't even hint to what existing form elements will be marked "composite" so we cannot even test the effects that it's going to have.

On top of that, this comment might as well not even exist. It says nothing about the actual problems it is trying to solve and because of that you can count on themers removing it because fieldsets are extremely hard to deal with from a CSS standpoint.

+++ b/core/includes/form.incundefined
@@ -3986,36 +3986,69 @@ function theme_form_element($variables) {
+  // Composite elements consist of more than one HTML form control. These must
+  // be grouped by a fieldset.

7 days to next Drupal core point release.

Sorry, I know this is not what you are looking to hear, but I really have to be honest. This is one is very high on my list of "OMGWTF this will be a nightmare" issues.

Jeff Burnz’s picture

I thought I might be done with comments on this issue but I was thinking about a set of form elements in my theme this evening and realized something about this patch that does not really add up - its inconsistent.

Two things in the OP really concern me:

We'd like there to be a distinction between visual & logical fieldsets:
1) visual fieldsets that can collapse and expand and show a visual border;

As per what sun said way way back, the collapsible fieldset is a Drupalism, its not meant to be like this. The Webaim article being cited to drive this issue clearly talks about visual borders and legends. Hiding such elements is yet another Drupalism.

What I am driving at here is that the fieldset was hijacked way back in Drupal-past to support a weird and crazy design pattern (collapsible fieldsets) and is now being proposed to build out another weird and crazy design pattern - the "invisible fieldset".

I have to agree with Jacine that hiding this is wrong and the use of element-invisible is getting out of control. If these injected fieldsets make logical sense, then show them, don't try to hide them. Hiding things by default is not always the right answer and may well be a crutch for bad design.

2) logical fieldsets that group a set of checkboxes, radios, or other fields (such as a date's month, day and year).

When you build forms you expect to have control over every aspect - not least of which where you will wrap related items in a fieldset. In HTML5 this may become "details"; regardless, grouping related field items is far more complex than the rather simplistic notions of this patch.

This patch offers to automate some wrapping of related elements. What about 4 related select lists - these might all need to go in a fieldset, but this patch cannot do this, afaict, so the developer needs to manually add a fieldset.

In other words, the human must make this decision, a machine is blunt, and no matter how sophisticated this patch gets, it's still rather dumb. It cannot account for sophisticated grouping of elements - and its not meant to, that task falls to the developer to have the awareness to do so.

Am I missing something here or does it not make sense to target only those problematic instances in core and "fix" those?

This issue is one of many which appears to jump strait into the patch/approach mentality without broader discussion and analysis of the design pattern - and exploration of alternatives such as enforcing such design patterns as a standard, and/or inspiring all round better form design in core and contrib by way of sensible form design.

And yes, there is no way that CSS can fly in core, better to do nothing. And yes, you can count on themers removing this, for the reasons Jacine stated.

Everett Zufelt’s picture

@Jeff

I haven't had time to think about this a lot recently, but I agree that we should only be using a fieldset in the clearest situations, like date / radios / checkboxes. And, also agree with you and @Jacine, let's keep making things invisible to a bare minimum.

bowersox’s picture

@Jeff Burnz, are you suggesting that part of the patch for this issue should actually include eliminating the concept of "collapsible fieldsets" in Drupal core? We would convert all the "collapsible fieldsets" into "collapsible divs" which is what they really should be. Then we would no longer be mis-using the fieldset and legend elements. Doing all this in this one issue might be too much, but I wanted to ask what you think.

Also, regardless of whether we do that or not, I don't see any reason why anything needs to be made invisible to fix this issue. We don't need to do any more invisibility work here. We just need to semantically tag proper compound form elements together (like date fields, radio groups and checkbox groups). To do that, nothing more needs to be made invisible, AFAIK. If anyone feels otherwise, please advise.

Liam Morland’s picture

Thanks for your feedback, everyone. As the author of the most recent patch on this thread (#122), I thought I would share what I was thinking when I wrote it.

The purpose of this patch is to semantically group related form controls without changing the appearance of the page. This purpose touches two separate concerns: semantics, expressed in HTML, and appearance, expressed in CSS. Following the principle of separate of concerns, let's look at each separately.

Semantics
In Drupal, a date is a single form component, but in HTML, it is rendered as three separate element, one for each of year, month, and day. This patch associates these element so that it is clear that all three are related and form part of the same date. The patch sets three component types as composite: date, radios, and, checkboxes (note these are the plural versions). Developers making new components can choose whether or not to set a component as composite. It could be written to use theme_fieldset(); that would add yet another div element. This patch does not attempt to add every fieldset that might be required by a form. It just adds the ones that are always required. Using the date example again, the "month" input element for your birthdate is always associated with the "day" input element so there should always be a fieldset for complete semantic markup.
Appearance
The current appearance of the form controls already makes it clear that the different parts are associated. No one who can see the screen would miss that the month and the day for a date component are related. The visual design is fine, so I wrote the patch in a way that wouldn't change it. I would be fine with ordinary visible fieldsets around dates, etc., but I thought that others would prefer to leave the appearance as-is, so I did. Unfortunately, it takes a bunch of CSS to make things look the same.

Bottom line: Currently, things look fine, but the semantic markup is lacking. The patch fixes the markup without changing the appearance.

bowersox’s picture

Hi @Liam Morland,

Your description actually sounds perfect to me. I think what troubled a few of us was that the last patch added a 'fieldset-invisible' class and a bunch more logic to hide visual aspects of fieldsets.

Instead of that approach, what about this:

Drupal's special collapsing fieldsets are the exception, not the rule. So when we output a semantic fieldset, we should not do anything special to hide anything. We should simply have base styles that are plain and simple with no borders, no collapsing, no fuss. When we actually do want a collapsible fieldset, that is when we should output extra markup and CSS classes to apply the visual borders and the collapsing Javascript.

Is that an approach that makes sense?

Liam Morland’s picture

Issue tags: +a11ySprint

Tagging

Liam Morland’s picture

Status: Needs work » Needs review
FileSize
4.67 KB

Thanks, bowersox. Attached is a patch much like the one in #122, but without any CSS or class on the fieldset. Please comment. I am looking at how to do this with theme_fieldset().

bowersox’s picture

Hi @Liam Morland, the patch looks good. I read the code and it seems to do the right stuff.

@Everett Zufelt and I discussed the CSS problems, and we suggest a 2-step approach: Step 1 is that we need to refactor some of Drupal's CSS so that the fieldset and legend elements do not end up with horrible styling when they are used appropriately (semantically). Then Step 2 is exactly what your patch does. The challenging part is Step 1, and Everett had made some inquiries to get someone to send him good "reset CSS" that will remove a browser's nasty default styles for fieldset and legend. Hopefully Everett will post that here soonish.

Thanks for your work moving this important issue ahead!

Liam Morland’s picture

Here is a version that uses theme_fieldset(). There is one problem, however: It treats #title_display = "after" the same as "before". Even if you put the legend element after the content, it still appears at the top. If we want to have a fieldset with a label that follows the content, it we would have to avoid the legend element and use a plain element associated with the fieldset through aria-labelledby.

Status: Needs review » Needs work
Issue tags: -Accessibility, -aria, -a11ySprint

The last submitted patch, theme_form_element-fieldset.patch, failed testing.

Liam Morland’s picture

Status: Needs work » Needs review
Issue tags: +Accessibility, +aria, +a11ySprint

#137: theme_form_element-fieldset.patch queued for re-testing.

mgifford’s picture

Realized that this issue hadn't been linked - #1645420: Use @aria-describedby for fieldset descriptions

Great that the last patch was good for the bot. Just need someone to test it now.

mgifford’s picture

Issue tags: -Accessibility, -aria, -a11ySprint

#137: theme_form_element-fieldset.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Accessibility, +aria, +a11ySprint

The last submitted patch, theme_form_element-fieldset.patch, failed testing.

mgifford’s picture

Status: Needs work » Needs review
FileSize
5.2 KB

Just a re-roll.

Status: Needs review » Needs work
Issue tags: -Accessibility, -aria, -a11ySprint

The last submitted patch, theme_form_element-fieldset-504962-143.patch, failed testing.

mgifford’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, theme_form_element-fieldset-504962-143.patch, failed testing.

Liam Morland’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: +Accessibility, +aria, +a11ySprint

The last submitted patch, theme_form_element-fieldset-504962-143.patch, failed testing.

mgifford’s picture

Ok, so applying this patch locally is just fine:
dm8 mike$ git apply theme_form_element-fieldset-504962-143.patch

The details of the test get down to:

7. Detect a Drupal installation failure

Which really has nothing at all to do with the patch being applied, right?

Are we supposed to get something from http://qa.drupal.org - should we just retest it?

Liam Morland’s picture

Yes, I had this problem with another issue some months ago. I tested it again a day later and it worked. It looked to me like the testing infrastructure was temporarily broken. I haven't done any core issues in the past few days. Are tests working in other issues?

mgifford’s picture

Status: Needs work » Needs review
mgifford’s picture

Status: Needs review » Needs work

The last submitted patch, theme_form_element-fieldset-504962-143.patch, failed testing.

Liam Morland’s picture

Try re-rolling and uploading it again. Maybe something is wonky with that particular patch or posting.

mgifford’s picture

I'm hoping to get an explanation here #1792910: Failures in early test review environment setup need better error messages. I've got a love/hate relationship with unit testing.

sun’s picture

The actual error message is

Installing: failed to arrive at database configuration page using install path http://drupaltestbot659-mysql/checkout/install.php?profile=standard&langcode=en

...which in this case most likely means that the test runner is not able to locate the form elements for configuring the database connection in the Drupal installer. That page is using #type radios, which is affected by this patch. The early-return in theme_form_element(), forwarding into theme_fieldset() looks suspicious to me.

mgifford’s picture

Ahh.. Ok.. This is useful..

jthorson’s picture

Crossposting from #1792910: Failures in early test review environment setup need better error messages ...

In this particular case, it's a WSOD on the database page caused by:

PHP Fatal error: Call to undefined function drupal_attributes() in core/includes/form.inc on line 2797

Liam Morland’s picture

drupal_attributes() has been replaced in D8 by the Attribute class, which can be used the same way, but also provides more features.

Liam Morland’s picture

Status: Needs work » Needs review
FileSize
5.19 KB

Reroll.

Status: Needs review » Needs work

The last submitted patch, theme_form_element-fieldset-504962-160.patch, failed testing.

mgifford’s picture

FileSize
515.7 KB

@jthorson - thanks. That was a bad mistake, sorry.

@Liam - thanks for re-rolling and nixing the parse error.

@sun - I still can't see what the problem is with the form. It's now failing on:

[@reason] => Installing: failed to set database information (database name, user, password were wrong)

Which is:

<div class="form-item form-type-textfield form-item-mysql-username">
  <label for="edit-mysql-username">Database username <abbr class="form-required" title="This field is required.">*</abbr></label>
 <input type="text" id="edit-mysql-username" name="mysql[username]" value="" size="45" maxlength="128" class="form-text required" required="required" aria-required="true" />
</div>
<div class="form-item form-type-password form-item-mysql-password">
  <label for="edit-mysql-password">Database password </label>
 <input type="password" id="edit-mysql-password" name="mysql[password]" size="45" maxlength="128" class="form-text" />
</div>

Screenshot attached.

jthorson’s picture

FileSize
30.69 KB

Here's what the testbot is seeing ... or at least, what I'm seeing on the testbot checkout.

Edit: ... Basically, the fieldsets aren't collapsed; though the markup indicates they do have the 'collapsed' class. As a result, we see the legend output twice, and no padding in the fieldset itself.

mgifford’s picture

Priority: Normal » Major

Moving this issue up in priority to major. See explanation from @falcon03 as to why this is important #1811216-8: Missing labels for profile and language selectors

mgifford’s picture

Status: Needs work » Needs review
Issue tags: -Accessibility, -aria, -a11ySprint

Status: Needs review » Needs work
Issue tags: +Accessibility, +aria, +a11ySprint

The last submitted patch, theme_form_element-fieldset-504962-160.patch, failed testing.

Liam Morland’s picture

Status: Needs work » Needs review
FileSize
5.19 KB

Reroll.

Status: Needs review » Needs work

The last submitted patch, theme_form_element-fieldset-504962-167.patch, failed testing.

mgifford’s picture

Patch applies nicely locally. Think it's a matter of the change affecting a form used in the install.

Liam Morland’s picture

Yes, I expected it to fail like the older patch. I just wanted to get it back to the point of applying properly.

mgifford’s picture

We're getting really close with #1168246: Freedom For Fieldsets! Long Live The DETAILS. so I'm hoping we might be able to bring closer with the sprint.

mgifford’s picture

Status: Needs work » Needs review
Issue tags: -Accessibility, -aria, -a11ySprint

Status: Needs review » Needs work
Issue tags: +Accessibility, +aria, +a11ySprint

The last submitted patch, theme_form_element-fieldset-504962-167.patch, failed testing.

mgifford’s picture

Status: Needs work » Needs review
FileSize
394.25 KB
5.19 KB

I think this error is related to the patch (using reference to empty & not isset):
Notice: Undefined index: #title in theme_form_element() (line 4547 of core/includes/form.inc)

Also, I haven't fixed this yet, but the aria-describedby I'd mentioned below doesn't exit. It should be what is in the

.
<fieldset aria-describedby="edit-update-status-module--description" id="edit-update-status-module" class="form-wrapper"><div class="fieldset-description">The system will notify you when updates and important security releases are available for installed components. Anonymous information about your site is sent to <a href="http://drupal.org">Drupal.org</a>.</div><div id="edit-update-status-module" class="form-checkboxes"><div class="form-item form-type-checkbox form-item-update-status-module-1">
 <input aria-describedby="edit-update-status-module--description" type="checkbox" id="edit-update-status-module-1" name="update_status_module[1]" value="1" checked="checked" class="form-checkbox" />  <label class="option" for="edit-update-status-module-1">Check for updates automatically </label>

</div>
<div class="form-item form-type-checkbox form-item-update-status-module-2">
 <input aria-describedby="edit-update-status-module--description" type="checkbox" id="edit-update-status-module-2" name="update_status_module[2]" value="2" checked="checked" class="form-checkbox" />  <label class="option" for="edit-update-status-module-2">Receive e-mail notifications </label>

</div>
</div></div></fieldset>

Might need to look at integrating #1645420: Use @aria-describedby for fieldset descriptions into this.

Liam Morland’s picture

Perhaps instead of integrating the patches, one should be committed before the other. It probably makes sense to commit #1645420: Use @aria-describedby for fieldset descriptions first because it is simple and probably uncontroversial once it is decided to use ARIA.

Status: Needs review » Needs work

The last submitted patch, theme_form_element-fieldset-504962-174.patch, failed testing.

mgifford’s picture

Re-rolled the patch with the update from @sun here:
https://drupal.org/node/1645420#comment-6791474

Have some screenshot with the re-rolled code. Probably should roll it again.

mgifford’s picture

Same as above, but without this patch applied - https://drupal.org/files/drupal8.aria-fieldset.16.patch

Status: Needs review » Needs work
Issue tags: -Accessibility, -aria, -a11ySprint

The last submitted patch, theme_form_element-fieldset-504962-178.patch, failed testing.

mgifford’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: +Accessibility, +aria, +a11ySprint

The last submitted patch, theme_form_element-fieldset-504962-178.patch, failed testing.

Liam Morland’s picture

Anyone know why when you "view details" on the failing test, you can't see the specific errors that caused the failure?

mgifford’s picture

Agreed:
49,267 pass(es), 0 fail(s), and 25 exception(s)

Let me see if we can escalate this.

mgifford’s picture

Status: Needs work » Needs review
Issue tags: -Accessibility, -aria, -a11ySprint

Status: Needs review » Needs work
Issue tags: +Accessibility, +aria, +a11ySprint

The last submitted patch, theme_form_element-fieldset-504962-178.patch, failed testing.

mgifford’s picture

Status: Needs work » Needs review
FileSize
226.51 KB
246.38 KB
4.52 KB

Ok, I've updated the patch and have a screenshots here for checkboxes & radios:
/admin/config/services/aggregator/settings
/admin/config/people/accounts

We'll see what the bot says, but it's really nice to see this now that Details is in core.

Status: Needs review » Needs work

The last submitted patch, theme_form_element-fieldset-504962-186.patch, failed testing.

mgifford’s picture

Issue tags: +Needs tests

tagging.

Soundvessel’s picture

Adding some additional info here on some validation errors we got with 7.16. When I originally posted the issue it was marked as duplicate and I was pointed here.

While the forms API properly creates for/id pairs for the radio buttons and their individual labels the label for the group label are given a "for" attribute without a matching id creating validation errors and potential accessibility issues.

http://screencast.com/t/QvqgGKy7

It’s a problem with the Form API. When you use ‘radio’ as the type of the control, it wraps all of the radio options in a div and gives that div the id that matches the for attribute of the label, so it freaks out that it doesn’t match a form element. The radio form elements each get the id that match their array key. So, in this case the form api element is keyed as ‘repeats’ and the radio options are keyed as ‘weekly’, ‘monthly’, and ‘yearly’, so the label gets a for attribute of ‘edit-repeats’, the div wrapper gets an id of ‘edit-repeats’, and each radio option gets an id of ‘edit-repeats-weekly’, ‘edit-repeats-monthly’, ‘edit-repeats-yearly’.

mgifford’s picture

@Soundvessel - thanks for all of this additional information. The images are always useful.

I pointed you here because problems like this need to be first fixed in D8 before they can be brought back to D7.

I don't know if this is an issue that will be backported, but agree that it is a problem. We need help on cleaning up the simpletests so that the changes to the Forms API don't break other functionality.

Look forward to your contributions in fixing this issue.

mgifford’s picture

Status: Needs work » Needs review
FileSize
4.89 KB

Let's see if this gets rid of the notice errors. Also keeping up with core.

Status: Needs review » Needs work

The last submitted patch, theme_form_element-fieldset-504962-191.patch, failed testing.

mgifford’s picture

What doesn't it like with:
$fieldset_variables['element']['#title'] .= !empty($element['#required']) ? ' ' . theme('form_required_marker', array('element' => $element)) : '';

The tests are claiming that there's a problem with an Undefined index: #title but not sure how.

All of the 31 exceptions are tied to this line of code.

jthorson’s picture

Status: Needs work » Needs review
Issue tags: -Accessibility, -form API, -HTML Validation

Status: Needs review » Needs work
Issue tags: +Accessibility, +form API, +HTML Validation

The last submitted patch, theme_form_element-fieldset-504962-191.patch, failed testing.

Liam Morland’s picture

Status: Needs work » Needs review
FileSize
4.94 KB

$fieldset_variables['element']['#title'] isn't set if $element['#title'] is empty() 5 lines above. This patch should fix that problem.

Status: Needs review » Needs work

The last submitted patch, theme_form_element-fieldset-504962-196.patch, failed testing.

mgifford’s picture

I'm hoping to find some sense of how to resolve the exceptions tied to:
simpletest_script_run_one_test('596', 'Drupal\system\Tests\Upgrade\BareMinimalUpgradePathTest')

I suspect when that's fixed, it will just work.

jthorson’s picture

That's a known random failure (i.e. unrelated to the patch) that we've been experiencing on D8 for a number of weeks ... it should clear itself up on a retest.

jthorson’s picture

Status: Needs work » Needs review
mgifford’s picture

Excellent, thanks so much @jthorson!

mgifford’s picture

I think we need to eliminate the box, but other than that it's working!

mgifford’s picture

Reroll with some CSS changes.

mgifford’s picture

mgifford’s picture

Status: Needs review » Needs work
Issue tags: +Accessibility, +form API, +HTML Validation

The last submitted patch, theme_form_element-fieldset-504962-203.patch, failed testing.

mgifford’s picture

Status: Needs work » Needs review
FileSize
5.52 KB
falcon03’s picture

I didn't read all the comments, but what do we still need to get this patch in?

Is code/accessibility review enough or do we still need some other reviews (e.g. usability)?

mgifford’s picture

I don't think at this point it needs UX reviews. There shouldn't be any change in the user interface.

falcon03’s picture

This patch works great with radios. But giving a look at the code, I noticed that it wraps in a fieldset each element which has the #composite property set to "TRUE".

So the obvious question from my novice point of view: what other elements than radios have the #composite property set to "TRUE"?

Does this patch add fieldset also to the date field widgets?

Liam Morland’s picture

This patch introduces the #composite property. It adds the property to date widgets.

mgifford’s picture

FileSize
6.21 KB

Interdiff going back to @Liam Morland's in 196 & up to 207 by me.

@falcon03 the patch includes #composite's for radios, checkboxes & date values. Those are the common ones which should come within a fieldset.

andypost’s picture

Issue summary needs update about #composite form-property for elements - how to define it and for which elements. Suppose the property needs to be defined by composite widgets like #1957670: Link field labels don't show in entity forms

+++ b/core/includes/form.incundefined
@@ -4516,7 +4516,7 @@
- *   An associative array containing:
+ *   ¶

trailing white-space

+++ b/core/includes/form.incundefined
@@ -4688,14 +4688,32 @@
+  if (isset($element['#composite']) && $element['#composite'] === TRUE) {

Where the #composite defaults are defined?

andypost’s picture

Issue summary: View changes

Writing up the summary.

mgifford’s picture

I've tried to update the summary a bit, but it could certainly use more attention. Thanks for pointing that out Andy!

The trailing space & removing "An associative array containing:" was a mistake. Putting it back.

We'd need something like this defined for sure:

http://api.drupal.org/api/drupal/developer!topics!forms_api_reference.ht...

#composite

Used by: checkboxes, date, radios

Description: Creates a fieldset that is a logical grouping of form elements that provides semantic information about related elements for Assistive Technology.

Values: TRUE.

Usage example (system.module):

<?php
  $types['radios'] = array(
    '#composite' => TRUE,
    '#input' => TRUE,
    '#process' => array('form_process_radios'),
    '#theme_wrappers' => array('radios'),
    '#pre_render' => array('form_pre_render_conditional_form_element'),
  );
?> 
mgifford’s picture

mgifford’s picture

benjifisher’s picture

falcon03’s picture

Status: Needs review » Postponed

Hopefully we can get this issue fixed during the code sprints tomorrow.

I have to postpone this issue on:
#2002336: Introduce a CSS class to hide borders of fieldset elements

Liam Morland’s picture

Status: Postponed » Needs review
FileSize
5.15 KB

The patch as it is removes the default border on fieldsets. I just tried it and it creates no visual changes when adding the fieldset.

Reroll.

Status: Needs review » Needs work
Issue tags: -Accessibility, -form API, -HTML Validation

The last submitted patch, theme_form_element-fieldset-504962-219.patch, failed testing.

mgifford’s picture

Status: Needs work » Needs review
mgifford’s picture

mgifford’s picture

FileSize
5.06 KB

Here's the interdiff. Would be great to figure out why that test failed and get this patch in Core.

mgifford’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
113.1 KB
134.37 KB

Well, guess the bot was grumpy. Marking this RTBC. Here are the screenshots:
Fieldsets And Radios.
Fieldsets And Checkboxes

xjm’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests
+++ b/core/includes/form.incundefined
@@ -4707,14 +4707,32 @@ function theme_form_element($variables) {
+  // Composite elements consist of more than one HTML form control. These must
+  // be grouped by a fieldset.
+  if (isset($element['#composite']) && $element['#composite'] === TRUE) {
+    $fieldset_variables = array('element' => array());
+    $fieldset_variables['element']['#children'] = $prefix . $element['#children'] . $suffix;
+    foreach (array('#attributes', '#description', '#id', '#title', '#title_display') as $property) {
+      if (empty($element[$property])) {
+        $fieldset_variables['element'][$property] = NULL;
+      }
+      else {
+        $fieldset_variables['element'][$property] = $element[$property];
+      }
+    }
+    unset($fieldset_variables['element']['#attributes']['disabled']);
+    $fieldset_variables['element']['#title'] .= !empty($element['#required']) ? ' ' . theme('form_required_marker', array('element' => $element)) : '';
+    return theme('fieldset', $fieldset_variables);

I think we need to add automated test coverage for the new #composite FAPI element?

+++ b/core/modules/system/system.moduleundefined
@@ -3008,6 +3011,8 @@ function _system_rebuild_theme_data() {
   $defaults = array(
     'engine' => 'phptemplate',
     'regions' => array(
+      'account' => 'Account links',
+      'main_menu' => 'Main navigation',
       'sidebar_first' => 'Left sidebar',
       'sidebar_second' => 'Right sidebar',

I think this hunk is a merge artifact? It doesn't seem to have anything to do with the patch.

+++ b/core/modules/system/system.theme.cssundefined
@@ -7,7 +7,7 @@
-  border: 1px solid #ccc;
+  border: 0;

So @effulgentsia, @Dries, and I discussed this, but despite the title, this is part of the scope of the issue: per the summary fieldsets are logical, while details are visual.

xjm’s picture

Title: Provide accessible labels (was "Use fieldset and legend") for compound form elements » Provide a compound form element with accessible labels
Dries’s picture

Title: Provide a compound form element with accessible labels » Provide accessible labels (was "Use fieldset and legend") for compound form elements
Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs tests

Is there a reason we don't call this new property 'fieldset' instead of 'composite'? It would be more logical as it seems to be generating a fieldset.

Dries’s picture

Title: Provide accessible labels (was "Use fieldset and legend") for compound form elements » Provide a compound form element with accessible labels
Issue tags: +Needs tests
Dries’s picture

Status: Reviewed & tested by the community » Needs review
Liam Morland’s picture

I don't mind if it is called fieldset or composite. I choose composite because semantically, the form component is composed of more than one thing. As a result, it should get wrapped in a fieldset. This is analogous to using a CSS class of "important" for text that shows up as red instead of a class "red".

Either way is fine with me.

effulgentsia’s picture

#219 no longer applies to HEAD due to the stray hunk referenced in #225.2. This patch just removes that hunk.

effulgentsia’s picture

Here's another approach that perhaps addresses #227. Instead of a new #composite property, it changes form_pre_render_conditional_form_element() to form_pre_render_composite_form_element(), since the newer name and implementation seems to be what's desired by this issue.

This also removes #composite from 'date'; I'm not clear on why that was added: it's a single HTML element.

No interdiff attached, since most of the patch is affected by this change.

Thoughts?

effulgentsia’s picture

Small fix.

Liam Morland’s picture

This also removes #composite from 'date'; I'm not clear on why that was added: it's a single HTML element.

Date has separate form controls for year, month, and date. Is this only true some of the time? If so, we need a way to detect this. Earlier versions of this patch, such as the one in #93, do not use a #composite property; instead, they detect if an element has element has element children and if so wraps them in a fieldset.

Status: Needs review » Needs work

The last submitted patch, theme_form_element-fieldset-504962-233.patch, failed testing.

falcon03’s picture

@Liam Morland: you're definitely right. From my experience, the only situation where the date form widget could consist of a single form element IMO is the one in which you want to collect just the year.

Also, another "composite" element that should be wrapped in a fieldset is the "link widget", when it has to collect both the URL and the "link title" (which is going to be renamed to "link text", see
#1801268: Change link field 'title' field to 'link text'
for more details).

mgifford’s picture

Date could be a single field easily enough. Type in "Sept 8, 2013, 5:01pm" and I think that PHP can handle much of it.

http://php.net/manual/en/function.strtotime.php
http://strtotime.onlinephpfunctions.com/

This could be enhanced with calendar widgets, but with some server side processing a single element can work for dates.

Unfortunately a date widget hasn't been built in core to allow for this, so unless we ant to build this...

So, since we need #composite from 'date' - @effulgentsia - are you willing to re-roll the patch? Also know why the patches are failing?

mgifford’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests, -Accessibility, -form API, -HTML Validation

Status: Needs review » Needs work
Issue tags: +Needs tests, +Accessibility, +form API, +HTML Validation

The last submitted patch, theme_form_element-fieldset-504962-233.patch, failed testing.

mgifford’s picture

Issue tags: +fieldset

tagging

larowlan’s picture

Issue tags: +Needs reroll

Adding reroll tag

effulgentsia’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
5.4 KB

Straight reroll. Will address tests and feedback separately.

Status: Needs review » Needs work
Issue tags: -Needs tests, -Accessibility, -fieldset, -form API, -HTML Validation

The last submitted patch, theme_form_element-fieldset-504962-242.patch, failed testing.

mgifford’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, theme_form_element-fieldset-504962-242.patch, failed testing.

Liam Morland’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: +Needs tests, +Accessibility, +fieldset, +form API, +HTML Validation

The last submitted patch, theme_form_element-fieldset-504962-242.patch, failed testing.

mgifford’s picture

Status: Needs work » Needs review
FileSize
5.43 KB

re-roll. Documentation seemed to change or something.

Status: Needs review » Needs work

The last submitted patch, theme_form_element-fieldset-504962-248.patch, failed testing.

falcon03’s picture

Quickly tested the patch.

First off, the "select" widget provided by the date field is not wrapped in a fieldset, as it should be.

And secondly, the "visually-hidden" class, when needed, should not be applied to the fieldset legend, but to a span element inside of it. Otherwise the fieldset will not work properly with some screen readers (e.g. Voiceover on Mac OS X).

mgifford’s picture

I think that the patch for the date field is actually in #1918994: Improve Datetime and Daterange Widget accessibility although might be room to merge the two.

Look at the radios in the Registration and cancellation section of this page /admin/config/people/accounts

Or the checkboxes in Active search modules on admin/config/search/settings

mgifford’s picture

Status: Needs work » Needs review
FileSize
6.31 KB

The patch below should fix the placement of the visually-hidden class. This is what it was in the previous patch (on a checkbox):

<fieldset id="edit-active-modules" class="form-wrapper" aria-describedby="edit-active-modules--description">
<legend class="visually-hidden">
<span class="fieldset-legend">Active modules</span>
</legend>
<div class="fieldset-wrapper">
<div id="edit-active-modules--description" class="fieldset-description">Choose which search modules are active from the available modules.</div>
<div id="edit-active-modules" class="form-checkboxes">
</div>

Status: Needs review » Needs work
Issue tags: -Needs tests, -Accessibility, -fieldset, -form API, -HTML Validation

The last submitted patch, theme_form_element-fieldset-504962-252.patch, failed testing.

mgifford’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: +Needs tests, +Accessibility, +fieldset, +form API, +HTML Validation

The last submitted patch, theme_form_element-fieldset-504962-252.patch, failed testing.

mgifford’s picture

Status: Needs work » Needs review
FileSize
5.96 KB

So I don't know where this file went, so not sure how to make the adjustments to the fieldset:
core/modules/system/css/system.theme.css

That should be addressed though after this gets in #2002336: Introduce a CSS class to hide borders of fieldset elements

Status: Needs review » Needs work

The last submitted patch, theme_form_element-fieldset-504962-256.patch, failed testing.

mgifford’s picture

Status: Needs work » Needs review
FileSize
6.01 KB
mgifford’s picture

Screenshots are #2002336-26: Introduce a CSS class to hide borders of fieldset elements - also link for testing in SimplyTest.me with the class patch.

Status: Needs review » Needs work
Issue tags: -Needs tests, -Accessibility, -fieldset, -form API, -HTML Validation

The last submitted patch, theme_form_element-fieldset-504962-258.patch, failed testing.

Anonymous’s picture

Issue summary: View changes

Updated issue summary.

mgifford’s picture

Issue summary: View changes
Issue tags: +Accessibility, +fieldset, +wcag
mgifford’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 258: theme_form_element-fieldset-504962-258.patch, failed testing.

mgifford’s picture

Status: Needs work » Needs review
FileSize
5.25 KB

I think the bot's still going to complain about this:

    // Check #field_prefix and #field_suffix placement.
    $elements = $this->xpath('//span[@class="field-prefix"]/following-sibling::div[@id="edit-form-radios-test"]');
    $this->assertTrue(isset($elements[0]), 'Properly placed the #field_prefix element after the label and before the field.');

But the patch failed apply and some of the tests have now been removed.

Status: Needs review » Needs work

The last submitted patch, 264: theme_form_element-fieldset-504962-264.patch, failed testing.

larowlan’s picture

This hunk is the issue, it causes the views ui replacement patterns and a view other places to be empty.

-  if (isset($element['#value'])) {
-    $output .= $element['#value'];
-  }

screenshot with that hunk:
busted views replacements

larowlan’s picture

The last submitted patch, 266: compound-element-504962.266ish.patch, failed testing.

mgifford’s picture

FileSize
516.36 KB

I suspect the

-  if (isset($element['#value'])) {
-    $output .= $element['#value'];
-  }

Was taking out the "1" & "Array" which I got when applying this patch and going to the user/1 edit page.

I also expect that this patch took out the span[@class="field-prefix"] & span[@class="field-suffix"] to as I couldn't find it anywhere on the radio buttons on that page.

results of the patch with errors and undesired output.

larowlan’s picture

FileSize
4.43 KB
8.8 KB

Aye, the

-  if (isset($element['#value'])) {
-    $output .= $element['#value'];
-  }

Was taking that out, but somewhere during re-roll this was removed from theme_details instead of theme_fieldset. So refactored.

Fixed prefix, suffix as well as the required marker/attributes.

Added the disabled properties, this requires increasing the elements matching the disabled xpath count to 41, because we added two wrappers here.

sun’s picture

  1. +++ b/core/includes/form.inc
    @@ -954,14 +954,28 @@ function theme_fieldset($variables) {
    -  $output = '<fieldset' . new Attribute($element['#attributes']) . '>';
    +  // Hide box for fieldset by default.
    +  $element['#attributes']['class'][] = 'fieldset-no-border';
    

    A general and more fundamental issue with the proposed change is that a fieldset can be rendered as an own element of #type 'fieldset' - OR - as a theme wrapper.

    Contrary to that, theme_form_element() is only ever invoked as a theme wrapper and cannot be used as a standalone element #type (you'd use #type 'item' instead).

    This explains why theme_form_element() builds a new $attributes array from scratch and selectively tests for additional #attributes that may be taken over for the wrapping form element container.

    Now, theme_fieldset() can be invoked as both a standalone #type as well as a theme wrapper. When it is invoked as a theme wrapper, then there is a risk of duplicating all of the #attributes of the inner/wrapped elements onto the wrapping fieldset, since the wrapping fieldset's $attributes are not built from scratch, but instead, added to the existing $element['#attributes'].

  2. +++ b/core/includes/form.inc
    @@ -954,14 +954,28 @@ function theme_fieldset($variables) {
    +  $prefix = isset($element['#field_prefix']) ? '<span class="field-prefix">' . $element['#field_prefix'] . '</span> ' : '';
    +  $suffix = isset($element['#field_suffix']) ? ' <span class="field-suffix">' . $element['#field_suffix'] . '</span>' : '';
    ...
    +  $output = $prefix . '<fieldset' . new Attribute($element['#attributes']) . '>';
    
    @@ -970,11 +984,8 @@ function theme_fieldset($variables) {
    +  $output .= "</fieldset>\n" . $suffix;
    

    Please note that #field_prefix and #field_suffix are supposed to be "prefix_inner" and "suffix_inner" - i.e., the specified output is supposed to appear WITHIN the element, right before and right after the main output element of the element.

    drupal_render() processes #prefix (!= #field_prefix) and #suffix (!= #field_suffix) already. #prefix and #suffix go OUTSIDE of the actual element.

    It is debatable where #field_prefix and #field_suffix could or should appear in a fieldset, but for consistency, they should probably wrap the main element output; i.e., like this:

    DIV.fieldset-wrapper
    #description
    #field_prefix
    #children
    #value
    #field_suffix
    /DIV

mgifford’s picture

@sun you highlighted "a risk of duplicating all of the #attributes of the inner/wrapped elements onto the wrapping fieldset" - any thoughts on how to mitigate that? We should be able to just

I wasn't sure if you wanted inline docs about #field_prefix and #field_suffix, but it might be a good idea put something in there for clarification. I'm not certain I'm clear enough on them, to write it though.

I've brought the wrapper to the outside of the fieldset as you've requested. I don't understand why it makes sense for the description to be moved before the prefix though. Shouldn't the prefix be on the outside of the fieldset?

Output of Roles from user/1/edit

<div class="fieldset-wrapper"><fieldset id="edit-roles" class="form-wrapper fieldset-no-border"><legend><span class="fieldset-legend">Roles</span></legend><div id="edit-roles" class="form-checkboxes"><div class="form-item form-type-checkbox form-item-roles-authenticated form-disabled">
 <input disabled="disabled" type="checkbox" id="edit-roles-authenticated" name="roles[authenticated]" value="1" checked="checked" class="form-checkbox" /> <label class="option" for="edit-roles-authenticated">Authenticated user</label>
</div>
<div class="form-item form-type-checkbox form-item-roles-administrator">
 <input type="checkbox" id="edit-roles-administrator" name="roles[administrator]" value="administrator" checked="checked" class="form-checkbox" /> <label class="option" for="edit-roles-administrator">Administrator</label>
</div>
</div></fieldset>
</div>
mgifford’s picture

Status: Needs review » Needs work

The last submitted patch, 272: compound-element-504962.272.patch, failed testing.

The last submitted patch, 272: compound-element-504962.272.patch, failed testing.

mgifford’s picture

What happened to:

  summary,
  legend {
    font-weight: bold;
    text-transform: uppercase;
  }

in core/themes/seven/style.css

That bit of code seems to be missing.

mgifford’s picture

Status: Needs work » Needs review
FileSize
8.69 KB

Without changes to core/themes/seven/style.css

jrdixey’s picture

Issue summary: View changes

Modified issue summary in accordance with guidelines at https://drupal.org/issue-summaries.

jrdixey’s picture

Issue summary: View changes

Added an Update comment link and added links to API Change patch/comment numbers.

jrdixey’s picture

Added a reference to CSS styling issue #2002336: Introduce a CSS class to hide borders of fieldset elements to the Remaining tasks section of the Issue Summary.

mgifford’s picture

Thanks! It's a big improvement.

mgifford’s picture

Status: Needs review » Needs work

The last submitted patch, 277: compound-element-504962.277.patch, failed testing.

mgifford’s picture

re-roll.

mgifford’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 284: compound-element-504962.282.patch, failed testing.

mgifford’s picture

Status: Needs work » Needs review
mgifford’s picture

Status: Needs review » Needs work

The last submitted patch, 288: compound-element-504962.288.patch, failed testing.

The last submitted patch, 288: compound-element-504962.288.patch, failed testing.

mgifford’s picture

Status: Needs work » Needs review
mgifford’s picture

Status: Needs review » Closed (duplicate)

#2192419: Use a WCAG-compliant fieldset (fieldgroup) for #type radios/checkboxes works for accessibility! I'm marking this one as a duplicate.

mgifford’s picture