Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Problem/Motivation
If using the <label> tag on a #prefix element, such as the Translation sub-heading provided by the content_translation module, the filterAdmin call will sanitize that string.
Proposed resolution
Instead of using #prefix, create a #markup element and move the HTML there.
Remaining tasks
Usability and accessibility testing.
User interface changes
See the after and before screenshots on #1.
API changes
None
| Comment | File | Size | Author |
|---|---|---|---|
| #8 | content_translation-2475231-8.patch | 864 bytes | jcnventura |
| #7 | add_label_to_filteradmin-2475231-7.patch | 1.3 KB | mgifford |
| #2 | add_label_to_filteradmin-2475231-2.patch | 1.3 KB | jcnventura |
| label_filteradmin_after.png | 96.6 KB | jcnventura | |
| label_filteradmin_before.png | 86.56 KB | jcnventura | |
Comments
Comment #1
jcnventura CreditAttribution: jcnventura commentedBefore:
After:
Comment #2
jcnventura CreditAttribution: jcnventura commentedReconsidering it, it makes absolutely no sense to solve the problem by adding label to filterAdmin. Instead, I've decided to move the #title to another element of type #markup, where filterAdmin is not called.
It solves the problem the same way, and it's a lot simpler to get approved.
Comment #3
andrefy CreditAttribution: andrefy commented@jcnventura it is working fine for me
Comment #4
xjmThanks @jcnventura and @andrefy! And thanks for the screenshots. I agree the fix in #2 seems more on the right track; the initial patch seems too much out of scope for this one small change.
However, is this change going to affect accessibility? Is the
<label>tag correct here at all? If I recall correctly, the way that labels are associated with their elements impacts form accessibility. In normal form elements in Drupal I think we usually do the explicit form (<label for="id">). The label affected by this bug has no association whatsoever for any form element so (as far as I know) it would actually be confusing to some users. And we're moving it even further away from the relevant form element here.I think the fact that the tag is being stripped is the result of the fact that it's not correct markup in the first place. It should probably be the legend of a fieldset instead, I think. It might be worth looking at other tab content to see how other elements are constructed. E.g., we should probably be using the Form API more correctly, or such.
Setting back to "Needs review" for more discussion.
Comment #5
jcnventura CreditAttribution: jcnventura commentedThe problem is that #title works well as "label for" when used with the checkboxES type, but not with the checkbox type. But you might be right, we should instead fix the checkbox to use title as "label for", which would be consistent with the checkboxes type, and would also link title correctly.
Comment #6
xjmAnother question -- why do we need the redundant "Translation" and "Enable translation" at all? It seems to me we could just remove the first non-FAPI label. The checkbox is already labeled, and "show language selector" followed by "enable translation" seems easy to understand for me. And in general fewer words improves usability.
Comment #7
mgiffordThanks @xjm, you're absolutely right that this would introduce accessibility problems. Better way of providing that visual markup illustrated here #2249089: Improper use of a LABEL for Last Saved & Author
It should be:
'<h4 class="label">' . t('Translation') . '</h4>'Comment #8
jcnventura CreditAttribution: jcnventura commentedOr in alternative, like xjm suggested in #6, simply remove the somewhat unnecessary label/header.
Comment #9
mgiffordTrue enough @jcnventura - thanks for rolling up @xjm's observation.
Comment #10
mgiffordMarking @jcnventura's patch in #8 RTBC. Tested the patch here /admin/structure/types/manage/article and it looks good.
Comment #11
alexpottLess is more. Committed 1d8464b and pushed to 8.0.x. Thanks!