dev/core#3302: Add accessible fieldset for checkboxes and radio buttons of price set and payment options #33491
Conversation
|
🤖 Thank you for contributing to CiviCRM! ❤️ We will need to test and review this PR. 👷 Introduction for new contributors...
Quick links for reviewers...
|
|
The issue associated with the Pull Request can be viewed at https://lab.civicrm.org/dev/core/-/issues/3302 |
| } | ||
| ); | ||
| // Copied from `updatePriceSetHighlight()` below which isn't available here. | ||
| // Copied from `updatePriceSetHighlight()` below which isn't available here.© |
There was a problem hiding this comment.
@monishdeb is the copywrite icon here necessary?
| {assign var="rowCount" value=$rowCount+1} | ||
| <div class="price-set-row {$element.name|escape}-row{$rowCount}"> | ||
| <fieldset class="crm-sr-fieldset"> | ||
| <legend class="label">{$form.$element_name.label|strip_tags}</legend> |
There was a problem hiding this comment.
@monishdeb i just wanted to check if the price field is required does this retain the asterisk sensibly?
There was a problem hiding this comment.
@seamuslee001 good point. This was applied on checkbox option label and I think other then checkbox label asterisk are not appended on Checkbox or Radio field's option. But still I think I will remove this strip_tags usage. Vaguely recall this was causing a accessibility issue but will see later.
| width: auto; | ||
| } | ||
|
|
||
| .crm-container .crm-sr-fieldset { |
There was a problem hiding this comment.
I'm not sure where style like this should be going but I suppose this is as good as any but thoughts @vingle
There was a problem hiding this comment.
I'm just adding RL support. The problem I've got is that each Stream handles the position of the labels differently. Position:absolute on the legend in Walbrook/Shoreditch gives this:
There was a problem hiding this comment.
afaik, the current agreed way to reset fieldsets and legends is:
fieldset {
min-width: 0;
margin: 0;
padding: 0;
border: 0;
}
legend {
padding: 0;
display: table;
}This might help with the RL support
There was a problem hiding this comment.
Those resets sound good - but am curious whey the display: table? Mostly RL is using float:left to reset the behaviour of legend centred on the fieldset border - which is maybe ok for some checkbox/radio button groups - but problematic for UI elsewhere. But that works the same with the user-agent default of display:block as table.
Either way @seamuslee001 - would suggest not adding the position absolute. That would need position: relative on the fieldset, then some margins adding (that prob won't work at all sizes).
|
@monishdeb I think this is a sensible direction, I do want to flag this PR #33466 which whilst not addresing the exact same issue is dealing with a similar siatution (in that case it is about radio / checkbox custom fields) but just not sure how much overlap between the markup is there |
monishdeb
force-pushed
the
issue_3302
branch
from
3de57b7 to
acdb5e4
Compare
|
Thanks @seamuslee001 for your feedback. I have updated the PR to address those points |
…ns of price set and payment options
monishdeb
force-pushed
the
issue_3302
branch
from
acdb5e4 to
5dec7a2
Compare
|
This looks good - just trying to make it work in RL then will PR to this branch. FTR - there's another active PR with another accessible radio/checkbox pattern that doesn't use fieldset/legend but just the divs there: For radios that sets |
|
In case you don't change this to the div pattern - which would mean themes don't have to unset the fieldset/legend browser defaults - I've added a PR to make this compatible with all four current streams in RiverLea: JMAConsulting#141 - which has required adding a new RL variable. |
|
Thank you @vingle for suggesting the div pattern and also to extend the current fieldset approach to support RL. I'll take a look and update the patch accordingly. |
|
@monishdeb that PR has been merged - #33466 - perhaps you can check if this is still needed? |
4 participants
Overview
As per https://www.w3.org/WAI/tutorials/forms/grouping , grouped form controls like radio buttons/checkboxes must be enclosed in
<fieldset>and the<legend>element act as a header to identify the group of elements. Currently, when we review such block with WAVE accessibility tool, it complains about:Proposal
crm-sr-fieldsetwhich hides it in front-end forms (public and backoffice forms)Before
After
Comments
ping @colemanw @vingle @seamuslee001