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.
Comment | File | Size | Author |
---|
Comments
Comment #1
pillarsdotnet CreditAttribution: pillarsdotnet commentedComment #2
Everett Zufelt CreditAttribution: Everett Zufelt commented@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()
Comment #3
Everett Zufelt CreditAttribution: Everett Zufelt commentedTry this out
Comment #4
Everett Zufelt CreditAttribution: Everett Zufelt commentedAdded doc for new $variables keys and fixed two mistakes.
Comment #5
droplet CreditAttribution: droplet commentedNice. but why not get it in this way ? more powerful and consistency.
Comment #7
Everett Zufelt CreditAttribution: Everett Zufelt commented#5: 1136680-theme_more_link-5.patch queued for re-testing.
Comment #8
Everett Zufelt CreditAttribution: Everett Zufelt commented@droplet
I like your patch, let's see if it passes this time.
Comment #10
droplet CreditAttribution: droplet commentedahh. 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.
What do you think?
Comment #11
mgiffordI 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.
Comment #12
Everett Zufelt CreditAttribution: Everett Zufelt commented@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.
Comment #13
droplet CreditAttribution: droplet commentedfor $options, it can do in this way to get in more easy to use:
"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?
Comment #14
Everett Zufelt CreditAttribution: Everett Zufelt commented@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.
Comment #15
droplet CreditAttribution: droplet commented@Everett Zufelt,
thanks for explanation.
im thinking more, $title same to element-invisible is not make sense sometimes.
taken #10 real example:
It will be "more about View this category's recent news."
so, add a extra $var for it
Comment #16
mgiffordJust marked this issue a duplicate & pointed here from #1136686: Aggregator Template Needs Context for Accessibility. It's a specific implementation for aggregation:
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
Comment #17
Everett Zufelt CreditAttribution: Everett Zufelt commented@droplet
I think the greatest flexibility comes from allowing implementers to set their own element-invisible where required through text
Comment #18
mgiffordMaking 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.
Comment #19
droplet CreditAttribution: droplet commentedit'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.
.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:
Comment #20
Everett Zufelt CreditAttribution: Everett Zufelt commented@droplet
Works for me, but we need a better name than 'element-invisible' for the $variables array key. I have no suggestions.
Comment #21
fgmNote this partly duplicates/overlaps with previous issue and patch on #1036190: theme_more_link() has incomplete parameter list.
Comment #22
mgiffordCan we get this rolled up into a new patch? Hopefully integrating with the issue in #21?
EDIT: Just ignore the patch attached.
Comment #24
mgiffordWould be good to do this consistently.
Comment #25
Alan D. CreditAttribution: Alan D. commentedTo 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.
Comment #26
mgiffordWhitespace issues.
Comment #28
fgmRerolled 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.
Comment #30
fgmOops wrong upload.
Comment #32
fgmActually, we can just as well expose the "options" key in hook_theme() and default it to array().
Comment #33
fgmRerolled on today's HEAD.
Comment #34
mgiffordThat 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
Comment #35
fgmJust 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.
Comment #36
mgiffordI 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.
Comment #37
mgiffordre-rolled patch.
Comment #39
chanderbhushan CreditAttribution: chanderbhushan commentedComment #40
jaffaralia CreditAttribution: jaffaralia commentedHere i attached the patch
Comment #42
mgiffordSeems that we need tests to allow forum topics block has a "more"-link in ForumBlockTest.php on line 66 & 126.
Comment #43
mgiffordtagging
Comment #44
mgiffordComment #45
mgifford#44: theme_more_link-should-hav-1136680-44.patch queued for re-testing.
Comment #46
mgifford#44: theme_more_link-should-hav-1136680-44.patch queued for re-testing.
Comment #47
mgiffordInterdiff between @jaffaralia's in 40 & mine in 44.
Comment #48
mgifford#44: theme_more_link-should-hav-1136680-44.patch queued for re-testing.
Comment #49
mparker17#44: theme_more_link-should-hav-1136680-44.patch queued for re-testing.
Comment #50
mgifford#44: theme_more_link-should-hav-1136680-44.patch queued for re-testing.
Comment #51
mgiffordremoving element-invisible
Comment #52
mgifford#51: theme_more_link-should-hav-1136680-51.patch queued for re-testing.
Comment #54
mgiffordComment #55
mgiffordstill needs tests, but should apply.
Comment #57
joelpittetThis change seems out of scope of the issue summary.
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.
The indent looks incorrect here and boolean should be title case.
Is this going to work with #type link as well?
Comment #58
RavindraSingh CreditAttribution: RavindraSingh as a volunteer and at Srijan | A Material+ Company commentedComment #59
RavindraSingh CreditAttribution: RavindraSingh as a volunteer and at Srijan | A Material+ Company commentedThis issue completely needs beta valuation. I code doesn't validated with latest API.
Comment #60
fgmtheme_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.
Comment #61
mgiffordOk, so the logic in this patch needs to move to core/lib/Drupal/Core/Render/Element/MoreLink.php into the class MoreLink.
Comment #62
mgiffordComment #63
joelpittet@mgifford thanks for giving the title more context.
Comment #64
mparker17Patch in #55 needs re-roll. Assigning to myself to do that.
This patch also needs beta evaluation.
Comment #65
mparker17Hmm... 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.Comment #66
mparker17When 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
, removingNeeds 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