Problem/Motivation

When the link field was moved to core (see
#501434: Move Link/URL field type into core
), support for the link title attribute hadn’t been included. Currently, the link field uses "link title" to indicate the link anchor text.

Proposed resolution

Rename the current "link title" occurrences to something different and introduce support for the "true" link title attribute.

Remaining tasks

Make the patch pass tests and write new test for the "true" link title attribute, as well ass figure out the proper string to refer to the link anchor text and get answer to these questions:

  1. Can we consolidate the two widget form validation handlers (link_field_widget_validate_anchor_text() and link_field_widget_validate_title()) into an unique one?
  2. A very bad practice for accessibility that I often find is setting the title attribute value to the same value of the link anchor text. Can we "enforce" link title to be different from link anchor text during widget form validation? Or should we handle it in a follow up?
  3. Should we store the value for the link title attribute in a separate schema field (patch behaves this way now) or in the serialized attributes schema field?

And finally the patch should be reviewed when it is complete!

User interface changes

The patch renames the old link title to "anchor text" and link title refers to the "true" link title attribute now.

API changes

Support for the link title attribute is added.

Original report by falcon03

To solve
http://drupal.org/node/1801268
we need to support the link title attribute. It's usage is described here:
http://www.w3.org/TR/2008/WD-WCAG20-TECHS-20081103/H33
(information on screen reader support is a bit out of date).
To do this, however, we need to rename the current "link title".
I am not expert at writing patches, but I tried to write one for this issue:
- Renamed "link title" to "anchor text" (we need to figure out the exact string to use);
- Introduced support for the "link title";
- Fixed url validation test to use anchor text;
The work is not completed yet, since I am not able to introduce support for the link title in link_field_formatter_view() and determine if we need additional tests (and, if so, if we can merge anchor text tests and link title tests into an unique test case).
I also have a more technical question: do we really need to validate the mandatory of the (current) anchor text and (current) link title via two separate functions?

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Status: Needs review » Needs work

The last submitted patch, link-anchor-text-and-title.patch, failed testing.

falcon03’s picture

Status: Needs work » Needs review
FileSize
11.84 KB

Another patch fixing two errors. The "anchor text" field still doesn't show in the widget form and gives two notices (no existing index "anchor_text").

Status: Needs review » Needs work

The last submitted patch, link-anchor-text-and-title-3.patch, failed testing.

falcon03’s picture

Assigned: Unassigned » falcon03
Status: Needs work » Needs review
FileSize
11.93 KB

The link widget form now works properly and the two notices went away! :D

Now I am trying to understand how to tell the link formatters to use the title attribute. Meanwhile, let's see what bot thinks of the current patch!

Status: Needs review » Needs work

The last submitted patch, link-anchor-text-and-title-4.patch, failed testing.

mradcliffe’s picture

Yes, the tests need to be fixed in Link field as its still trying to assert title instead of anchor text, and also update to test Field UI (admin) in Link Field UI.

I would first start by updating the test wherever you see field_create_field() or field_create_instance() and the equivalent form tests.

Nice job so far.

falcon03’s picture

@mradcliffe: thank you very much for the help! :D

Do you have any idea on how to make drupal render the title attribute when displaying the value of a link field? I am looking over and over at the code, but I am not finding any good solution...

mradcliffe’s picture

As in use the HTML title attribute for anchor tags? Or switch between using title and anchor title?

falcon03’s picture

I was referring to the title attribute in the anchor tag.

mradcliffe’s picture

You can add HTML attributes to links in the $options array in l(), url(), or theme_link(). Link module should already be doing something similar for the placeholder attribute.

In Line 239 (patched), the $options array for link is defined. The attributes come directly from what's stored in the database for that field in the serialized FIELD_NAME_attributes column. So since title attribute is stored separately the attributes array needs to be modified.

function link_field_prepare_view($entity_type, $entities, $field, $instances, $langcode, &$items) {
  foreach ($entities as $id => $entity) {
    foreach ($items[$id] as $delta => &$item) {
      // Split out the link into the parts required for url(): path and options.
      $parsed = drupal_parse_url($item['url']);
      $item['path'] = $parsed['path'];
      $item['options'] = array(
        'query' => $parsed['query'],
        'fragment' => $parsed['fragment'],
        'attributes' => &$item['attributes'],
      );  
      // $item['options']['attributes'][] = $item['link_title']; // This would modify $item, but may not matter..?
    }   
  }
}

Basically, if you add to the $options['attributes'] array, then it should apply the attribute to the tag via Attribute (formerly drupal_attributes()).

Oh, also, in link.install, you should fix the field schema for title attribute and anchor text to not be the same:

  $schema['columns']['anchor_text'] = array(
    'description' => 'The link anchor text.',
    'type' => 'varchar',
    'length' => 255,
    'not null' => FALSE,
  );    
  $schema['columns']['anchor_text'] = array(
    'description' => 'The link title.',
    'type' => 'varchar',
    'length' => 255,
    'not null' => FALSE,
  ); 
falcon03’s picture

Status: Needs work » Needs review

Thanks to mradcliffe's help in #10, the title attribute value is now rendered properly! Yeah!

Now I have some questions:

  1. Do we need two separate test cases for link anchor text and link title? Or can we merge them into an unique one?
  2. Can we consolidate the two widget form validation handlers into an unique one?
  3. A very bad practice for accessibility that I often find is setting the title attribute value to the same value of the link anchor text. Can we "enforce" link title to be different from link anchor text during widget form validation? Or should we handle it in a follow up (of course I can write the validation rule! :D)?

Let's see what bot thinks: I am not able to run tests locally due to an Ajax error :(

falcon03’s picture

Sorry, I forgot the patch! :-)

Status: Needs review » Needs work

The last submitted patch, link-anchor-text-and-title-11.patch, failed testing.

mradcliffe’s picture

The next step should be to look at core/modules/link/lib/Drupal/link/Tests/LinkFieldTest.php and update those tests to include the new field settings. The tests fail because the test assert that a form element should be there when it is not.

As an example, in the following code snippet from LinkFieldTest.php, SimpleTest needs to test the form fields and new expected output. Before this snippet on line 190, the field and field instance are created programmatically. The new settings and such need to be updated there as well.

    // @mradcliffe: this comment should be fixed ;-)
    // Verify that a link without title is rendered using the URL as link text.
    $value = 'http://www.example.com/';
    $edit = array(
      "{$this->field['field_name']}[$langcode][0][url]" => $value,
      // @mradcliffe: this needs to be adjusted for the new link title attribute and anchor text element names.
      "{$this->field['field_name']}[$langcode][0][title]" => '', 
    );  
    $this->drupalPost(NULL, $edit, t('Save'));
    preg_match('|test-entity/manage/(\d+)/edit|', $this->url, $match);
    $id = $match[1];
    $this->assertRaw(t('test_entity @id has been created.', array('@id' => $id)));

    $this->renderTestEntity($id);
    // @mradcliffe: This is expecting <a href="http://www.example.com">http://www.example.com</a>, but now
    // it should be <a href="http://www.example.com" title="">http://www.example.com</a> because title attribute is set
    // to an empty string above. It needs an $options array to be passed to l() with the title attribute.
    $expected_link = l($value, $value);
    $this->assertRaw($expected_link);
falcon03’s picture

Status: Needs work » Needs review
FileSize
26.84 KB

Here we go: updated tests for Anchor text.

Still not sure of what test coverage to add to check the "true" link title functionality.

I also need answers for questions in #11.

Status: Needs review » Needs work
Issue tags: -Usability, -Accessibility

The last submitted patch, link-title-and-anchor-text-15.patch, failed testing.

falcon03’s picture

Status: Needs work » Needs review
Issue tags: +Usability, +Accessibility
falcon03’s picture

Let's try this re-roll. I hope it wil pass tests!

BTW, why testbot says "detected an invalid patch url" and "detected an invalid patch format"? Isn't this a p1 format patch (I am not experienced at using git :D).

Status: Needs review » Needs work

The last submitted patch, link-anchor-text-and-title-18.patch, failed testing.

mradcliffe’s picture

I think, most likely, your local branch of Drupal 8.x is out-of-date. There are several ways to update your local branch from a remote branch:

  1. git fetch origin; git merge --ff-only origin/8.x (This fetches from remote repository and then does a fast-forward only merge).
  2. git pull (This does the same as the above, but may create a "merge commit").
  3. git fetch origin; git rebase origin/8.x
  4. git pull --rebase (this does the same as above).

If you're not tracking your changes locally (i.e. making local commits), then this might have some issues. You could either commit first, and then do the above, or use git stash.

Most of these should be available from the Git context menu on Windows as well.

mradcliffe’s picture

Status: Needs work » Needs review
FileSize
11.93 KB

Here's a re-roll of your patch based on latest 8.x changes.

Status: Needs review » Needs work

The last submitted patch, link-anchor-text-and-title-19.patch, failed testing.

falcon03’s picture

Maybe we have to deal with a testbot issue. Here's what I get trying to run tests locally (the issue hasn't been solved by fetching the origin):

An AJAX HTTP error occurred. HTTP Result Code: 200 Debugging information follows. Path: /repo/batch?id=3&op=do_nojs&op=do StatusText: OK ResponseText: Fatal error: Cannot access protected property Drupal\field_test\Plugin\Core\Entity\TestEntity::$isDefaultRevision in /Applications/MAMP/htdocs/repo/core/lib/Drupal/Core/Database/StatementPrefetch.php on line 301

The PHP maximum execution time is set to 60 seconds and the memory limit is set to 64M.

falcon03’s picture

Assigned: falcon03 » Unassigned

School and homework come back again, so I don't know when I'll work again on this patch. Unassigning from myself in case someone else can work on it before than me!

mgifford’s picture

Issue tags: +Needs tests

Tagging.

mgifford’s picture

Status: Needs work » Needs review
FileSize
11.98 KB

reroll

Status: Needs review » Needs work

The last submitted patch, link-anchor-text-and-title-26.patch, failed testing.

mradcliffe’s picture

Okay, looks like this is at the same state as it was in #15-#20 now.

mgifford’s picture

What did I loose in #21? So damn hard to keep up with Core & satisfy the Bot. Hadn't meant to revert functionality.

mradcliffe’s picture

Err, sorry, that was my fault. I meant to #21.

mgifford’s picture

Yup. The very same "34 fail(s), and 18 exception(s)."

We've got to start editing the tests that are failing unfortunately.

mgifford’s picture

Status: Needs work » Needs review
FileSize
12.22 KB

Ok, I've pushed it ahead a bit more now. I hadn't realized the patch didn't work. Now you can at least save the title. Note there was a DB change so testing this you might need to remove the link module & re-enable it.

I'm still not seeing the output though. The code looks like:

<div class="field-item even"><a href="http://ox.ca/2">silly anchor 2</a></div>

And I can't seem to get the title to express itself. I'm a bit closer, but the option isn't showing up. Looked at the API - http://api.drupal.org/api/drupal/core!includes!common.inc/function/l/8

I'm seeing these errors:

User error: "title" is an invalid render array key in element_children() (line 6074 of core/includes/common.inc).
Notice: Undefined variable: entity_type in link_field_formatter_view() (line 401 of core/modules/link/link.module).

But when I look at the output of $element in link_field_formatter_view() it looks like the options are properly presented. Unless it needs to be in the fragment..
Array ( [0] => Array ( [#type] => link [#title] => silly anchor 2 [#href] => http://ox.ca/2 [#options] => Array ( [query] => Array ( ) [fragment] => [attributes] => Array ( ) ) [options] => Array ( [attributes] => Array ( [title] => title attribute 2 ) ) ) )

Anyways, partly need a placeholder for this. May not be able to get back to it today.

mgifford’s picture

Ok, this one works, just a matter of dealing with the tests now.

Status: Needs review » Needs work

The last submitted patch, link-anchor-text-and-title-33.patch, failed testing.

mgifford’s picture

Status: Needs work » Needs review
FileSize
11.62 KB

Some minor progress. Down to 27 fails I think. Least locally.

I'm assuming that there's nothing quirky with SimpleTests that would cause this not to work 'settings' => array('placeholder_anchor_text' => 'Enter anchor text for this link',

Is there a trick associated with adding a title attribute into a assertRaw statement? Should be something like $link_attributes['#options']['attributes']['title'] = "example title";

It does very much feel like stumbling around in the dark & pulling leavers. I left in some comments so I can remember where I bumped my head last time.

Status: Needs review » Needs work

The last submitted patch, link-anchor-text-and-title-35.patch, failed testing.

jessebeach’s picture

FileSize
12.12 KB

Rerolled on current D8 HEAD (466fac1e6819d10b4dd8827254628ec5fd3204c7).

The error starts on line 207 of LinkFieldTest.php

preg_match('|test-entity/manage/(\d+)/edit|', $this->url, $match);
$id = $match[1];
$this->assertRaw(t('test_entity @id has been created.', array('@id' => $id)));

The $match array doesn't have a value at index 1. It causes a cascade of errors. Either the regex in preg_match is wrong, or $this->url really doesn't have any digits in it after /manage. I can't seem to get a breakpoint to catch and I can't dump the vars here, so I have no way to know what the variable values are at this point in the code execution.

mgifford’s picture

Status: Needs work » Needs review

Sounds like it's going to fail again, but would like to see if there are any changes in how the bot tests this.

Thanks Jesse. Hopefully we can find someone to look at this and address the failing patch.

Status: Needs review » Needs work

The last submitted patch, link-title-1874354-37.patch, failed testing.

falcon03’s picture

Ok, it seems that patch in #33 takes care of link.module and link.install (and not of tests), while patch in #35 takes care o tests (and not of link.module and link.install). Or at least I understood this looking at the two patches.

So... The obvious solution is that... We should merge these two patches and re-roll the resulting patch on latest head!

So I have to ask if someone could take care of this merge/re-roll... Or give me guidance on how to do it! :-) (most of all I need help merging the two patches).

falcon03’s picture

Status: Needs work » Needs review
FileSize
23.83 KB

Ok, let's see if I did the merge/reroll properly.

Can someone try to apply the patch to see if it can be applied successfully?

Status: Needs review » Needs work

The last submitted patch, link-title-and-anchor-text-41.patch, failed testing.

mgifford’s picture

Applied latest patch just fine in SimplyTest.me just now.

falcon03’s picture

Status: Needs work » Needs review
Issue tags: -Usability, -Needs tests
FileSize
28.82 KB

Updated some tests. Maybe we have less fails to work on.. I don't expect this patch to pass, though.

But I have no idea on how to fix eventual failures at this time.

@jessebeach: could you help me with the problems you explained in #37 if they aren't solved with this patch?

falcon03’s picture

Issue tags: +Usability, +Needs tests

Sorry, I didn't mean to remove tags.

I forgot to say: once the patch passes tests I will add new tests for the title attribute.

Status: Needs review » Needs work

The last submitted patch, link-anchor-text-title-44.patch, failed testing.

jessebeach’s picture

@jessebeach: could you help me with the problems you explained in #37 if they aren't solved with this patch?

I'll try!

falcon03’s picture

Issue tags: +Needs usability review

I added an issue summary based on the standard issue template.

Also tagging as "needs usability review" to get help figuring out the proper string to refer to the link anchor text.

jessebeach’s picture

Just wanted to give a quick update. This issue is in my to do queue. It's just behind a couple other larger, time sensitive issues. The biggest one I'm wrestling with now is #1913086: Generalize the overlay tabbing management into a utility library. I'm making good headway on it. After I've got a patch up in that issue, I'll be able to turn my attention here.

sun’s picture

"Anchor text" is very techy. The average Joe does not know what an "anchor" is. Please rename to "Link text".

Along the same lines, "Link title" is confusingly ambiguous. I'd suggest to use a wording like "Tooltip" or "Summary".

Ultimately though, I'm not sure whether an input for the 'title' HTML attribute should be supported by core.

The only precedent we have is Image module's Image field widget, which optionally supports 'alt' and 'title' field widget attributes.

falcon03’s picture

@sun:

"Anchor text" is very techy. The average Joe does not know what an "anchor" is. Please rename to "Link text".

Along the same lines, "Link title" is confusingly ambiguous. I'd suggest to use a wording like "Tooltip" or "Summary".

Thanks for your comment. It will be done in the next re-roll of the patch.


Ultimately though, I'm not sure whether an input for the 'title' HTML attribute should be supported by core.

The only precedent we have is Image module's Image field widget, which optionally supports 'alt' and 'title' field widget attributes.

Unfortunately I disagree. When screen readers will have better support for the link title attribute it would allow everyone to increase links accessibility very much (e.g. when a link open the page in a new window). So I think that link title needs to be in core as well as image Alt and Title attributes.

mgifford’s picture

We added title attributes to the D7 admin menu. I'd have to look up the exact discussion about that for a comparison.

I decided to see what I could find on the subject in 2013 and found two blogs talking about how it was no longer useful for accessibility:
http://blog.paciellogroup.com/2013/01/using-the-html-title-attribute-upd...
http://blog.silktide.com/2013/01/i-thought-title-text-improved-accessibi...

@Steve Faulkner also points out that it doesn't work well for mobile users either.

We do need to see that we have a solution that we think will work well for 2014-2020. Hard to figure out in 2013 mind you.

@falcon03 can you give us some examples of how this would be useful to you?

mgifford’s picture

Also if our goal is essentially tooltip functionality for links, maybe we should be looking at including something like:
http://hanshillen.github.com/jqtest/#goto_tooltip
http://wiki.jqueryui.com/w/page/12138112/Tooltip

falcon03’s picture

Well, I have always thought that the title attribute worked properly as a tooltip for sighted users. But this is not the case, it seems.

In addition to this, let's consider that the only screen reader that uses the title attribute properly is Voiceover (built-in Mac OS X screen reader).

I am starting to be inclined to mark this "Won't fix"...

@mgifford: Two days ago I was surfing the JQuery UI website and I was reading about the tooltip component. But I think we'd better deal with it at #1919940: Build API to Replace Links using Title Attributes with Proper Accessible, Themable Tooltips, but I am not sure we are on time for D8.
BTW, thank you very much for linking to Steve Faulkner's blog posts. I knew only the second one. As you said, it is very hard to understand what will work in the next years and what won't. It would be great if you could link the discussion about admin_menu links title attribute too.

mradcliffe’s picture

If this is won't fix, then the branched off issue, #1801268: Change link field 'title' field to 'link text', still needs to be addressed with regard to a confusing UI. The original plan there was to change the form label of the link field title.

falcon03’s picture

@mradcliffe: yes, you're right. But if we rename link title to e.g. link text, then IMO we should rename the #title property when you use something like #type = "link" (sorry, I didn't know how to explain it better; let me know if the explanation is not clear and I will provide a code snippet). Anyway, I am not sure about what we should do here.

mgifford’s picture

Status: Needs work » Closed (duplicate)

On the issue of usability & Tooltips I'd suggest we move the discussion over to #1919940: Build API to Replace Links using Title Attributes with Proper Accessible, Themable Tooltips

And yes, #1801268: Change link field 'title' field to 'link text' should still get addressed too.

So I'm going to mark this as a duplicate rather than Won't fix.

mgifford’s picture

Issue summary: View changes

Adding issue summary.