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:
- 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?
- 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?
- 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?
Comment | File | Size | Author |
---|---|---|---|
#44 | link-anchor-text-title-44.patch | 28.82 KB | falcon03 |
#41 | link-title-and-anchor-text-41.patch | 23.83 KB | falcon03 |
#37 | link-title-1874354-37.patch | 12.12 KB | jessebeach |
#35 | link-anchor-text-and-title-35.patch | 11.62 KB | mgifford |
#33 | link-anchor-text-and-title-33.patch | 12.2 KB | mgifford |
Comments
Comment #2
falcon03 CreditAttribution: falcon03 commentedAnother 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").
Comment #4
falcon03 CreditAttribution: falcon03 commentedThe 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!
Comment #6
mradcliffeYes, 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.
Comment #7
falcon03 CreditAttribution: falcon03 commented@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...
Comment #8
mradcliffeAs in use the HTML title attribute for anchor tags? Or switch between using title and anchor title?
Comment #9
falcon03 CreditAttribution: falcon03 commentedI was referring to the title attribute in the anchor tag.
Comment #10
mradcliffeYou 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.
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:
Comment #11
falcon03 CreditAttribution: falcon03 commentedThanks to mradcliffe's help in #10, the title attribute value is now rendered properly! Yeah!
Now I have some questions:
Let's see what bot thinks: I am not able to run tests locally due to an Ajax error :(
Comment #12
falcon03 CreditAttribution: falcon03 commentedSorry, I forgot the patch! :-)
Comment #14
mradcliffeThe 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.
Comment #15
falcon03 CreditAttribution: falcon03 commentedHere 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.
Comment #17
falcon03 CreditAttribution: falcon03 commented#15: link-title-and-anchor-text-15.patch queued for re-testing.
Comment #18
falcon03 CreditAttribution: falcon03 commentedLet'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).
Comment #20
mradcliffeI 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:
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.
Comment #21
mradcliffeHere's a re-roll of your patch based on latest 8.x changes.
Comment #23
falcon03 CreditAttribution: falcon03 commentedMaybe 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.
Comment #24
falcon03 CreditAttribution: falcon03 commentedSchool 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!
Comment #25
mgiffordTagging.
Comment #26
mgiffordreroll
Comment #28
mradcliffeOkay, looks like this is at the same state as it was in #15-#20 now.
Comment #29
mgiffordWhat did I loose in #21? So damn hard to keep up with Core & satisfy the Bot. Hadn't meant to revert functionality.
Comment #30
mradcliffeErr, sorry, that was my fault. I meant to #21.
Comment #31
mgiffordYup. The very same "34 fail(s), and 18 exception(s)."
We've got to start editing the tests that are failing unfortunately.
Comment #32
mgiffordOk, 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:
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.
Comment #33
mgiffordOk, this one works, just a matter of dealing with the tests now.
Comment #35
mgiffordSome 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.
Comment #37
jessebeach CreditAttribution: jessebeach commentedRerolled on current D8 HEAD (466fac1e6819d10b4dd8827254628ec5fd3204c7).
The error starts on line 207 of
LinkFieldTest.php
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.Comment #38
mgiffordSounds 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.
Comment #40
falcon03 CreditAttribution: falcon03 commentedOk, 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).
Comment #41
falcon03 CreditAttribution: falcon03 commentedOk, 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?
Comment #43
mgiffordApplied latest patch just fine in SimplyTest.me just now.
Comment #44
falcon03 CreditAttribution: falcon03 commentedUpdated 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?
Comment #45
falcon03 CreditAttribution: falcon03 commentedSorry, 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.
Comment #47
jessebeach CreditAttribution: jessebeach commentedI'll try!
Comment #48
falcon03 CreditAttribution: falcon03 commentedI 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.
Comment #49
jessebeach CreditAttribution: jessebeach commentedJust 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.
Comment #50
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".
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.
Comment #51
falcon03 CreditAttribution: falcon03 commented@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.
Comment #52
mgiffordWe 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?
Comment #53
mgiffordAlso 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
Comment #54
falcon03 CreditAttribution: falcon03 commentedWell, 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.
Comment #55
mradcliffeIf 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.
Comment #56
falcon03 CreditAttribution: falcon03 commented@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.
Comment #57
mgiffordOn 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.
Comment #57.0
mgiffordAdding issue summary.