Issue summary updated through comment #142.

Problem

To ensure that all form elements are accessible, we need a mechanism to ensure that #title is set up for all the form elements that should use it, so that defining that form element without using #title is not possible.

There was a proposal to create an abstracted "Property Validation API" that can be used to verify that all the required properties of a certain element have been set properly. This was rejected since the feature was only useful when developers are building the form, and is needless code execution otherwise.

Proposed resolution

In #128, tstoeckler suggested a core test that would check every module's forms and ensure that relevant form elements have a #title attribute. This would prevent any new core from being added without necessary #title attributes, since those forms would not pass this test.

Remaining tasks

  1. mikey_p suggested on IRC, Current AccessibilityTest checking may probably fail for quite a few forms, notably any entity form. Need to do something to get all the entity forms via the entityform system / EntityFormBuilder. Here's some code that attempts to work with any entity form: http://cgit.drupalcode.org/formblock/tree/src/Plugin/Block/EntityFormBlo...
  2. Reroll #134 removing sections that have been solved by #1987726: Convert language content page related callback to new style controller
  3. or possibly start over.
Contributor tasks needed
Task Novice task? Contributor instructions Complete?
Update the issue summary Instructions
Update the issue summary noting if allowed during the beta Instructions

Original report

In #882694: Add missing form element titles for accessibility @Sun suggested that a mechanism be put in place to ensure that all form fields have a #title.

the proper fix for D8 is to additionally require a non-empty #title for all form elements via form_builder(), throwing an exception or error message if one without is encountered, so as to ensure that every form is accessible.

EDIT: That is, because Form API is not able to throw a form validation error for a form element without #title. You simply see the same form, without any error message.(http://drupal.org/node/882694#comment-3513452)

CommentFileSizeAuthor
#211 interdiff-204-211.txt7.39 KBsmustgrave
#211 933004-211.patch27.32 KBsmustgrave
#204 drupal-n933004-204-88x.patch27.4 KBDamienMcKenna
#202 drupal-n933004-202.interdiff.txt3.42 KBDamienMcKenna
#202 drupal-n933004-202.patch27.4 KBDamienMcKenna
#196 933004-form_builder-196-interdiff.txt3.51 KBtim.plunkett
#196 933004-form_builder-196.patch27.32 KBtim.plunkett
#194 933004-form_builder-194.patch26.55 KBtim.plunkett
#191 933004-form_a11y_test-squashed-191-interdiff.txt28.9 KBtim.plunkett
#191 933004-form_a11y_test-squashed-191.patch57.27 KBtim.plunkett
#189 933004-form_a11y_test-189-interdiff.txt5.84 KBtim.plunkett
#189 933004-form_a11y_test-189.patch35.82 KBtim.plunkett
#186 933004-form_a11y_test-186.patch41.66 KBtim.plunkett
#181 933004-form_a11y_test-181.patch43.08 KBtim.plunkett
#181 933004-form_a11y_test-181-interdiff.txt7.96 KBtim.plunkett
#178 933004-form_a11y_test-178.patch43.08 KBtim.plunkett
#178 933004-form_a11y_test-178-interdiff.txt7.96 KBtim.plunkett
#178 933004-form_a11y_test-177.patch43.08 KBtim.plunkett
#178 933004-form_a11y_test-177-interdiff.txt7.96 KBtim.plunkett
#175 933004-form_a11y_test-175-interdiff.txt11.91 KBtim.plunkett
#175 933004-form_a11y_test-175.patch42.44 KBtim.plunkett
#174 933004-form_a11y_test-174.patch38.59 KBtim.plunkett
#173 933004-form_a11y_test-173.patch37.13 KBtim.plunkett
#160 test_that_all_form-933004-160.patch5.62 KBsiva_epari
#160 interdiff.txt1.07 KBsiva_epari
#153 test_that_all_form-933004-153.patch5.58 KBsiva_epari
#152 933004-152.patch3.17 KBrpayanm
#152 933004-interdiff.txt748 bytesrpayanm
#150 933004-149.patch3.17 KBrpayanm
#144 933004-144.patch8.99 KBLowell
#134 933004-134.patch11.39 KBtstoeckler
#127 form-933004-127-do-not-test.patch17.42 KBtim.plunkett
#117 form_property_validation-117.patch68.37 KBfalcon03
#116 form_property_validation-933004-116.patch68.21 KBtim.plunkett
#116 interdiff.txt726 bytestim.plunkett
#114 form_property_validation-114.patch67.84 KBfalcon03
#112 admin_people.zip567.56 KBfalcon03
#109 form-933004-109.patch67.53 KBtim.plunkett
#104 form-933004-104.patch68.27 KBtim.plunkett
#99 form-933004-99.patch75.31 KBtim.plunkett
#99 interdiff.txt3.52 KBtim.plunkett
#93 form-933004-93.patch75.18 KBtim.plunkett
#93 interdiff.txt1.87 KBtim.plunkett
#91 form-933004-91.patch74.02 KBtim.plunkett
#91 interdiff.txt10.86 KBtim.plunkett
#89 form-933004-89.patch64.46 KBtim.plunkett
#89 interdiff.txt16.36 KBtim.plunkett
#87 form-933004-87.patch52.3 KBtim.plunkett
#87 interdiff.txt17.75 KBtim.plunkett
#85 form-933004-85.patch36.61 KBtim.plunkett
#85 interdiff.txt14.59 KBtim.plunkett
#83 form-933004-83.patch20.53 KBtim.plunkett
#83 interdiff.txt11.5 KBtim.plunkett
#78 property-validate-933004-78.patch19.23 KBtim.plunkett
#78 interdiff.txt10.81 KBtim.plunkett
#76 property-validate-933004-76.patch18.28 KBtim.plunkett
#76 interdiff.txt664 bytestim.plunkett
#74 form_property_validation-74.patch17.77 KBfalcon03
#64 form_property_validate-64.patch13.26 KBfalcon03
#62 form_property_validate-62.patch12.52 KBfalcon03
#59 form_property_validation-58.patch10.38 KBfalcon03
#51 form_property_validate_abstracted.patch10.02 KBfalcon03
#29 drupal_933004_29.patch3.61 KBXano
#27 drupal_933004_27.patch3.07 KBXano
#25 drupal_933004_25.patch1.07 KBXano
#22 Require-title-933004-22.patch936 bytesmgifford
#21 Require-title-933004-21.patch908 bytesmgifford
#12 Require-title-933004-12.patch908 bytesmgifford
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Everett Zufelt’s picture

Issue tags: +Accessibility

Tagging

bowersox’s picture

+1

mgifford’s picture

+1

Everett Zufelt’s picture

Would the best approach be to do this in form_builder() and to set a drupal error message, or to do this as a simpletest?

Form_builder will catch all form elements, be they core or contrib. Simple test is less obtrusive, as it will not throw errors in the face of developers of contrib modules with forms.

So, the question is, do we want a test for core, or a error message to educate developers and force them to add #title to all form elements, be they in core or contrib?

mgifford’s picture

It would be good to have an error message or watchdog call set into form_builder, even if it gets taken out at release.

Everything in core's going to need to be run through SimpleTest before it gets in too, but I'm worried that there may just be too many to tackle with all of the initiatives.

Education is always good! But simpletest is also going to do that for core. I suspect that might be sufficient for serious modules in contrib too. Although I'm not certain how simpletest will do this.

Bringing it into Coder is also a good way for those who want to see the best practices get implemented.

Steven Jones’s picture

Currently the Form API doesn't validate the arrays that it's passed, and so adding something that did would most likely be a big performance hit when, ideally forms should be valid because the module author will have added the correct code when writing the module.

So, I think that this issue is actually one of documentation and education, enforced by a coder module rule.

We should document that form element titles are required and why, and then have a coder module rule so that module authors can be notified when they don't comply.

nevets’s picture

Requiring #title seems a bit drastic. For example a form element that uses markup to add addition explanatory text should not need a title

Everett Zufelt’s picture

@nevets

To ensure that all form fields have a programmatically determinable name (WCAG 2.0 success criteria 4.1.2) we need each form element to have a title. This title can be visually hidden with the system class .element-invisible. See the #title_display property in fapi for more information about visually hiding form element titles.

Everett Zufelt’s picture

@Steven Jones

Actually FAPI validates things all over the place. As a quick example form_builder() validates that each $element has a #id property set, and if it does not it sets the #id. So, although there might be other reasons for not requiring #title, performance isn't one of them.

mgifford’s picture

Issue tags: +FAPI

Just adding a FAPI tag.

bowersox’s picture

Proposed Approach

Add validation into form_builder() and throw up a drupal_set_message error on any form that has a missing #title. We discussed watchdog errors, coder module, and simpletests, but none of those are likely to get noticed by most developers making forms with FAPI. So the thought here is that if we really want to make this useful, we should put up an on-screen error.

Of all the available FAPI field types, all of them need a #title except the following: tableselect, vertical_tabs, actions, button, container, image_button, submit, form, hidden, token, markup, item, value.

This approach should be easy to program, because it is just adding an IF statement inside form_builder() to look for #title and throw an error.

This was the consensus as discussed by @Everett Zufelt, @mgifford, and @bowersox at the Drupal Accessibility Sprint.

mgifford’s picture

Status: Active » Needs review
FileSize
908 bytes

This is still pretty rough, but wanted to put the rough idea up as a patch.

franz’s picture

Status: Needs work » Needs review

#12: Require-title-933004-12.patch queued for re-testing.

Xano’s picture

Status: Needs work » Needs review

This approach is too hardcoded:
1) Form API should not be written to handle specific elements, but specific, abstract features. Also, does the title requirement really apply to form elements only, or to a subset of renderable elements (which do not necessarily have to be processed by FAPI).
2) Forms can contain *any* renderable element and not just form elements. As it happens, renderable elements do not necessarily have to have a human-readable title.

If we really want to make the renderable element API check this, a better approach would be to use a process callback that is applied for all elements that require a title. This would keep FAPI clean of element-specific behavior and allow the 'designers' of a specific renderable element to configure its behavior.

Everett Zufelt’s picture

Status: Needs review » Needs work

@Xano

If I understand correctly, you are suggesting that each render element that "requires" a #title have a process callback added by default. The process callback receives the element and tests to see if #title is set. Future elements, or contrib elements, could also set a process callback to add the same function. Developers could optionally remove the callback from the element if they wish to avoid the check for #title.

If this is correct, then I agree with the suggested implementation. It is easy to understand, extend, and override.

Xano’s picture

@Everett Zufelt: you understand correctly.

Also, just to be clear: we shouldn't check if #title is set, but if it actually has a value, possibly using drupal_strlen().

bowersox’s picture

@Xano, this approach sounds good to me too.

mgifford’s picture

@Xano Can you write up a new patch? I think we also need tests.

mgifford’s picture

FileSize
908 bytes

Just re-rolling the patch from #12. Would be good to have the additions suggested by @Xano.

mgifford’s picture

Status: Needs work » Needs review
FileSize
936 bytes

Noticed a bunch of notices that needed to be fixed.

tim.plunkett’s picture

Issue tags: +Needs tests

This should use trigger_error(), not drupal_set_message(). I believe drupal_render() has an example.

Also it needs tests.

Xano’s picture

Status: Needs work » Needs review
FileSize
1.07 KB
  1. The patch uses an exception instead of a message or an error, as we want to require #title properties to be set.
  2. The code still applies to a fixed list of form elements. Instead, elements' info arrays need to get a flag, such as #title_required, or perhaps even a #required_properties array, so we can let developers choose which properties, if any, should be required for their elements.

IMHO, ideally, we'd do something like this by default:

$element_definition['#required_properties'] += array(
  '#title' => TRUE,
);

This way, all elements have a required title by default, but this can be turned off and developers can add their own custom properties.
My question is: where should we merge in such default values? element_info()?

Attached a patch that just throws an error if a title is missing for most elements to see what will break in core.

Xano’s picture

Status: Needs work » Needs review
FileSize
3.07 KB
  1. The patch adds the #property_validate property to all form elements, by replacing calls to element_info() with calls to the new form_element_info_merge(), which merges default values into form elements.
  2. The #property_validate property is an array of which keys are element properties, and values are callbacks that return a boolean to see if the property value is valid.
  3. Form element titles get strlen() as their validation callback.

Some examples of how this approach can be re-used:

array(
  '#property_validate' => array(
    '#title' => 'strlen',
    '#options' => 'is_array',
    '#currency_code' => 'currency_load',
    '#node_id' => 'node_load',
  ),
);

The alternative was to use empty() as a hardcoded callback, and make the #property_validate array values booleans. This is maybe twenty more characters, but adds an enormous amount of flexibility, without making the defaults any harder to work with for developers.

Xano’s picture

Status: Needs work » Needs review
FileSize
3.61 KB

This should work. Instead of just merging arrays, I used \Drupal\Utility\NestedArray, which allows for simpler code.

Xano’s picture

Status: Needs work » Needs review

There may be some more occurrences of form functions calling element_info() rather than form_element_info(), though.

// Edit: so apparently the patch from #29 works. The installation fails, because some form elements don't have their required properties set.

// Edit 2: now that I think of it, is there a central element processing function we can add the validation to? I can imagine this is useful for non-form elements too.

mgifford’s picture

#29: drupal_933004_29.patch queued for re-testing.

mgifford’s picture

Yes, it applies fine locally. There was one whitespace issue

drupal_933004_29.patch:23: trailing whitespace.
      ))); 
warning: 1 line adds whitespace errors.

Thanks for all your work on this Xano. Hopefully the bot just had a bad cycle.

Xano’s picture

The fail is expected, because the code works, but the element definitions have not been updated. This means that any form element that is not a
will cause an exception if its title is not set.

mgifford’s picture

Ultimately I'm trying to bring this into Core. The test above says "Detect a Drupal installation failure." in the details. That's not usually the error message I see when it's simply about updating the tests.

Although, I suppose if it's for the installation scripts, that would make sense... Ok, slowly cluing in here.

That being said, how do we take this to the next step.

Xano’s picture

The test fails are entirely expected.

We don't need to update tests. We need to update the element definitions and usage in core. If we agree that this is the right approach to solve our problem, then we can start updating those elements. When we've done that, the testbot will be able to install Drupal again.

falcon03’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests, -FAPI, -Accessibility

#29: drupal_933004_29.patch queued for re-testing.

falcon03’s picture

Priority: Normal » Major
Status: Needs work » Needs review

This is at least major IMO. FAPI should "enforce" usage of #title.

I could work on this issue, but only after July 1st. And maybe it will be too late to get this in at that time...

larowlan’s picture

Will try and work on this next week.

falcon03’s picture

Ok, so I spent some time working on this patch today. But, unfortunately, I think that the code is not working properly, or maybe there's something that I'm not understanding.

After applying the patch, if you try to install drupal, you get a message saying that install_language_select_form has a non-valid #title property. But here's the code of the language select component in install_language_select_form() in install.core.inc:

  $form['langcode'] = array(
    '#type' => 'select',
    '#title' => t('Choose language'),
    '#title_display' => 'invisible',
    '#options' => $select_options,
    // Use the browser detected language as default or English if nothing found.
    '#default_value' => !empty($browser_langcode) ? $browser_langcode : 'en',
  );

So, could someone explain me why the error is shown, please?

Steven Jones’s picture

Status: Needs review » Needs work

Thanks for working on this issue, here's a quick visual review of the patch in #29.

+++ b/core/includes/form.incundefined
@@ -1781,18 +1781,18 @@ function form_builder($form_id, &$element, &$form_state) {
+    if (!isset($element[$property]) || call_user_func($callback, $element[$property])) {
+      throw new \UnexpectedValueException(t('@type element in form @form_id has a non-valid @property property.', array(
+        '@type' => $element['#type'],
+        '@form_id' => $form_id,
+        '@property' => $property,
+      ))); ¶
+    }

If I'm reading this if statement correctly then the Exception will be thrown when the $callback returns TRUE, but it looks like the default callback for validating the #title element will return FALSE when it's invalid, not TRUE. I'd suggest adding a negation here.

Also, call_user_func is super slow, and adding lots and lots of calls to it will certainly effect page load time. I would imagine that it's going to be worth adding a check to see if $callback == 'strlen' and calling strlen directly.

+function form_element_info_merge(array &$element) {
+  if (isset($element['#type']) && empty($element['#defaults_loaded'])) {
+    $element = NestedArray::mergeDeep(form_element_info($element['#type']), $element);
+  }
+}

The naming of this function doesn't really reflect what it does IMHO, I would expect this function to say, take two parameters of form elements and merge them together in some way, not magically add in form element defaults. Maybe this could be re-named to form_element_info_ensure_defaults or similar?

falcon03’s picture

Title: Require #title for all form elements » Some form elements should require #title to ensure their accessibility
Category: task » bug
Issue tags: -Needs tests +Needs architectural review

@Steven Jones: Thank you very much for your in-dept review.

But unfortunately the patch has an issue: not all form elements, in fact, should require the #title property (e.g. actions) as the patch currently assumes. So, I was thinking of a new patch that:

  1. Adds a "#requires_title" index set to true for form elements that should require #title in system_element_info();
  2. Adds a function to check wether a certain element requires the #title;
  3. Checks in form_builder() wether a form element requires the #title and, if it is not set or it is empty, throws an exception;

I think we need the function to check wether a certain form element requires #title because we should also enforce elements like #submit to have a #value set up; having this checks in a separate function instead of adding them directly in form_builder() should improve code readability.

Can you (or anyone else) validate this "battle plan", so that I can start working on a patch?

Xano’s picture

But unfortunately the patch has an issue: not all form elements, in fact, should require the #title property (e.g. actions) as the patch currently assumes. So, I was thinking of a new patch that:

It doesn't assume that. That's why the #property_validate property is introduced, which allows you to configure what properties should be validated and how. See #27.

falcon03’s picture

@Xano: thanks for clarifying that. I didn't read all comments and I'm dealing with this issue as a novice developer. :-)

There's something that is not working properly, though: the patch throws an exception for the language_select element in the first installation screen, but that form element has a proper #title set up. In addition to this, patch seems to require that #actions and other elements (e.g. submit) require the #title and it is throwing exceptions for them when it shouldn't. How could we fix it?

Xano’s picture

+++ b/core/includes/form.inc
@@ -4843,6 +4838,48 @@ function _form_set_attributes(&$element, $class = array()) {
+/**
+ * Returns default properties for any form element.
+ *
+ * @return array
+ */
+function form_element_info_defaults() {
+  return array(
+    '#array_parents' => array(),
+    '#defaults_loaded' => TRUE,
+    '#required' => FALSE,
+    '#attributes' => array(),
+    '#property_validate' => array(
+      '#title' => 'strlen',
+    ),
+    '#title_display' => 'before',
+  );

The patch required that all elements have a title, unless they explicitly say they don't. What you can do is remove these default values, but then we need to add the property validation to every element that needs it explicitly.

falcon03’s picture

ok, I'm starting to understand. Basically, we're trying to accomplish the same thing, but your solution is more abstracted than mine.

So, understanding how the patch works, I think that any element should implement #property_validate in system_element_info() declaring what properties it requires to work. We can start with the #title property for form fields that should require it and then we can extend to other properties.

However, I think that we need "abstracted" callbacks for validation. E.G. for "title" I think that we should add somewhere (maybe in a separate file) a element_validate_title() function, so that the validation can take advantage of a more complex logic (element_validate_title, for instance, should "trim" the string before passing it to strlen()).

At this point, though, I'm wondering:

  1. How would this kind of validation impact performance?
  2. Are there any other properties that should be validated by Form API and that are not validated anywhere else? If they're already validated, should we convert their validation to #property_validate?
  3. Can we implement property_validate in D8, given that code freeze is on July 1st?
falcon03’s picture

Issue tags: -Needs architectural review +Needs committer feedback

It's July 1st. So, before working anymore on this, I think we need feedback from committers.

Can we go for the abstracted solution proposed by the current patch abstracted as per #48 or should we go for the simpler solution that I proposed in #44?

If needed, I'll update the issue summary as soon as we get feedback.

falcon03’s picture

And ya, here's another option: we could add a #pre_render callback on form elements that require the label in system_element_info() that checks wether a form element has a label. If the answer is yes, then returns $element; otherwise, it throws the exception.

Considering this and #49 (my previous comment), I'm not sure which is the best way to go right now...

falcon03’s picture

Status: Needs work » Needs review
FileSize
10.02 KB

So, I wrote a patch to demonstrate @xano's approach abstracted as my proposal. This patch is completely untested.

Disclaimer: I strongly need some advice on the proper way to go (@see #49) before working anymore on this.

falcon03’s picture

Status: Needs work » Needs review
Issue tags: +Needs architectural review

I was expecting failing tests, but I need some kind of review (see previous comment). So setting status back to NR.

Pancho’s picture

Issue tags: +API change

Note that I listed this issue on both #2034999: [meta] Comply with WCAG 1.1: Text Alternatives and #2035139: [meta] Comply with WCAG guideline 4.1: Compatibility.
This issue is crucial for at least passing "A" level of WCAG guidelines.
Also, the API change is quite minor, given that providing a #title was already strongly recommended.

Steven Jones’s picture

Issue tags: -API change

So if we're using custom validation callbacks anyway, then maybe we should pass the entire element, because otherwise you wouldn't be able to do validation based on other properties in the element, which one may want to do at some point.

falcon03’s picture

@Steven Jones: Are you confirming that this is the most appropriate approach to solve this issue (@see my previous comments)?

If so, I'll be more than happy to work on this. Implementing your suggestion would require just a few seconds! :D

Steven Jones’s picture

Status: Needs review » Needs work

I'm saying that it would be a shame to introduce an abstraction that doesn't allow for some possibly useful use-cases.
It would seem to me to be better to do this in an abstract way rather than just checking the #title property only.

I would suggest that getting a working patch written asap would be best, as at the moment all of the patches posted have stopped Drupal being installed.

falcon03’s picture

Attaching an updated patch that incorporates feedback from @Steven Jones in the previous comment. I've managed to successfully install Drupal after applying this patch, let's see what testbot thinks! :-)

Disclaimer: I expect this patch to fail testing. But, if I am right, all failing tests will uncover accessibility issues! :-)

Seems a critical task to me. This is a huge step towards WCAG compliance!

falcon03’s picture

Title: Some form elements should require #title to ensure their accessibility » Add a form property validation API and implement it for #title property to ensure form accessibility
Priority: Major » Critical
Status: Needs work » Needs review
FileSize
10.38 KB

forgot patch.

Pancho’s picture

Priority: Critical » Major

Thanks for the new patch, and yes, this is quite important for WCAG.
But while WCAG compliance is a stated aim, it currently is not a release criterion for D8, so this doesn't block D8 from being released.
Therefore setting back to major.

falcon03’s picture

Status: Needs work » Needs review
FileSize
12.52 KB

So, we're getting to the hardest part: adding #title where it is missing. This is what I've got after digging into code for some hours. It is not too much, but I am not an experienced drupal developer.

I am not able to make the form in user.admin.inc work at all. I also have serious troubles understanding where the form to add a block is generated. Reality is: I don't know anything about our controller/interface/all_kind_of_abstraction that we have in core. Is there somewhere a good tutorial that explains it, let's say, to someone that is not too familiar with OOP?

In any case, if someone wants to work on this, please feel free to do it; I'd be happier to review an eventual patch, at this point! :D

falcon03’s picture

Status: Needs work » Needs review
FileSize
13.26 KB

Fixing stupid errors of mine! :-)

Steven Jones’s picture

@falcon03 thanks for all the hard work. Are module developers going to feel this much pain if the patch lands? We need to be able to make it really obvious what the error is, and where the place to fix it is, if it's not already obvious. Keep going! I'd imagine that the high numbers are just because the same forms are used over and over?

Steven Jones’s picture

All, I'm assuming that we can just look at the exceptions in the Simpletest output really, and not worry about the failures too much until the exceptions are fixed, as those are the things we've introduced?

Xano’s picture

I wonder if we can't use Symfony constraints for this instead of developing a Drupalism that will require developers to write a completely new kind of validation function for their form elements.

Steven Jones’s picture

@Xano do Symfony constraints apply to arrays? I thought it was just objects?

Xano’s picture

I've never used it for arrays yet, but there is \Drupal\Core\TypedData\Plugin\DataType\Map, which can represent arrays and be validated using Symfony's constraint validator, for instance.

Steven Jones’s picture

Okay, well if there's a nice way to do this with Symfony validators then let's do that, but converting FormAPI entirely to objects isn't at all possible this late in the development cycle of D8.

falcon03’s picture

@Steven Jones, @xano: not sure that using Symfony constraints is doable with our form API and, most of all, if this will impact on performance more than our current API implementation. But I am the less experienced person to talk about this! :-)

Drupal's form API will be converted entirely to Symfony Forms during D9 development (there's already an issue for that).

We have a lot of work to do, but the high number of failures is related to all the exceptions. But, honestly, I see some fails that I cannot understand (e.g. the book module). I've tried it, but no exceptions were thrown during my test.

Also, there should be one or more forms related to entities (e.g. config Entities, entity translation, etc) that are abstracted in some place and reused in various places. I haven't still found them! :D

Pancho’s picture

If the #68 road works out that would be amazing!
We should then check our codebase for more possible Symfony constraints conversions.

[edit:] sorry, crossposted with #72, but anyway.

falcon03’s picture

Status: Needs work » Needs review
FileSize
17.77 KB

Worked a bit more on this; let's see how far from passing tests we are.

It's really hard form me to work on this, because I don't still understand a lot of things about the Drupal architecture; so, finding the proper places to fix errors is really time-consuming. In any case, it has a really positive impact on my Drupal skills: I am reading a lot of code.... And (maybe) I am learning a lot of things by doing it! :D

I don't expect the patch to pass tests, though, because there are some paths that do not work after applying it:

  • /block/add
  • /admin/content
  • /admin/people
  • /admin/config/content/formats/manage/full_html
  • /admin/structure/types/manage/article

The problem is... That I don't have any idea on how/where to fix the errors related to them. Any help/suggestion will be greatly appreciated! :D

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
664 bytes
18.28 KB

One bug, which seems to be causing many of these failures, is that a details element inside a vertical_tabs element never uses its title, so we shouldn't require one.

tim.plunkett’s picture

Status: Needs work » Needs review
Issue tags: -Needs architectural review
FileSize
10.81 KB
19.23 KB

Last change from me. I think this should be nested one layer further, so that a single property like #title could have more than one validator.

Also, this should be using trigger_error() like element_children() does.

tim.plunkett’s picture

Many of these errors are from dynamically generated elements, not hand-coded ones. So they'll have a single generic solution that will solve many of these errors.

falcon03’s picture

@tim.plunkett: Thank you very much for working on this.

After applying the latest patch you posted I can't see the text of the error message when I visit a page where some #title were missing. Instead, I can see some errors related to the database. I'm wondering if this is due to the trigger_error() usage...

If the patch is intended to work this way, I think we have a problem: as a novice developer, it is really hard to understand that those database-related errors are due to a missing #title definition on a form element (to see the errors I am talking about you can visit admin/people or admin/content)...

tim.plunkett’s picture

@falcon03 Is it possible for you to paste the database errors here? I'm not experiencing those.

Throwing an exception will halt all processing of the page, and masks further errors. You would have to fix one at a time, never knowing how many errors might be on a page.

Triggering an error is supposed to print the same message, but in the usual errors section, and the page will finish loading. This also means you can know how many errors are on a page.

Also, I'm sure this page is all but unusable with a screen reader, but https://qa.drupal.org/pifr/test/575478 (the "View details" link on the failing patch) now lists all 5000+ errors as exceptions.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
11.5 KB
20.53 KB

I found another one of the big causes of failures, in WidgetBase.
Also I renamed our validate method and moved it into the section of form.inc with other validators.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
14.59 KB
36.61 KB

Okay, another major problem is that #process callbacks weren't being evaluated first.

It turns out we've been specifying the #title for machine_name all this time with no need to.

Also fixed views exposed forms and the permissions page.

This issue is blocked by #1812048: Build the exposed form using form API functions, so I've combined both patches for now.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
17.75 KB
52.3 KB

Whew. There are quite a bit of these.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
16.36 KB
64.46 KB
tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
10.86 KB
74.02 KB
tim.plunkett’s picture

Status: Needs work » Needs review
Issue tags: -Needs committer feedback
FileSize
1.87 KB
75.18 KB

Drumroll...

Steven Jones’s picture

@tim.plunkett Congrats on the tests passing, awesome work!

The patch in #93 introduces a call_user_func, it was my understanding that such calls are very slow and should be avoided if possible? Are we introducing a performance regression here? There are a lot of forms in Drupal.

tim.plunkett’s picture

Steven Jones’s picture

Okay fair enough, this is going to be a general performance issue for D8 then, and we don't need to worry about it in this issue.

Xano’s picture

I didn't get to review the entire patch as my train will pull into the station shortly.

And ya, here's another option: we could add a #pre_render callback on form elements that require the label in system_element_info() that checks wether a form element has a label. If the answer is yes, then returns $element; otherwise, it throws the exception.

This suggestion seems to have been ignored. I'm just quoting it to see if it may have been overlooked accidentally.

+++ b/core/includes/form.inc
@@ -1907,6 +1907,23 @@ function form_builder($form_id, &$element, &$form_state) {
+  if (isset($element['#property_validate']) && is_array($element['#property_validate'])) {

We should throw an exception if the property value is not an array.

+++ b/core/includes/form.inc
@@ -1907,6 +1907,23 @@ function form_builder($form_id, &$element, &$form_state) {
+        if (!isset($element[$property]) || !call_user_func($callback, $element)) {

I like that we don't use is_callable()/function_exists() like we do everywhere else. If the callback does not exist, this *should* raise a warning.

+++ b/core/includes/form.inc
@@ -3360,6 +3377,20 @@ function form_pre_render_actions_dropbutton(array $element) {
+  // Ensures that $title does not start with, end with or contain only spaces,
+  // and that it contains a string by verifying its length.

Should be simpler and more accurate along the lines of "Ensures that $title is a string that does not only contain whitespace."

+++ b/core/modules/config/lib/Drupal/config/Form/ConfigImportForm.php
@@ -17,12 +17,15 @@ public function buildForm(array $form, array &$form_state) {
-    $form['submit'] = array(
+
+    $form['actions']['#type'] = 'actions';
+    $form['actions']['submit'] = array(
       '#type' => 'submit',
       '#value' => t('Upload'),

Mix-up with another patch?

+++ b/core/modules/config/lib/Drupal/config/Form/ConfigImportForm.php
@@ -17,12 +17,15 @@ public function buildForm(array $form, array &$form_state) {
-    $form['submit'] = array(
+
+    $form['actions']['#type'] = 'actions';
+    $form['actions']['submit'] = array(
       '#type' => 'submit',
       '#value' => t('Upload'),
+      '#button_type' => 'primary',

Mix-up with another patch?

+++ b/core/modules/field/lib/Drupal/field/Plugin/Type/Widget/WidgetBase.php
@@ -180,10 +180,19 @@ protected function formMultipleElements(EntityInterface $entity, array $items, $
+          '#title' => '',

This shouldn't work, as the property validation checks for string length.

+++ b/core/modules/locale/css/locale.admin.css
@@ -85,9 +85,10 @@
-#locale-translation-status-form label {
+#locale-translation-status-form .label {
   color: #1d1d1d;
   font-size: 1.15em;
+  font-weight: bold;

Patch mix-up?

+++ b/core/modules/locale/lib/Drupal/locale/Tests/LocaleUpdateTest.php
@@ -167,7 +167,7 @@ function testUpdateImportSourceRemote() {
-    $this->assertRaw('<label for="edit-langcodes-de" class="language-name">German</label>', 'German language found');
+    $this->assertRaw('<label class="visually-hidden" for="edit-langcodes-de">Update German</label>', 'German language found');

Patch mix-up?

+++ b/core/modules/locale/locale.pages.inc
@@ -544,13 +545,16 @@ function locale_translation_status_form($form, &$form_state) {
+        'title' => array('class' => array('label'), 'data' => array('#title' => $title, '#markup' => $title)),

Can we make this multiline for readability?

tim.plunkett’s picture

The ['#type'] = 'actions'; stuff is a bit out of scope.

The test change and CSS change are required. The language form was not using tableselect properly.

The empty #title = '' is followed by '#property_validate' => array(), which should be more specific, but disables the validation. Otherwise you'd have two titles there.

There is no reason to throw any exceptions here.

We don't do is_callable() checks first anymore in D8. That's a D7 only thing.

tim.plunkett’s picture

mgifford’s picture

Status: Needs review » Reviewed & tested by the community

Seems to apply nicely to SimplyTest.me. Applied this patch and navigated about to see if I could see any new errors... I also applied it locally and it worked fine there too.

In looking at Views, I discovered this error in Views UI #2044511: Label missing in Page: The title of this view & The menu path or URL of this view then applied the patch in #99 and the title was there.

Forcing the title and ensuring that users hide the label the proper way is going to be a great improvement!

falcon03’s picture

@mgifford: to make the patch pass tests, we had to ensure that there weren't any form items without a label. @tim.plunkett did a great job in fixing views and a lot of other stuff!

Did you try to define a form in a module and add a form component that the patch introduces validation for (e.g. '#type' => 'text') without defining the '#title' property? If so, what behavior did you get?

Anyways, I'll test the patch tomorrow and see if I can get again the SQL errors I mentioned previously in this issue.

falcon03’s picture

Oh, and I was forgetting:

  1. This patch still contains patch from #1812048: Build the exposed form using form API functions which is RTBC as well. So, not sure what to do here: do we need to wait for that patch to be committed and, after that, re-roll this patch to get it in?
  2. This kind of validation IMO is going to have a significant negative impact on performance. Do we need profiling for this patch? Or can we simply add it to the "known performance regressions from D7" meta issue?
tim.plunkett’s picture

Issue tags: +needs profiling

This has to wait for that issue to get in.

We should profile admin/content/comment with 50 comments on it.

tim.plunkett’s picture

FileSize
68.27 KB

Rerolled, the blocker has gone in.

mgifford’s picture

@falcon03 - "Did you try to define a form in a module and add a form component that the patch introduces validation for (e.g. '#type' => 'text') without defining the '#title' property?"

I was actually just looking at the Views UI. I'd previously found a number of instances where the UI didn't have a label so I could just apply the patch from here and see that there was a label.

Challenge is that it made the Title visible since Views hadn't removed the label properly, but instead had just deleted the title.

This patch made the title visible on the page.

mgifford’s picture

Issue tags: +TwinCities

This would be a great for accessibility.

falcon03’s picture

#104: form-933004-104.patch queued for re-testing.

tim.plunkett’s picture

Status: Needs work » Needs review
Issue tags: -TwinCities
FileSize
67.53 KB

I swore I rerolled this. Bizarre.

falcon03’s picture

@tim.plunkett: thanks for rerolling this patch. I am going to profile it. This is the first time profiling Drupal for me, so sorry for these questions but:

  1. Should I profile using xhprof and the Xhprof kit used to test twig-related patches?
  2. What should we profile? In a previous comment, you suggested profiling admin/content/comment with 50 comments on it; do you confirm that? If so, is it enough?
falcon03’s picture

Oh, and I was forgetting... In comment #81, I mentioned MySQL-related errors thrown after applying this patch and viewing a page that contains a form with an element that doesn't specify a label using #title when it should do it.

It is very hard for me to report those errors because Voiceover goes really crazy on that page and doesn't allow me to read it. This is weird, and I really don't know why it happens. Since I can't take a screenshot, here with enclosed you can find a zip file containing the same page (admin/people) output in Safari's webarchive format when it works and when it doesn't. The second one is 2,9 MB bigger than the first one: is it normal?

falcon03’s picture

FileSize
567.56 KB

Uffff, d.o removed the attachment from the previous comment. Here it is :-)

tim.plunkett’s picture

@falcon03
I think xhprof-kit is a great start. And yes, the comment administration page should be a fair indicator.

falcon03’s picture

Rerolled patch. Please forgive any eventual mistake I did -- it's the first time I'm doing it! :-)

Profiling to come soon (no later than tomorrow), once I understand how to use the xhprof-kit; otherwise I'll use xhprof through the devel module.

tim.plunkett’s picture

falcon03’s picture

@tim.plunkett: thank you for fixing the patch.

Once again the patch needed a re-rol: here it is.

catch’s picture

Category: bug » task
Priority: Major » Normal

There's two separate issues here - that some form elements don't have titles, and adding a validation API.

I'm not at all convinced about adding a form property validation API at this stage, to the point where I'd consider bumping the patch to 9.x. For specific accessibility issues we should split those into a separate issue (which should be major).

webchick’s picture

I actually agree with catch. :\ It's too late to be introducing new APIs like this in core, IMO, especially given where we are with technical debt cleaning up the things we already committed to shipping with.

However, I'd rather not ice this initiative for 3+ years, because obviously it's finding bugs, and that's awesome. I wonder if this patch couldn't fit into something like Devel module or Coder Review module? Then it could work even for D6/D7 sites, and it would catch the problem right at the point it needs to: while the module developer's writing code, not after the code has been published as a project and downloaded onto someone's site.

tim.plunkett’s picture

Assigned: Unassigned » tim.plunkett

I can reroll without the API addition and we can at least fix the missing #title properties we've found.

falcon03’s picture

Assigned: tim.plunkett » Unassigned

Disclaimer: I didn't mean at all to offend anyone with this comment. If you feel this way, please contact me and I will be happy to clarify what I mean't to say (I'm not a native English speaker). This comment also contains *a lot* of personal opinions: I'm not an experienced developer and, if I am wrong, I'm sorry and I'm happy to understand the reason why I'm wrong. The comment is *not so short* but I felt I needed to give my two cents.

@catch, @webchick: well, I don't really agree. First off, this patch is not changing any API usage -- it's simply enforcing the correct usage of the current Form API. So, according to http://buytaert.net/drupal-8-api-freeze I would consider it an API addition. At the same time, though, even if this would be considered an API change, "It could solve a critical/major issue.". Another point is that if we want Drupal to pass WCAG 2.0 AA validation we should *make sure* that each form element is properly labeled. And according to the current patch we were not in such a situation -- this underlines once again how much we need this kind of validation to enforce Drupal accessibility.

However, I can understand that this change could have a significant impact on module developers, especially in contrib. But, the consequences of this change (apart from any module that does not use FAPI properly stop working) would be -- you can easily guess -- accessibility improvements. So, I am of course in favor of them. Drupal APIs changed really a lot from D7 to D8 (e.g. let's simply think to the hook_menu -> routing system stuff). Porting a module from D7 to D8 requires a lot of work; using the classic form API properly (that is something that any developer is supposed to do even right now) is a relatively simple task compared to making a module work with all the new D8 APIs.

Finally, if the patch can't really make into D8, I would suggest committing all the accessibility improvements we did while working on this issue in a single patch.

On a side note: IMHO this situation is a bit frustrating. This issue sat for 16 days as needing committer feedback. And the feedback has come only more than a month after those 16 days, when the patch is ready and passes tests -- it only needs profiling (that I was going to do before replying to your comments). I've started working on this issue and I've done anything I could to push it forward; I don't know how @tim.plunkett (he made the patch pass tests and wrote most of it) feels, but if he's feeling the same as myself it's not a good feeling: we wasted time on this effort while we could have worked on something else. And regarding accessibility there is really a lot of work to do: we can't afford wasting time.

I strongly understand that committer's time is really limited, but I'm convinced that committer's feedback is even more important than committing a patch itself. And you guess, during the 16 days the patch has been sitting as "needs committer feedback" a lot of patches have been committed.

falcon03’s picture

@tim.plunkett: I didn't mean to unassign the issue from yourself. Feel free to re-assign; if this is the way to go I'll review your patch as soon as possible.

webchick’s picture

Sorry, I don't follow that tag, only the RTBC queue. :( That's the easiest way to attract the attention of a core maintainer, since most RTBC issues get dealt with within a few days, or a week at the latest. If there is documentation that says that tag should be used, we should update it, IMO. Sorry for your frustration. :(

Let's leave aside the release cycle timing question altogether for a moment, though, and discuss this patch on its own merits. Here's my concern with the approach. It's causing ~15 lines of code to run every single time any form in Drupal is built. And while that might not seem like a lot (and in fact, really isn't when we're just talking about #title properties on form elements), what happens when other rules get added to this validator, both in core and contrib, like making sure #type select and radios has #options and #type foo has bar?

Where we need to be catching this problem is at development time, when the module developer is first building out and testing their forms. Once all of the elements on a form have the proper properties, this code is just wasted processing.

That's why I feel Devel/Coder Review is actually a better fit for this code than in core itself. We don't typically do this kind of "code babysitting" in core, and definitely not during runtime code, where it becomes a performance impact.

webchick’s picture

That said, yes absolutely let's get a patch to fix all of the transgressions you've found!

falcon03’s picture

@webchick: See https://drupal.org/needs-tags I learnt there about the "needs committer feedback" tag existence. And since there are other issues opened with this tag applied, maybe we should "fix" them as well. :-) In any case, moving an issue to RTBC just to get feedback seems quite a "hackish" workaround, but I don't have any problem with it if you and other maintainers agree that this is the proper way to ask for feedback! :D

Now, speaking of this patch, you outlined a problem that I've had in mind since I started working on the patch. The real problem is that if we add this kind of validation (now I'm talking about #title specifically) in coder module/devel module, it won't get the same visibility and it won't really enforce the proper API usage. Maybe I do a mistake, but I never validate my custom modules using the coder module. And since neither core passes coder review, I'm wondering how many developers use it...

So this could fall back to one of my previous proposals: what about not adding an abstracted API and simply checking that #title is set up in form.inc, making a special case because of the #title accessibility implications?

Anyways (ok maybe I'm giggling, I don't know very well the Drupal architecture), isn't there any way to cache the form structure and use it while rendering the page instead of building the form each time? Maybe it's a stupid question, but maybe it could be worth asking....

On a side note: I think that a framework should do this kind of code-babysitting as much as possible and, of course, use some kind of caching so that code-babysitting doesn't impact on performance. I can see this as a seriously different architecture than the current Drupal's architecture, but maybe it could be worth discussing for D9... ;)

tim.plunkett’s picture

Assigned: Unassigned » tim.plunkett

Another possibility would be the API addition of the ability to do per-property validation, but don't actually use it outside of test modules, and put the implementation into devel module.

tim.plunkett’s picture

Issue tags: +API addition
FileSize
17.42 KB

#2074509: Add missing #title property to ensure form accessibility now contains the non-API addition parts.

This is the rest, which will fail until that's in.

tstoeckler’s picture

I haven't thought this idea through completely, but a different approach would be to write a test, that automatically parses all of a module's forms and checks the relevant form elements for a title. We could either find all of a module's classes and see if they implement FormInterface or we could skip classes that don't have 'form' in their name for performance or something. Anyway, I think the benefit would be that this would be right in core, and anyone that tried to introduce an inaccessible form into core would have to fix it because the test would fail. We could also find a solution that enables contrib to have the same test coverage.

tim.plunkett’s picture

webchick’s picture

I like the sound of #128 a lot!

mgifford’s picture

@tstoeckler Can you take this on? Agreed that this sounds like a great approach.

mgifford’s picture

Issue summary: View changes

Clarifying issue summary.

tstoeckler’s picture

Assigned: Unassigned » tstoeckler
Issue summary: View changes

Working on this now.

tstoeckler’s picture

Found #2138151: ContentLanguageSettingsForm is unused (and broken) while working on this. The patch here will also need to contain that patch for now.

tstoeckler’s picture

FileSize
11.39 KB

Here we go. This doesn't actually yet test anything, but if we get this to pass at least the discovery and instantiation works properly.

tstoeckler’s picture

Status: Needs work » Postponed

That was marked duplicate, so this is now postponed on #1987726: Convert language content page related callback to new style controller :-/

tstoeckler’s picture

Title: Add a form property validation API and implement it for #title property to ensure form accessibility » Test that all form elements have a title for accessibility
Assigned: tstoeckler » Unassigned

Also re-titling for the new approach.
And unassigning. Don't know when the other issue will get in, and if I can get back to it then.

YesCT’s picture

just updating issue summary for what this is postponed on.

tstoeckler’s picture

Status: Postponed » Needs work
mgifford’s picture

Issue tags: +Needs reroll

Needs a complete re-roll even after the path change to core/modules/language/src/Form/ContentLanguageSettingsForm.php

mgifford’s picture

Issue tags: +TwinCities

Hopefully we can work on this in the upcoming DrupalCamp.

mgifford’s picture

Issue tags: -TwinCities +TCDrupal 2014
Les Lim’s picture

Issue summary: View changes

Updated issue summary.

Lowell’s picture

Status: Needs work » Needs review
FileSize
8.99 KB

Attempted reroll of 933004-134.patch, please review closely. Got patch to apply cleanly to origin/8.0.x but have no idea if it is still functional.

mgifford’s picture

Hey @Lowell - well the bot likes it. It applies nicely on SimplyTest.me. Thanks for pushing this ahead.

I was just comparing ContentLanguageSettingsForm.php between patch #134 & #144. There are a lot of differences. More than I'd expect in a re-roll.

#134 also removed language_content_settings_page() and modified language.admin.inc, language.module & language.routing.yml.

We should be able to test if this works by simply removing the title from a form element anywhere in Drupal. That should be enough to verify this I think.

kattekrab queued 144: 933004-144.patch for re-testing.

kattekrab’s picture

Issue is 7 months old. Sent to bots for a retest.

manningpete’s picture

Issue tags: -Needs reroll

I could apply the latest patch, so no reroll is necessary.

tim.plunkett’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

This needs a reroll because it's using PSR-0 not PSR-4. The test is not being run.

rpayanm’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
3.17 KB
rpayanm’s picture

Status: Needs work » Needs review
FileSize
748 bytes
3.17 KB
siva_epari’s picture

Issue summary: View changes
FileSize
5.58 KB

Patch from #152 is not a proper reroll using PSR-4. So rerolling patch from #144 again.

  1. Patch from #144 rerolled using PSR-4
  2. ContentLanguageSettingsForm.php
    1. Drupal\Core\Form\FormBase changed to Drupal\Core\Form\ConfigFormBase in existing core/modules/language/src/Form/ContentLanguageSettingsForm.php . #144 patch was trying to create that file
    2. ConfigFormBase needs function getEditableConfigNames(). So getEditableConfigNames() added.
  3. AccessibilityTest.php
    1. namespace Drupal\system\Tests\Form changed to namespace Drupal\Tests\Core\Form
    2. File moved from /core/modules/system/src/Tests/Form/AccessibilityTest.php to /core/tests/Drupal/Tests/Core/Form/AccessibilityTest.php
    3. moduleHandler::install (Deprecated) Changed to moduleInstaller::install . ModuleInstaller initialised using setUp()
    4. SystemListing (Deprecated) changed to ExtensionDiscovery
YesCT’s picture

Issue summary: View changes
Issue tags: +drupaldevdays, +Needs issue summary update

since this is a normal task, let's do a beta evaluation on it. added instructions to remaining tasks in the issue summary.

related to #1493324: Inline form errors for accessibility and UX

mgifford’s picture

I'm assuming we can strikethrough "2. Reroll #134 removing sections that have been solved by #1987726: Convert language content page related callback to new style controller" from the issue summary. That must have been incorporated already I would hope.

Not sure how to move this issue ahead at the moment. Patch at #153 still applies nicely.

siva_epari’s picture

Status: Needs work » Needs review
FileSize
1.07 KB
5.62 KB
  • Added to core/tests/Drupal/Tests/Core/Form/AccessibilityTest.php
    * @coversDefaultClass \Drupal\Core\Form\Accessibility
    * @group Form
    
  • Removed debug statement accidentally left in previous patch
  • Added @inheritdoc for setUp()
mgifford’s picture

Issue tags: +Needs tests

Thanks @epari.siva we still need the tests, but it is coming along.

siva_epari’s picture

Yes, Mike. I am working on entity tests.

tim.plunkett’s picture

What about going back to changing FormBuilder, but using the new assert() capabilities?

mgifford’s picture

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

mgifford’s picture

@tim.plunkett It's been about a year and nobody has replied yet to your point.

I do think it would be worth while looking at the new assert() capabilities to see if this addresses this problem.

Also wanting to highlight @xjm's point about timelines for this:
https://www.drupal.org/node/2504847#comment-11750733

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

mgifford’s picture

Version: 8.3.x-dev » 8.4.x-dev

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

tim.plunkett’s picture

Assigned: Unassigned » tim.plunkett
Status: Needs work » Needs review
Issue tags: -FAPI, -API addition, -needs profiling, -TCDrupal 2014, -drupaldevdays, -Needs tests
FileSize
37.13 KB

Resurrecting this issue.
Completely new patch, inspired by the most "recent" approach.
I have some work to do yet, and a couple of the changes could or should be split out to blocking issues.

This makes a change to the recent additions of #2905527: Introduce a sample value entity method to ease plugin filtering via context.

tim.plunkett’s picture

This removed all of the @todos I added. Still needs an IS update and some of the fixes need to be split out

tim.plunkett’s picture

Removed some previous changes to other core code.

tim.plunkett’s picture

tim.plunkett’s picture

tim.plunkett’s picture

Core's moving quick today. Rerolled.

Status: Needs review » Needs work

The last submitted patch, 181: 933004-form_a11y_test-181.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

andrewmacpherson’s picture

Priority: Normal » Major

Bumping this back to major, as it relates to WCAG at level-A. I've seen comments #118 and #119, but those predate the suggestion in #128 to solve it through the testing framework instead of inside FAPI itself.

I think that making #title mandatory in FAPI is the most robust approach to address WCAG "Name, Role, Value" in a framework like Drupal, and is still worth pursuing, but if that means waiting until D9 for backwards compatibility reasons then I'm fine with that. A FAPI #title doesn't get in the way of themers who want to use a different approach with aria-labelledby or table cell headers attributes where appropriate.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
41.66 KB

Rerolled

Status: Needs review » Needs work

The last submitted patch, 186: 933004-form_a11y_test-186.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

tim.plunkett’s picture

Assigned: tim.plunkett » Unassigned

There are 3 real fails and 2 deprecation fails.
I can't reproduce 2 of the real failures and I don't understand how the third is possible :(

tim.plunkett’s picture

Unsure about these changes to ContentEntityStorageBase, so removing them (and the test changes) for now.

Status: Needs review » Needs work

The last submitted patch, 189: 933004-form_a11y_test-189.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

tim.plunkett’s picture

There are several bugs in core preventing this from working. Fixing them here for now, but they should likely be split out.

Also this test is one of the most ridiculous things I've ever written. Not really in a good way.

Interdiff is against #186

tim.plunkett’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 191: 933004-form_a11y_test-squashed-191.patch, failed testing. View results

tim.plunkett’s picture

Status: Needs work » Needs review
Issue tags: -Accessibility +accessibility
FileSize
26.55 KB

The approach I was taking previously was very very brittle and would also only test one code path per form, meaning that if any form elements were added or changed in a conditional, only one was tested.

Inspired by all of the work going on to trigger errors for deprecated code, here's a fresh approach that will catch essentially every case.

This is more similar to the original approach, but without all of the API additions and extraneous flexibility.

Still a bit of work to do, but let's see how far this gets.

Status: Needs review » Needs work

The last submitted patch, 194: 933004-form_builder-194.patch, failed testing. View results

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
27.32 KB
3.51 KB
tim.plunkett’s picture

  1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldWidget/OptionsSelectWidget.php
    @@ -31,6 +31,7 @@ public function formElement(FieldItemListInterface $items, $delta, array $elemen
    +      '#title' => '@todo',
    

    Wasn't sure what to do with this one.

  2. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
    @@ -998,6 +998,22 @@ public function doBuildForm($form_id, &$element, FormStateInterface &$form_state
    +    $types_to_skip = [
    +      'field_ui_table',
    +      'table',
    +      'tableselect',
    +      'hidden',
    +      'token',
    +      'value',
    +      'item',
    +      'datelist',
    +    ];
    

    Need to codify this somehow.

  3. +++ b/core/modules/content_translation/content_translation.admin.inc
    @@ -138,6 +138,8 @@ function _content_translation_form_language_content_settings_form_alter(array &$
    +              '#title' => $definition->getLabel(),
    +              // @todo Remove this?
                   '#label' => $definition->getLabel(),
    
    +++ b/core/modules/language/src/Form/ContentLanguageSettingsForm.php
    @@ -118,7 +118,10 @@ public function buildForm(array $form, FormStateInterface $form_state) {
    +          // @todo Remove this?
               '#label' => $bundle_info['label'],
    +          '#title' => $bundle_info['label'],
    +          '#title_display' => 'invisible',
    

    Wasn't sure about this either. #label isn't a thing

  4. +++ b/core/modules/field_ui/src/Form/EntityDisplayFormBase.php
    @@ -426,9 +426,10 @@ protected function buildFieldRow(FieldDefinitionInterface $field_definition, arr
    +            '#title' => $this->t('Edit'),
    ...
    -            '#attributes' => ['class' => ['field-plugin-settings-edit'], 'alt' => $this->t('Edit')],
    +            '#attributes' => ['class' => ['field-plugin-settings-edit']],
    

    Note for reviewers: \Drupal\Core\Render\Element\ImageButton::preRenderButton() copies the #title into the #attributes][alt itself

  5. +++ b/core/modules/node/src/Plugin/Search/NodeSearch.php
    @@ -822,7 +822,10 @@ public function buildConfigurationForm(array $form, FormStateInterface $form_sta
    +        '#title' => $this->t("Influence of '@title'", ['@title' => $values['title']]),
    +        '#title_display' => 'invisible',
    ...
    +        // @todo Is this still necessary with the invisible title?
             '#attributes' => ['aria-label' => $this->t("Influence of '@title'", ['@title' => $values['title']])],
    

    Not sure why this aria-label was used here instead...

tim.plunkett’s picture

Issue tags: -accessibility (duplicate tag) +Accessibility

Fixing tag

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

DamienMcKenna’s picture

The patch still applies against 8.9.x, though half of the hunks had line offsets.

DamienMcKenna’s picture

I removed the #label items per timplunkett's comment, and added a comment about the image title to let folks know why it isn't added there.

For this part..

    $element += [
      '#type' => 'select',
      '#title' => '@todo',

This field is output alongside a 'label' field anyway, so does it need a separate 'title'? That said might something like this work?

      '#title' => isset($element['#title']) ? $element['#title'] : '',

Status: Needs review » Needs work

The last submitted patch, 202: drupal-n933004-202.patch, failed testing. View results

DamienMcKenna’s picture

Status: Needs work » Needs review
FileSize
27.4 KB

Rerolled for 8.8.x and 8.9.x.

Status: Needs review » Needs work

The last submitted patch, 204: drupal-n933004-204-88x.patch, failed testing. View results

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

smustgrave’s picture

Status: Needs work » Needs review
FileSize
27.32 KB
7.39 KB

Rerolled based on #204. Good to review again

Status: Needs review » Needs work

The last submitted patch, 211: 933004-211.patch, failed testing. View results

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.