Alternative KISS solution for #504962: Provide a compound form element with accessible labels
Requires changes from #2002336: Introduce a CSS class to hide borders of fieldset elements as base building block.
The interdiff contains the (minimal) changes requires on top of #2002336.
I do think it would be good to explain why it is better than #504962-228: Provide a compound form element with accessible labels but it meets the desired results from an accessibility point of view.
Comment | File | Size | Author |
---|---|---|---|
#9 | interdiff.txt | 1.26 KB | sun |
#9 | form.composite.9.patch | 5.67 KB | sun |
#7 | changing-case.png | 226.59 KB | mgifford |
#7 | form.composite.7.patch | 5.23 KB | mgifford |
#6 | form.composite.6.patch | 5.12 KB | sun |
Comments
Comment #2
sunInterdiff is still against parent issue.
Comment #3
mgiffordWell, WAVE likes it:
This applies nicely. Still has that weird formatting issue:
Thanks @sun!
Comment #4
sunPostponing on #2002336: Introduce a CSS class to hide borders of fieldset elements
Comment #5
mgiffordGreat to see the CSS issue in Core.
Comment #6
sunRebased — now contains changes for #type radios and checkboxes only.
Comment #7
mgiffordThis seems to work just fine. Just noticed that the CSS changes such that the Legend is in all caps. Rerolled adding text-transform: none;
Comment #8
sunThat
text-transform
for fieldset legends and details summaries is custom to the Seven theme, and therefore the Seven theme is responsible for not setting it for .form-composite.The default theme styles in system.theme.css are only for setting basic styles.
Comment #9
sunInterdiff is against #6.
Comment #10
mgiffordThis looks great! I'm marking this RTBC. It does what needs to be done from an accessibility/semantic point of view.
Comment #11
falcon03 CreditAttribution: falcon03 commentedBumping priority. This is a critical issue if we want to comply with WCAG 2.0...
Comment #12
mgiffordCritical for WCAG 2.0 AA, but not for the Drupal definition of Critical.
Comment #13
catchCommitted/pushed to 8.x, thanks!
Comment #14
mgiffordOk, time for a little dance of joy! Thanks @catch. This has been a very long time coming!
@sun - really appreciate your efforts on both patches!
Comment #15
tim.plunkettThis means that if you are using template_preprocess_form_element or form-element.html.twig, you can affect all form elements, except checkboxes and radios.
I think this is a regression.
See #1493324: Inline form errors for accessibility and UX
Comment #16
mgiffordOk, so for now I'm bringing this back to Needs work. If this needs to be addressed in a new issue, fine.
Comment #17
sunI just followed up on #1493324-165: Inline form errors for accessibility and UX and presented a plan to move forward.
In short, we should do #2152209: Convert theme_fieldset() to Twig first, then consolidate.
Comment #18
tim.plunkettI think this should be reverted until #2152209: Convert theme_fieldset() to Twig is resolved.
Comment #19
joelpittet@tim.plunkett i've re-rolled theme_fieldset. Hope that help.
Comment #20
tim.plunkettPlease revert. Converting theme_fieldset needs to happen before this issue, so that radios and checkboxes are not turned into second class citizens.
Comment #21
sun#2152209: Convert theme_fieldset() to Twig is RTBC already.
In addition, #1493324: Inline form errors for accessibility and UX is not "blocked" by any of these changes. As with any other commit to HEAD, other change proposals may need to be adapted. That's no news.
The Accessibility team has been waiting for the required WCAG compliance fix here since 2009. This is not a regression.
Comment #22
joelpittetWanted to reference this issue that I was working on here to remove the theme_form_required_marker() and replace it with CSS.
#2152217: Remove theme_form_required_marker() from the theme system - use CSS instead
Comment #23
Wim LeersThis has caused an important regression: #2236789: Styling of inline radios broken: inappropriate trailing colons (breaks EditorImageDialog).
Comment #24
YesCT CreditAttribution: YesCT commentedWould #2275373: Add function for shared logic between fieldset and form element help for the problem mentioned in #15?
Or is what we need a different issue (title?) like "make template_preprocess_form_element or form-element.html.twig, affect all form elements, including checkboxes and radios" by ... I dont know what to write in the "by" part.
Comment #25
mgiffordThis issue and it's related spinoffs seems to be stalled. I'm not sure how to get us @YesCT I'm not sure how to move us beyond this.
Think we need direction from @sun & @tim.plunkett at this point.
Comment #26
joelpittetCan we revert this, maybe there is a way we can get the form_element's tag to swap out or some other less invasive surgical array manipulations, but this seems to have hindered the inline error messages issue by quite a bit.
#1493324: Inline form errors for accessibility and UX
Comment #27
mgiffordIf I could only have one in 8.0.0. I'd choose good inline form errors over fieldsets for radios & checkboxes.
A considerable amount of work has gone into both, but better error messages will benefit everyone.
Comment #28
rooby CreditAttribution: rooby commentedWhat is the technical reason why form errors can't be made to work with this patch?
Comment #29
tim.plunkettSee #15 for the problematic change. Instead of being treated like a form element, it is added to a fieldgroup. Form errors are for form elements. Making checkboxes and radios form elements would mean undoing the fieldgroup changes.
Comment #30
rooby CreditAttribution: rooby commentedI see what you mean by form errors are for form elements.
Is there a nice way to still have the form_element theme wrapper and still end them up wrapped in a fieldset, or is that going to end up too hacky? (sorry for being lazy and not actually investigating for myself, but I figure someone has possibly already investigated it.)
I feel both these issues are important.
Comment #31
tim.plunkettI believe last time we tried that, it ended up with two labels and descriptions, see #1493324-225: Inline form errors for accessibility and UX
Comment #32
mgiffordAny chance that this problem changed now that #1493324: Inline form errors for accessibility and UX is in Core? There were over 300 comments between that post & the commit so hoping perhaps we won't still have two labels and descriptions with this approach to radios & checkboxes. A lot has changed since #9 though.
Comment #33
mgiffordComment #34
mgiffordComment #39
andypostLooks it was commited
Comment #40
YesCT CreditAttribution: YesCT commented@andypost #36and #37 are automated messages, due to the branch/release automated changes and the commit in #13 http://cgit.drupalcode.org/drupal/commit/?id=2c7b99d from 2014-02-20
I'm not sure why it was set to needs review by @mgifford in #34
Maybe (the "new") https://www.drupal.org/governance/core can identify people who can clarify how this should be closed, and what issue is for the current work is (or a new issue for revert. It has probably too late to be a "revert" and would have to be new work (and an issue) to change it).
Comment #41
mgifford@YesCT - Probably Needs Work would be better. The code in #9 is way old.
Comment #46
tim.plunkettThis issue should never have been committed.
But after it was decided to not be reverted, it should have closed.
This issue has caused a massive amount of ripple effect through other issues, but they all adapted eventually. Let's just close this one out for good.