Add theme function for form labels in JavaScript

At @lauriii's request, took an initial stab at this patch.

Hello,

I've tried several different ways to pass the checkbox to theme function, but the shown checkbox in administration was always without attached js events. So i've made formLabel theme and pass only label params and left the prepend function to attach the checkbox.

What do you think about this solution?

How are labels associated with inputs? I don't see any handling of the for attribute.in these patches.

Most of our inputs (in twig-generated markup) use <label for> rather than wrapping inputs. Media libary's select-all is an odd case of nesting the checkbox inside the label, but it's OK because it satisfies WCAG "Name, Role, Value".

Patch #7 is better, in a way, because it doesn't force the input to be nested inside the label. Having them as distinct elements is useful for layout (label above vs label inline, say).

On the other hand, we don't want to force developers to use the for attribute. Nesting inputs inside labels is attractive too, because it means you don't have to manage IDs to get accessible names.

I'm not clear what the scope of this issue is. The new JS theme function is intended for general use, I think? Media-library is just an existing use which needs to be updated. As it happens, media-library does a separate jQuery.prepend() to put the checkbox inside the label afterwards.

So I don't know whether this theme function needs to have special handling of the for attribute. Developers can manually add a for attribute to any label easily enough, using jQuery.attr('for'), say. (Similarly, tableselect doesn't bother with a label element; it makes an accessible name with jQuery.attr('title'). You could do the same with aria-label.

Maybe it would be better to explicitly document the fact that developers are responsible for ensuring that inputs have accessible names? The docblock would note that there is no special handling of the for attribute, and a change record could contain several examples.

Having them as distinct elements is useful for layout

I agree that this theme function is more flexible if it just handles the label and doesn't enforce the nested input.
It also allows for this specific use case in media_library to have unchanged markup, without the extra div.

we don't want to force developers to use the for attribute

I modified the theme function to allow for an optional "for".
In this case I'm not adding that attribute since the input is nested.

we miss tooling for generating unique ids in JavaScript

In this particular case it won't matter because of the prepend - the input gets nested by jQuery so the for isn't necessary.
I was searching core for other places labels are created, I found one in Drupal.theme.quickeditImageToolbar and the for is static, it could be that it could just be passed as a string or generated inline.

However, if it made sense to add a uniqueID method, that could be a small utility which gets required separately case-by-case.

Maybe it shouldn't be part of this patch- but the "announcement" logic was backwards, so I swapped that ternary around.

+++ b/core/misc/form-label.es6.js
@@ -0,0 +1,21 @@
+  Drupal.theme.formLabel = ({ label, className, labelFor }) => {

I would separate the label and attributes Drupal.theme.formLabel = (label, attributes) => {} and make the attribute thing generic, if it's a theme function may want to make it support more than just 2 attributes.

@lauriii pointed me to #3070521-11: Trigger deprecation notifications on JavaScript deprecations to notify developers that deprecated code is being used ok with having a single object as argument.

The rest of the patch looks good to me. The ternary swap should be in another patch though.

@nod_ would it work to have Drupal.theme.formLabel = ({ label, attributes }) and then destructure attributes in the function?

The ternary swap should be in another patch though.

I'll remove that from this patch and make a new issue.

I'd be ok with that, it's dangerously starting to ressemble FormAPI though let's be careful not to go overboard in other issues :)

Removed the ternary swap and updated the object passed into the theme function.

let's be careful not to go overboard

It was tempting to make an even more nested object like a render array!
The single object should be a good compromise on flexibility, making it simpler for a developer to override which properties are passed in and how to handle them.

#13

The ternary swap should be in another patch though.

Posted #3085935: Media library "select all" button announcement is reversed

What's left here?

@nod_ perhaps you can RTBC this? :)

Looks good to me, thanks!

Rerolled the patch from #17

Thanks for the re-roll @ shashikant_chauhan

There was just a single line change, patch still applies fine and tests are green. This was RTBC in #21, setting it back.

Hello, @lauriii, changes are added.

This addresses #25.1, The object is now directly inline with the theme function.
This formatting looks correct to me but Prettier shows an error here unless the line breaks are slightly different:

-        const $label = $(Drupal.theme('formLabel', {
-          label: Drupal.t('Select all media'),
-          properties: { className: 'media-library-select-all' },
-        }));
+        const $label = $(
+          Drupal.theme('formLabel', {
+            label: Drupal.t('Select all media'),
+            properties: { className: 'media-library-select-all' },
+          }),
+        );

#25.2 looks good as well, the library is no longer attached in the form alter and instead as a dependency in media_library/view

The only other nits are some extra spaces in:

+++ b/core/modules/media_library/js/media_library.view.js
@@ -21,10 +21,15 @@
+        ¶

and 2 places (form-label.js, media-library.view.js) where the ending newlines are not removed.
For me the extra space and newlines are removed by running yarn build:js.

#27 fixes

Thanks @bnjmnm, now running yarn build:js does not produce any changes and the es6 file matches Prettier formatting suggestions.

This was RTBC in #21, rerolled in #23 and @tinko addressed @lauriii feedback in #25 with #26.

Added a change record, I wasn't totally sure what to put for the "introduced in" since this is on the 8.9.x branch now but will hopefully be backported to 8.8.

Reroll of #28

Need to fix #32 feedbacks

Hello @lauriii, Here's a fix for #32

1. documenting supported properties in the docblock 📄

         +   * @param {string} [options.properties.className]
         +   * The class attribute of the label.
         +   * @param {string} [options.properties.labelFor]
         +   * The for attribute of the label.
       

2. update the documentation for properties object.

         +   * @param {object} [options.properties]
         +   * An object with the following properties:
       

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

This would still be nice. It would also need to be added to tableselect.js so the checkbox there also has a label for accessibility. https://git.drupalcode.org/project/drupal/-/blob/11.x/core/misc/tablesel...

I have attached a patch for 11.x, please review

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

I rerolled it, please review.

New feature will need test coverage. Tagging for that.