Problem/Motivation
Tabledrag library has a button to swap between draggable handles, and numeric weight fields, which is important for assistive tech users (screen readers, speech control) and keyboard-only users. The button has a clear label, so that's good.
However it also has a title attribute, which provides a slightly more verbose explanation of what the button is for. The trouble is, this title attribute is basically useless. It can only be perceived by sighted mouse users (i.e. hover-capable pointers). More importantly, the title attribute can't be perceived by the intended audience - people who need to use the row weights feature. The title attribute is not available to sighted keyboard users, speech control users, or (most) screen reader users. When the button element has text content (like this one does) the title attribute is not supposed to be used when calculating the button's accessible name for assistive technology. Screen magnifier users with a mouse can trigger it, but it's a long title which may not fit in their magnifier viewport.
Background reading: Using the HTML title attribute.
Drupal has a lot of unnecessary title attributes, and it's something we have been called out for in the wider accessibility community (see Tenon Research first glimpse: The best & worst of content management systems).
Note also:
- We have another variant of this in MediaLibraryWidget (which says "media items" instead of rows).
- The behaviour of this button is documented in System module's hook_help. This can stay, and we'll probably also document it with the help_topics, once #2920309: Add experimental module for Help Topics lands.
Proposed resolution
Remove the title attribute from the "show/hide row weights" button.
Also remove the similar title attribute from MediaLibraryWidget.
Remaining tasks
Patch to remove the title attribute from these buttons:
- Update tabledrag.es6.js, and re-compile the JS in tabledrag.js.
- Update MediaLibraryWidget.php
Does this affect any tests? The title string appears in core/modules/migrate_drupal/tests/fixtures/drupal7.php
- is this relevant?
User interface changes
Remove some title attributes which are of very little use, and cannot be perceived by the intended audience.
Removes a couple of translatable strings, which are no longer used.
Comment | File | Size | Author |
---|---|---|---|
#15 | 3023966-remove-title-show-row-weights-15.patch | 2.01 KB | pawandubey |
Comments
Comment #2
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedThis will make a good first-time contribution, maybe during the upcoming Global Contribution Weekend 2019 later this month.
Comment #3
phileas75 CreditAttribution: phileas75 as a volunteer and commentedComment #4
eblue CreditAttribution: eblue as a volunteer commentedFirst-time contributor here! Here goes.
Comment #5
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedThanks @eblue!
I looked at your patch on my phone, and I will test it properly soon.
It looks like your patch addresses the change needed in the tabledrag.js files.
Can you move it forward to address the media library widget as well? It's another simple change.
Comment #6
eblue CreditAttribution: eblue as a volunteer commentedHi Andrew,
Apologies - I initially only deleted the title attr in MediaLibraryWidget.php that was the same as the one in the tabledrag.js files. I deleted the title attr 'weight' this time as well since it was the only other title attribute I saw associated with row weights.
Comment #7
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedI'm very sorry, but I was mistaken in #5. I think I skim-read it in a hurry during a busy sprint weekend.
Your first patch in #4 did address both the tasks in the issue summary correctly. I manually tested the markup for both changes, and the HTML attributes are removed as intended. The extra change in #6 isn't needed. You're right that isn't a HTML title attribute. It's a Drupal Form #title property, which gets turned into a HTML label element, which we want to keep.
Looking closer at patch #4...
The code change is correct. There's one minor nitpick - the extra blank line shouldn't be here, for readability.
So we can go with the patch from #4, but would you mind re-rolling it without the extra blank line? Once that's done we can mark the issue Reviewed & tested by the community.
Thanks, sorry about the mix-up.
Comment #8
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedComment #9
Karan Sen CreditAttribution: Karan Sen as a volunteer and at Srijan | A Material+ Company commentedPlease review the patch.
Comment #10
Karan Sen CreditAttribution: Karan Sen as a volunteer and at Srijan | A Material+ Company commentedupdated comment.
Comment #12
Karan Sen CreditAttribution: Karan Sen as a volunteer and at Srijan | A Material+ Company commentedSorry for the faulty patch, fixed it now. please review.
Comment #14
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedThanks for the patch @Karan Sen.
Patch #9 had a netbeans config file doesn't exist in Drupal core. It looks like the patch was rolled against a personal repo, which differs from the upstream drupal core repo. You've fixed that now, thanks.
Here's a review of patch #12:
The indentation of the second line doesn't need to be changed.
Overall patch #12 is a step backwards, I'm afraid, because of the missing MediaLibraryWIdget change.
Patch #4 is the best one so far, it just needs the blank line fixed (see #7).
Comment #15
pawandubey CreditAttribution: pawandubey as a volunteer and at TATA Consultancy Services commented@andrewmacpherson
I have just re-rolled the patch#4 where remove the one extra empty line as per your suggestion.
Please review this patch and share your feedback.
Comment #16
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedThanks @pawandubey. That's all correct now.
Comment #17
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedComment #18
Karan Sen CreditAttribution: Karan Sen at Srijan | A Material+ Company commentedHi @andrewmacpherson yes i noticed that later when I reviewed my patch, apologies for that. Hope it helps.
Comment #20
pawandubey CreditAttribution: pawandubey as a volunteer and at TATA Consultancy Services commentedComment #21
alexpottCommitted 9096cb6 and pushed to 8.7.x. Thanks!
Comment #24
mgiffordFixing the accessibility tag.