rabol commented 3 years ago

Bug report

the label property of a field does not add the for="field_name"

Not having the for="field_name" breaks the compatibility with screen readers

What I did

add the 'label' property to any field

What I expected to happen

that the <label> have the 'for' property set

What happened

the 'for' property is not set

What I've already tried to fix it

Backpack, Laravel, PHP, DB version

When I run php artisan backpack:version the output is:

### PHP VERSION:
PHP 8.0.0 (cli) (built: Nov 27 2020 12:26:05) ( NTS )
Copyright (c) The PHP Group
Zend Engine v4.0.0-dev, Copyright (c) Zend Technologies
    with Zend OPcache v8.0.0, Copyright (c), by Zend Technologies

### LARAVEL VERSION:
v8.24.0@d16e9f875e4d7609a05d5007393e22ba95efd1fc

### BACKPACK VERSION:
4.1.30@62856b1f01fc3e05c80140fc22f85ea2aac8368c

promatik commented 3 years ago

Hi @rabol! I agree with you, accessibility is a must 🙌 We should review all fields to make sure it uses the best accessibility practices.

tabacitu commented 3 years ago

Agreed - thanks @rabol this was intentionally left out a while back to focus on features, but now that we're feature-packed it's time to fix it. I've marked this as a to-do for 4.2, our next iteration (due soon, 1-2 months).

Thanks for reminding us about it. Cheers!

tibbsa commented 3 years ago

I ran into this today, it was a show-stopper for the site in question (where the back-end is being managed by a group of predominantly blind folks).

The attached diff should get matched labels on most fields types although there are broader accessibility and labeling issues with some that will need more detailed handling, particularly where fields with multiple items (e.g. uploads) are concerned. This has NOT been validated with repeatable fields -- I have never used them and do not know how that is accomplished, but at a minimum the potential for duplicative ID values would have to be addressed.

I'm not filing this as PR because it is incomplete and (except for the field types I'm using) untested, but it may help the OP in the interim.

backpack-crud-field-label-fix.patch.txt

promatik commented 3 years ago

Hi @tibbsa, Thank you for your report and for the patch file! Accessibly is a must, specially in backpack, this will be addressed as soon as we approach v4.2.

I'll make sure to pay special attention to repeatable field, there the label should specify the row index.

tibbsa commented 3 years ago

Not sure how far we are from a v4.2 - the 'next iteration (due soon, 1-2 months)' has become more like 6, so in case anyone needs this, I attach the latest re-roll.

2021-07-06_add_crud_field_label_refs.patch.txt

promatik commented 3 years ago

Hi @tibbsa thank you for the patch you've provided, and sorry for the long wait for v4.2.

I've checked your patch in the hope we could do something now, unfortunately it really is a breaking change for 4.1, due to the many id we would be introducing on everyone code. We'll also need to address the fact that these id will create a conflict on repeatable field — Note for future us.

Can't wait to get to work on this 🙌

tibbsa commented 2 years ago

Patch refreshed up to 4.1.62. (This still does not address repeating fields, etc.)

2022-01-01_add_crud_field_label_refs.patch.txt

tibbsa commented 1 year ago

We're now two years out from this being reported. Any hope of this being addressed in the soon-due v6?

It is blocking our upgrade to v5 on one site where the admins NEED labels to be able to use it at all, because I'd have to re-engineer the fix for both the public and the PRO fields, which adds complexity every time there is an update.

tabacitu commented 1 year ago

Ouch! I wasn't aware we haven't fixed this yet. Yes, definitely time to do this now in v6 @tibbsa . Time to prioritise this - I've labeled it a MUST an put it on our v6 board so that it's done before launch. Thanks for the kind nudge 🙏

Also, something I said in a different issue:

So that when you click the label, the right input gets selected.

Note that:

this needs to be done for ALL field types, so it's not quick task;

certain field types should point to the main input (text, number, textarea etc) others should do something using JS on label click (ex: select2, ckeditor, etc);

So maybe an alternative solution would be a JS one, that would focus the input when the label is clicked? Don't know. Open to debate.

Indeed... we could use our CrudField JS API to target all fields on page, and then for each field... get its label. And we can get the "hidden input" for each complex field type (we can do that with CrudField.activeInput()). But what we cannot reliably do is to get the first visible input. We can try though... and it would probably work for most fields... but it won't work for ALL of them. SOME fields will still need manual tinkering. For example, the code below works for 70-80% of the field types we have:

$('form [bp-field-name]').each(function(index, element) {
    var fieldName = $(element).attr('bp-field-name');

    $(crud.field(fieldName).inputWrapper).find('label').attr('for', 'field-' + fieldName);
    $(crud.field(fieldName).inputWrapper).find('input,textarea,select').filter(':first').attr('id', 'field-' + fieldName);
});

But... I don't think that's a great idea, to be honest. It doesn't feel like something we should do using JS. It feels to me like something each field should do on its own. Especially since all complex fields have an initialization function... those fields can do it there!

So I think we should:

[ ] 🔴 create PR towards CRUD where all labels have a FOR attribute (eg. for="crud-field-{{ $field['name'] }}") and all inputs have the proper ID (eg. id="crud-field-{{ $field['name'] }}");
[ ] 🔴 create PR towards PRO where all labels have a FOR attribute (eg. for="crud-field-{{ $field['name'] }}") and all inputs have the proper ID (eg. id="crud-field-{{ $field['name'] }}") where possible (where that input is visible from start); or where the ID is set in the initialization function, where that is not possible;

Let's do this! Let's FINALLY do this! 💪

tibbsa commented 1 year ago

There remains the issue of handling the 'repeating' fields. Thinking generically, perhaps ID's need to include a unique component (crud-field-fieldName-XXX) - the XXX either being some random string (think UUID) or even just a globally incrementing number.

tabacitu commented 1 year ago

Indeed, repeatable would be more difficult to solve. I think instead of IDs, we could "fake it" there. The repeatable field could watch for clicks on the tabel... and on click put the focus on the very first input 🤷‍♂️

tibbsa commented 1 year ago

Note that the label/for association for my purposes has nothing to do with being able to click on the label and having the focus put in the right place, though. My issue is that as users tab through the fields using a screen reader (JAWS, NVDA), the name of the field is not announced, or not reliably announced. So JavaScript that associates a click on a label to a field will not assist in this respect.

pxpm commented 1 year ago

@tibbsa I never used screen readers, but I assume the requirement here is that the for=something matches the id=something? It could be something random, whatever, but the for and id are the ones that should match right ?

My questions are:

what's the screen reader going to read ? The label text, or the field name associated with the for/id ?
we also have the hint, that we display under the fields. How screen readers interpret/work with it, if it works at all?
is there any way that I can trigger the screen reader to "read this" without adding for/id on the inputs that you are aware of ?

Sorry for all my questions, I just asked you because you seem to be well aware of this problem. If you don't know the answers no worries, I will ask uncle google later.

Trying to gather the most knowledge possible before advancing further on any solution.

Thanks for your time am patience here @tibbsa 🙏

Cheers

tibbsa commented 1 year ago

When tabbing through a form, the screen readers will announce the title of the field based on the matching element. So for example,

<label for="field-fn">First name:</label>
<input type="text" name="firstname" id="field-fn"><br>

The <label> is associated with the <input> on the basis of the for attribute on <label> matching the id attribute on <input>.

There are other ways to do this. For example, rather than explicitly associating a <label> with a <input>, you can use the aria-label attribute on the <input> to accomplish more or less the same thing. Historically <label>/<input> has been better supported but these days, most assistive technology will understand aria-label as well. e.g.

<input type="text" name="search_field" aria-label="Search text">
<button type="submit">Search</button>

Using aria-label has the advantage of not requiring the establishment of a unique ID between the <label> and <input> and may assist in resolving some of the complexity around labeling repeated fields. aria-label is also something that is visible only to screen readers, so can be used even if there is no need for a visual label in the first place (e.g. when using 'placeholder text' to indicate the purpose of a field).

On the other hand, going to the discussion above: accessibility labels attached by way of aria-label or aria-labelledby will not trigger focus events, i.e. if you want to be able to click on a label to get the focus on the control, then it has to be an actual <label> or JavaScript is required.

These labels are especially critical when it comes to check boxes and radio buttons, because there is no consistent UX for these that users can rely on. Sometimes the labels precede the checkboxes, sometimes they come after. If you're navigating through the page and you hear, in this order, "I accept the terms and conditions; checkbox, checked; I want to receive spam every day from you, please", it can be ambiguous as to whether that second checkbox relates to accepting the terms and conditions or accepting the "newsletter".

Field hints are basically read after the field itself. So if you are reading down through a form, you will hear the field label, the field itself, and then the "hint" text. I don't know of a great solution to that. Adding the hint text to the aria-label would make it announce automatically, but could make for some very long and verbose labels, too. In the example on the third page below, you see that you can actually associate multiple labels to a particular form field and at least some assistive technology will combine them and read them together, e.g.:

<label id="expLabel" for="expire">Expiration date:</label>
<span>
    <input type="text" name="expire" id="expire" aria-labelledby="expLabel expDesc">
    <span id="expDesc">MM/YYYY</span>
</span>

References:

tibbsa commented 8 months ago

On the docs repo, I've proposed to at least add an example of using aria-label as an attribute to work around the fact that the fields are, by default, not accessible (or at least not labeled in any meaningful way).

However, that begs the question of whether you could just default to adding an aria-label attribute on fields copying over the provided or computed label? (Perhaps include a $crud->disableAriaLabels() if someone really needs to opt-out, but it seems to me this should be a default 'on' functionality since many / most developers won't necessarily even be aware of the issue.)

Laravel-Backpack / community-forum

[Bug] label property on a field does not add 'for' in the output #206

Bug report

What I did

What I expected to happen

What happened

What I've already tried to fix it

Backpack, Laravel, PHP, DB version