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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

andrewmacpherson created an issue. See original summary.

andrewmacpherson’s picture

This will make a good first-time contribution, maybe during the upcoming Global Contribution Weekend 2019 later this month.

phileas75’s picture

Assigned: Unassigned » phileas75
eblue’s picture

Assigned: phileas75 » Unassigned
Status: Active » Needs review
FileSize
2.01 KB

First-time contributor here! Here goes.

andrewmacpherson’s picture

Thanks @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.

eblue’s picture

Hi 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.

andrewmacpherson’s picture

I'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...

diff --git a/core/misc/tabledrag.es6.js b/core/misc/tabledrag.es6.js
index 76643c3a47..23ec123106 100644
--- a/core/misc/tabledrag.es6.js
+++ b/core/misc/tabledrag.es6.js
@@ -229,10 +229,7 @@
     // Add a link before the table for users to show or hide weight columns.
     $table.before(
       $('<button type="button" class="link tabledrag-toggle-weight"></button>')
-        .attr(
-          'title',
-          Drupal.t('Re-order rows by numerical weight instead of dragging.'),
-        )
+
         .on(
           'click',

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.

andrewmacpherson’s picture

Status: Needs review » Needs work
Karan Sen’s picture

Karan Sen’s picture

Status: Needs work » Needs review

updated comment.

Status: Needs review » Needs work

The last submitted patch, 9: 3023966-remove-title-show-row-weights-9-D8.patch, failed testing. View results

Karan Sen’s picture

Status: Needs work » Needs review
FileSize
1.33 KB

Sorry for the faulty patch, fixed it now. please review.

Status: Needs review » Needs work

The last submitted patch, 12: 3023966-remove-title-show-row-weights-9-D8.patch, failed testing. View results

andrewmacpherson’s picture

Thanks 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:

  1. The update to tabledrag.es6.js is correct
  2. diff --git a/core/misc/tabledrag.js b/core/misc/tabledrag.js
    index 97ead1c..05df370 100644
    --- a/core/misc/tabledrag.js
    +++ b/core/misc/tabledrag.js
    @@ -86,8 +86,8 @@ var _typeof = typeof Symbol === "function" && typeof Symbol.iterator === "symbol
           self.makeDraggable(this);
         });
    
    -    $table.before($('<button type="button" class="link tabledrag-toggle-weight"></button>').attr('title', Drupal.t('Re-order rows by numerical weight instead of dragging.')).on('click', $.proxy(function (e) {
    -      e.preventDefault();
    +    $table.before($('<button type="button" class="link tabledrag-toggle-weight"></button>').on('click', $.proxy(function (e) {
    +       e.preventDefault();

    The indentation of the second line doesn't need to be changed.

  3. The change needed in MediaLibraryWidget is missing in patch #10. It was correctly done in patch #4

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).

pawandubey’s picture

Status: Needs work » Needs review
FileSize
2.01 KB

@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.

andrewmacpherson’s picture

Status: Needs review » Reviewed & tested by the community

Thanks @pawandubey. That's all correct now.

andrewmacpherson’s picture

Karan Sen’s picture

Hi @andrewmacpherson yes i noticed that later when I reviewed my patch, apologies for that. Hope it helps.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 15: 3023966-remove-title-show-row-weights-15.patch, failed testing. View results

pawandubey’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Accessibility +accessibility
alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 9096cb6 and pushed to 8.7.x. Thanks!

  • alexpott committed 9096cb6 on 8.7.x
    Issue #3023966 by Karan Sen, eblue, pawandubey, andrewmacpherson: Remove...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

mgifford’s picture

Issue tags: -accessibility (duplicate tag) +Accessibility

Fixing the accessibility tag.