Problem

  • Drupal abuses FIELDSET and LEGEND HTML elements to achieve "grouped, collapsible form elements".
  • Fieldsets/legends are very hard to style, since all web browsers are handling them differently.
  • Fieldsets/legends were never intended be used for the task that Drupal is using them for.

Goal

  • Introduce the new HTML5 DETAILS and SUMMARY elements as replacement for current fieldsets/legends.
  • Leverage native user agent handling for details instead of collapse.js when supported.

Details

  • The rendering of fieldsets and legends vastly differs across major browsers. Previous major tasks like #676800: Fieldsets break design badly required an awful lot of time to invent nasty workarounds to achieve a consistent x-browser styling of fieldsets/legends.
  • The use-case of fieldsets in Drupal is to bundle/group multiple form elements into a collapsible container. The contained controls are always advanced controls, which allow the user to customize more detailed settings.
  • To resolve the rendering/styling issues, we originally considered to get rid of the fieldset/legend combo and just simply use DIV containers instead.
  • But luckily, the HTML5 spec supplies a native answer for this use-case: The details element
  • HTML5 Details management summary: HTML5: How to Use <DETAILS> and <SUMMARY> Tags

Proposed solution

  1. Introduce '#type' => 'details'.
  2. Use details everywhere we're (ab)using fieldsets currently. (including vertical tabs)
  3. Retain fieldsets for... regular HTML fieldsets.

Notes

  • Native user agent implementations of Details do not support "uncollapsible" (always-open) details; i.e., all details elements are always collapsible. While this sounds like a regression, it is not. The fact that details are always collapsible and thus always details is very beneficial, because it enforces that the user interface pattern is only used where appropriate.

  • Latest Chrome is the only browser that natively supports Details as of now. To test the native implementation, just apply this patch and browse the site in Chrome. The collapse.js JavaScript behavior is not active and not applied for browsers that support details natively.

Follow-up issues

  1. #1839158: Replace collapse.js with a proper polyfill for <details> — will completely remove core/misc/collapse.js and replace it with a proper HTML5 polyfill.

    The current patch here introduces a minor regression for IE8, but it does not make sense to fix and battle-test collapse.js any further, since we will delete it anyway.

  2. #1856178: '#group' Form API property only works with <details> elements — Determine whether/how to address the '#group' Form API property in light of the more specific semantics and functionality of <details>.
  3. A few configuration settings forms are literally abusing collapsible fieldsets, which contain a single form element only, and even the entire form may consist of a single fieldset only. Those forms ought to be fixed and simplified either way, the switch to details merely forces us to clean up those forms, and to do it right.

    Only a few settings pages are affected, and those need to be fixed in separate follow-up issues, since each of them requires its own, dedicated discussion on how exactly we can simplify the interface.

Additional remarks

  • This patch introduces a new testing module, design_test.module. Coincidentally, I originally authored that module to test a major refactoring and clean-up of fieldsets for Drupal 7. ;) Since then, @Jacine and I advanced on it for various other D7 core patches, and eventually turned it into the contributed Design module.

    The purpose of the design_test module is to allow focused, dedicated testing of theme/markup/design components in an isolated but also "cumulated" environment; i.e., testing all possible incarnations of a certain thing on a single page, without having to hop through the entire system, and also without having to manually switch themes back and forth like hell. The module provides a very simplistic infrastructure for authoring and "executing" (using) the design test cases (which are totally manual tests, of course; but that's just how design works).

    The test cases/pages in the module will have to use unorthodox ways to achieve all possible incarnations of markup at times, and we shouldn't be scared by that. The module and its test cases should be exempt from code quality rules and guidelines. It should be treated as kind of a "sandbox" for core contributors who are working on frontend/theme components, and it must be crystal clear that its primary and only purpose is to facilitate manual cross-browser and cross-core-theme testing of markup/theme changes. At some point in the future, we can and may want to consider to turn this into an actual, user-facing module that has a concrete and helpful purpose, but that's absolutely not the idea and purpose right now, which is why it is added as a hidden test module only.

    Also, only recently I added the precursor for design_test module to facilitate manual testing of the new dropbuttons to the theme_test module. This code is moved into a design test case as part of this patch.


Original summary

The comment on #504962-65: Provide a compound form element with accessible labels as well as bugs like #1015798: Fieldsets inside vertical tabs have no title and can't be collapsed make pretty clear that using fieldsets to form a visual containment, which may be visually overridden collapsible, or even more overridden to be no more collapsible but tab-switchable instead, is quite an abuse and extreme stretch of the original meaning of HTML fieldsets.

Monster issues like #676800: Fieldsets break design badly make clear that it's a pretty bad idea to base any kind of advanced visual styling on fieldsets, since all major browsers are rendering them differently, especially with regard to LEGENDs.

In fact, we're abusing fieldsets to be something they were never designed for: Sections Details

What we actually want is a renderable element #type 'section' that mostly behaves like #type 'item':

  • It has a #title and a #description.
  • It has renderable #children (sub-elements).
  • It is rendered as a container, similar to #type 'container', to group multiple other elements, whereas its structured markup follows that of #type 'fieldset'.
  • It is shrinked to an expandable title if it is .collapsible and .collapsed.
  • It is rendered without/specially positioned title if it is a .tab-pane.

Introducing a proper and unique new element type allows us to finally forget about browser quirks on fieldsets, and extend our presentation layer for new concepts that involve multiple sections that each contain a set of grouped other elements.

CommentFileSizeAuthor
#165 node-add-no-tabs.png86.19 KBtim.plunkett
#164 core-1168246-164.patch484 bytesJelle_S
#157 platform.details.157.patch161.08 KBsun
#157 interdiff.txt6.35 KBsun
#151 platform.details.151.patch158.87 KBsun
#147 platform.details.144.patch160.22 KBsun
#144 platform.details.144.patch160.22 KBsun
#133 platform.details.133.patch162.75 KBdeviantintegral
#118 platform.details.118.patch160.1 KBsun
#118 interdiff.txt2.79 KBsun
#117 core--long-live-details--1168246--117.suggestions.diff1.77 KBry5n
#116 details_elements.txt7.66 KBmbrett5062
#115 vertical-tabs-freedom-fieldsets.png478.87 KBry5n
#111 platform.details.111.patch159.8 KBsun
#105 Views-Details.png335.19 KBmgifford
#105 ViewsDetailsExpanded.png359.64 KBmgifford
#105 DetailsAccountSettings.png278.87 KBmgifford
#105 DetailsAccountSettingsSomeCollapsed.png276.1 KBmgifford
#105 DetailsCron.png221.54 KBmgifford
#99 platform.details.99.patch157.5 KBsun
#99 interdiff.txt8.78 KBsun
#98 pre-advanced.png74.44 KBtim.plunkett
#98 post-advanced.png71.57 KBtim.plunkett
#98 pre-filter.png41.35 KBtim.plunkett
#98 post-filter.png35.39 KBtim.plunkett
#95 details-ie9-stark.png42.99 KBsun
#95 details-ie9-seven.png36.2 KBsun
#95 details-ie9-bartik.png35.57 KBsun
#93 DetailsUI.png99.91 KBmgifford
#93 DetailsCache.png93.58 KBmgifford
#93 DetailsUICollapsed.png98.94 KBmgifford
#93 DetailsTranslate.png100.17 KBmgifford
#93 DetailsTranslateCollapsed.png23.31 KBmgifford
#93 DetailsSiteInfo.png99.45 KBmgifford
#92 platform.details.92.patch149.27 KBsun
#92 interdiff.txt933 bytessun
#91 platform.details.91.patch148.81 KBsun
#91 interdiff.txt986 bytessun
#87 platform.details.87.patch147.38 KBsun
#85 platform.details.85.patch147.33 KBsun
#85 interdiff.txt2.24 KBsun
#83 platform.details.83.patch146.12 KBsun
#83 interdiff.txt6.99 KBsun
#81 DetailsBustedCSS.png71.99 KBmgifford
#81 Details-WAVE-Expanded.png83.17 KBmgifford
#81 Fieldset-WAVE.png77.23 KBmgifford
#81 Chrome-Details.png41.65 KBmgifford
#81 Chrome-Details-Expanded.png54.2 KBmgifford
#78 UserInterfaceTranslation.png69.95 KBmgifford
#70 platform.details.70.patch143.87 KBsun
#70 interdiff.txt81.55 KBsun
#67 details-stark.png49.42 KBsun
#66 platform.details.66.patch68.18 KBsun
#66 interdiff.txt32.88 KBsun
#65 changing-cache-to-details.patch1.07 KBmgifford
#63 DetailsOnCache.png115.31 KBmgifford
#61 platform.details.61.patch43.57 KBsun
#61 interdiff.txt43.54 KBsun
#58 1168246-58-make-fieldsets-history.patch6.45 KBmgifford
#50 1168246-50-make-fieldsets-history.patch6.52 KBmgifford
#47 1168246-47-make-fieldsets-history.patch6.59 KBmgifford
#44 1168246-44-make-fieldsets-history.patch6.04 KBmgifford
#41 1168246-41-make-fieldsets-history.patch1.02 KBManuel Garcia
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

aspilicious’s picture

The "monster issue" was one of the issues that got me into working on core.
I always wondered why we had such an overly complex fieldset system into core.
So it wasn't intended to be so complex ;)

I'm totally into this for D8.

OFFTOPIC
----------
I never heard of the container type so I went digging in the api and there are no docs for container...
http://api.drupal.org/api/drupal/developer--topics--forms_api_reference....

dman’s picture

Not silly, not silly indeed.
Overloading fieldsets sure is a problem. Yes, we could do with a better, more flexible alternative, though the legend was very handy. So, doesn't HTML5 have a replacement element to try?

Bojhan’s picture

Following.

Everett Zufelt’s picture

Tracking for accessibility, but not quite worth tagging yet.

aspilicious’s picture

I think sun means we are going to emulate fieldsets in a cleaner and more cross brower way (with divs I suppose). In html5 they still say you can use fieldset like we do now, and that would be the best solution in theory. But in the real world fieldset rendering is completly different (if you try to do some advanced theming) for all major browsers.

jherencia’s picture

Subscribing...

jide’s picture

Fieldsets introduce numerous issues, that's a good initiative. Subscribing.

pixelmord’s picture

Good initiative- let's create a good UX in forms without torturing fieldsets and developers!

mgifford’s picture

Sounds good to me!

geerlingguy’s picture

Subscribe, and +1.

Liam Morland’s picture

I agree with the direction of this. We need a classes, CSS, and JS to create collapsible containers without using fieldset. We need fieldset to be used for semantically grouping form controls, sometimes in collapsible containers (like it is now) and sometimes invisibly (such as for the components of a date selector).

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, plus some CSS adjustments, addresses the form semantics part of this issue.

bryancasler’s picture

subscribe

ckng’s picture

+1

I think it is worth to note that naming it as section could confuse with HTML5 section tag, also notice a lot of HTML5 themes are using .section class. Something to consider during implementation.

jide’s picture

Agreed with ckng. Even if it's a little early for this kind of discussion, I'd opt for "group", simple and efficient.

sun’s picture

Title: Freedom For Fieldsets! Long Live The Section. » Freedom For Fieldsets! Long Live The Group.
aspilicious’s picture

Question will it act like an extension of "container"? Else we can just expand the container element...

Xano’s picture

Just for illustration, we currently have two ways of grouping renderable elements:
1) We put them in the same parent array. A good example of this are form buttons, which are often put in $form['buttons'].
2) We set the #group property.

sun’s picture

Title: Freedom For Fieldsets! Long Live The Group. » Freedom For Fieldsets! Long Live The DETAILS.
Issue tags: +html5

@Jacine just pointed me to the details (+ summary) element. Even though the spec description is extremely abstract (as usual), this looks like a perfect match.

Along those lines, some concerns regarding accessibility were raised. However, I don't think there's anything to care for, since

  1. we're merely switching HTML tags from FIELDSET to DETAILS, and LEGEND to SUMMARY.
  2. the LEGEND/SUMMARY will still be a link.
  3. collapsed fieldset contents will still be hidden via .element-hidden and/or appropriate CSS styles.
  4. Screen-readers and user-agents should understand this, regardless of how old or new they are.
Everett Zufelt’s picture

@Sun

I don't want to be 'that guy', but...

As it stands, with the fieldset / legend elements, when a screen-reader tabs into a fieldset the legend is spoken, along with the name and role of the element they land on.

<fieldset>
<legend>Select all categories you wish to subscribe to:</legend>
<label><input type="checkbox" />Sports</label>
<label><input type="checkbox" />Movies</label>
<label><input type="checkbox" />Cooking</label>
</fieldset>

When tabbing into this fieldset the user would hear: " Select all categories you wish to subscribe to, Sports, checkbox not checked". Once AT begins to support the details / summary elements this will likely do the same. It is a WCAG requirement to group similarly related form fields in this way, and speaking from personal experience it is annoying, and can be disorienting when they are not.

I am not completely against the ideas of using details / summary where we are just using collapsible fieldsets now, but where it is actually part of grouping fields that are actually meaningfully related, such as on a sub-form or in a compound element, we should keep using fieldset / legend. I understand that this is a pain to theme, but the tradeoff of decreased accessibility needs to be taken into consideration in making the decision.

sun’s picture

@Everett Zufelt: I don't see why any screen-reader would behave differently on a SUMMARY element. Reason being:

  1. In our current FIELDSET/LEGEND combos, the LEGEND is shown and visible. That's why you hear it.
  2. In the proposed DETAILS/SUMMARY combos, the SUMMARY is still shown and visible. That's why you'll hear it.
Everett Zufelt’s picture

@Sun

I need to do some testing, but I am quite certain that you are incorrect.

There is a programmatic relationship, recognized by the screen-reader, between fieldset / legend, and the children of fieldset, this is why when tabbing into a fieldset it is spoken. I am not sure if this relationship is recognized directly by the screen-reader, or if it is made explicit by UAs in their mapping of the DOM to the platform specific accessibility API.

In either case, my presumption (untested) is that either, or both, UAs and screen-readers will need to be updated to get the same results from details / bbbbsummary as from fieldset / legend.

I'll ping David Bolter, accessibility dev at Mozilla on this to see if he can provide any clarity.

fago’s picture

Subscribing.

>What we actually want is a renderable element #type 'section' that mostly behaves like #type 'item': ..
I fully agree!

dman’s picture

Be sure not to add rendering to #item by accident, though.
Assumption so far has been it's just an invisible group. Making that visible and looking like a fieldset could have side effects.
Not that I expect that would be much of a danger, just flagging it to remember what not to do.

droplet’s picture

subscribe

Shadlington’s picture

Subscribe

Jacine’s picture

I'd love to see us stop abusing fieldsets. Subscribing ;)

Everett Zufelt’s picture

FYI

html to platform accessibility APIs (details and summary)

Not conclusive information by any means, but I thought I'd throw it out there.

aspilicious’s picture

Everett Zufelt’s picture

I sent the question about whether screen-readers / UAs should treat details / summary as fieldset / legend to Steve Faulkner, who is doing a lot of the work on this part of the html5 spec.

casey’s picture

I think we should first determine where exactly we are abusing fieldsets. For example vertical-tabs should definitely not use <details>.

Maybe some collapsible fieldsets that could be replaced, but not all.

Why don't we just drop #type container in favor of a #type fieldset and #type detail?

dtarc’s picture

subscribing

Everett Zufelt’s picture

bowersox’s picture

following

jstoller’s picture

In some cases—like the list of tokens available to a form field—<details> and <summary> elements seem like a natural fit. However, there are still many cases where <fieldset> and <legend> elements are perfectly appropriate and should still be used (and yes, they should be visible, even when nested in a vertical tab). Furthermore, in many of the cases where people inappropriately use <fieldset> to group content now, using <details> would be just as inappropriate. In those cases, we really should be using a <section> element with a <header> for the label.

The section element represents a generic section of a document or application. A section, in this context, is a thematic grouping of content, typically with a heading. [read more]

So, in addition to making <details> a viable option, there should also be a simple way to create a collapsable <section>. The vertical tabs content should also be in a <section>, IMHO.

Everett Zufelt’s picture

The details element represents a disclosure widget from which the user can obtain additional information or controls.

http://www.w3.org/TR/html5/interactive-elements.html

The examples given are hiding technical details in a progress report, and hiding controls. I think that anywhere that it is desired to provide a hide / show widget for content or controls that details/summary will do the trick. This is an interactive, and not a purely semantic element, so implementation will drive the clarity of the explanation in the spec.

andrewmacpherson’s picture

Screen Readers and details/summary is an informative assessment of the current state of screen reader behaviour with the summary element. It's quite a long article, so you may want to skip ahead to the conclusion.

Main upshot: beware of links in the summary element, for the time being.

bowersox’s picture

My take on the article is that we can go ahead and use details/summary markup in D8:

"Users of most screen readers and browsers, at least more recent ones, will have little to no problem interacting with them."

+1 for this. The whole idea is to separate out semantic fieldsets from visual or collapsible fieldsets. We need the ability for different sections of forms or pages to be collapsible even if they are not containing a group of fields that should semantically be a fieldset. Similarly, we need the ability to use fieldset/legend markup around groups of semantically related form fields without those fields necessarily becoming styled as a collapsible group. Right now, these two concepts are bound together because our CSS and templates are using the same markup for both.

The article says that we cannot use links inside the summary, since that is not well supported at present. That sounds fine. In Drupal we don't really have links within the "legend" currently. Instead the whole legend is one clickable region that toggles the collapsible area. So I don't think this limitation will be a problem.

If we can achieve "freedom for fieldsets" in D8, it will greatly help with accessibility. #504962: Provide a compound form element with accessible labels explains this in more detail.

I think the next step would be a small proof of concept with some markup and javascript showing how details/summary tags can be used for making accessible, collapsible sections of a page.

Jacine’s picture

Priority: Normal » Major

In an effort to get a better picture of issues remaining in the HTML5 Initiative, I'm going through the existing issues and prioritizing them. I'm Bumping the priority of this one to major as it needs to be resolved soon for Drupal 8 as part of the HTML5 Initiative.

mgifford’s picture

@sun, any chance you could draft up a rough patch. Maybe by revisiting how we deal with sections will help us get over some other barriers with fieldsets.

Manuel Garcia’s picture

Ran a recursive search on all drupal 8 for fieldset: http://www.pasteall.org/33490/bash

Some good starting point changes:

./core/includes/form.inc:2769: function theme_fieldset($variables) {

./core/modules/system/system.admin.inc:2563: function theme_system_modules_fieldset($variables) {

Manuel Garcia’s picture

Status: Active » Needs review
FileSize
1.02 KB

Hoping to get the ball rolling...

Status: Needs review » Needs work

The last submitted patch, 1168246-41-make-fieldsets-history.patch, failed testing.

sun’s picture

Priority: Major » Normal
mgifford’s picture

Status: Needs work » Needs review
FileSize
6.04 KB

Updating the tests.

Status: Needs review » Needs work

The last submitted patch, 1168246-44-make-fieldsets-history.patch, failed testing.

sun’s picture

Issue tags: +API clean-up

I think that one essential change that needs to happen here is to introduce a new #type 'details' and theme_details(). Essentially, copying the all the existing stuff for fieldsets for the moment.

This will allow us to introduce the new element and demonstrate/test it in an example conversion, but at the same time, retain (as in: not break) the entire rest of the system. We're using fieldsets all over the place, so converting everything will be quite some work. I'd therefore suggest this plan of attack:

1) Duplicate #type/#theme 'fieldset' into 'details'.

2) Convert a (simple) example. (Ideally one that doesn't rely on vertical tabs, but just a simple collapsible fieldset somewhere)

3) Get this working, reviewed, approved, and committed.

4) Convert the entire rest of core in follow-up issues.

5) Clean up #type/#theme 'fieldset' to remove all the stuff that's no longer necessary then.

mgifford’s picture

Ok, this still needs work, but it's a start in the direction suggested by @sun.

nod_’s picture

Just use what details provide. We'll have to put a polyfill in there to make it work on other browsers (only chrome understands details afaik). No reason to keep the collapsible/collapse thing since if you're using details you'll want the collapsible thing and collapsed is the "open" attribute for details.

Details don't seem right for handling vtabs too.

sun’s picture

Thanks @mgifford! :) That looks like step 1 is done. ;)

(Note: Minor phpDoc glitch in there, the opening /* should be /**)

Next: Find a simple target for conversion and demonstration. Alas, I just had admin/config/development/testing/settings open in my browser, and that looks almost perfect? ;) The form consists of two fieldsets, one uncollapsible, the other collapsible. :)

mgifford’s picture

Thanks @nod_ & @sun, not sure we're as close as I'd like.

I fixed up the patch a bit more (thanks for catching the cut/paste error with the /**)

I've got an example here of #2 which should be a pretty simple example of details.

Figure the caching page is a good place to start replacing fieldsets. So I tried to make the 2nd box via the new details function and nothing is returned:
admin/config/development/performance

The function form_process_details() returns an array, but not sure what's missing.

mgifford’s picture

Status: Needs work » Needs review

bot nudge.

Everett Zufelt’s picture

I mostly like this approach, it will be intersting to see a element that currently uses fieldset using details. What are we going to do to polyfill?

mgifford’s picture

Think the best approach for the polyfill is - #1252178: Add Modernizr to core

dagmar’s picture

Status: Needs review » Needs work
+++ b/core/includes/form.incundefined
@@ -3667,6 +3700,43 @@ function form_process_fieldset(&$element, &$form_state) {
+print_r($parents);
+print_r($form_state);
+print_r($element);

I think this is used for debug and should be removed... Right?

mgifford’s picture

yes. Woops. Anything else @dagmar? Would love to get some more feedback before re-rolling the patch.

dagmar’s picture

+++ b/core/includes/form.incundefined
@@ -3738,6 +3808,79 @@ function form_pre_render_fieldset($element) {
+/*
+  // Collapsible details
+  if (!empty($element['#collapsible'])) {
+    $element['#attached']['library'][] = array('system', 'drupal.collapse');
+    $element['#attributes']['class'][] = 'collapsible';
+    if (!empty($element['#collapsed'])) {
+      $element['#attributes']['class'][] = 'collapsed';
+    }
+  }
+*/

This code is commented. Should be removed?

mgifford’s picture

Following @sun's description in #46 it should be there. Unfortunately I believe when I did the last patch it was giving me some grief so I commented it out.

Please test it with/without the comments. The new details function should be a duplicate (essentially) of the fieldset function.

mgifford’s picture

Status: Needs work » Needs review
FileSize
6.45 KB

Reposting without the comments.

mgifford’s picture

Issue tags: +Accessibility

Because of this issue we can't really start using fieldsets properly in core. This is an accessibility issue and so tagging it as such. We need to make progress here so that we can deal better with new multi-field input forms like #1802278: Add a Date component to core.

I know there are ways to simplify the front end forms #1638078: [meta] Simplify complex form elements where possible - but there are still going to be modules that want to insert complex forms.

mgifford’s picture

Priority: Normal » Major
Status: Needs review » Needs work

I'm moving this up as major issue. It just affects so much of the UI and we have to transition out of using fieldsets as we have in previous releases. This is the time to change the interface so that we are using DETAILS for visual representations of sections and are finally able to use fieldsets properly for radios, checkboxes, date fields, phone numbers and other relational input fields.

The patch in #58 passes the bots, but really shouldn't as some of the sections in the Performance section are missing.

Certainly information is getting to function form_process_details but it is never being rendered beyond that.

This is a roadblock for an important accessibility issue that we couldn't deal with in D7, but we should be able to address in D8.

sun’s picture

Assigned: Unassigned » sun
Status: Needs work » Needs review
FileSize
43.54 KB
43.57 KB

The approach is still correct, I think, but has the major downside that the copied *_fieldset*() functions are all out of date :-/ — E.g., the patch still contains drupal_attributes() which should be new Attribute() by now, etc.pp. I'm entirely not sure how to address this issue/patch workflow problem. As long as we're working out the new functions, the old ones may see changes, a never-ending circle of doom.

That said, I think I just found a solution that limits potential regressions to theme_fieldset(), but that function is not very large/complex.

@mgifford: If you don't mind, I'm taking this issue over. :)

Development moved into platform-details-1168246-sun branch of the Platform sandbox.

Changes in attached patch:

Fixed new details process/theme functions contain stale code.
Fixed 'details' theme hook is not registered.
Reverted system_performance_settings() conversion, converted simpletest_settings_form() instead.
Converted collapse.js to details.
Converted vertical tabs to details.
Updated core themes for details. (equivalent selector updates only, no styling adjustments)
Converted remaining instances of 'fieldset' and 'legend' in module CSS and JS.
Added todos.

@todo
- Convert tests.
- Adjust styles in core themes for details.
- Replace all #type 'fieldset' with 'details' throughout core.
- Resolve remaining todos.
- Remove fieldset styles from system.*.css
- Remove #type fieldset processing.

The resulting output, look, and feel fills me with endless joy. :)

nod_’s picture

There is some js clean-up integrated in the patch (which is a good thing) just tagging. I agree, it looks great.

mgifford’s picture

FileSize
115.31 KB

@sun - thanks! I was definitely struggling with this one.

I attached a screenshot after converting all the FIELDSETs on the performance page to DETAILS.

That shouldn't be a big deal, but will need to be tested more.

nod_’s picture

Make sure you clear your browser cache, ctrl+f5 isn't enough for the overlay iframe.

mgifford’s picture

@nod_ it looked worse when I changed browsers.

To be clear though @sun's patch looked fine. It's just when I added this additional test in. Which was in my earlier test in #58.

EDIT: @sun's patch above is what this is applied to. The patch here is just for testing.

sun’s picture

FileSize
32.88 KB
68.18 KB

Massive changes today:

  • Updated for _form_set_class() renamed to _form_set_attributes().
  • Added design_test module to Drupal core. @see http://drupal.org/project/design
  • Implemented new default styling for details + summary. ROCKS.
  • Updated Bartik styles for new details + summary. ROCKS.
  • Updated Seven styles for new details + summary. ROCKS.
  • Added design test for theme_fieldset().
  • Fixed default styling for fieldset.
  • Updated and cleaned up Bartik styles for fieldsets.
  • Updated and cleaned up Seven styles for fieldsets.
  • Updated Filter module text format styles for details.
  • Cleaned up Bartik's text format widget styles.
  • Cleaned up Seven's text format widget styles.

I SERIOUSLY cannot repeat often enough how f****** AWESOME the new theming experience is.

The amount of utterly ugly hacks being removed from Bartik and Seven in this patch basically tells the entire story.

@todo

  • Replace all fieldsets with details throughout core.
  • A grep -R 'fieldset' *.css resulted in gazillion of newly introduced custom styles in Views module. :-(

Note: These two final changes will turn this patch into a true monster patch, so I'm posting this patch here as a pre-final milestone that is still reviewable. Expect some insanity in a bit.

sun’s picture

FileSize
49.42 KB

Oh. And witness the awesomeness:

details-stark.png

Status: Needs review » Needs work
Issue tags: -JavaScript, -Accessibility, -html5, -API clean-up, -JavaScript clean-up

The last submitted patch, platform.details.66.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
Issue tags: +JavaScript, +Accessibility, +html5, +API clean-up, +JavaScript clean-up

#66: platform.details.66.patch queued for re-testing.

sun’s picture

FileSize
81.55 KB
143.87 KB

Now, insanity:

Changed all #type 'fieldset' into 'details'.

Does not contain any changes to Views yet, which is the last and final step for this issue. Views shows the following two issues that may need to be flexed:

1) A custom #fieldset property allows views plugins to group themselves into predefined groups. This is essentially the same as the built-in #group property of former fieldsets/now details in combination with #type vertical_tabs. I'm not sure why Views re-invented that wheel, but I almost assume that we can leave this code entirely alone, since it does not actually affect "fieldsets"; merely the #fieldset property name happens to be named like that.

2) Fieldsets/Details all over the place in Views' CSS files. I suspect an extraordinary disaster in Views UI right now. Did not check yet.

Status: Needs review » Needs work

The last submitted patch, platform.details.70.patch, failed testing.

sun’s picture

Status: Needs work » Needs review

OMG. Those test exceptions are irrelevant here, and only caused by:
#1333730: [Meta] PHP DOM (libxml2) misinterprets HTML5

sun’s picture

Quick update: I tested Views UI and it surprisingly works just fine. Only super-minimal glitches. This might mean that we can remove a lot of CSS there.

However, I'm not sure whether I feel comfortable with doing that as part of this patch, since there's an awful lot of special styling and scripting going in Views UI, and I'm not super familiar with that code. I'd prefer if we could handle that in a separate follow-up issue, if possible.

Aside from that, I will try to get the blocker #1333730: [Meta] PHP DOM (libxml2) misinterprets HTML5 out of the way ASAP. Meanwhile, this patch is absolutely ready and could use some manual testing confirmations from others.

sun’s picture

Feedback, anyone?

aspilicious’s picture

Issue tags: +Needs manual testing

Needs manual testing in our core themes. To make sure we don't create regressions. (also for RTL)

sun’s picture

Yes :)

Btw, this patch introduces a new Design Test [design_test] module, originating from Design project, originally developed as markup_test module during D7 code slush. The module's sole purpose is manual testing of design, markup, CSS, and JS changes, and allows to quickly switch between enabled themes. It is supposed to be hidden, of course. But as long as this needs review, I commented out the hidden status, so you can easily enable it on the modules page. The screenshot in #67 shows its output in Stark.

The Details design test exposes all possible combinations and incarnations of details (formerly fieldsets) on a single page.

That said, Filter module implements a heavily customized details element with the text format widget (e.g., on the node add/edit form), which is overridden by all core themes, and which thus needs to be tested separately. (We can look into adding that as a design test case later on.)

nod_’s picture

Status: Needs review » Needs work

The code looks great, I've had a look and on the PHP side of thing it's pretty straightforward, don't have much to say.

I have one problem that could be big, chrome does support details/summary and collapse.js is is activated at the same time (when opened in chrome there are 2 arrows, native and Drupal) when clicking on the summary text, collapse.js triggers and expands but everything is hidden still since the details element is closed. When clicking on the "row" the native details behavior trigger and opens the element. This would solve a pretty messy mobile feature request to have the collapsible element on the whole width of the fieldset, not just the legend. Unfortunately that makes it useless on chrome.

By default there should be the "open" attribute to details element to keep the current behaviors of collapsible fieldsets.

Something that might be a bigger issue is that #collapsible should be removed. Reading the doc and messing around with Chrome implementation, I don't see a way to have a non-collapsible details/summary in native implementations. That might be a bigger issue than assumed since there will always be an arrow or something next to the details summary.

Then there is the question of what is relevant or not to use details for as it was pointed out earlier.

I'd like to get rid of collapse.js for a details/summary polyfill that would be the best way forward and will make sure we don't have wrong assumptions around this shiny HTML5 widget.

mgifford’s picture

FileSize
69.95 KB

It's looking great @sun - was able to navigate through and see a lot of details with the config pages. Great work.

Ran into a bit of a problem with the User interface translation page:
#overlay=admin/config/regional/translate

I'm not sure how this could be tied to the fieldset->details work, but removing the patch made this quirk go away. It could possibly be because I had Arabic as one of the languages, but I tried it with French too and it was the same.

The rest of the pages appeared just fine.

sun’s picture

@mgifford: Thanks for testing! I will fix that in the next re-roll.

@nod_'s findings are a little bit more concerning right now. I wasn't really aware that 1) user agents would be supposed to implement the collapsing behavior on their own, and somewhat worse, 2) that there's even a user agent around that implements this already.

This puts us into a very unfortunate situation, since (as @nod_ already alluded to) we'd have to replace collapse.js with a proper polyfill. That, however, is a relatively big task of its own, and if that wasn't enough, that task logically has an interdependency on the changes being introduced here (without any details+summaries in core, there's nothing to polyfill). I'd be extremely uncomfortable with mixing the details polyfill into this issue/patch.

To be honest, I'm a bit stuck on this problem space. The most sensible and reasonable approach from my perspective would be to temporarily accept the confusing/partially-broken state (in Chrome only), and to create a major/critical follow-up issue for replacing collapse.js with a details polyfill (potentially ahead of time).

The reverse procedure does not really seem to be possible, since the polyfill issue would inherently have to introduce the details.

Does this procedure sound reasonable to you? What do you think?

nod_’s picture

We need to get this cleared up first:

1) Do we want a collapsible behavior on every single thing we're using details for?

if yes: We go ahead with this change BUT is it accessible enough? Right now collapsible.js makes all kind of nice a11y improvements, how much of that do we need for summary? This might be a nasty one to solve.
if no: we can't use details/summary.

It'd be hard to leave the polyfill for after, we need to do some tweaking to to collapsible at least so that this won't make Drupal completely useless on chrome.

In any case it makes sense to add the details type in case contrib would like to use it knowingly.

mgifford’s picture

Having DETAILS as an option in Core is good for accessibility as it allows us to use FIELDSETS properly (at least in some places) for multi-part forms like RADIOS, CHECKBOXES & date/phone number fields. So a good part of this patch is good regardless of how we choose to use it for collapsible fields.

Using this patch as a direct replacement would be best as we could then start using fieldsets & legends properly as per #504962: Provide a compound form element with accessible labels .

Took some screenshots of admin/reports/dblog

It is definitely messed in Chrome. With FireFox it's keyboard accessible, but not in Chrome. Collapse/Expand is barely works in Chrome with a mouse, so yes, definitely that needs to be fixed (possibly with this patch).

I haven't tested this for accessibility with a screen reader, but the patch works fine in FF for keyboard only users. For accessibility it's really a matter of a tradeoff now for a likely improvement later. Still would be a bit of a shock if screen reader users suddenly didn't have access to any of the information within a collapsed detail because they were still using IE8.

I think we should go ahead, but we will need to enlist screen reader users to help evaluate the polyfill.

nod_’s picture

Thanks for the screens, makes things clearer.

tagging for a11y

sun’s picture

Status: Needs work » Needs review
FileSize
6.99 KB
146.12 KB

Alright, here we go:

  1. Fixed Locale string translation filter interface CSS style overrides.
  2. Added browser feature detection for details (source), adjusted collapse.js, and updated CSS styles. (stop-gap fix)

On the collapse.js changes:
Since 'open' is a boolean property, .prop() would actually be more correct than .attr(), but .prop() does not change the HTML attribute, so the CSS was not able to apply styles and react to that, so I had to use .attr(). Anyway, as mentioned before, the entire collapse.js needs to be re-evaluated and very potentially be replaced with an entirely new polyfill for details. I found a potential candidate for that (apparently I now see that it uses .attr(), too), but as mentioned before, this really should be done in a separate follow-up issue.

I've tested this manually in Chrome and Firefox, and it works as expected: Chrome exposes native details, Firefox falls back to collapse.js.

Due to the new collapsible details icon/marker styles that are using :before, the individual theme styles surprisingly did not need any updates to cope with the native details in Chrome. The reason for that seems to be that the user agent's native styles are also using :before to inject the icon/indicator for the open state (although I was not able to verify/confirm that, since Chrome did not reveal the actual user agent style it applies to achieve the icon). The polyfill referenced above uses an even simpler and very interesting approach to inject the open indicator, which is also based on :before. It's possible that we can entirely switch to that, but again, for now, I'd really prefer to make that entire polyfill business + restyling a separate follow-up issue.

And lastly, re @nod_:
Yes, I'm highly confident that we do want to use details in all places where we're using collapsible fieldsets currently. The fact that natively supported details (e.g., in Chrome) always make them collapsible is actually an improvement from my perspective. That is, because the usability pattern that we're trying to apply with collapsible fieldsets is to hide not necessarily needed details in the user interface, so as to not overload the user with too many options, in particular regarding advanced options. I will even say that wherever we're currently using an uncollapsible fieldset, the interface has a completely different problem and we poorly tried to solve it by tacking a fieldset on it. So yes, I think this is an improvement and moves us into the right direction on all fronts.

nod_’s picture

Awesome, just wanted to make sure of the uses.

Patch looks good, works well in chrome (I find the native details behavior way better to be honest :).

Looks like states.js relies on the .collapsed class instead of the attribute, I'm guessing that'd break things. Also we'll need to see what to make of the state:collapsed event in this case. I'm assuming we woudn't need collapsible fieldsets ever so that would make sense to remove all fieldsets-related things from #states.

I really like this patch :) I'd say is there is no major a11y issue, it should get in.

sun’s picture

FileSize
2.24 KB
147.33 KB

Fixed states trigger and remote condition for updated details/collapse.js.

Status: Needs review » Needs work

The last submitted patch, platform.details.85.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
147.38 KB

Merged HEAD.

Would be great to move forward here.

Status: Needs review » Needs work

The last submitted patch, platform.details.87.patch, failed testing.

sun’s picture

Status: Needs work » Needs review

Test failures are still caused by #1333730: [Meta] PHP DOM (libxml2) misinterprets HTML5, not by this patch.

sun’s picture

Meanwhile, the patch in #1333730: [Meta] PHP DOM (libxml2) misinterprets HTML5 should be quasi RTBC and could use some pressure.

sun’s picture

Issue tags: -Needs manual testing
FileSize
986 bytes
148.81 KB
  1. Merged HEAD.
  2. Fixed drupalPostAjax() does not silence DOMDocument::loadHTML() errors like everywhere else.

This effectively unblocks this issue from #1333730: [Meta] PHP DOM (libxml2) misinterprets HTML5, which needs more time to resolve. Apparently, all DOMDocument::loadHTML() calls throughout core are suppressing errors, except of the one in drupalPostAjax(). Thus, correcting that oversight allows us to move forward here independently.

In turn, I think this patch is ready to fly now.

sun’s picture

FileSize
933 bytes
149.27 KB
  1. Merged HEAD.
  2. Updated for new code in HEAD.
mgifford’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
99.45 KB
23.31 KB
100.17 KB
98.94 KB
93.58 KB
99.91 KB

Looks great in Seven & Bartik.

I provided some screenshots here from both highlighting both the expanded & collapsed use of details.

Sadly some of them are blue as I wanted to highlight both the UI & HTML in the screenshot and so some of the screenshots have a blue overlay from Firebug (Linux/Firefox). Still, this isn't a color issue so it should be fine.

This is a big move forward. Thanks @sun!

aspilicious’s picture

Status: Reviewed & tested by the community » Needs review

I would like to have more manual testing on this. Where are the internet explorer screenshots?

sun’s picture

IE9 screenshots attached.

They look and feel and work identical to all earlier screenshots, so I'm not embedding them. :)

sun’s picture

Status: Needs review » Reviewed & tested by the community

Tentatively moving back to RTBC, since screenshots have been provided and that seems to have been the only final desired confirmation.

I also want to amend that it's going to be a lot easier to correct any minor hiccups post-commit (if there are any in the first place).

tim.plunkett’s picture

Status: Reviewed & tested by the community » Needs work

This breaks the Views UI, screenshots to follow.

tim.plunkett’s picture

FileSize
35.39 KB
41.35 KB
71.57 KB
74.44 KB

Here is the Views UI and the configuration of a filter, before and after. You'll notice the fieldsets are completely gone.

Before:
pre-advanced.png
After:
post-advanced.png

Before:
pre-filter.png
After:
post-filter.png

sun’s picture

Status: Needs work » Needs review
FileSize
8.78 KB
157.5 KB

Updated Views UI styles for details.

That said, I wasn't aware of the markup that Views UI produces in some spots, and I could only use explicit language to describe how wrong it is. I've added a @todo to one particular style override in Seven. The markup produced by Views UI needs to be cleaned up in a separate issue. For now, the additional changes in this patch retain its current output and styling.

mgifford’s picture

Worth adding a new issue to "Clean up Views UI markup"?

I can try to test this in Chrome (on Linux) in a bit.

aspilicious’s picture

IE9 is not IE testing. We still support ie8. Shouldn't make a difference but you never know...

sun’s picture

I checked IE8 compatibility mode, and it looked identically.

Furthermore, there is #1787012: [policy, no patch] Write D8 JS against ECMAScript 5. Prevent errors with feature detection (drop IE8 support), which becomes even more likely, now that IE10 has been released.

sun’s picture

#99: platform.details.99.patch queued for re-testing.

tim.plunkett’s picture

Status: Needs review » Needs work

The latest patch does not change the screenshots from #98 :(

Yes, the Views UI abuses fieldsets to accomplish the collapsibility of the "Advanced" section.

mgifford’s picture

Annoying, but it seems to be working well. Here's some screenshots of Views with @sun's latest patch.

Looks like it's working. Not sure what other testing should be done. @tim.plunkett any suggestions on what else to hit?

Interesting the differences between Chrome & FF in how the details are always collapsible in Chrome.

Some concerns with how /admin/config/system/cron is displayed. It's collapsible right now with a title that doesn't display.

Maybe that should be a fieldset?

sun’s picture

Status: Needs work » Needs review

Like #105, Views UI also worked in my testing. I didn't see anything that wouldn't be more broken than it already is (In general, the current Views UI doesn't appear very polished either way for me).

Regarding the System Cron settings, yeah, that's exactly one of the current places in which we are abusing fieldsets/details for absolutely no reason. I'm glad that this nonsense gets revealed by this patch :) However, we won't change any actual markup and user interfaces in this patch. Fixing those things has to happen in separate issues.

mgifford’s picture

Yes, agreed on the cron page. No idea why that needs to be enclosed like that and agreed about it being a separate issue along with the other cleanup of Views UI.

sun’s picture

It turned out that #723392: Tame seven's reset.css was the actual culprit to cause style regressions in Seven on a few admin pages.

Thus, can we move forward here?

sun’s picture

Status: Needs review » Needs work
Issue tags: +JavaScript, +Accessibility, +html5, +Needs accessibility review, +API clean-up, +JavaScript clean-up

The last submitted patch, platform.details.99.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
159.8 KB
  1. Merged HEAD.
  2. Converted newly introduced fieldsets to details.

Friends, this patch is a PITA to re-roll.

nod_’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me and views ui works.

mbrett5062’s picture

Trying to do a review of this and following was immediately obvious.
On Windows 8 OS.
In Chrome 23.0.1271.64
Moving cursor over collapsible or collapsed item, cursor does not change to indicate an action can be taken. It does not indicate that user can click the element to expand it. Perhaps changing cursor to pointer? Actually I see in IE10 and FF 16 that the text is a link, in Chrome not so, therefore no cursor change. Clicking it does work though.

On add content page, in vertical tabs, no details appear in menu/path/comment/author/publishing but they do in revision information. Then when saved, even revision information is gone. Similar on edit content type. That's for all browsers.

Also near beginning of patch. The English went a little weird. Just a nitpick.

function _authorize_filetransfer_connection_settings_set_defaults(&$element, $key, array $defaults) {
- // If we're operating on a form element which isn't a fieldset, and we have
+ // If we're operating on a form element which isn't a details, and we have
// a default setting saved, stash it in #default_value.

Should perhaps read

+ // If we're operating on a form element which isn't details, and we have

That's all for now, further review to follow.

rooby’s picture

Status: Reviewed & tested by the community » Needs work

If you want go go a step further:

+ // If we're operating on a form element which isn't details, and we have

should actually be

+ // If we're operating on a form element that isn't details, and we have

ry5n’s picture

@sun First thing is I want to say how much I appreciate your work on this. It's a big task, thank you for taking the lead.

Now for some less fun stuff: In FF 16, Opera 12.02 and Safari 6, vertical tab contents don't display at all (image). The cause seems to be some css that hides the contents of the <details>:

from vertical-tabs.css:71

.vertical-tabs-pane > summary {
    display: none;
}

from system.base.css:45

.js details:not([open]) .details-wrapper {
    display: none;
}

The js adds 'display: block' to the active <details> element, but the contents are still hidden because the css above still applies to its children.

In Chrome, vertical tabs don't have this problem, but don't display as vertical tabs. Instead, they display as standard <details> widgets. We still want to keep the vertical tabs as a UI component, correct?

Other issues, less important: in Chrome, it is disconcerting to have no hover effect on the summary (also, I miss the animation). In other browsers, the clickable area does not span the full summary width and height as it does in Chrome, and really should.

mbrett5062’s picture

FileSize
7.66 KB

I would also like to thank @sun for all the work you have put into this. I hope these reviews can get this cleaned and committed ASAP.

I don't think we should be adding in a 'display: block' in system.base.css for HTML5 elements, this is taken care of in normalise.css. Though this is nothing to do with this patch. Just noting here. Will search for existing issue to fix this, and if none will raise it.

So back to review of this patch.

I wonder if we should not throughout documentation be describing 'details' as elements.
I.E. details elements
It makes the english more readable in my opinion.

This is a large task, and I do not expect @sun to have to bother with it. I have created and attached a text file detailing changes I think should be made.

If you agree to the changes @sun, just assign the issue to me and I will create the revised patch, and upload.

ry5n’s picture

@sun Although you're assigned, I'd like to help where I can. I'm going to post this diff for your consideration (it applies against the current patch, not HEAD). Here's what it does:

  • Makes styling of details more consistent between native and polyfilled versions (pointer and link-like styling for native elements);
  • Makes polyfilled summary element block-level, so the entire width is clickable;
  • Aligns the inner margins of the summary and .details-wrapper elements.
sun’s picture

Status: Needs work » Needs review
FileSize
2.79 KB
160.1 KB
+++ b/core/modules/system/system.theme.css
@@ -195,7 +195,7 @@ details {
 details > .details-wrapper {
-  padding: 0.5em 1.5em;
+  padding: 0.5em 1.25em;
 }

Reverted this to 1.5em, since the goal/purpose is to vertically align the summary with the details' inner contents. With this decrease to 1.25em, the visual vertical flow was very disturbed.

...

Sorry, I actually reverted all of the changes in #117, since they attempt to make the collapse.js-enhanced summary element behave identically to the details/summary element in natively supporting browsers. As mentioned before, that's out of scope for this issue, and we need to replace our collapse.js with a proper polyfill instead. That's to be resolved in a separate follow-up issue, I've created #1839158: Replace collapse.js with a proper polyfill for <details> for that.

On the same front, re: #113 @mbrett5062:

[In Chrome] Moving cursor over collapsible or collapsed item, cursor does not change to indicate an action can be taken.

That's the browser's native handling of the details element, and I don't think we want to touch or "improve" the fundamental handling in any way. At least I think that's an entirely separate discussion of its own, because it would essentially mean that we'd introduce some sort of "polyfill", but not for older browsers, and instead for modern browsers that actually support a new HTML5 element already. Doing so would introduce a visual/conceptual behavior of certain HTML5 elements on Drupal sites, but not on any other web sites that are using the same elements but which are not applying any additional behavior on top. So again, I don't think we want to do that, and in any case, that needs its own discussion.

On add content page, in vertical tabs, no details appear

Thanks, and sorry, that was an oversight — I hadn't tested the vertical tabs in native Chrome after changing collapse.js to use the 'open' attribute. That was a one-line fix.

I additionally went through Views UI once more to make sure that the details looks correct with both collapse.js-enhanced and native output. Turns out that several CSS overrides in views-ui.admin*.css are obsolete. (In general, i.e., unrelated to this issue, the Views UI CSS badly needs a giant clean-up though; it is doing very unholy things that are strictly forbidden in core.)

  1. Fixed details in vertical tabs are empty/invisible, since not opened.
  2. Removed bogus nested details border override in Seven.
  3. Removed obsolete CSS overrides from Views UI admin stylesheets.
mbrett5062’s picture

@sun I have looked into why this looks good on FF and IE and not on Chrome.

It seems the script from Mathias that you have is out of date.

The new script works better across browsers, and I notice that he no longer tries to add a link because Chrome now supports this element. Instead his script now makes all browsers look and work the same. Then just style in CSS as you wish.
I.E. change cursor to pointer and text color to blue on 'summary' element to emulate a link and give user feedback.

He also includes new aria roles for accessibility.

I think you should look into this, as it is much improved, providing a real polyfill as you noted was required in the comments of collapse.js.

sun’s picture

@mbrett5062:
Again, there is no polyfill added here. And we will not add a polyfill here. That is deferred to #1839158: Replace collapse.js with a proper polyfill for <details>. All this patch adds is a feature detection for details, so as to not execute the collapse.js behavior on user agents that support it natively. What you see in Chrome is Chrome's native handling of details. Neither collapse.js nor a polyfill is involved. Please leave any further comments on a polyfill on details on #1839158: Replace collapse.js with a proper polyfill for <details>, not here.

mgifford’s picture

So given the polyfill issue @sun just addressed, do we just need a quick review (since it's already been marked RTBC) of the latest patch before we mark it RTBC again or do the changes need a larger review?

ry5n’s picture

@sun Agreed that additional styling on top of native details is its own discussion, although one I would like to have.

I’ve tested the latest patch pretty thoroughly. My only remaining questions/nitpicks:

  • There is a bit of weirdness with the collapse behaviour in IE8 (using IE9’s emulation mode):

    • All details start with the 'open' appearance, even ones that are actually in a 'closed' state. Opening one of these triggers the open animation, though it already appeared open. After that it works.
    • The twistie arrow is always in its 'open' configuration.
  • The design test (which is completely awesome, BTW) has an uncollapsible details, which is actually still collapsible when native. This is why in Chrome there are lots of places in core where we have collapsible details now where they're not needed. I assume that we’ll replace these with regular fieldsets or other markup before we ship. As long as that's the case, should be fine.
  • nit: View UI has a minor style glitch in its modal dialogs: .views-ui-dialog details.collapsible:not(.collapsed) { padding-top: 1.5em; } which makes the summary have, well, too much top padding.
  • nit: on admin/reports/dblog the 'filter messages' .details-wrapper needs a clearfix, but that isn't the fault of this patch.

My take is that the IE8 glitches can be dealt with in the polyfill. I suggest a follow-up to convert 'non-collapsible' details to fieldsets or the appropriate semantic element. Everything else is minor or another separate issue (like whether/how to enhance native details behaviour/appearance). If we agree then RTBC.

ry5n’s picture

Status: Needs review » Reviewed & tested by the community

Aw heck, RTBC. If someone thinks one of the above is an issue for right now (or finds something else), please mark back to needs work. Otherwise… :)

mbrett5062’s picture

@sun you have made quite clear the purpose of this patch and the follow up to add a polyfill. Sorry for the noise on that, just a misunderstanding on my part.

I notice you did not comment on my docs cleanup.

As we need this in ASAP. I will leave RTBC and raise a seperate issue to clean up the docs, are you OK with that??

ry5n’s picture

@mbrett5062 Oh, I wanted to second your proposal to use the term 'details element' where 'details' is used in the singular. I'll also leave it up to @sun if he wants to post a new patch with that change.

sun’s picture

Thanks for the final reviews and testing!

  1. #122.1: It's perfectly possible that there might be some glitches in IE, but I don't think that matters for this patch, since it is solely based on collapse.js, and we're going to swap out and replace collapse.js entirely with a proper polyfill for details in #1839158: Replace collapse.js with a proper polyfill for <details> anyway. Thus, any fixes we'd apply to collapse.js now are needless and obsolete, because that entire script will cease to exist. I think we can live with that minor regression (on IE, anyway, hrhrhr) for a few days until there's a new, proper polyfill.

  2. #122.2a: The "formal addition" of design_test module to core as part of this patch is actually noteworthy for core committers:

    This patch introduces a new testing module, design_test.module. Coincidentally, I originally authored that module to test a major refactoring and clean-up of fieldsets for Drupal 7. ;) Since then, @Jacine and I advanced on it for various other D7 core patches, and eventually turned it into the contributed Design module.

    The purpose of the design_test module is to allow focused, dedicated testing of theme/markup/design components in an isolated but also "cumulated" environment; i.e., testing all possible incarnations of a certain thing on a single page, without having to hop through the entire system, and also without having to manually switch themes back and forth like hell. The module provides a very simplistic infrastructure for authoring and "executing" (using) the design test cases (which are totally manual tests, of course; but that's just how design works).

    The test cases/pages in the module will have to use unorthodox ways to achieve all possible incarnations of markup at times, and we shouldn't be scared by that. The module and its test cases should be exempt from code quality rules and guidelines. It should be treated as kind of a "sandbox" for core contributors who are working on frontend/theme components, and it must be crystal clear that its primary and only purpose is to facilitate manual cross-browser and cross-core-theme testing of markup/theme changes. At some point in the future, we can and may want to consider to turn this into an actual, user-facing module that has a concrete and helpful purpose, but that's absolutely not the idea and purpose right now, which is why it is added as a hidden test module only.

    Also, only recently I added the precursor for design_test module to facilitate manual testing of the new dropbuttons to the theme_test module. This code is moved into a design test case as part of this patch.

  3. #122.2b: As mentioned (and repeated) since #83 already, the fact that Chrome's native implementation of details does not support "uncollapsible" (always-open) details is a big ++ from a usability and UX perspective. It reveals that we're abusing fieldsets in horrible ways currently. I am very glad that the native implementation does not support that. It means we have to rethink and clean up the affected user interfaces. We wanted to do that clean-up anyway, so this change merely forces us to do it, and to do it right. Only a few settings pages are affected, and those need to be fixed in separate follow-up issues, since each of them requires its own, dedicated discussion on how exactly we can simplify the interface.

  4. #122.3 + 4: Yeah, I think those should be adjusted in follow-up issue as well. Views UI seems to undergo major changes anyway currently, as I was literally looking at other major UI changes in there during the course of testing this patch. Also, as mentioned earlier already, Views UI's CSS badly needs a major clean-up/refactoring anyway; the code in there does not adhere to any core standards in the first place. (If Drupal core would take themer experience seriously, that would actually have to be filed as a critical task [or actually critical bug, since the current code is effectively "broken" as far as TX is concerned].)

  5. #124: I agree that some of the CSS comment changes are suboptimal, but then again, when I was touching those, I considered to correct them, but quickly realized that the existing CSS comments aren't of any quality in the first place, so instead of duct-taping them, I concluded that doesn't make sense and we should rather review and fix all CSS comments throughout core in a proper way. In any case, this change here is bound to feature freeze. Polishing of CSS comments is super-minor and can happen later.

Thanks again! Yes, I do think we're ready to move forward here.

webchick’s picture

Don't have a chance to review this in-depth right now, but why are we removing the fieldset element? I can see using it less, but this is a standard HTML tag, and I would think people would expect it to be there.

The issue summary could use an update.

webchick’s picture

Issue tags: +Avoid commit conflicts

Oops. Nevermind. I was getting confused by the prerender/process changes.

Tagging as "avoid commit conflicts" cos this touches a lot of stuff.

Everett Zufelt’s picture

Taking a look at the issue summary I cannot determine what the actual change is. Without that I can't determine if any accessibility evaluation needs to be conducted in order for this issue to pass the gate.

Can someone please let me know if / when the last accessibility evaluation took place, and update the issue summary to explain what exactly is RTBC?

yoroy’s picture

Status: Reviewed & tested by the community » Needs work

Awesome progress in here, excited to see this rtbc. Everetts questions and the need to update the issue summary means this needs a bit more work explaining the changes made here.

yoroy’s picture

Issue summary: View changes

Updated issue summary.

sun’s picture

Issue summary: View changes

Updated issue summary.

sun’s picture

Status: Needs work » Reviewed & tested by the community

Summary added, back to RTBC.

Also, I manually tested keyboard navigation for native details (in Chrome), which works excellently.

All other browsers and user agents that do not support details natively, are still using collapse.js, which is only marginally adjusted to work with the new HTML markup but otherwise works identically to now. Thus, if there are any accessibility issues with collapsible details, then those are pre-existing bugs in collapse.js.

yoroy’s picture

Thanks!

deviantintegral’s picture

FileSize
162.75 KB

Here's a reroll against 8.x. One minor change with the refactoring into FileWidget.php.

deviantintegral’s picture

There's no feedback from VoiceOver when expanding or collapsing a group. Also, it's not possible to tell if a fieldset is expanded or collapsed. Fieldsets currently behave exactly the same way. Blocker, or followup issue?

ry5n’s picture

@deviantintegral Sounds like an existing bug, not a regression, so I'd say follow-up. If so, feel free to add that to the issue summary under the "Follow-up Issues" heading.

mgifford’s picture

Status: Reviewed & tested by the community » Needs work

In the review it was discovered that we need a way to let users know that the element :

http://dev.d8-allysprint.gotpantheon.com/search/node/

<a class="fieldset-title" href="#">
<span class="fieldset-legend-prefix element-invisible">Show</span>
Advanced search
</a>

Now it's really hard to find an instance where we are actually using this.

But because DETAILS are all collapsible (in good browsers) we need to add this by default to core/misc/collapse.js:

   $('<span class="details-summary-prefix element-invisible"></span>')
     .append(this.$node.attr('open') ? Drupal.t('Hide') : Drupal.t('Show'))

var $link = $('<a class="details-title" href="#"></a>')

With that it should be good to go.

yoroy’s picture

@mgifford: Incomplete sentence here:

In the review it was discovered that we need a way to let users know that the element :

The link you posted leads to an access denied. I think you have to try again and explain why/were this needs work :)

sun’s picture

Status: Needs work » Needs review

Hm. To me, #136 rather sounds like something that should be filed as an issue upstream; i.e., against WebKit/Chromium — because you essentially claim that all details elements on the net are not fully accessible, not just Drupal's, no?

Right now, collapse.js is not executed at all for modern browsers that support the details element natively (only Chrome as of now, Opera claims to catch up soon). Thus, the suggestion to add an accessibility fix to collapse.js doesn't really work, since the JS behavior isn't executed in the first place.

Furthermore, we're going to replace the entire collapse.js script with a proper HTML5 polyfill in #1839158: Replace collapse.js with a proper polyfill for <details>, which essentially means that we will use a generic, external script that has been developed and adopted by the overall HTML5 web developer community. In turn, the proposed fix would have to be brought into the upstream polyfill library. — But that said, off-hand I do not know whether there is a generic stance of HTML5 polyfill web developers with regard to "partial fixes within polyfills" — to my knowledge, polyfills are usually binary enhancements; i.e., either a user agent supports a feature natively, or it does not. Only in the latter case, the polyfill is triggered and this is where accessibility can be ensured in user-space code (just like collapse.js does now). In other words, the polyfill wouldn't run for browsers that support it natively, and thus there'd be no chance to tweak the accessibility of the browser's native handling of an element. The browser's native handling and accessibility has to be ensured/fixed in the browser itself. At least that's my understanding.

mgifford’s picture

@sun I'm tending to agree with you. If this is a general problem with the browsers then it should be reported:
http://code.google.com/p/chromium/issues/list
https://bugs.webkit.org/query.cgi?format=specific&product=WebKit

And fixed there. Where would the HTML5 polyfills come from?

Let me get a few more opinions on this though. I'd like to see this in.

EDIT - forgot to respond to @yoroy's point. This happened at the end of the sprint.

"In the review it was discovered that we need a way to let users know that the element" was open or closed. In navigating over the DETAIL it wasn't clear if the tab had been closed or was open. Behavior might be different in Safari & Chrome which are the only two browsers that I think support it.

sun’s picture

Exactly. A quick search on Chromium reveals: http://code.google.com/p/chromium/issues/detail?id=108938

...which in turn references the following resources:
https://bugs.webkit.org/show_bug.cgi?id=75478
http://trac.webkit.org/changeset/107548
https://bugs.webkit.org/show_bug.cgi?id=78499

In fact, the actual bug reported in that ticket seems to be resolved already, since I was perfectly able to use the keyboard (ENTER) to open and close the details containers in Chrome (which is not a beta/nightly version, but the regular, released version).

yoroy’s picture

Soooo, needs work or rtbc? :)

mgifford’s picture

I'd like to involve ARIA into DETAILS as per:
http://wet-boew.github.com/wet-boew/demos/details/details-eng.html

<summary aria-expanded="false" aria-pressed="false" role="button" aria-controls="details-1" style="-moz-user-select: none;" tabindex="0">Example 2</summary>
<div style="display: none;">
<summary aria-expanded="true" aria-pressed="true" role="button" aria-controls="details-1" style="-moz-user-select: none;" tabindex="0">Example 2</summary>
<div style="display: block;">

I am hoping that could be introduced after Dec 1st though. There are other things that depend on DETAILS, that we want in place before Dec 1st....

mgifford’s picture

#133: platform.details.133.patch queued for re-testing.

sun’s picture

FileSize
160.22 KB

Oh my... quite some conflicts with latest HEAD.

A simple grep after resolving them shows no further occurrences, but I hope you understand that it's impossible to perform a full manual system/interface test with every single merge of HEAD. :-/ If there are any minor regressions with this patch, we should simply fix them in follow-ups.

sun’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs issue summary update

#144 came back green. (PIFT isn't reporting back to d.o currently)

I'm tentatively moving this back to RTBC, since it is my impression that all concerns and considerations have been addressed (or will be addressed in follow-up issues that have been created ahead of time already), and I assume that @mgifford sent the patch for re-testing in order to mark it as such.

mgifford’s picture

I created a new issue earlier today to address ARIA #1848684: Add ARIA role to DETAILS I was just waiting for the green light.

sun’s picture

FileSize
160.22 KB

Just in case a green testbot confirmation is required... same as #144, should still come back green.

sun’s picture

I just learned that #1838114: Change node form’s vertical tabs into details elements is basically blocked on this change... Would be great to move forward here, so we can try to get at least some parts of the new create content design done for D8.

ry5n’s picture

I support RTBC on this. Follow-ups should address cross-browser consistency, styling and further a11y enhancements, but I don't think there's anything blocking this patch being committed now.

mgifford’s picture

Agreed. Let's get it into core.

#504962: Provide a compound form element with accessible labels is also waiting for this.

sun’s picture

FileSize
158.87 KB

Merged HEAD, which involved a range of conflicts.

mgifford’s picture

We should re-run the bot, right?

sun’s picture

Nope, no need for that. The test queue was merely full. It'll come back in a few minutes.

mgifford’s picture

Brilliant, thanks.

webchick’s picture

So the change overall looks sane. I'm really nervous committing this right now, though, given that we don't appear to have had any manual testing with any of the later patches, and given that the end result of this appears to be an accessibility regression, according to #134. I understand that this is because of an upstream bug in Chrome/Webkit, but given the prevalence of this pattern all over the admin interface, it seems like this might end up going from a major task to a critical, release-blocking bug trying to find a workaround for it if it's not fixed (and widely distributed on user agents worldwide) in time for D8's release. :\

It looks like Mike is ok with proceeding despite that, though. And I do agree that the patch is a complete PITA to re-roll making minor follow-ups for regressions easier to commit as a follow-up thing, but I'd like to see one more confirmation of someone manually hitting some of the trickier major pages (node creation form, views UI, field UI, user settings maybe...) and not seeing any regressions before committing.

sun’s picture

re: #155 @webchick:

Note that - in case accessibility in native user agent implementations turns out to be an issue - then we can always decide to replace the new details/summary markup with custom DIVs and our current collapse.js behavior. The new details/summary markup + rendering is much closer to plain DIVs, and if you scroll back to the beginning of this issue, then you'll notice that we originally wanted to just get rid of the fieldsets and replace them with DIVs using custom classes and custom behavior. Then came HTML5 with a native solution and we switched the direction over to that. I still think it makes perfect sense to try the native HTML5 approach first, and only turn around back to DIVs if it really turns out that native HTML5 implementations won't be sufficiently ready yet for D8. That will be easy to do, if necessary, and would be a pure front-end adjustment. I'd rather give user agent vendors a chance to improve until code freeze first though.

sun’s picture

FileSize
6.35 KB
161.08 KB

Alright, I performed a full stack manual testing once again, including Chrome's native HTML5 implementation and the collapse.js fallback in older browsers.

Everything still works as expected. I only recognized a weird hiccup in Filter module's text format widget, which apparently still used a fieldset for the appended text format selector after the textarea which of course got converted into a details, too — since collapse.js still works identically, that problem was only visible in Chrome, which of course turned that entire text format selector container into... a collapsible details element ;) — the usage of a fieldset there dates back to our D6-era "Input format" fieldset (as visible here on drupal.org) and really should have been converted into a plain container back then already, since no one wants to style that as a collapsible fieldset anymore. :)

So I fixed that glitch, which required a few additional tweaks to .filter-wrapper in filter.admin.css and core themes, since the existing styles assumed the fieldset/details styles as base styles.

And to make sure that this is covered in future design/markup tests, I also added a text format widget to the details design test. Thus, that design test truly delivers the full picture now.

Thus, final changes:

  1. Fixed Filter module text format widget uses a bogus details element for text format selector container.
  2. Added #type text_format to Details design test.
  3. Fixed text fields in vertical tabs are too wide, they have a bogus width instead of max-width.
  4. Improved theme switcher functionality in design_test module.

It's about time to get this in...

webchick’s picture

Status: Reviewed & tested by the community » Needs review

Awesome, thanks for the thorough testing. I'm happy to commit this, but of course you can't RTBC your own patch. ry5n has offered to take a look.

ry5n’s picture

Status: Needs review » Reviewed & tested by the community

I've gone through the admin UI thoroughly in Chrome, Firefox, Opera, IE9 and IE8 and I found no issues here besides ones already earmarked as follow-ups, specifically #1839158: Replace collapse.js with a proper polyfill for <details>, #1848684: Add ARIA role to DETAILS, plus the ones outlined in #122 and accounted for in #126.

In short, RTBC!

webchick’s picture

Title: Freedom For Fieldsets! Long Live The DETAILS. » Change notice: Freedom For Fieldsets! Long Live The DETAILS.
Priority: Major » Critical
Status: Reviewed & tested by the community » Active
Issue tags: -Avoid commit conflicts +Needs change record

Ok, excellent. :D Thanks both for all the thorough testing. With that...

Committed and pushed to 8.x. YEAH!

Let's get a change notice for this sucker.

sun’s picture

Title: Change notice: Freedom For Fieldsets! Long Live The DETAILS. » Freedom For Fieldsets! Long Live The DETAILS.
Priority: Critical » Major
Status: Active » Fixed
Issue tags: -Needs change record
tim.plunkett’s picture

Status: Fixed » Needs work

This means that "uncollapsible fieldsets" are no longer possible

Isn't that incorrect? You can still use '#type' => 'fieldset' without the #collapsible/#collapsed properties.

Also, #collapsible as a property is obsolete correct? It still appears in code.

As a side note, that is a very opinionated change notice. Phrases like "abused" and " typically over-used" aren't really appropriate here.

Can we have some clarification on the first point, a follow-up for the second, and a toning-down for the third?

sun’s picture

Status: Needs work » Fixed

3) Toned down.

2) Follow-up: #1852104: #type details: Remove #collapsible property

1) #collapsible on #type details only works currently on older browsers that do not natively support the details element yet, in which case the pre-existing collapse.js behavior is still executed, and the pre-existing custom JS/CSS still differentiates between the .collapsible class. This is scheduled for removal as part of 2) and the existing polyfill follow-up issue #1839158: Replace collapse.js with a proper polyfill for <details>

Jelle_S’s picture

Status: Fixed » Needs review
FileSize
484 bytes

The new "Design Test" module shows up at admin/modules.

Patch is so small I didn't see the need to create a whole new follow up issue for it. Feel free to do so if you think it's needed.

tim.plunkett’s picture

FileSize
86.19 KB

A quick question while this is reopened:
Did this patch purposefully remove vertical tabs from the node form?

node-add-no-tabs.png

ry5n’s picture

@tim.plunkett Not that I'm aware of. There is a Breakpoint test that must pass for vtabs to activate (runs on load, not resize). Your screenshot looks like it's fairly narrow, I wonder if that's what's happening?

attiks’s picture

Status: Needs review » Reviewed & tested by the community

#164: Patch looks good

webchick’s picture

Status: Reviewed & tested by the community » Fixed

I actually already committed that patch as part of a different issue.

Tim, can you verify your problem isn't what ry5n said and if not, open a follow-up?

tim.plunkett’s picture

I just came back here to follow-up, I think it was actually fixed by #1851098: Breakpoint groups aren't enabled using the standard profile, since vertical tabs depend on breakpoints. Either way, it's working now.

tim.plunkett’s picture

#1853636: Add Views wizard clues are lost is the newest follow-up. It seems that page was never tested.

webchick’s picture

:( I specifically asked for testing of that page, too. :(

sun’s picture

That page was tested earlier, and I've explained the situation with Views markup/CSS/JS a couple of times. The page works, but it is more than obvious that it is one of the pages that (ab)used collapsible fieldsets in a wrong way.

Thanks for creating the follow-up issue. I'll try to post a fix soon. However, I can't promise that to happen before feature freeze.

@all: Please continue to create follow-up issues if you find broken UIs and link to them here. Thanks! :)

sun’s picture

Issue summary: View changes

Updated issue summary.

ry5n’s picture

Created follow-up: #1856178: '#group' Form API property only works with <details> elements to discuss the '#group' Form API property, which may need changes resulting from this patch (also added follow-up to the issue summary).

jhodgdon’s picture

I'm having problems using this element as currently implemented in an older browser -- filed
#1858600: "Filter log messages" details on Recent log messages page applies bogus floats
Just cross-referencing here in case anyone is interested.

Status: Fixed » Closed (fixed)

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

YesCT’s picture

webchick’s picture

I think, but am not positive, this patch might've caused #1885956: Tab index broken on details..

webchick’s picture