Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Manage fields >> Tags label doesn't match input ID. I'm not sure where the edit-field-container-cardinality
comes from but it doesn't match.
<label for="edit-field-container">Maximum number of values users can enter</label> <span class="field-prefix"><div class="container-inline"></span> <div class="form-item form-type-select form-item-field-container-cardinality">
<select id="edit-field-container-cardinality" name="field[container][cardinality]" class="form-select"><option value="1">1</option><option value="2">2</option><option value="3"></select>
Comment | File | Size | Author |
---|---|---|---|
#66 | 1932074-followup-66.patch | 857 bytes | andypost |
#62 | cardinality-label-1932074-62.patch | 989 bytes | mgifford |
#54 | cardinality-label-1932074.54.patch | 1.83 KB | mgifford |
#50 | cardinality-label-1932074.50.patch | 1.82 KB | mgifford |
#45 | cardinality-label-1932074.45.patch | 1.82 KB | mgifford |
Comments
Comment #1
larowlanSo this is some weird markup where the two fields have been grouped.
Moving the title to the select instead fixes the a11y issues, but does change the look - screenshots attached - adding tag accordingly.
Shot with normal cardinality
Shot with 'more' cardinality aka other
Wave toolbar pass
Comment #2
mgiffordLooks good to me and worked in SimplyTest.me
Comment #3
larowlanI still think we need UX change sign off here, will try and ping UX folks in irc.
Comment #4
larowlanHere's the before shot
Comment #5
Bojhan CreditAttribution: Bojhan commentedI am definitely not sure about this change, @mgifford please give form changes like this a little bit more review.
The reason the labels are on top is for both a11y and ux reasons, changing that doesnt seem like a good idea - I am sure there are other methods to associate after all all our other input elements are associated? The fact that labels are above the forms actually has significant usability reasoning - do we need to rehash that?
Comment #6
larowlanWill add a clear-left class to the select
Comment #7
larowlanThis seems hacky but I couldn't find a css way to do it because they share the same .container-inline parent.
The other options are re-factoring the theme_form_element_label function to allow passing in classes for the label.
Comment #8
larowlanWrong patch
Comment #9
mgifford@Bojhan I was just focusing on the label/input issue, totally missed that the position had changed. Sorry.
I was just focusing on the output (which is now good):
<label for="edit-field-container-cardinality">Maximum number of values users can enter</label> <span class="field-prefix"><span class="clearfix"> </span></span> <select id="edit-field-container-cardinality" name="field[container][cardinality]" class="form-select"><option value="1" selected="selected">1</option><option value="2">2</option><option value="3">3</option><option value="4">4</option><option value="5">5</option><option value="-1">Unlimited</option><option value="other">More</option></select>
Lee, your other changes look good.
Comment #10
larowlanJust so it is clear - this latest patch adds the following as a field prefix to force the field onto a new line after the label
Comment #11
mgifford#8: field-max-vals-1932074.7.patch queued for re-testing.
Comment #12
mgiffordJust ran this on SimplyTest.me and it looks like the pre shot in #4 to me so marking it RTBC again.
Screen Shot With patch applied:
Screen Shot highlighted by WAVE Toolbar:
Comment #13
xjm#8: field-max-vals-1932074.7.patch queued for re-testing.
Comment #14
larowlanSpoke with Mike about this on skype.
We should be using a fieldset for the label here
So 'Maximum number of fields...' goes in the legend element.
The other two fields get their own labels and we end up with more semantic markup and no empty span element.
Comment #15
swentel CreditAttribution: swentel commented#680546: Add a custom 'number of values' for 'multiple / multi-value' field. (e.g. nodereference, imagefield) is also changing the form structure to 'redesign' the cardinality settings, but we'll see who gets in first.
Comment #16
nick_schuch CreditAttribution: nick_schuch commentedHere is a patch as per the recommendations in #14.
Comment #17
larowlanScreenshot of the above
Assigning to bojhan for a review of this change.
Before shot is at #4
Comment #18
mgiffordThis is great -> admin/structure/types/manage/article/fields/field_tags/field-settings
Now this is great from an accessibility perspective and way nicer code too.
Comment #19
xjmHmm, can we make sure that the accessibility fixes from this are going into #680546: Add a custom 'number of values' for 'multiple / multi-value' field. (e.g. nodereference, imagefield), and fix both issues there? Edit: Since that issue makes some of the needed changes here obsolete.
Comment #20
Bojhan CreditAttribution: Bojhan commentedWhy is this in a fieldset?
Comment #21
larowlanSpoke with Bojhan about this in irc.
Fieldset element is fine but we don't want the styling (border).
Investigating hidden styling in views ui as pointed out by Bojhan.
Comment #22
larowlanThis adds .fieldset-no-border class to field.css to remove the border
Comment #23
Bojhan CreditAttribution: Bojhan commentedLooking good, I hope front-enders wont kill us for this :)
Comment #24
mgiffordThat's slick. Nice semantic output. Did some testing in VoiceOver and it worked well there too.
Comment #25
swentel CreditAttribution: swentel commentedCan we move this fix #680546: Add a custom 'number of values' for 'multiple / multi-value' field. (e.g. nodereference, imagefield) (that one needs a re-roll) into as outlined by xjm in #19 ?
Comment #26
mgifford@swentel I'm moving this back to RTBC.
I don't see this as a race, but this was just marked RTBC & although the patches have some overlap, they aren't the same issue and I don't know if we should bundle them together. Least without a good explanation.
#680546: Add a custom 'number of values' for 'multiple / multi-value' field. (e.g. nodereference, imagefield) looks like a nice improvement.
Would like to hear other's opinions though. I'm not sure how best to proceed.
Comment #27
xjmSince they are both followups fixing issues with the exact same ten lines or so of code, I agree with @swentel, which is why I asked in #19. That change makes this one obsolete, and currently, it will also revert the fixes being applied here. So let's apply the accessibility fix to that patch, which is already a usability fix for the same form element.
Comment #28
swentel CreditAttribution: swentel commentedI was going to merge this one in the other (already re-rolled, and should be good), but left it as is for now and set this to needs work for 2 reasons
Comment #29
mgiffordThanks for the explanation @xjm
swentel, no problem moving the CSS to field_ui.admin.css. Do you want me to roll a new patch with that? Just want to help move this along.
Let's make any decision on upper case a new issue. It could well be really ugly, but that's something that can be changed quickly as it's own issue. Let's not open up the UI if we don't have to.
Comment #30
swentel CreditAttribution: swentel commented@mgifford yes go ahead - fine for me to split it off in separate issue re: uppercase.
Comment #31
mgiffordI don't think it works in field_ui.admin.css. I think the patch in #22 is still better.
Here's what I did though.
Comment #32
swentel CreditAttribution: swentel commentedYou will have to add the css where this form is loaded in PHP.
field.css is for field templates, not for Field UI (which will be come entity ui soon)
Comment #33
mgiffordOk, this is working now.
Comment #34
mgifford#33: field-max-vals-1932074.33.patch queued for re-testing.
Comment #36
mgiffordDid field_ui.admin.inc get completely re-written? 3 out of 3 hunks FAILED to apply. Couldn't even find pieces of them when grepping for pieces of each of those 3 chunks of code.
Comment #37
larowlanYeah would have gone with the FormInterface cleanup, will see if I can get a patch with some resetting on a local branch in git.
Comment #38
larowlanHere's a re-roll to allow for the move of the field settings form to form interface.
Screenshot
Looks identical to existing form but now with 111.2% more accessibility*.
Also added
So the text is lower case.
*Made that figure up
Comment #39
Bojhan CreditAttribution: Bojhan commentedRTBC?
Comment #40
mgiffordLooks great. .fieldset-no-border is going to be required for other elements too.
Comment #41
mgiffordI just want clarification that core/modules/field_ui/field_ui.admin.css is the right place for this as if we want to use this CSS more generally this might be a problem.
Comment #42
mgifford#38: cardinality-label-1932074.38.patch queued for re-testing.
Comment #43
mgiffordComment #45
mgiffordI think this piece can't be applied, so yanked it. Not sure if there is somewhere else for this code, but took it out of the patch and re-rolled it so it could be applied via git:
Comment #46
mgifford#45: cardinality-label-1932074.45.patch queued for re-testing.
Comment #47
mgiffordtagging.
Comment #48
mgifford#45: cardinality-label-1932074.45.patch queued for re-testing.
Comment #50
mgiffordre-roll
Comment #51
mgifford#50: cardinality-label-1932074.50.patch queued for re-testing.
Comment #53
falcon03 CreditAttribution: falcon03 commentedWhen I opened #2002336: Introduce a CSS class to hide borders of fieldset elements I was thinking to this kind of use-case :-)
I think that the styling should be provided by the system module so that it can be used across core without any problem since we will need something like that for other issues as well! :-)
Comment #54
mgiffordRe-roll.
Comment #56
mgifford54: cardinality-label-1932074.54.patch queued for re-testing.
Comment #58
mgiffordreroll
Comment #59
mgiffordPatch still applies. Easy to test with SimplyTest.me by going to /admin/structure/types/manage/article/fields/node.article.field_tags/field
I think we should be using #2002336: Introduce a CSS class to hide borders of fieldset elements rather than adding more css to core/modules/field_ui/field_ui.admin.css
Which would mean adding the class fieldgroup rather than fieldset-no-border
However, this still doesn't deal with the Legend being all caps...
Comment #60
mgiffordFollowing @sun's example in #2192419: Use a WCAG-compliant fieldset (fieldgroup) for #type radios/checkboxes... Took me long enough.
Comment #62
mgiffordHmm.. Let's try that again.
Comment #63
Bojhan CreditAttribution: Bojhan commentedBack at RTBC.
Comment #65
catchCommitted/pushed to 8.x, thanks!
Comment #66
andypostThere's duplicated '#type' definition now
Comment #67
mgiffordWoops.. Correction looks good, applies nicely, and works.
Comment #68
webchickCommitted and pushed to 8.x. Thanks!