Problem/Motivation

"More" links simply say "Read more" (or "More"), which is not enough context for people using screenreaders. Specifically, this violates Web Content Accessibility Guideline 2.4.9: "Link Purpose (Link Only)" (for more information, see Understanding Success Criterion 2.4.9).

When @mparker17 manually tested this on 2015-09-03, "Read more" links come out as Read more<span class="visually-hidden"> about Lorem ipsum dolor sit amet</span>.

Exploring the commit history suggests this was initially fixed in #49428: Include node title in "Read more" link for better accessibility and SEO, but that way of generating "Read more" links wasn't used universally until #1026616: Implement an entity render controller unified "Read more" links across all entities and display modes.

It appears that no work remains to be done.

Proposed resolution

Add the title of the destination to the link text, but make it visually hidden, for example:

<a href="/node/1" rel="tag" title="Lorem ipsum dolor sit amet" hreflang="en">Read more<span class="visually-hidden"> (Lorem ipsum dolor sit amet)</span></a>

Remaining tasks

This appears to have been fixed with both #49428: Include node title in "Read more" link for better accessibility and SEO and #1026616: Implement an entity render controller, and it appears that no work remains to be done.

User interface changes

Visually, none.

Screenreaders will see the title of the page being linked to.

API changes

None.

Data model changes

None.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pillarsdotnet’s picture

Everett Zufelt’s picture

@mike

If I pass 'Read more about my fun events' as the title in $variables, then the link text would be: 'More<span class="element-invisible">Read more about my fun events</span>

Perhaps it would be better for title to be repurposed so that it is 'More' by default, and can be overridden.

e.g.

theme('more_link', array('url' => 'foo', 'title' => 'Read more about my fun events'));

Then we can use $variables['title'] for both the link text and the title of the link generated.

Alternatively, we can add 'text' as a key in $variables that defaults to 'More', in which case we might also add 'html' defaulting to false. This way developers can pass all values into theme_more_link() that are necessary for l()

Everett Zufelt’s picture

Status: Active » Needs review
FileSize
1.13 KB

Try this out

Everett Zufelt’s picture

Added doc for new $variables keys and fixed two mistakes.

droplet’s picture

Nice. but why not get it in this way ? more powerful and consistency.

The last submitted patch, 1136680-theme_more_link-5.patch, failed testing.

Everett Zufelt’s picture

Status: Needs work » Needs review
Issue tags: +Accessibility

#5: 1136680-theme_more_link-5.patch queued for re-testing.

Everett Zufelt’s picture

@droplet

I like your patch, let's see if it passes this time.

Status: Needs review » Needs work

The last submitted patch, 1136680-theme_more_link-5.patch, failed testing.

droplet’s picture

Component: block.module » theme system

ahh. forgot to update the test case and other modules.

one problem here if we do in my way #5. It will be more hard to use theme_more function. as now, about all usages in D7 are have TITLE, so $OPTION would get them one more depth.

$read_more = theme('more_link', array('path' => 'aggregator/categories/' . $category->cid, 'title' => t("View this category's recent news.")));

What do you think?

mgifford’s picture

I just skimmed this, but curious how the additional information is going to be hidden for sighted users so that it doesn't clutter up the page.

Initially I'd suggested using element-invisible, but that doesn't seem to be included in yesterday's patches. Think it boils down to #2 but I'm still missing something.

Everett Zufelt’s picture

@droplet

I do think that things would be easier if $variables included a key for 'title' and if it wasn't burried in 'options'. However, things are more consistent if we follow the pattern for l(). I will leave the judgement call up to you, we need to look at all of the usages in core anyway once we get the patch finalized.

@mgifford

This redesign of the theme function means that we are free to pass in whatever we like a both the link text and as the title.

E.g.

$output = theme('more_link', array('text' => 'More<span class="element-invisible"> about foo</span>', 'path' => 'path/to/foo', 'title' => 'More about foo', 'options' => array('html' => TRUE)));
droplet’s picture

for $options, it can do in this way to get in more easy to use:

function theme_more_link($variables) {
  $variables['options']['title'] = $variables['title'];
  return '<div class="more-link">' . l($variables['text'], $variables['path'], $variables['options']) . '</div>';
}

"element-invisible" is for screen reader, it can be same as $variables['title']. and basically designed for image buttons i think.
why we need to hidden some word while the links have own anchor texts?

Everett Zufelt’s picture

@droplet

I like the approach of copying $variables['title'] into $options.

.element-invisible is for screen readers. There are some cases where 'More' is sufficient link text for a user looking at the screen, since it is visually grouped closeley to a list of content, but where it is useful to have 'More comments' or 'More recent users' for screen-reader users. In this case implementers can make the choice on their own.

By default $variables => array('title' => t('More')) will be great.

droplet’s picture

@Everett Zufelt,
thanks for explanation.

im thinking more, $title same to element-invisible is not make sense sometimes.

taken #10 real example:

$read_more = theme('more_link', array('path' => 'aggregator/categories/' . $category->cid, 'title' => t("View this category's recent news.")));

It will be "more about View this category's recent news."

so, add a extra $var for it

function theme_more_link($variables) {
  $variables['options']['html'] = TRUE;
  
  if(isset($variables['title'])){
    $variables['options']['title'] = $variables['title'];
  }

  if(isset(!$variables['element-invisible'])){
    $variables['element-invisible'] = '<span class="element-invisible">' . $variables['options']['title']  . '</span>';
  }

  return '<div class="more-link">' . l($variables['text'] . $variables['element-invisible'], $variables['path'], $variables['options']) . '</div>';
}
mgifford’s picture

Just marked this issue a duplicate & pointed here from #1136686: Aggregator Template Needs Context for Accessibility. It's a specific implementation for aggregation:

<div class="links">
  <a href="<?php print $source_url; ?>"><?php print t('More') . '<span class="element-invisible"> ' . t('about') . ' ' . $title . '</span>'; ?></a>
</div>

that is used in modules/aggregator/aggregator-summary-items.tpl.php

and came up in an accessibility review - http://groups.drupal.org/node/143769#comment-477074

Everett Zufelt’s picture

@droplet

I think the greatest flexibility comes from allowing implementers to set their own element-invisible where required through text

$variables = array(
'text' => 'Read more<span class="element-invisible"> about foo</span>',
'title' => 'View foo',
'options' => array('html' => TRUE,),
);
theme('more_link', $variables);
mgifford’s picture

Making sure that there is a consistent way to pass this through is good. I'm also interested in setting best practices for how it is implemented in D8. Where are the places where we are by default providing enough context for screen readers.

droplet’s picture

it's one will be very close to theme_link. But theme_link use url(), why? performance issue ? #318636: Make l() themable

@Everett Zufelt,

I can see the flexibility but developer should copy <span class="element-invisible"></span> every time.

This way only change texts. when element-invisible provided, html should be TRUE by default.

$variables = array(
'text' => 'Read more
'element-invisible' => about foo,
'title' => 'View foo',
);
theme('more_link', $variables);

.element-invisible used everywhere. Developer can override it themselves if they want it to be different.

all of above is suggested use $title as element-invisible by default. can we use another wordy ?

eg. "about this content"
full output "More <span class="element-invisible">about this content</span>"

some case, we don't need to set element-invisible everytime.

eg. exist node module code:

      $output .= theme('more_link', array('path' => 'admin/content', 'title' => t('Show more content')));
Everett Zufelt’s picture

@droplet

Works for me, but we need a better name than 'element-invisible' for the $variables array key. I have no suggestions.

fgm’s picture

Note this partly duplicates/overlaps with previous issue and patch on #1036190: theme_more_link() has incomplete parameter list.

mgifford’s picture

Status: Needs work » Needs review
FileSize
992 bytes

Can we get this rolled up into a new patch? Hopefully integrating with the issue in #21?

EDIT: Just ignore the patch attached.

Status: Needs review » Needs work

The last submitted patch, theme_more_link-1136680-22.patch, failed testing.

mgifford’s picture

Priority: Normal » Minor
Issue tags: +Drupal SEO

Would be good to do this consistently.

Alan D.’s picture

To avoid additional theming, is it worth toggling the element to handle either DIV or SPAN? For example, the aggregator uses a SPAN, and contrib modules provide inline options too, although I don't know if many of these actually use this due to its' limitations.

mgifford’s picture

Status: Needs work » Needs review
FileSize
992 bytes

Whitespace issues.

Status: Needs review » Needs work

The last submitted patch, theme_more_link-1136680-26.patch, failed testing.

fgm’s picture

Status: Needs work » Needs review
FileSize
992 bytes

Rerolled for tests.

Also, there as a security issue in the original version: if option HTML was not set, the function could not assume that the title was safe, so patch now checks for this if it sets option HTML on its own, based on element-invisible.

Status: Needs review » Needs work

The last submitted patch, theme_more_link-1136680-26.patch, failed testing.

fgm’s picture

Status: Needs work » Needs review
FileSize
2.51 KB

Oops wrong upload.

Status: Needs review » Needs work

The last submitted patch, 0001-Issue-1136680-and-1036190-theme_more_link-should-hav.patch, failed testing.

fgm’s picture

Status: Needs work » Needs review
FileSize
3.08 KB

Actually, we can just as well expose the "options" key in hook_theme() and default it to array().

fgm’s picture

Rerolled on today's HEAD.

mgifford’s picture

That patch here applies nicely. I went to go and test it though and got this error (that I believe is unrelated):

Drupal\Component\Plugin\Exception\PluginException: The plugin (aggregator) did not specify an instance class. in Drupal\Component\Plugin\Factory\DefaultFactory->getPluginClass() (line 60 of /DRUPAL8/core/lib/Drupal/Component/Plugin/Factory/DefaultFactory.php).

I don't think it's related to your patch. Found a similar issue in #1676202: DX: D8 Plugin Type writing

fgm’s picture

Just to be sure it's unrelated, can you reproduce the manipulation that led to the error you mention ? We could then check whether there is a test case for it.

mgifford’s picture

I reset the git repository & cleared cache. Then went here:
admin/config/services/aggregator

Added Drupal Planet and then clicked on "update items" and got that error.

EDIT: Also get this with running cron with Aggregator enabled.

Confirmed that it's got nothing to do with this patch & really should be opened up as a new issue.

mgifford’s picture

Status: Needs review » Needs work
chanderbhushan’s picture


function theme_more_link($variables) {
 return '<div class="more-link">' . l(t('More'), $variables['url'], array('attributes' => array('title' => $variables['title']))) . '</div>';

                               Replace above code with this code i think it will work 

 return '<div class="more-link">' . l($variables['text'], $variables['url'], array('attributes' => array('title' => $variables['title'])), $variables['html']) . '</div>';
 }
jaffaralia’s picture

Status: Needs work » Needs review
FileSize
1.62 KB

Here i attached the patch

Status: Needs review » Needs work
mgifford’s picture

Seems that we need tests to allow forum topics block has a "more"-link in ForumBlockTest.php on line 66 & 126.

mgifford’s picture

Issue tags: +Need tests

tagging

mgifford’s picture

Status: Needs work » Needs review
Issue tags: -Need tests
FileSize
3.66 KB
mgifford’s picture

Issue tags: -Accessibility, -Drupal SEO
mgifford’s picture

Issue tags: +Accessibility, +Drupal SEO
mgifford’s picture

FileSize
4.85 KB

Interdiff between @jaffaralia's in 40 & mine in 44.

mgifford’s picture

Issue tags: -Accessibility, -Drupal SEO
mparker17’s picture

mgifford’s picture

Issue tags: +Accessibility, +Drupal SEO
mgifford’s picture

removing element-invisible

mgifford’s picture

Issue tags: -Accessibility, -Drupal SEO

Status: Needs review » Needs work

The last submitted patch, theme_more_link-should-hav-1136680-51.patch, failed testing.

mgifford’s picture

Issue summary: View changes
Issue tags: +Needs tests
mgifford’s picture

Status: Needs work » Needs review
FileSize
3.62 KB

still needs tests, but should apply.

Status: Needs review » Needs work

The last submitted patch, 55: theme_more_link-should-hav-1136680-55.patch, failed testing.

joelpittet’s picture

  1. +++ b/core/includes/theme.inc
    @@ -1283,8 +1283,17 @@ function template_preprocess_image(&$variables) {
    -    if (isset($variables[$key])) {
    -      $variables['attributes'][$key] = $variables[$key];
    +    // At least 'alt' defaults to an empty string; allow to omit an attribute by
    +    // overriding the variable to NULL.
    +    if (array_key_exists($key, $variables)) {
    +      // If there is a value for the variable, it overrides the attribute.
    +      if (isset($variables[$key])) {
    +        $attributes[$key] = $variables[$key];
    +      }
    +      // Otherwise, remove the attribute.
    +      else {
    +        unset($attributes[$key]);
    +      }
    

    This change seems out of scope of the issue summary.

  2. +++ b/core/includes/theme.inc
    @@ -1772,9 +1781,37 @@ function template_preprocess_feed_icon(&$variables) {
    +  *   - options: A l()-compatible options array. Extra subkey:
    

    We are trying to avoid l() function in core for render cache, so maybe this doesn't need to be referenced here and the indent is incorrect.

  3. +++ b/core/includes/theme.inc
    @@ -1772,9 +1781,37 @@ function template_preprocess_feed_icon(&$variables) {
    + *     - visually-hidden: boolean for screen readers. Will set HTML option.
    

    The indent looks incorrect here and boolean should be title case.

Is this going to work with #type link as well?

RavindraSingh’s picture

Assigned: Unassigned » RavindraSingh
Issue tags: +Needs reroll
RavindraSingh’s picture

Assigned: RavindraSingh » Unassigned

This issue completely needs beta valuation. I code doesn't validated with latest API.

fgm’s picture

theme_more_link() has been replaced by #type 'more_link' : see https://www.drupal.org/node/2281851 or the issue where it happened: #2031301: Replace theme_more_link() and replace with #type 'more_link', so these changes need to be applied differently.

mgifford’s picture

Ok, so the logic in this patch needs to move to core/lib/Drupal/Core/Render/Element/MoreLink.php into the class MoreLink.

mgifford’s picture

Title: theme_more_link should have more context » #type 'more_link' - previously theme_more_link() - should have more context
joelpittet’s picture

@mgifford thanks for giving the title more context.

mparker17’s picture

Assigned: Unassigned » mparker17
Issue tags: +Needs reroll, +Needs beta evaluation

Patch in #55 needs re-roll. Assigning to myself to do that.

This patch also needs beta evaluation.

mparker17’s picture

Hmm... this is not easily re-rolled, because theme_more_link() no longer exists.

Also, it seems that, between #40 and #44, some changes to theme_image() snuck in. I don't think these are related, but I'm going to spend some time reading through all the comments in detail and updating the issue summary, before writing a new patch.

mparker17’s picture

When I actually tested this, I discovered that it appears to already be fixed... as of 2015-09-03, "Read more" links come out as Read more<span class="visually-hidden"> about Lorem ipsum dolor sit amet</span>.

Exploring the commit history suggests this was initially fixed in #49428: Include node title in "Read more" link for better accessibility and SEO, but that way of generating "Read more" links wasn't used universally until #1026616: Implement an entity render controller unified "Read more" links across all entities and display modes.

It appears that no work remains to be done, removing Needs reroll, Needs beta evaluation tags, unassigning myself, and marking the ticket as "Closed (duplicate)".

This functionality has tests in core/modules/node/src/NodeViewBuilder.php, removing Needs tests tag.

Issue summary updated in this ticket, removing Needs issue summary update tag.

Adding related issue #1036190: theme_more_link() has incomplete parameter list as per #21