Here's a very simple patch...
I noticed that the default pager's links were missing rel
attributes, which can be useful if you happened to have become addicted to some Site Navigator browser extension... With this Drupal 5.0 patch, the pager will make the browser aware of first
, prev
, next
and last
links, where that feature is available, of course...
Included with this post is a diff file for the includes/pager.inc script I generated from winMerge. I'm sorry I couldn't post a more standard compliant patch. As I'm still just getting acquainted with Drupal's internals, I haven't set up my installation under a full development environment.
I hope it helps more than it annoys...
Comment | File | Size | Author |
---|---|---|---|
#52 | rel-115753-52.patch | 2.32 KB | mgifford |
#42 | rel-115753-42.patch | 1.27 KB | mgifford |
#34 | pager_rel_attr-115753-34.patch | 5.37 KB | drclaw |
#27 | pager_rel_attr-115753-do-not-test.patch | 2.21 KB | drclaw |
#26 | pager_rel_attr-115753.patch | 2.23 KB | drclaw |
Comments
Comment #1
cosmicdreams CreditAttribution: cosmicdreams commentedhas this change been implemented in Drupal already? I'm going to triage this out to the drupal project so that it can be properly addressed.
Comment #2
StevenPatzSet to review for more eyes to look at.
Comment #3
geodaniel CreditAttribution: geodaniel commentedHere's an updated patch against 6.0b2.
I'm not sure this has the right behaviour when there are only two pages though. Right now, on the first page, the link to the second page should be the same as 'next' but both are tagged with rel last, which is also true, but perhaps not expected behaviour?
Comment #4
PanchoThis is a small but good improvement. However we need to fix the problem geodaniel points us to in #3.
Comment #5
lilou CreditAttribution: lilou commented+1
Comment #6
lilou CreditAttribution: lilou commentedIt would be nice to also add
<link rel="prev">
and<link rel="next">
to the<head>
like in book module.See also : http://www.w3.org/TR/REC-html40/types.html#type-links and #35153: META LINK REL support. New feature? New Module?
Comment #7
cosmicdreams CreditAttribution: cosmicdreams commentedThe way the book module handles these kind of attributes is described here: http://api.drupal.org/api/function/template_preprocess_book_navigation/7
Perhaps we should approach this the same way. For Drupal 7 the API for theme_pager_next has changed. The new API supports the passing in of any kind of extra attribute though the parameters variable. http://api.drupal.org/api/function/theme_pager_next/7
I'll see if I can get a patch in for the administrative theme Seven as a starting point for further discussion.
Comment #8
cosmicdreams CreditAttribution: cosmicdreams commentedAre there any cases where we don't want the extra attributes on these pager links?
Comment #9
cosmicdreams CreditAttribution: cosmicdreams commentedI look into how nodes and polls call upon the pager and there doesn't seem to be an opprotunity to pass into pager additional attributes for the links that are generated. Which means I could attempt to implement a pre-process function to the themes templates OR attempt to modify pager.inc to include the extra attributes for links.
I'll try to do both patches and see which one gathers support.
Comment #10
cosmicdreams CreditAttribution: cosmicdreams commentedsorry, looks like there were some commits before last patch.
Comment #11
cosmicdreams CreditAttribution: cosmicdreams commentedHere's an example implementation of it is possible to put this code in the pre-process stage for a specific theme. Please, let me know if I'm on the right path here.
Comment #12
cosmicdreams CreditAttribution: cosmicdreams commentedI don't understand why my patches are being ignored by the test bot.
Comment #13
lilou CreditAttribution: lilou commentedComment #14
cosmicdreams CreditAttribution: cosmicdreams commented#11: pager-rel3.patch queued for re-testing.
Comment #15
Dries CreditAttribution: Dries commentedAny reason why we do this in Seven, and not in the base implementation? We should try to add this to every theme by tackling this at a higher level IMO.
Comment #16
cosmicdreams CreditAttribution: cosmicdreams commentedNo reason, I've just been focusing on Seven. I'll see if I can generalize this patch.
Comment #17
cosmicdreams CreditAttribution: cosmicdreams commentedDries, are you suggesting that I modify the /misc/theme.inc or the /sites/all/themes/engines/phptemplate.engine when you ask me to look at moving this patch to a higher level?
To me both places seem to be poor places for this patch. theme.inc already provides the functions that can generate the pager links as we need them to here. And phptemplate.engine seems to be a bad place in general.
Comment #18
Dries CreditAttribution: Dries commentedI was thinking it should be part of theme.inc -- any reason it should not be?
Comment #19
Dries CreditAttribution: Dries commentedComment #20
mcncyo CreditAttribution: mcncyo commented#3: pager_rel.patch queued for re-testing.
Comment #21
droplet CreditAttribution: droplet commentedhttp://googlewebmastercentral.blogspot.com/2011/09/pagination-with-relne...
agreed with Dries.
Comment #22
zhuber CreditAttribution: zhuber commentedI have implemented this in drupal 6 by modifying pager.inc. I'm putting it up here for anyone who may find it useful.
Comment #24
StevenPatzComment #25
nate.dame CreditAttribution: nate.dame commentedSaw this in Google's Webmaster central blog, was curious if it's been implemented in Drupal. This string is the closest I've found...
http://googlewebmastercentral.blogspot.com/2012/03/video-about-paginatio...
Comment #26
drclaw CreditAttribution: drclaw commentedHere's a D8 patch that should do the trick...
Comment #27
drclaw CreditAttribution: drclaw commentedIt passed!
Here's a D7 patch for anyone who is interested
Comment #28
oriol_e9gYeah! This is the way!
Comment #29
Dries CreditAttribution: Dries commentedCommitted to 8.x. Updating the version so we can consider it for inclusion in 7.x. Thanks!
Comment #30
msellers CreditAttribution: msellers commentedIf I understand this patch correctly, it modifies the pager links to include the rel="prev" and rel="next" attributes.
From what I understand by reading the Google docs, the implementation should place in the section of the document, a
for the first page, bothand
on page 2, etc
I don't think this patch does what google wants. What am I missing?
Comment #31
msellers CreditAttribution: msellers commentedWow. I placed the tag in #30 and it was filtered out! Anyway, second paragraph should read:
... should place in the <head> section of the document
Comment #32
drclaw CreditAttribution: drclaw commentedHuh. How 'bout that. @msellers is correct it seems. According to google (http://googlewebmastercentral.blogspot.ca/2011/09/pagination-with-relnex...):
It seems like putting them in the
<a>
tags is a misconception... or something... And here I was thinking how easy it was to fix...So what do we do here? All the logic about which page we're on and which page is next (etc.) is done in the theme function... but we probably shouldn't be adding drupal_add_html_head_link() into the theme function... Maybe in a preprocessor? We can preprocess theme functions now in D7 and D8 right?
Comment #33
David_Rothstein CreditAttribution: David_Rothstein commentedSounds like maybe this needs additional discussion/work for Drupal 8 still, then?
Comment #34
drclaw CreditAttribution: drclaw commentedI'm not sure if this is necessarily the correct approach but here's a first attempt. Here's a quick description:
1) Added a preprocessor for theme_pager() that adds the necessary links to the head tag
2) Created a new helper function that builds the pager link query (pulled the code out of theme_pager_link and made it a reusable function)
I'll just post the D8 Patch for now, but can backport it if accepted.
Comment #35
greatfield CreditAttribution: greatfield commentedFor the time being until this is fixed in the core, i created something working for D6, based on this blog: http://pivica.me/blog/how-create-drupal-pagination-use-rel-next-and-rel-... (with some small changes). Just add the function to template.php. I hope this is of use to someone.
Comment #36
mgifford#34: pager_rel_attr-115753-34.patch queued for re-testing.
Comment #37
mgiffordI'm not certain if there are accessibility enhancements with this patch but there certainly are SEO benefits, but it's looking like there are now more reasons to support this.
Related issues:
#1665156: Support for rel="next" / rel="prev" pagination <link>
#1617962: rel="next" and rel="prev" when not using "Show Full Page" option
Related links:
http://www.w3.org/TR/html5/links.html#linkTypes
http://googlewebmastercentral.blogspot.ca/2012/03/video-about-pagination...
http://demosthenes.info/blog/190/Using-link-rel-to-preload-web-content-a...
http://www.webfoundation.org/accessibility/
Comment #38
mgifford#34: pager_rel_attr-115753-34.patch queued for re-testing.
Comment #40
netentropy CreditAttribution: netentropy commentedI updated this for Drupal 7 if anyone needs it.
http://www.netentropy.com/how-create-pagination-uses-relnext-and-relprev...
Comment #41
herderwu CreditAttribution: herderwu commentedD7 solution:
add two section code in theme_pager function(include/pager.inc), we could hook in template.php
1.rel="prev"
2.rel="next"
Comment #42
mgiffordinvmodule_url_rewrite() isn't defined which is a critical error. This is what was in the D7 example too.
Tere's a D8 patch with your code to help move this ahead.
Comment #43
mgiffordforgot the bot.
Comment #45
mgifford#42: rel-115753-42.patch queued for re-testing.
Comment #47
mgiffordWhere did function theme_pager go?
Comment #48
stefan006 CreditAttribution: stefan006 commentedHi greatfield! Your code works great. It gives me a dynamic link tags with atributes rel=prev and rel=next. Regards!!!
Comment #49
stefan006 CreditAttribution: stefan006 commentedHi mgifford! function theme_pager_link is in folder includes->pager.inc
Comment #50
mgiffordI still can't find it with
grep -ir theme_pager core/includes/*
Can't see how it is still there.
The patch was applied the theme_pager() in
@@ -245,7 +245,12 @@ function theme_pager($variables) {
That must have been renamed.
Comment #51
aleksijohansson CreditAttribution: aleksijohansson commentedinvmodule_url_rewrite() gives not defined error. You can replace this line to get it working:
$next_link = invmodule_url_rewrite($next_match[1]);
with this line:
$next_link = $GLOBALS['base_url'] . $next_match[1];
Comment #52
mgiffordComment #54
jwilson3My issue with this patch is conceptual:
What happens if there are two sets of pagers on a page? Which one is determined to be the "main" pager, whose next/prev links make it up into the head section in the dom?
Comment #55
droplet CreditAttribution: droplet commented@jwilson3,
Shouldn't it only one Pager per page? In template_preprocess_pager function, it used global vars to indicated current pager status.
Comment #56
jwilson3Search the codebase for the text 'multiple pagers'. The functionality is there, particularly for Comments, but I'm not sure how much test coverage there is for this feature.
Comment #57
mgiffordThis is an old issue. But to get to @jwilson3 point, I don't know that it will matter.
There is nothing in the specs I could find which indicate that there is anything else that is needed for multiple pagers:
http://www.w3.org/TR/html5/links.html#link-type-next
I do think that machines (including assistive technology) will put the previous/next in context of the series of links that they are browsing. They will be nested semantically with a group of other links.
I don't know how to prove this though, but it is next within a context of a series of links, not just a bunch of random links.
The whole patch needs to be rewritten but at least the functions still exist in core/includes/pager.inc core/includes/theme.inc
Comment #58
tanmoy1981 CreditAttribution: tanmoy1981 as a volunteer commentedPlease check my solution here.
Comment #59
mgiffordSo essentially:
Not sure how that deals with the issue of multiple pagers though.
Comment #60
tanmoy1981 CreditAttribution: tanmoy1981 as a volunteer commented@mgifford - This solution will work properly with multiple pagers as well.
You need some minor modifications if you change the prev or next texts across different pagers.
Comment #61
mgifford@tanmoy1981 Can you roll up a patch for D8? If it's fixed in D8 for Views there's a better chance it will be backported. I still don't understand why that was marked Closed (Won't fix).
Comment #72
Joe Huggans.
Comment #73
mgiffordComment #79
quietone CreditAttribution: quietone at PreviousNext commentedI'm following up on issues that have been committed and re-opened.
This issue was committed to Drupal 8.x in 2012 and re-opened in #33 due to questions about the implementation. The last discussions were 7 years ago.
Is there anything still to do here? Is, update the Issue Summary and add a comment.
Thanks
Comment #81
quietone CreditAttribution: quietone at PreviousNext commentedThere hasn't been any further information provided so I am marking this as fixed, because it was committed to Drupal 8.
Cheers.