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
- 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...
- Reroll #134 removing sections that have been solved by #1987726: Convert language content page related callback to new style controller
- or possibly start over.
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)
Comment | File | Size | Author |
---|---|---|---|
#211 | interdiff-204-211.txt | 7.39 KB | smustgrave |
#211 | 933004-211.patch | 27.32 KB | smustgrave |
#204 | drupal-n933004-204-88x.patch | 27.4 KB | DamienMcKenna |
#202 | drupal-n933004-202.interdiff.txt | 3.42 KB | DamienMcKenna |
#202 | drupal-n933004-202.patch | 27.4 KB | DamienMcKenna |
Comments
Comment #1
Everett Zufelt CreditAttribution: Everett Zufelt commentedTagging
Comment #2
bowersox CreditAttribution: bowersox commented+1
Comment #3
mgifford+1
Comment #4
Everett Zufelt CreditAttribution: Everett Zufelt commentedWould 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?
Comment #5
mgiffordIt 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.
Comment #6
Steven Jones CreditAttribution: Steven Jones commentedCurrently 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.
Comment #7
nevets CreditAttribution: nevets commentedRequiring #title seems a bit drastic. For example a form element that uses markup to add addition explanatory text should not need a title
Comment #8
Everett Zufelt CreditAttribution: Everett Zufelt commented@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.
Comment #9
Everett Zufelt CreditAttribution: Everett Zufelt commented@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.
Comment #10
mgiffordJust adding a FAPI tag.
Comment #11
bowersox CreditAttribution: bowersox commentedProposed 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.
Comment #12
mgiffordThis is still pretty rough, but wanted to put the rough idea up as a patch.
Comment #14
franz#12: Require-title-933004-12.patch queued for re-testing.
Comment #16
XanoThis 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.
Comment #17
Everett Zufelt CreditAttribution: Everett Zufelt commented@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.
Comment #18
Xano@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().
Comment #19
bowersox CreditAttribution: bowersox commented@Xano, this approach sounds good to me too.
Comment #20
mgifford@Xano Can you write up a new patch? I think we also need tests.
Comment #21
mgiffordJust re-rolling the patch from #12. Would be good to have the additions suggested by @Xano.
Comment #22
mgiffordNoticed a bunch of notices that needed to be fixed.
Comment #24
tim.plunkettThis should use trigger_error(), not drupal_set_message(). I believe drupal_render() has an example.
Also it needs tests.
Comment #25
Xano#title
properties to be set.#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:
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.
Comment #27
Xano#property_validate
property to all form elements, by replacing calls toelement_info()
with calls to the newform_element_info_merge()
, which merges default values into form elements.#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.strlen()
as their validation callback.Some examples of how this approach can be re-used:
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.Comment #29
XanoThis should work. Instead of just merging arrays, I used \Drupal\Utility\NestedArray, which allows for simpler code.
Comment #31
XanoThere 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.
Comment #32
mgifford#29: drupal_933004_29.patch queued for re-testing.
Comment #33
mgiffordYes, it applies fine locally. There was one whitespace issue
Thanks for all your work on this Xano. Hopefully the bot just had a bad cycle.
Comment #35
XanoThe 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.
Comment #36
mgiffordUltimately 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.
Comment #37
XanoThe 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.
Comment #38
falcon03 CreditAttribution: falcon03 commented#29: drupal_933004_29.patch queued for re-testing.
Comment #40
falcon03 CreditAttribution: falcon03 commentedThis 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...
Comment #41
larowlanWill try and work on this next week.
Comment #42
falcon03 CreditAttribution: falcon03 commentedOk, 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:
So, could someone explain me why the error is shown, please?
Comment #43
Steven Jones CreditAttribution: Steven Jones commentedThanks for working on this issue, here's a quick visual review of the patch in #29.
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 callingstrlen
directly.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?Comment #44
falcon03 CreditAttribution: falcon03 commented@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:
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?
Comment #45
XanoIt 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.
Comment #46
falcon03 CreditAttribution: falcon03 commented@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?
Comment #47
XanoThe 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.
Comment #48
falcon03 CreditAttribution: falcon03 commentedok, 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:
Comment #49
falcon03 CreditAttribution: falcon03 commentedIt'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.
Comment #50
falcon03 CreditAttribution: falcon03 commentedAnd 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...
Comment #51
falcon03 CreditAttribution: falcon03 commentedSo, 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.
Comment #53
falcon03 CreditAttribution: falcon03 commentedI was expecting failing tests, but I need some kind of review (see previous comment). So setting status back to NR.
Comment #54
PanchoNote 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.
Comment #55
Steven Jones CreditAttribution: Steven Jones commentedSo 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.
Comment #56
falcon03 CreditAttribution: falcon03 commented@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
Comment #57
Steven Jones CreditAttribution: Steven Jones commentedI'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.
Comment #58
falcon03 CreditAttribution: falcon03 commentedAttaching 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!
Comment #59
falcon03 CreditAttribution: falcon03 commentedforgot patch.
Comment #60
PanchoThanks 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.
Comment #62
falcon03 CreditAttribution: falcon03 commentedSo, 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
Comment #64
falcon03 CreditAttribution: falcon03 commentedFixing stupid errors of mine! :-)
Comment #66
Steven Jones CreditAttribution: Steven Jones commented@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?
Comment #67
Steven Jones CreditAttribution: Steven Jones commentedAll, 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?
Comment #68
XanoI 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.
Comment #69
Steven Jones CreditAttribution: Steven Jones commented@Xano do Symfony constraints apply to arrays? I thought it was just objects?
Comment #70
XanoI'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.
Comment #71
Steven Jones CreditAttribution: Steven Jones commentedOkay, 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.
Comment #72
falcon03 CreditAttribution: falcon03 commented@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
Comment #73
PanchoIf 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.
Comment #74
falcon03 CreditAttribution: falcon03 commentedWorked 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:
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
Comment #76
tim.plunkettOne 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.
Comment #78
tim.plunkettLast 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.
Comment #80
tim.plunkettMany 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.
Comment #81
falcon03 CreditAttribution: falcon03 commented@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)...
Comment #82
tim.plunkett@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.
Comment #83
tim.plunkettI 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.
Comment #85
tim.plunkettOkay, 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.
Comment #87
tim.plunkettWhew. There are quite a bit of these.
Comment #89
tim.plunkettComment #91
tim.plunkettComment #93
tim.plunkettDrumroll...
Comment #94
Steven Jones CreditAttribution: Steven Jones commented@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.Comment #95
tim.plunkettThanks!
We need call_user_func() instead of $function() so that methods can work.
For precedent, see
#1805688: Support methods as #element_validate callbacks
#1992052: Allow machine_name 'exists' to accept a callable
#1993312: Change pre_render, post_render, and after_build callbacks to accept callables
Comment #96
Steven Jones CreditAttribution: Steven Jones commentedOkay 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.
Comment #97
XanoI didn't get to review the entire patch as my train will pull into the station shortly.
This suggestion seems to have been ignored. I'm just quoting it to see if it may have been overlooked accidentally.
We should throw an exception if the property value is not an array.
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.
Should be simpler and more accurate along the lines of "Ensures that $title is a string that does not only contain whitespace."
Mix-up with another patch?
Mix-up with another patch?
This shouldn't work, as the property validation checks for string length.
Patch mix-up?
Patch mix-up?
Can we make this multiline for readability?
Comment #98
tim.plunkettThe
['#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.
Comment #99
tim.plunkettThis still contains #1812048: Build the exposed form using form API functions.
Comment #100
mgiffordSeems 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!
Comment #101
falcon03 CreditAttribution: falcon03 commented@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.
Comment #102
falcon03 CreditAttribution: falcon03 commentedOh, and I was forgetting:
Comment #103
tim.plunkettThis has to wait for that issue to get in.
We should profile admin/content/comment with 50 comments on it.
Comment #104
tim.plunkettRerolled, the blocker has gone in.
Comment #105
mgifford@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.
Comment #106
mgiffordThis would be a great for accessibility.
Comment #107
falcon03 CreditAttribution: falcon03 commented#104: form-933004-104.patch queued for re-testing.
Comment #109
tim.plunkettI swore I rerolled this. Bizarre.
Comment #110
falcon03 CreditAttribution: falcon03 commented@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:
Comment #111
falcon03 CreditAttribution: falcon03 commentedOh, 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?
Comment #112
falcon03 CreditAttribution: falcon03 commentedUffff, d.o removed the attachment from the previous comment. Here it is :-)
Comment #113
tim.plunkett@falcon03
I think xhprof-kit is a great start. And yes, the comment administration page should be a fair indicator.
Comment #114
falcon03 CreditAttribution: falcon03 commentedRerolled 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.
Comment #116
tim.plunkettBroken by #1987602: Convert ajax_form_callback() to a new style controller
Comment #117
falcon03 CreditAttribution: falcon03 commented@tim.plunkett: thank you for fixing the patch.
Once again the patch needed a re-rol: here it is.
Comment #118
catchThere'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).
Comment #119
webchickI 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.
Comment #120
tim.plunkettI can reroll without the API addition and we can at least fix the missing #title properties we've found.
Comment #121
falcon03 CreditAttribution: falcon03 commentedDisclaimer: 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.
Comment #122
falcon03 CreditAttribution: falcon03 commented@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.
Comment #123
webchickSorry, 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.
Comment #124
webchickThat said, yes absolutely let's get a patch to fix all of the transgressions you've found!
Comment #125
falcon03 CreditAttribution: falcon03 commented@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... ;)
Comment #126
tim.plunkettAnother 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.
Comment #127
tim.plunkett#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.
Comment #128
tstoecklerI 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.
Comment #129
tim.plunkettMy focus is on #2074509: Add missing #title property to ensure form accessibility for now.
Comment #130
webchickI like the sound of #128 a lot!
Comment #131
mgifford@tstoeckler Can you take this on? Agreed that this sounds like a great approach.
Comment #131.0
mgiffordClarifying issue summary.
Comment #132
tstoecklerWorking on this now.
Comment #133
tstoecklerFound #2138151: ContentLanguageSettingsForm is unused (and broken) while working on this. The patch here will also need to contain that patch for now.
Comment #134
tstoecklerHere we go. This doesn't actually yet test anything, but if we get this to pass at least the discovery and instantiation works properly.
Comment #136
tstoecklerThat was marked duplicate, so this is now postponed on #1987726: Convert language content page related callback to new style controller :-/
Comment #137
tstoecklerAlso 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.
Comment #138
YesCT CreditAttribution: YesCT commentedjust updating issue summary for what this is postponed on.
Comment #139
tstoecklerComment #140
mgiffordNeeds a complete re-roll even after the path change to core/modules/language/src/Form/ContentLanguageSettingsForm.php
Comment #141
mgiffordHopefully we can work on this in the upcoming DrupalCamp.
Comment #142
mgiffordComment #143
Les LimUpdated issue summary.
Comment #144
Lowell CreditAttribution: Lowell commentedAttempted 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.
Comment #145
mgiffordHey @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.
Comment #147
kattekrab CreditAttribution: kattekrab commentedIssue is 7 months old. Sent to bots for a retest.
Comment #148
manningpete CreditAttribution: manningpete commentedI could apply the latest patch, so no reroll is necessary.
Comment #149
tim.plunkettThis needs a reroll because it's using PSR-0 not PSR-4. The test is not being run.
Comment #150
rpayanmComment #152
rpayanmComment #153
siva_epari CreditAttribution: siva_epari commentedPatch from #152 is not a proper reroll using PSR-4. So rerolling patch from #144 again.
Comment #154
YesCT CreditAttribution: YesCT commentedsince 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
Comment #155
mgiffordI'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.
Comment #160
siva_epari CreditAttribution: siva_epari as a volunteer commentedComment #163
mgiffordThanks @epari.siva we still need the tests, but it is coming along.
Comment #164
siva_epari CreditAttribution: siva_epari as a volunteer commentedYes, Mike. I am working on entity tests.
Comment #165
tim.plunkettWhat about going back to changing FormBuilder, but using the new assert() capabilities?
Comment #166
mgiffordComment #169
mgifford@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
Comment #171
mgiffordComment #173
tim.plunkettResurrecting 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.
Comment #174
tim.plunkettThis removed all of the @todos I added. Still needs an IS update and some of the fixes need to be split out
Comment #175
tim.plunkettRemoved some previous changes to other core code.
Comment #177
tim.plunkettMore changes. Blocked by #2915034: \Drupal\Core\Entity\ContentEntityStorageBase::createWithSampleValues() does not handle language or bundles correctly
Comment #178
tim.plunkettForgot the patch. This now parses forms in \Drupal\Core
Comment #181
tim.plunkettCore's moving quick today. Rerolled.
Comment #185
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedBumping 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 cellheaders
attributes where appropriate.Comment #186
tim.plunkettRerolled
Comment #188
tim.plunkettThere 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 :(
Comment #189
tim.plunkettUnsure about these changes to ContentEntityStorageBase, so removing them (and the test changes) for now.
Comment #191
tim.plunkettThere 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
Comment #192
tim.plunkettComment #194
tim.plunkettThe 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.
Comment #196
tim.plunkettComment #197
tim.plunkettWasn't sure what to do with this one.
Need to codify this somehow.
Wasn't sure about this either. #label isn't a thing
Note for reviewers:
\Drupal\Core\Render\Element\ImageButton::preRenderButton()
copies the #title into the #attributes][alt itselfNot sure why this aria-label was used here instead...
Comment #198
tim.plunkettFixing tag
Comment #201
DamienMcKennaThe patch still applies against 8.9.x, though half of the hunks had line offsets.
Comment #202
DamienMcKennaI 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..
This field is output alongside a 'label' field anyway, so does it need a separate 'title'? That said might something like this work?
Comment #204
DamienMcKennaRerolled for 8.8.x and 8.9.x.
Comment #211
smustgrave CreditAttribution: smustgrave at Mobomo commentedRerolled based on #204. Good to review again