Updated: Comment #1
Commit message:
Issue #2074509 by tim.plunkett, falcon03, mgifford, Xano: Fixed Add missing #title property to ensure form accessibility.
Problem/Motivation
#933004: Test that all form elements have a title for accessibility proposed a new FAPI property to force #title properties on certain element.
That led us to finding dozens of form elements with missing #titles.
However, it might be too late to add that API, but we can still salvage the form additions.
Proposed resolution
Add in #title in missing places, using '#title_display' => 'invisible',
when the title is not visually appropriate.
Remaining tasks
N/A
User interface changes
Some form elements will have titles, but very few are visibly changed.
API changes
N/A
Related Issues
#933004: Test that all form elements have a title for accessibility
Comment | File | Size | Author |
---|---|---|---|
#15 | title-2074509-15.patch | 56.84 KB | tim.plunkett |
#11 | form-title-2074509-11.patch | 56.84 KB | tim.plunkett |
#9 | title-2074509-9.patch | 56.72 KB | tim.plunkett |
#9 | interdiff.txt | 7.82 KB | tim.plunkett |
#1 | title-2074509-1.patch | 51.04 KB | tim.plunkett |
Comments
Comment #1
tim.plunkettI added the proper commit message to the summary, that was based on what dreditor generated for the original issue.
Comment #2
tim.plunkettTagging.
Comment #3
falcon03 CreditAttribution: falcon03 commentedOk, so I started reviewing the patch but I don't think I'm still awake enough (It's 5 past 1 AM here) to keep reviewing it. I'll complete the review tomorrow; if someone wants to review the remaining part, you can start from the update module files. Also, I think this is my first serious review; let me know if I did something wrong. And finally... Sorry in advance for eventual stupid questions contained in the review ;)
Comment #4
tim.plunkettt('Machine name') is added as a default value for all #type => machine_name.
So that was all redundant, no need to worry about me removing it :)
I'll respond about the other feedback later, thanks for the review!
Comment #5
falcon03 CreditAttribution: falcon03 commented@tim.plunkett: thanks for clarifying that. I'm going to finish reviewing the patch: would you prefer me posting the whole review again or just the missing part?
Comment #6
falcon03 CreditAttribution: falcon03 commentedHere's part 2 of the review. I reviewed the whole patch, but I didn't apply nor manually-tested it.
Comment #7
falcon03 CreditAttribution: falcon03 commented@tim.plunkett: I've completed my first serious review. Maybe I outlined some non-existent issues; if so, let me know -- I'm always happy to learn! :D
I tried to be as short as possible in my notes in the review; if you need further details let me know! ;)
Comment #8
tim.plunkettResponse to review in #3
The array wrapping it is the form element itself. #title is only useful with a #type.
This is the style needed for
'#type' => 'tableselect'
The CSS change is because we have a standard way of doing tabledrag, and the language code was working around it. To maintain visual consistency while improving the accessibility, I had to make a CSS change.
The #markup is for the actual printed value, the #title is for the title text or label, depending on the #type it is used in.
I changed the structure of $options, we now sort by #title
This is the simplification I made that resulted in the CSS change.
Yes, % results in the placeholder being wrapped in an EM tag. I just picked one, it doesn't matter really since it is a test code.
I am using t() here, and yes that's fine.
I personally think its fine, there will only ever be that in the row, no further additions (this is the syntax needed by tabledrag).
Response to review in #6
The "weird nesting" is pretty standard when it comes to 'data' in table code.
It is a list of actions to be used as the default action, so I think "Default actions" is appropriate.
Yes we are.
I will remove the extra use Drupal\Component\Utility\String; and fix the #title/#title_display ordering later.
Comment #9
tim.plunkettI fixed all usages of #title_display to be directly after #title.
I removed the extra String use statement.
I also, after investigating its usage, removed the ambiguous 'Remove' title you questioned above. That input element is ALWAYS display:none, and is just used for JS.
All oddities around the array nesting are common enough patterns.
Comment #10
falcon03 CreditAttribution: falcon03 commented@tim.plunkett: Patch looks really good, so I'd happily RTBC it... But, unfortunately it does not apply! :(
Also, regarding the "remove field": I'm fine with removing #title for it, since it is never displayed; however, if the property validation API would get in could that missing #title make tests fail? If so, let's keep that "Remove" title in without worrying too much about it! :-)
BTW: Not sure if I am able to be onLine until wednesday.
Comment #11
tim.plunkettQuick reroll for the OverviewTerms form being converted.
I'd rather not add it in wrongly, and the other issue (if/when it happens) will catch it easily.
Comment #12
disasm CreditAttribution: disasm commentedCompleted a review of #11. This looks really good. Everything that stuck out at me in it as out of place, were addressed in earlier comments (like the css changes). Marking RTBC.
Comment #13
falcon03 CreditAttribution: falcon03 commentedYAY! RTBC++! :-)
Comment #14
webchickMost of this looks great. One "needs work" comment and a couple of questions.
AFAIK we are never, ever to do t($var). Gábor would know more about why that is.
We removed check_plain here?
Is that translated somewhere?
Comment #15
tim.plunkett$bundle was already translated, good catch!
This is the full context here, I moved it to a local variable so as to not run it twice
$options[$perm] = $perm_item['title'];
is translated, its straight from hook_permission()Comment #16
falcon03 CreditAttribution: falcon03 commentedPatch applies cleanly. Bot can disagree if he wants to! :-)
Comment #17
tim.plunkettI forgot the interdiff, it was just removing t() from the t($bundle) in the first snippet webchick pointed out.
Thanks falcon03!
Comment #18
falcon03 CreditAttribution: falcon03 commented@tim.plunkett: No problem for the interdiff. I knew what to look for in the new patch, so I quickly checked it out and it was fine for me! :-)
Looking forward to getting this into core! Great job Tim! :-)
Comment #19
tim.plunkettReposting the suggested commit message, since this was split off from another issue:
Issue #2074509 by tim.plunkett, falcon03, mgifford, Xano: Fixed Add missing #title property to ensure form accessibility.
Comment #20
webchickAwesome!!
Committed and pushed to 8.x. Thanks!
Comment #22
YesCT CreditAttribution: YesCT commentedthis might have caused: #2112303: Random extra text around translatability configuration is confusing
Comment #22.0
YesCT CreditAttribution: YesCT commentedupdated for credit
Comment #23
Gábor HojtsyThese caused #2112303: Random extra text around translatability configuration is confusing. Note that as explained there these checkboxes get a
<label for=""></label>
elsewhere that is **more accessible** than the immediate simple text label that this patch introduced, because they include all levels of the hierarchy in visually hidden span parts.So this does not only splintered the UI with some random (and inconsistent looking) labels, it is also *less accessible* (although there are now two
<label for=""></label>
elements for these, so who knows which one would a browser pick up on when reading out the form element.Please see #2112303: Random extra text around translatability configuration is confusing for rollback of this part of the patch.
Comment #23.0
Gábor Hojtsylinking with the nice format to the issue it was split off of.
Comment #24
schifazl CreditAttribution: schifazl commentedHello,
I'm sorry if I'm not writing in the right place. I'm in doubt, so I'm not changing the issue status.
I have this problem in Drupal 7, and it's a big accessibility problem: the #title property isn't generated for textfields. I was trying to check it myself, but I don't have much time and knowledge of Drupal development.
I discovered this problem while checking another problem in the Views module, this is the link to the related issue: https://www.drupal.org/node/1137724
Thanks!
Comment #25
mgifford@schifazl - just to confirm. This isn't the right area. This is a Drupal 8 issue and there's no call to backport it to Drupal 7.
I'd open up a new issue and focus on describing what the problem is an how to replicate it.
If it is a Drupal Core issue you should be able to describe how to replicate it from http://simplytest.me/project/drupal/7.x
If you need technical help because you don't have the capacity, there are lots of people you could hire to help you do this. If you don't have the $$ either, then it's probably going to take longer to find a solution.
Screenshots help. Examples of bad code helps. There are lots of examples of textfields which use labels and are accessible in Drupal Core. It's the norm not the exception. So describe to us how to see the exception.
Just do it in a new issue and tag it "accessibility" if you see it as an accessibility problem.