Problem
- Drupal abuses
FIELDSET
andLEGEND
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
andSUMMARY
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
- Introduce
'#type' => 'details'
. - Use details everywhere we're (ab)using fieldsets currently. (including vertical tabs)
- 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
-
#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.
- #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>
. -
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 LEGEND
s.
In fact, we're abusing fieldsets to be something they were never designed for: Sections Details
- For many years already, people are trying hard (and often fail) to render non-form material in a collapsible fieldset.
- Since D7, many people would like to render arbitrary stuff in vertical tabs - but you figured it: #857124: Collapsible fieldsets and vertical tabs do not work without form_builder() (Form API)
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.
Comment | File | Size | Author |
---|---|---|---|
#165 | node-add-no-tabs.png | 86.19 KB | tim.plunkett |
#164 | core-1168246-164.patch | 484 bytes | Jelle_S |
#157 | platform.details.157.patch | 161.08 KB | sun |
#157 | interdiff.txt | 6.35 KB | sun |
#151 | platform.details.151.patch | 158.87 KB | sun |
Comments
Comment #1
aspilicious CreditAttribution: aspilicious commentedThe "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....
Comment #2
dman CreditAttribution: dman commentedNot 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?
Comment #3
Bojhan CreditAttribution: Bojhan commentedFollowing.
Comment #4
Everett Zufelt CreditAttribution: Everett Zufelt commentedTracking for accessibility, but not quite worth tagging yet.
Comment #5
aspilicious CreditAttribution: aspilicious commentedI 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.
Comment #6
jherencia CreditAttribution: jherencia commentedSubscribing...
Comment #7
jide CreditAttribution: jide commentedFieldsets introduce numerous issues, that's a good initiative. Subscribing.
Comment #8
pixelmord CreditAttribution: pixelmord commentedGood initiative- let's create a good UX in forms without torturing fieldsets and developers!
Comment #9
mgiffordSounds good to me!
Comment #10
geerlingguy CreditAttribution: geerlingguy commentedSubscribe, and +1.
Comment #11
Liam MorlandI 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.
Comment #12
bryancasler CreditAttribution: bryancasler commentedsubscribe
Comment #13
ckng+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.
Comment #14
jide CreditAttribution: jide commentedAgreed with ckng. Even if it's a little early for this kind of discussion, I'd opt for "group", simple and efficient.
Comment #15
sunComment #16
aspilicious CreditAttribution: aspilicious commentedQuestion will it act like an extension of "container"? Else we can just expand the container element...
Comment #17
XanoJust 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.
Comment #18
sun@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
Comment #19
Everett Zufelt CreditAttribution: Everett Zufelt commented@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.
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.
Comment #20
sun@Everett Zufelt: I don't see why any screen-reader would behave differently on a SUMMARY element. Reason being:
Comment #21
Everett Zufelt CreditAttribution: Everett Zufelt commented@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.
Comment #22
fagoSubscribing.
>What we actually want is a renderable element #type 'section' that mostly behaves like #type 'item': ..
I fully agree!
Comment #23
dman CreditAttribution: dman commentedBe 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.
Comment #24
droplet CreditAttribution: droplet commentedsubscribe
Comment #25
Shadlington CreditAttribution: Shadlington commentedSubscribe
Comment #26
JacineI'd love to see us stop abusing fieldsets. Subscribing ;)
Comment #27
Everett Zufelt CreditAttribution: Everett Zufelt commentedFYI
html to platform accessibility APIs (details and summary)
Not conclusive information by any means, but I thought I'd throw it out there.
Comment #28
aspilicious CreditAttribution: aspilicious commentedMore info about Accessibility for this http://dev.w3.org/html5/html-api-map/overview.html#summary-details
Comment #29
Everett Zufelt CreditAttribution: Everett Zufelt commentedI 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.
Comment #30
casey CreditAttribution: casey commentedI 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?
Comment #31
dtarc CreditAttribution: dtarc commentedsubscribing
Comment #32
Everett Zufelt CreditAttribution: Everett Zufelt commentedMarked #190946: Replace collapsible-fieldsets with Summaries as duplicate.
Comment #33
bowersox CreditAttribution: bowersox commentedfollowing
Comment #34
jstollerIn 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.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.Comment #35
Everett Zufelt CreditAttribution: Everett Zufelt commentedhttp://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.
Comment #36
andrewmacpherson CreditAttribution: andrewmacpherson commentedScreen 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.
Comment #37
bowersox CreditAttribution: bowersox commentedMy take on the article is that we can go ahead and use details/summary markup in D8:
+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.
Comment #38
JacineIn 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.
Comment #39
mgifford@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.
Comment #40
Manuel Garcia CreditAttribution: Manuel Garcia commentedRan 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) {
Comment #41
Manuel Garcia CreditAttribution: Manuel Garcia commentedHoping to get the ball rolling...
Comment #43
sunComment #44
mgiffordUpdating the tests.
Comment #46
sunI 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.
Comment #47
mgiffordOk, this still needs work, but it's a start in the direction suggested by @sun.
Comment #48
nod_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.
Comment #49
sunThanks @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. :)Comment #50
mgiffordThanks @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.
Comment #51
mgiffordbot nudge.
Comment #52
Everett Zufelt CreditAttribution: Everett Zufelt commentedI 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?
Comment #53
mgiffordThink the best approach for the polyfill is - #1252178: Add Modernizr to core
Comment #54
dagmarI think this is used for debug and should be removed... Right?
Comment #55
mgiffordyes. Woops. Anything else @dagmar? Would love to get some more feedback before re-rolling the patch.
Comment #56
dagmarThis code is commented. Should be removed?
Comment #57
mgiffordFollowing @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.
Comment #58
mgiffordReposting without the comments.
Comment #59
mgiffordBecause 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.
Comment #60
mgiffordI'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.
Comment #61
sunThe 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. :)
Comment #62
nod_There is some js clean-up integrated in the patch (which is a good thing) just tagging. I agree, it looks great.
Comment #63
mgifford@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.
Comment #64
nod_Make sure you clear your browser cache, ctrl+f5 isn't enough for the overlay iframe.
Comment #65
mgifford@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.
Comment #66
sunMassive changes today:
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
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.
Comment #67
sunOh. And witness the awesomeness:
Comment #69
tim.plunkett#66: platform.details.66.patch queued for re-testing.
Comment #70
sunNow, 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.
Comment #72
sunOMG. Those test exceptions are irrelevant here, and only caused by:
#1333730: [Meta] PHP DOM (libxml2) misinterprets HTML5
Comment #73
sunQuick 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.
Comment #74
sunFeedback, anyone?
Comment #75
aspilicious CreditAttribution: aspilicious commentedNeeds manual testing in our core themes. To make sure we don't create regressions. (also for RTL)
Comment #76
sunYes :)
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.)
Comment #77
nod_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.
Comment #78
mgiffordIt'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.
Comment #79
sun@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?
Comment #80
nod_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.
Comment #81
mgiffordHaving 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.
Comment #82
nod_Thanks for the screens, makes things clearer.
tagging for a11y
Comment #83
sunAlright, here we go:
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.
Comment #84
nod_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 thestate: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.
Comment #85
sunFixed states trigger and remote condition for updated details/collapse.js.
Comment #87
sunMerged HEAD.
Would be great to move forward here.
Comment #89
sunTest failures are still caused by #1333730: [Meta] PHP DOM (libxml2) misinterprets HTML5, not by this patch.
Comment #90
sunMeanwhile, the patch in #1333730: [Meta] PHP DOM (libxml2) misinterprets HTML5 should be quasi RTBC and could use some pressure.
Comment #91
sunThis 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.
Comment #92
sunComment #93
mgiffordLooks 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!
Comment #94
aspilicious CreditAttribution: aspilicious commentedI would like to have more manual testing on this. Where are the internet explorer screenshots?
Comment #95
sunIE9 screenshots attached.
They look and feel and work identical to all earlier screenshots, so I'm not embedding them. :)
Comment #96
sunTentatively 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).
Comment #97
tim.plunkettThis breaks the Views UI, screenshots to follow.
Comment #98
tim.plunkettHere is the Views UI and the configuration of a filter, before and after. You'll notice the fieldsets are completely gone.
Before:
After:
Before:
After:
Comment #99
sunUpdated 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.
Comment #100
mgiffordWorth adding a new issue to "Clean up Views UI markup"?
I can try to test this in Chrome (on Linux) in a bit.
Comment #101
aspilicious CreditAttribution: aspilicious commentedIE9 is not IE testing. We still support ie8. Shouldn't make a difference but you never know...
Comment #102
sunI 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.
Comment #103
sun#99: platform.details.99.patch queued for re-testing.
Comment #104
tim.plunkettThe latest patch does not change the screenshots from #98 :(
Yes, the Views UI abuses fieldsets to accomplish the collapsibility of the "Advanced" section.
Comment #105
mgiffordAnnoying, 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?
Comment #106
sunLike #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.
Comment #107
mgiffordYes, 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.
Comment #108
sunIt 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?
Comment #109
sun#99: platform.details.99.patch queued for re-testing.
Comment #111
sunFriends, this patch is a PITA to re-roll.
Comment #112
nod_Looks good to me and views ui works.
Comment #113
mbrett5062 CreditAttribution: mbrett5062 commentedTrying 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.
Comment #114
rooby CreditAttribution: rooby commentedIf 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
Comment #115
ry5n CreditAttribution: ry5n commented@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
from system.base.css:45
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.
Comment #116
mbrett5062 CreditAttribution: mbrett5062 commentedI 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.
Comment #117
ry5n CreditAttribution: ry5n commented@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:
Comment #118
sunReverted 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:
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.
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.)
Comment #119
mbrett5062 CreditAttribution: mbrett5062 commented@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.
Comment #120
sun@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.
Comment #121
mgiffordSo 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?
Comment #122
ry5n CreditAttribution: ry5n commented@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):
.views-ui-dialog details.collapsible:not(.collapsed) { padding-top: 1.5em; }
which makes the summary have, well, too much top padding.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.
Comment #123
ry5n CreditAttribution: ry5n commentedAw 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… :)
Comment #124
mbrett5062 CreditAttribution: mbrett5062 commented@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??
Comment #125
ry5n CreditAttribution: ry5n commented@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.
Comment #126
sunThanks for the final reviews and testing!
#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.
#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.
#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.
#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].)
#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.
Comment #127
webchickDon'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.
Comment #128
webchickOops. Nevermind. I was getting confused by the prerender/process changes.
Tagging as "avoid commit conflicts" cos this touches a lot of stuff.
Comment #129
Everett Zufelt CreditAttribution: Everett Zufelt commentedTaking 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?
Comment #130
yoroy CreditAttribution: yoroy commentedAwesome 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.
Comment #130.0
yoroy CreditAttribution: yoroy commentedUpdated issue summary.
Comment #130.1
sunUpdated issue summary.
Comment #131
sunSummary 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.Comment #132
yoroy CreditAttribution: yoroy commentedThanks!
Comment #133
deviantintegral CreditAttribution: deviantintegral commentedHere's a reroll against 8.x. One minor change with the refactoring into FileWidget.php.
Comment #134
deviantintegral CreditAttribution: deviantintegral commentedThere'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?
Comment #135
ry5n CreditAttribution: ry5n commented@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.
Comment #136
mgiffordIn 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/
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:
var $link = $('<a class="details-title" href="#"></a>')
With that it should be good to go.
Comment #137
yoroy CreditAttribution: yoroy commented@mgifford: Incomplete sentence here:
The link you posted leads to an access denied. I think you have to try again and explain why/were this needs work :)
Comment #138
sunHm. 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.
Comment #139
mgifford@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.
Comment #140
sunExactly. 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).
Comment #141
yoroy CreditAttribution: yoroy commentedSoooo, needs work or rtbc? :)
Comment #142
mgiffordI'd like to involve ARIA into DETAILS as per:
http://wet-boew.github.com/wet-boew/demos/details/details-eng.html
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....
Comment #143
mgifford#133: platform.details.133.patch queued for re-testing.
Comment #144
sunOh 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.
Comment #145
sun#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.
Comment #146
mgiffordI created a new issue earlier today to address ARIA #1848684: Add ARIA role to DETAILS I was just waiting for the green light.
Comment #147
sunJust in case a green testbot confirmation is required... same as #144, should still come back green.
Comment #148
sunI 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.
Comment #149
ry5n CreditAttribution: ry5n commentedI 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.
Comment #150
mgiffordAgreed. Let's get it into core.
#504962: Provide a compound form element with accessible labels is also waiting for this.
Comment #151
sunMerged HEAD, which involved a range of conflicts.
Comment #152
mgiffordWe should re-run the bot, right?
Comment #153
sunNope, no need for that. The test queue was merely full. It'll come back in a few minutes.
Comment #154
mgiffordBrilliant, thanks.
Comment #155
webchickSo 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.
Comment #156
sunre: #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.
Comment #157
sunAlright, 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:
It's about time to get this in...
Comment #158
webchickAwesome, 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.
Comment #159
ry5n CreditAttribution: ry5n commentedI'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!
Comment #160
webchickOk, 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.
Comment #161
sunChange notice: http://drupal.org/node/1852020
Comment #162
tim.plunkettIsn'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?
Comment #163
sun3) 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>
Comment #164
Jelle_SThe 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.
Comment #165
tim.plunkettA quick question while this is reopened:
Did this patch purposefully remove vertical tabs from the node form?
Comment #166
ry5n CreditAttribution: ry5n commented@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?
Comment #167
attiks CreditAttribution: attiks commented#164: Patch looks good
Comment #168
webchickI 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?
Comment #169
tim.plunkettI 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.
Comment #170
tim.plunkett#1853636: Add Views wizard clues are lost is the newest follow-up. It seems that page was never tested.
Comment #171
webchick:( I specifically asked for testing of that page, too. :(
Comment #172
sunThat 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! :)
Comment #172.0
sunUpdated issue summary.
Comment #173
ry5n CreditAttribution: ry5n commentedCreated 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).
Comment #174
jhodgdonI'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.
Comment #176
YesCT CreditAttribution: YesCT commentedRelated #1884148: field widget disappears when increase cardinality for image to allow multiple values
Comment #177
webchickI think, but am not positive, this patch might've caused #1885956: Tab index broken on details..
Comment #177.0
webchickLink to #1856178: '#group' Form API property only works with <details> elements in issue summary.