Problem

In order for Drupal to be WCAG 2.0 compliant, compound form elements (radios and checkboxes, for instance) should be wrapped into fieldsets. However, wrapping these controls into fieldsets has an undesirable effect from a usability point of view: it adds a visual border around the control.

Proposed resolution

Add a CSS class that can be used to hide borders from elements such as fieldsets and table headings.

CommentFileSizeAuthor
#95 hide or remove.png8.73 KBkarimiehsan1819
#86 Screen Shot 2014-02-10 at 11.35.26 AM.png540.95 KBmgifford
#84 User-1-Edit.png149.51 KBmgifford
#84 Configure-Site-Screen.png551.11 KBmgifford
#57 fieldset-css-2002336-57.patch2.64 KBLewisNyman
#50 fieldset-css-2002336-50.patch2.89 KBLewisNyman
#47 Screen Shot 2013-12-08 at 15.51.55.png41.01 KBLewisNyman
#47 Screen Shot 2013-12-08 at 15.50.31.png32.23 KBLewisNyman
#47 Screen Shot 2013-12-08 at 15.49.34.png39.63 KBLewisNyman
#47 Screen Shot 2013-12-08 at 15.47.25.png94.56 KBLewisNyman
#47 Screen Shot 2013-12-08 at 15.46.03.png26.01 KBLewisNyman
#36 Screen Shot 2013-11-26 at 2.15.36 PM.png150.64 KBmgifford
#34 fieldset-css-2002336-34.patch1.78 KBLewisNyman
#26 Screen Shot 2013-10-18 at 5.42.28 PM.png22.4 KBmgifford
#26 Screen Shot 2013-10-18 at 5.42.09 PM.png44.49 KBmgifford
#26 Screen Shot 2013-10-18 at 5.37.49 PM.png232.97 KBmgifford
#26 Screen Shot 2013-10-18 at 5.36.28 PM.png238.33 KBmgifford
#77 installer-fieldgroup.png35.78 KBsun
#77 drupal8.fieldgroup.patch3.25 KBsun
#75 drupal8.fieldset-no-box.73.patch429 bytessun
#69 fieldset-css-2002336-69.patch479 bytesmgifford
#67 Screen Shot 2014-01-27 at 10.48.16 AM.png92.09 KBmgifford
#66 fieldset-css-2002336-66.patch4.06 KBmgifford
#64 Screen Shot 2014-01-22 at 12.40.31 AM.png43 KBwebchick
#64 Screen Shot 2014-01-22 at 12.40.52 AM.png44.37 KBwebchick
#61 fieldset-css-2002336-64.patch1.86 KBLewisNyman
#43 fieldset-css-2002336-43.patch3.26 KBLewisNyman
#19 fieldset-css-2002336-19.patch505 bytesmgifford
#14 fieldset-css-2002336-14.patch486 bytesmgifford
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

falcon03’s picture

Issue tags: -adopting D7 usability to D6 +Usability
Liam Morland’s picture

The latest patch in #504962: Provide a compound form element with accessible labels removes the default border on fieldsets.

falcon03’s picture

@Liam Morland: does it remove borders from fieldsets using a class or just redefining the "fieldset" tag appearance?

Maybe we should introduce the CSS class in this separate issue, because it will be useful for other things than fieldsets too: for instance, adding tableheadings to the status report.

Liam Morland’s picture

With the other patch installed, fieldsets do not have a border by default. Basically, it is:

fieldset { border: 0; }

falcon03’s picture

@Liam Morland: since we need to hide borders also from tableheadings, do you think we should re-define the html elements any time instead of assigning a CSS class to them?

I don't know what the best option is.

Liam Morland’s picture

I think it is best for the default CSS to have no borders and for a class to add borders when they are needed. Maybe tables should be an exception, since they normally have borders. A table without a border would be an exception.

ry5n’s picture

+1 to no fieldset borders by default, and add a class when needed.

From #5, when do we need to hide borders on table headers?

falcon03’s picture

re#7: For instance, we would like to add table headings to the status report. See this issue for more details:
#1811128: No table header on install requirements

mgifford’s picture

So with this we'd be basically stating that fieldsets in Drupal are semantic groupings. If you want a visual group, then use a detail.

This works for me. So should it go in core/modules/system/system.base.css

ry5n’s picture

@mgifford On the contrary, both fieldsets and details have specific semantics and behavior (details are collapsibles). I think the idea here is that if you want a visual grouping, add a class. These kinds of repeated visual styles are ripe for abstraction; in the Seven style guide sandbox, we tentatively used a '.panel' class for fieldsets *and* details to apply border and padding. But we could also follow http://csswizardry.com/2011/10/the-island-object/

mgifford’s picture

Yes, ry5n. I was being sloppy in my description. Fieldsets by default though in D8 would have no visual styling and would be largely used for grouping related elements like radios, checkboxes, dates & other elements which need them for accessibility. The approach suggested by the island object wouldn't help with the accessibility challenges we are trying to resolve.

ry5n’s picture

@mgifford That was a bit of an aside about the island object. I understand the a11y reasons for this issue; +1. My point was to not use details for any visual effect, but I understand that’s not what you meant.

mgifford’s picture

Issue tags: +table, +fieldset

Tagging.

mgifford’s picture

Status: Active » Needs review
FileSize
486 bytes

I'm still a bit confused as to what we're looking for with the tables, but thought I'd roll up a patch to see bout addressing the fieldsets here in anycase.

I think that this is all stuff that should be part of system.theme.css. There is a problem making table headings invisible, but available to the screen reader.

th {
  padding-right: 1em; /* LTR */
  text-align: left; /* LTR */
}
thead > tr {
  border-bottom: 1px solid #000;
}

This is just the CSS from https://drupal.org/files/cardinality-label-1932074.45.patch

mgifford’s picture

Issue tags: -Usability, -table, -Accessibility, -fieldset

#14: fieldset-css-2002336-14.patch queued for re-testing.

mgifford’s picture

#14: fieldset-css-2002336-14.patch queued for re-testing.

mgifford’s picture

#14: fieldset-css-2002336-14.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Usability, +table, +Accessibility, +fieldset

The last submitted patch, fieldset-css-2002336-14.patch, failed testing.

mgifford’s picture

Status: Needs work » Needs review
FileSize
505 bytes

The core css definitions for fieldsets have moved to core/misc/drupal.base.css

I do think though that this patch probably should stay in core/modules/system/css/system.theme.css

I'm not clear though about why those elements have been moved over or why the drupal.base.css was created in the first place. Guess it has to do with conforming to the new coding standards here #1924430: Add drupal.base library for base CSS styles

perandre’s picture

The patch applies nicely, and the CSS would indeed remove borders from fieldset tag.

But: If this code is also intended for tables (and future UI elements), wouldn't it be better to just have a classed named "no-border"? The no-border css would still take presedence over the base styles anyways, and we get less bloat.

bserem’s picture

Status: Needs review » Reviewed & tested by the community

changing to RTBC

I believe Tables and future UI elements is better to be discussed in a new issue.

mgifford’s picture

Title: Introduce a class to hide borders from elements » Introduce a class to hide borders fieldsets elements
Issue tags: -table

@perandre I'm not opposed to generalizing this, but in our testing with accessibility & tables we'd need to take a bit more invasive approach. I'm not sure that it could be generalized in a way that is also accessible.

I'd like to see tables discussed in a new issue. Say #2099761: Introduce a class to hide borders for tables

webchick’s picture

Status: Reviewed & tested by the community » Needs review

Just asking (feel free to say no and set back to RTBC), is it possible to also convert one of the core fieldsets (such as the installer one) to using this class so we can confirm that it does what it says?

mgifford’s picture

@webchick just to clarify, I can't recall what we'd change with the fieldsets in the installer. I'm not sure if you're asking so we can get a screenshot or if you want to commit this with an example. In either case, there's a related patch #504962: Provide a compound form element with accessible labels would probably be better.

Can we get this CSS change in, and then focus on bringing in the example ASAP?

webchick’s picture

I was asking for an example to go in *with* the CSS change because currently there's no way to evaluate whether or not these classes actually do what they say they're going to do. :) Or, barring that, a before/after screenshot would also be fine.

mgifford’s picture

FileSize
22.4 KB
44.49 KB
232.97 KB
238.33 KB

Ok, so here's a good example on STM. With the patch from #504962-258: Provide a compound form element with accessible labels .

admin/structure/views/settings

Views Settings Radiobox

user/1/edit

Editing User 1 with radio & checkboxes

This is with the fieldset class applied. Without it, well, these 3 fieldsets just look like the boxy fieldsets we got rid of when we moved to details. Essentially.

Ok, I'll include the before shots, but just not include them as they are.. Well, not nice..

mgifford’s picture

Issue tags: -Usability, -Accessibility, -fieldset

#19: fieldset-css-2002336-19.patch queued for re-testing.

mgifford’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

@webchick - The images & use case were asked for and provided so putting this back to RTBC. Hope that's ok.

Xano’s picture

19: fieldset-css-2002336-19.patch queued for re-testing.

webchick’s picture

Title: Introduce a class to hide borders fieldsets elements » Change notice: Introduce a class to hide borders fieldsets elements
Priority: Normal » Major
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

Awesome, thanks!

Committed and pushed to 8.x.

I'm thinking we might want a change notice for this? Including when module authors should use it.

LewisNyman’s picture

Status: Active » Needs work
Issue tags: +CSS

Hi, sorry I missed reviewing this. The class that this selector has introduced is a conflict with our CSS standards.

Creating style rules that undo other rules, like .component-no-padding makes CSS over-complex, hard to understand and maintain, and bloats the stylesheet. Needing such styles usually indicates that some existing rules are doing too much.

As it happens, I've opened an issue to introduce a generic 'fieldset like' styling class for when module developers want to group items together. #2113903: Introduce a 'pane' component to visually group form items

I would suggest we remove the fieldset styling completely from system, and leave it up to a theme to define this styling.

mgifford’s picture

@LewisNyman We need a way as part of Core to add fieldsets semantically to improve accessibility without affecting the page visually.

I think browsers apply fieldset styling by default which is against the look/feel of Core themes.

I'm not sure what the process is when Drupal's CSS standards conflict with implementing the WCAG 2.0 standards.

webchick’s picture

Title: Change notice: Introduce a class to hide borders fieldsets elements » Introduce a class to hide borders fieldsets elements
Issue tags: -Needs change record

Ok, sounds like this still needs some more discussion so, rolled this patch back for now.

LewisNyman’s picture

Status: Needs work » Needs review
FileSize
1.78 KB

@mgifford I don't think it's that dramatic, I don't think there is a direct conflict between accessibility standards and our CSS standards. The solution that was committed did conflict with our standards.

If we have to add a class to undo styling, that's a very good clue that our styling is too broad. The correct way to handle this is to remove the default styling completely, and introduce a class to add the styling.

Here's a proposal in patch form, it might be more clear.

tstoeckler’s picture

The new approach makes a lot of sense, in my opinion. Is there any reason the class should be limited to forms, though?

mgifford’s picture

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

I tested this with SimplyTest.me and this approach works fine! This applies the latest patch from @LewisNyman above with this introduction of fieldsets for multi-part forms..

The secreenshot from /node#overlay=user/1/edit
screenshot from user/1/edit with firebug

webchick’s picture

Status: Reviewed & tested by the community » Needs review

Cool, this works for me. I expected this to remove border from everywhere (e.g. on pages like admin/config/development/performance) but it didn't seem to do that, so cool.

#35 raised a decent point, though, which wasn't really addressed. By naming the class "form-group" it implies that it can only be used on forms when in reality, it could potentially be used wherever.

This issue has been dragging on awhile, so let's timebox the discussion to a week (to allow for the holiday in the US). If we can't come up with anything better by then, we'll go ahead with this.

LewisNyman’s picture

I expected this to remove border from everywhere (e.g. on pages like admin/config/development/performance) but it didn't seem to do that, so cool.

It should be, but there are very few places in core that actually use fieldset tags after #1168246: Freedom For Fieldsets! Long Live The DETAILS.

If we created a class like .form-group that added the same visual spacing, we wouldn't have to style details elements directly either and just apply this class.

This is the same reasoning behind #2113903: Introduce a 'pane' component to visually group form items. Maybe we should take lead from the style guide and implement the same class in system. I know pane is a loaded term but so is module and we're using that in CSS.

mgifford’s picture

34: fieldset-css-2002336-34.patch queued for re-testing.

mgifford’s picture

Issue tags: +Usability, +Accessibility, +fieldset

Not sure why these tags got removed. Wasn't intentional.

LewisNyman’s picture

Title: Introduce a class to hide borders fieldsets elements » Remove default fieldset styling and introduce a class to add styling

Renaming to new objective. Hopefully it will be more clear.

mgifford’s picture

Status: Needs review » Reviewed & tested by the community

It's been a week, so bumping it back to RTBC.

LewisNyman’s picture

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

Based on the suggestion in #35 I changed the class name to pane to match #2113903: Introduce a 'pane' component to visually group form items. This won't conflict with Panels because it uses the class panels-pane.

I also found a few more places in core where we were using fieldsets that would need the class.

Jeff Burnz’s picture

+++ b/core/modules/system/css/system.theme.css
@@ -99,6 +99,16 @@ abbr.form-required, abbr.tabledrag-changed, abbr.ajax-changed {
+fieldset {
+  border: 0;
+  margin: 0;
+  padding: 0;
+}
+.pane {
+  border: 1px solid #c0c0c0;
+  margin: 0 2px;
+  padding: 0.35em 0.625em;
+}

Why reset and add back, that affects all fieldsets, we should only normalize not reset. I don't see anything in the CSS standards that says we can't override browser default styles (which is actually what this still does).

To my mind this adds several layers of CSS wtfs and breaks CSS standards by using an overly generic class name and will highly likely false positive against something (like an external library). It also lacks any sort of real semantics, where I styled fieldset in Bartik I know what that is implicitly, now I have "pane", well, what is that?

Look, I get where you are coming from, you are saying fieldset is most likely to be used to wrap form elements as some sort of compound element, right, therefor remove the border. I would say that is an assumption and that core should not assume the OP as default, aka that groups of form elements should not have a border etc. That is a style and core should not really be in the business of styling my theme for me (at least as little as possible).

LewisNyman’s picture

Hey Jeff :)

The points you've raised is something I've been trying to figure out as well. Adding a normalize to strip it out seems weird but what can we do when we are importing the normalize as a library? We can't modify it.

The point that this issue raises is that visual styling should be separate from this semantic meaning, because there are situations where you want one and not the other. See #2113903: Introduce a 'pane' component to visually group form items as well. This seems to be in line with our CSS standards.

  1. Maybe the class name is bad? I'm open to suggestions, I would like this component to be used outside of forms though.
  2. Maybe this should only be in the Seven theme or system.admin.css? The objective of this issue is for the admin interface after all?
Jeff Burnz’s picture

Hey Lewis, long time no see and I apologise for that, I was away for more than 2 years due to ill health.

Seven or system.admin.css I think is more appropriate, in fact its was the system.theme placement which raised my eyebrows the most.

When I question the semantics I do so merely on the level of human comprehension, having a class for separation is fine, it's just the name of it. Generically named classes are kind of dangerous though, they do false positive and often badly because of external js libs that hook it somehow.

I'm not sure of what value it has outside of forms/admin, if it did then yes we're looking at something like a "pane object" and that has value, I'm just not sure what that value is.

LewisNyman’s picture

Nice to have you back Jeff. I'm on Skype and IRC most weekdays if you feel like a chat.

If @mgifford is happy with moving this into admin CSS then let's move in this direction.

The reason we decided to expand the scope of #2113903: Introduce a 'pane' component to visually group form items beyond grouping form items is that the use cases we looked at where you would want to group items visually were consistent with the fieldset use case. These all perform the same purpose but the ones that are implemented are slightly different and consistent.

Examples:

Most exposed filters on views listing pages.

The navigation on the config page.

Details components

The sidebar thingy on the content creation page and block layout page

mgifford’s picture

For most instances putting it in system.admin.css will probably be fine as the most common instances are in Seven's admin theme. However, there are going to be a number of places where someone would have radios/checkboxes or other multi-part forms in something other than the admin side. The Advanced search is certainly a big one as listed in fieldset-css-2002336-34.patch... This pattern would probably need to be repeated in a D8 version of Webforms for instance.

I just want this to be a common pattern and don't want to have to replicate it in other themes if we want to leverage that pattern. What's the concern with:
core/modules/system/css/system.theme.css

Other than that you don't want anything loading on the public pages if it isn't needed.

Jeff Burnz’s picture

The point is Core should not be applying styles to html elements globally by default, the reset is a global style applied to all fieldsets, whether I want it or not. That's the sort of thing I as a themer do not want.

I think "no reset" is an entirely fair expectation for themers. I can live with normalize, but stripping off padding, margin and border is a very aggressive reset in any themers book.

In no way does this preclude your ability to have a reusable style for fieldsets, i.e. load the styles into .pane or what have you.

Look, Lewis was right to say that fieldset-no-border was a violation, it was, but the alternative proposed is worse. However, the basic underlying idea to load up an object like class that is reusable is good, so do it. Just don't kill kittens while your at it, aka reset elements to save a few lines of CSS, it impacts on all forever after.

LewisNyman’s picture

Ok, here's a patch that only applies the change to admin facing styles.

Status: Needs review » Needs work

The last submitted patch, 50: fieldset-css-2002336-50.patch, failed testing.

mgifford’s picture

Status: Needs work » Needs review

50: fieldset-css-2002336-50.patch queued for re-testing.

mgifford’s picture

Ok, so based on this:

array_flip(): Can only flip STRING and INTEGER values!
array_flip(Array)

I'm guessing we can't insert this attribute here: '#attributes' => array('class' => array('pane')),.

LewisNyman’s picture

Status: Needs review » Needs work
LewisNyman’s picture

I found a page that still uses a fieldset!

/admin/config/media/image-toolkit

mgifford’s picture

Status: Needs work » Needs review

50: fieldset-css-2002336-50.patch queued for re-testing.

LewisNyman’s picture

FileSize
2.64 KB

I removed the class from the node search forms because they are front facing forms anyway. I've added the class to the image styles form.

philipz’s picture

I've applied patch #57 and I'm looking on theme settings and performance pages.
There are no elements with class '.pane' and therefore the patch is not styling anything. I wonder if something else changed and patch needs re-roll to adapt the changes or I'm missing something.

mgifford’s picture

57: fieldset-css-2002336-57.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 57: fieldset-css-2002336-57.patch, failed testing.

LewisNyman’s picture

Status: Needs work » Needs review
FileSize
1.86 KB

Reroll.

I'm struggling to find anywhere in core where we actually use fieldsets now? It looks like they've all bee converted to details elements...

mgifford’s picture

It's easy to find if you apply the patch from #504962: Provide a compound form element with accessible labels

Most of them were taken out, but I know that there are one or two left. I'm not sure exactly where though, sorry.

Most of them were converted to details.

Jeff Burnz’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me.

FYI I use fieldset a lot still (in D8), and this actually helps me out a heap, so great!

webchick’s picture

Status: Reviewed & tested by the community » Needs work
FileSize
44.37 KB
43 KB

I found a spot, which is Views UI wizard. I've no idea why/how, but this patch seems to make it very inconsistent. (Firefox 26.0, Mac OSX)

Before:
Each section of Views UI wizard is in a fieldset

After:

Only half of the Views UI wizard sections are in a fieldset

mgifford’s picture

fieldset id="edit-name" class="fieldset-no-legend pane form-wrapper">
<legend>
<span class="fieldset-legend">View basic information</span>
</legend>
<fieldset id="edit-show" class="container-inline pane form-wrapper">
<legend>
<span class="fieldset-legend">View settings</span>
</legend>
<fieldset id="edit-page" class="views-attachment fieldset-no-legend form-wrapper">
<legend>
<span class="fieldset-legend">Page settings</span>
</legend>
<fieldset id="edit-block" class="views-attachment fieldset-no-legend form-wrapper">
<legend>
<span class="fieldset-legend">Block settings</span>
</legend>

The last two are inconsistent as they are missing the CSS class "pane".

How are they generated in Views?

mgifford’s picture

Status: Needs work » Needs review
FileSize
4.06 KB

Ahh.. Wizards can really suck some times. Because we hadn't edited core/modules/views/lib/Drupal/views/Plugin/views/wizard/WizardPluginBase.php

It would have failed on Page settings, Page display settings, Block settings & Block display setting

mgifford’s picture

With the latest patch.

sun’s picture

Hrm. Sorry, coming late to this party.

I vehemently disagree with the proposal to apply a default styling to fieldsets that no longer makes them appear as regular fieldsets.

The reasoning is simple:

  1. Despite the year being 2014, browsers are still using different default styles for fieldsets and their rendering engines are partially handling them in very special ways. Normalize helps a little bit, but unlike pretty much all other HTML elements, you won't be able to get back to the original default styling.
  2. To achieve a regular fieldset with default styling, you'd have to override Drupal's default styles and use more specific selectors, in order to not break the expected default styling everywhere else. If anything, that is a clear violation of CSS standards.

The quoted part of our CSS standards in #31 was applied to the wrong context — only custom styles should not have to override other custom styles to revert their styling.

But this is about browser default styling. If I output a <fieldset>, then I expect to get a standard fieldset. With default borders, padding, and the legend in the top border.

Overriding the default styling of a standard HTML element for a particular and custom use-case in the system's default CSS is the exact opposite of what default styles should do. → If you have a specific use-case for styling a standard HTML element in a custom way, then that's what classes are for.

Therefore, the originally proposed patch that used fieldset.fieldset-no-border was correct.

But that said, the original patch used a too specific selector that duplicated itself; it should either be fieldset.no-border or just .fieldset-no-border.

Additionally, "no-border" does not fully encompass the effect, because the default padding is removed, too. → .no-box or .plain or .flat or similar would be more accurate class names.

mgifford’s picture

FileSize
479 bytes

I don't care one way or the other about how this box is over-ridden, as long as there is a way to do it.

As usual, @sun makes a good argument, so I've included the patch.

The previous patch addressed border, margin & padding by making them all 0. Not sure if that's a better approach or not:

 fieldset {
  border: 0;
  margin: 0;
  padding: 0;
}
LewisNyman’s picture

But this is about browser default styling. If I output a , then I expect to get a standard fieldset. With default borders, padding, and the legend in the top border.

Thanks Sun.

I'm not sure it is. This is about how admin themes handle fieldset styling. I don't think should be any expectation of style from an html element alone, unless you're developing a theme, which we addressed in #44.

We also have to consider the use case of wanting the style without the element. See #47 That's a strong argument for separating styling from the element. Our CSS standards are built around the goal of reusable classes.

Maybe this should just sit within the Seven theme? I think that would give contrib admin themes a bit of a tricky situation to deal with.

mgifford’s picture

@LewisNyman, there has to be something in core (not just Seven) that allows us to deal with #504962: Provide a compound form element with accessible labels

Ultimately this is a very small amount of CSS we're considering here. It may not fit perfectly within the guidelines, but I do know that it will allow an important accessibility issue to be brought into Core.

sun’s picture

Thanks!

  1. The diff context of this patch reveals that we have another CSS class .fieldset-no-legend already, so for the sake of consistency, I'd recommend to use .fieldset-no-box here for now.

    → Let's consider to open a follow-up issue to remove the fieldset- prefix from both afterwards.

  2. Regarding the fieldset no-box styles, I'd (1) restrict the border to just border-width and (2) margin most likely should be left alone, because fieldsets typically appear in between of .form-items as well as after other fieldsets, and we want to retain some sane vertical spacing. Thus:

    .fieldset-no-box {
      border-width: 0;
      padding: 0;
    }
    
  3. Regarding the legend no-box styles, I don't see why those are contained in system.theme.css. Only the Seven theme applies a text-transform:uppercase to fieldset legends, so at minimum, that style override needs to be moved into Seven's CSS.

    Additionally, the selector should only target the immediate child, because fieldsets can contain fieldsets:

    .fieldset-no-box > legend
    

    That said, I'm not sure I understand why the uppercase styling is reverted here? I can only guess that we're trying to cater for different use-cases:

    1. With regard to #504962: Provide a compound form element with accessible labels , the legend will be the #title of #type 'radios' and 'checkboxes'.
    2. With regard to #23 and #2113903: Introduce a 'pane' component to visually group form items, the legend will be an arbitrary heading-alike label.

    For B), this override should actually not happen, because fieldsets and details would appear differently in Seven otherwise.

    For A), I assume that the selector for Seven's CSS should actually be more targeted, along the lines of this:

    .form-wrapper.fieldset-no-box > legend {
      text-transform: none;
    }
    

    In other words, only revert the uppercasing if the fieldset is used as a wrapper for the compound radios/checkboxes form elements.

    All of that being said, this adjustment has nothing to do with this issue here and belongs into #504962: Provide a compound form element with accessible labels instead, because that issue is the one that wants to use fieldsets in a new/additional way.

    → Let's remove the text-transform override styling entirely from this patch.

Status: Needs review » Needs work

The last submitted patch, 69: fieldset-css-2002336-69.patch, failed testing.

The last submitted patch, 69: fieldset-css-2002336-69.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
429 bytes

@LewisNyman:

This is about how admin themes handle fieldset styling.

That's the first misconception from my perspective, because fieldsets can appear in front-end themes, too. The context of an "admin theme" is irrelevant here.

I don't think should be any expectation of style from an html element alone

By saying that, I can only assume that you're a fan of the former reset.css approach instead of the normalize.css approach. The former concept did exactly what you're saying: Force-remove all expectations that anyone could possibly have from all HTML elements. Conversely, the latter concept is no longer doing that, but instead: Harmonize the default styling of all HTML elements across browser vendors.

This means that (1) there are expectations for how a particular HTML element is rendered by browsers by default and (2) as a themer, I expect basic HTML elements to mostly appear with their default styling, give or take a few sane assumptions — but overriding the default style for a completely custom use-case is not at all a sane assumption.

@Jeff Burnz appears to have raised the exact same red flags before already.

Maybe this should just sit within the Seven theme?

That makes little sense, because the ultimate goal is to introduce this alternate styling for fieldsets as a base concept for Drupal's user interface. We'll end up with:

  1. Details
  2. Unboxed fieldsets to group elements into a labeled container
  3. "Transparent" fieldsets to group compound radios/checkboxes form elements
LewisNyman’s picture

Title: Remove default fieldset styling and introduce a class to add styling » Introduce a class to hide borders fieldsets elements
Status: Needs review » Reviewed & tested by the community

That's the first misconception from my perspective, because fieldsets can appear in front-end themes, too. The context of an "admin theme" is irrelevant here.

If this is the case then I agree with everything you say after because we definitely don't want to affect default styling on the front-end.

Reading over the requirements in #48 that looks right:

For most instances putting it in system.admin.css will probably be fine as the most common instances are in Seven's admin theme. However, there are going to be a number of places where someone would have radios/checkboxes or other multi-part forms in something other than the admin side.

So this patch is the only way to solve this problem. RTBC.

sun’s picture

Assigned: Unassigned » sun
Status: Reviewed & tested by the community » Needs review
FileSize
3.25 KB
35.78 KB

Sorry, I really don't want to hold this up any further (since this issue still blocks progress elsewhere), but I wanted to address the concern that @webchick originally raised in #25 already → include an example to prove that this works as intended.

In order to address that, I need to introduce a new #type in this issue already, because we don't want to add arbitrary CSS classes to individual form elements (which essentially makes #2113903: Introduce a 'pane' component to visually group form items obsolete).

I did a quick poll in IRC to see whether "pane" is really an accurate name for this user interface component. After a brief discussion, for now I decided to go with #type "fieldgroup" and renamed the CSS class accordingly.

Note that various people would prefer if #type 'container' would simply be adjusted to conditionally become an unboxed fieldset, in case a #title property has been set. — I'm not sure whether it is technically legit to use a <fieldset> element outside of a <form> context, since the HTML5 spec for 'fieldset' and HTML language reference both contain very clear wording like this (emphasis mine):

The fieldset element represents a set of form controls grouped under a common name.

Since #type 'container' is definitely used outside of forms, conditionally turning it into a fieldset might not be valid (again, I'm not sure).

Because the only purpose for now is to have a proof for the concept, I think it would be best if we'd hash out the final incarnation of the element #type in a follow-up issue; possibly re-using/re-purposing the existing #2113903: Introduce a 'pane' component to visually group form items


All of that being said, here's the visual proof based on attached patch, which converts the details in the installer into #type fieldgroup:

barraponto’s picture

@sun I guess those forms were already WCAG compliant (and were fine with a bordered fieldset). Instead, what the issue wants to address are other groupable form controls that shouldn't have visible fieldset styles.

The first example at the WCAG technique page seems to be exactly the case. http://www.w3.org/TR/2008/WD-WCAG20-TECHS-20080430/H82.html

mgifford’s picture

@sun the main reason for this patch as far as I'm concerned is so that we can introduce fieldsets in #504962: Provide a compound form element with accessible labels without changing the UI.

This addresses an outstanding issue we've been trying to address since 2009. This is the biggest outstanding accessibility problem in D7's (and D8) forms.

That is what we should be testing this patch against rather than creating some other patch to test. And that's what I've been trying to do since @webchick's post in #25.

EDIT: @barraponto the issue with compound form elements is a great example of that WCAG technique you linked to.

barraponto’s picture

@mgifford if I understood correctly, this issue is a blocker for the compound form elements issue, right?

sun’s picture

To address the remarks that came up: (not sure whether the recent comments were meant to raise concerns/problems on the actual patch?)

  1. Yes. This issue has the unfortunate aspect of a dual-purpose attached to it:

    The cleverly designed CSS class for fieldsets that is being introduced here not only presents a dependency for #504962: Provide a compound form element with accessible labels — it also happens to present a base building block for a new + primary user interface component that is long overdue for Drupal core to offer:

    A fieldset whose legend just simply appears similar to a heading, which is not "collapsible", but yet, which still groups child form elements into a semantically correct container.

    Since the day we replaced our custom notion of collapsible fieldsets with HTML5 standards-compliant Details, Drupal's user interface is littered with Details elements that by design, should (literally) contain "details" only — because it is the one and only user interface component that presents a grouping mechanism in core.

    By introducing the CSS component, we're inherently enabling the badly needed user interface component, because the UI component does not need anything else aside from the CSS class that is being introduced here.

    The concrete example given in #77 only underlines that: It replaces the Details elements of the last page in the Drupal installer with the new UI component for grouping form elements, because none of these are "details"; they present the primary interaction point for the end-user and each set of the fields has to be processed by the user.

  2. Compound form elements of radios and checkboxes are to be grouped (invisibly). But form elements contained in a (visible) group need to be grouped for accessibility, too.

    → The proposed solution not only unblocks #504962: Provide a compound form element with accessible labels , it additionally ensures that WCAG requirements are met for intentionally visible groups of form elements in Drupal's user interface.

    In fact, given the design of this implementation here, it might be possible that #504962 can be resolved in a very different way. I'll explore that once this issue is done.

  3. Everyone of us is actually shooting for the exact same goal: Moving forward. :-)

    As already mentioned in #77, the exact and final name of the new element #type + possibly also its technical implementation should be re-evaluated and can happily be refactored in a follow-up issue. The whole point was and is to address a fundamental concern that has been raised at the beginning of this issue: Supply an actual proof to assert that the new CSS component actually fulfills its job. :)

mgifford’s picture

@barraponto - Yes. As I understand it, this is a blocker.

@sun - Thanks for detailed responses.

1. I now understand better the example in #77 where fieldsets are a more appropriate pattern than details.

2. Agreed.

3. Good point.

I thought I had provided that proof back in #36 where I had combined two patches using SimplyTest.me. I'm happy for it to be illustrated a different way as well.

barraponto’s picture

Status: Needs review » Needs work

The implementation provided by @sun is enough for case 1 (which is basically #504962: Provide a compound form element with accessible labels ). But for case 2, the WCAG 2.0 compliant fieldgroups (which is the subject of this issue), we still need to work on a visual proof.

I think we can add a new #theme_wrapper to checkboxes and radios element definitions to add the fieldgroups. Or maybe wrap it in a proper fieldgroup element during form_process_radios and form_process_checkboxes.

I tried the latter and failed.

mgifford’s picture

FileSize
551.11 KB
149.51 KB

This SimplyTest.me link combines the patch (similar to the previous one).

Attached are screenshots. The duplication of the fieldgroup class is an issue (caused by the two patches). Also the width of the text inputs seems off.

sun’s picture

Status: Needs work » Needs review

I don't quite understand why the provided proof of replacing the details on the "Configure site" installer page is not sufficient?

Btw, quoting myself from #81.2:

In fact, given the design of this implementation here, it might be possible that #504962: Provide a compound form element with accessible labels can be resolved in a very different way. I'll explore that once this issue is done.

I preemptively explored that and it works excellently. :-) — In order to not hi-jack the original issue/thread with my alternative proposal, I supplied a working patch in a new sister/competing issue: #2192419: Use a WCAG-compliant fieldset (fieldgroup) for #type radios/checkboxes

As a result, I still think (1) the latest patch here is RTBC and (2) we can happily have a follow-up issue to re-evaluate/discuss the name and technical implementation of the element #type.

mgifford’s picture

I think there's still a spacing issue in the example.

screen on the left has the patch.

I think I was unsure where you were going with the other code. If we can get the formatting cleaned up I think we can mark this RTBC.

sun’s picture

Thanks for discovering that — I did not notice that gem, since my browser window was wide enough.

I spent quite some time with debugging the CSS, studying the specs and common references, as well as researching potential workarounds. It appears to be a known and unresolved problem with input elements and the only working solution seems to be this:

#2193271: Remove default #size attribute from core

sun’s picture

Title: Introduce a class to hide borders fieldsets elements » Introduce a CSS class to hide borders of fieldset elements
mgifford’s picture

So can we mark this RTBC before we remove the #size attribute?

Or do we just need to set the css width to have a minimum width of 90% of it's container?

sun’s picture

Technically yes, we should be able to move forward, as these are two independent issues — the 'size' attribute problem exists in HEAD already; it's just less apparent.

mgifford’s picture

Status: Needs review » Reviewed & tested by the community

In that case... I've looked over the code, applied it and tested it with a similar use case.

Thanks @sun!

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

longwave’s picture

As noted in #86, this patch broke the layout on the "Configure site" page of the installer, this has already been reported in the following issues:

#2195781: Configure Site page on install is badly formatted
#2197187: Install output error

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

karimiehsan1819’s picture

FileSize
8.73 KB

hi , i want hide or remove label and border of field collection , how can i do them?

Liam Morland’s picture

Please open a new support issue for your question.