Update pagination markup for new CSS standards and improved accessibility

Seems like #1913076: twig general pager may be a duplicate of this, but that has some alternate idea, to lose the ul/li entirely.

i had more my head swapped around how the twig template could work than actually the markup ;)

to repeat the idea
pager.html.twig:

{#
need the following variables
  first
  previous
  last
  next
  ellipsis  
  page_count
  current
 #}

{% if pager %}
<nav class="pager">
  {% if first is defined %}
    <span class="first">
      <a href="{{ url }}" title=" {{ help text }}">« </a>
    </span>
  {% endif %}

  {% if previous is defined %}
    <span class="pre">
      <a href="{{ url }}" title=" {{ help text }}">‹ </a>
    </span>
  {% endif %}

  {% for page in page_count %}
    {% if page != current %}
      <span>
        <a href="{{ url }}" title=" {{ help text }}">{{ count }}</a>
      </span>
    {% else %}
      <span class="active"><b>{{ count }}</b></span>
    {% endif %}
  {% endfor %}

  {% if ellipsis is defined %}
    <span>...</span>
  {% endif %}

  {% if next is defined %}
    <span class="next">
      <a href="{{ url }}" title=" {{ help text }}"> ›</a>
    </span>
  {% endif %}

  {% if last is defined %}
    <span class="last">
      <a href="{{ url }}" title=" {{ help text }}"> »</a>
    </span>
  {% endif %}
</nav>
{% endif %}

got pointed to some good info here about pagers at the sprint in sydney
https://github.com/KnpLabs/KnpPaginatorBundle

I'm kind of with morten on this one...

I'm not sure but if screen readers read the ordinal in ordered lists, that would introduce confusion. eg based on the sample code in comment #1, a screen reader would read something like "Six, Go to page twelve", "Seven, Go to page thirteen", "Eight, Go to next page".

The only thing I half worry about with spans is the whitespace generated between them. Would be good to see if someone could come up with a jsbin prototype using spans, (completely themed with some border/background around each item, and prove that there isn't any whitespace between the items. Also, keep in mind that simply inlining them may not work correctly on narrow screens.

We should look for some solutions that other projects are already using that are mobile friendly, and see how they are doing it.

I think UL, tbh.

~All css frameworks refer to an LI at the very least, if not a UL explicitly, so if the goal is to make it easy for newbie themers then css frameworks should be taken into consideration. It's also consistent with other navigation. If menu is a UL and breadcrumbs are a UL (i'm assuming), then pagination should be the same.

I also think we should be aiming to put the link context inside the link text rather than a title attribute (as suggested in the accessible pagination pattern). The title attribute isn't used by screen readers as it was intended and is no longer recommend for links (see http://www.paciellogroup.com/blog/2013/01/using-the-html-title-attribute...).

Something like:

<p id="paginglabel" class="element-invisible">Pagination</p>
<ul class="pagination" role="navigation" aria-labelledby="paginglabel">
  <li class="prev"><a href="" rel="prev"><span class="element-invisible">Go to the previous page </span>&laquo;</a></li>
  <li class="active"><a href=""><span class="element-invisible">You are currently on page </span>2</a></li>
  <li><a href=""><span class="element-invisible">Go to page </span>3</a></li>
  <li><a href=""><span class="element-invisible">Go to page </span>4</a></li>
  <li class="ellipses unavailable"><a href="">&hellip;</a></li>
  <li><a href=""><span class="element-invisible">Go to page </span>12</a></li>
  <li><a href="" ><span class="element-invisible">Go to page </span>13</a></li>
  <li class="next"><a href="" rel="next"><span class="element-invisible">Go to the next page </span>&raquo;</a></li>
</ul>

A bit more markup I know, but more accessible...

UPDATE: moved rel onto the anchor.

#5 sounds reasonable, but...

* by "~All css frameworks", which ones did you check, exactly? link?
* why is the rel="next" on the LI and not the anchor tag itself?

I didn't think much about the rel prev/next. My understanding is they belong in the <head>.
But failing that I would agree they should be on the anchor tag.

As for css frameworks:

Foundation: http://foundation.zurb.com/docs/navigation.php#pagCode
with css selector: ul.pagination li

Bootstrap: http://twitter.github.com/bootstrap/components.html#pagination
with css selector: .pagination ul > li

Inuit: http://inuitcss.com/
with css selector: .pagination > li

Gumby: http://gumbyframework.com/
with css selector: ul.pagination li

GetWirefy: http://getwirefy.com/features.html#pagination
with css selector: .pagination li

It's pretty fair to say if the framework covers pagination it's using an li.

tbh i dont think we should look at other cms'es and how they do the markup - we should look for the best kinda markup we can posibbel bring to the table.

The reason i dont like the li model & wants a span instead is that its not a list + we then have all the fun of having to add additional css to make it work the way we want it. (float:left, display:inline-block etc)

remember the pager.html.twig template will be easy to find, so it can be changed very easy to whatever you want.

It's not a list of available pages?
I know it requires extra css. Ideally we already have those float/display rules defined in a reusable .inline-list (or similar) class, applied to all menus. I just think you'll get a lot of people changing it back to a list.. possibly more than those who agree that pagination isn't a list of links. Sorry to be pest...

you are not a pest this is whats needed to get better markup :)

I think at this point we need two side-by-side jsfiddles themed exactly the same way (inline horizontal format, that supports mobile/small screens), that can help everyone compare and see visually if a div.pager span is better than a ul.pager li.

The criteria for calculating which is the winner should be:

1. how clean and how brief is the markup?
   Eg, its already obvious that div and spans add more bytes than ul and li ;)
2. how structurally semantic is the markup?
   Eg, its already obvious that divs and spans are only there for decoration/wrappers, while ul/li adds semantic structure (ie, elements grouped as a sequential list).
3. how clean and how brief are the styles?
   By cleanliness, we should consider things like extra css rules to make something appear inline that by default is not (i.e. ul/li loses that one). We need to ensure both solutions satisfy the basic default pagination guidelines:
   • a) inline, such that the pagination can be centered on the page, or aligned left or right, with a single text-align
   • b) zero whitespace between the wrapper elements, such that they can be wrapped in "blocks" with background image and border treatments that dont exhibit whitespace issues
   • c) mobile friendly (working decently on screen-widths between 320px and 1200px).

Other than these, what other criteria am I missing?

Also one observation:

Ideally we already have those float/display rules defined in a reusable .inline-list (or similar) class, applied to all menus.

I really am starting to get bothered by this pattern in Drupal. It is just not semantic to keep holding onto concepts like "div.inline-list ul".

In addition to not being semantic, this makes it too easy for your theme CSS to become really ugly and bloated, because if you want to make *one* exception out of all the places on a site that use this you have to add YET ANOTHER WRAPPER (or at least use an existing one from higher up in the dom) to *override* this specific case. Its at least been my experience that adding another class on the ul itself, would be the ideal solution, but that it is really more of an inconvenience than a convenience, its easier just to dirty up your CSS with something like:

.block-block-somemodule-someclass div.inline-list ul {
 /* code to remove the display: inline; */
}
.block-block-somemodule-someclass div.inline-list ul li {
/* code to remove inline assumptions */
}
.block-block-somemodule-someclass div.inline-list ul li a {
/* code to remove inline assumptions and style how we want it */
}

Because of the wrapper that applies styles, even if you added a class to the ul, your code would *still* look something like this horrendous confusing information:

div.inline-list ul.someclass {
 /* code to remove the display: inline; */
}
div.inline-list ul.someclass li {
/* code to remove inline assumptions */
}
div.inline-list ul.someclass li a {
/* code to remove inline assumptions and style how we want it */
}

When this should all really just look like:

ul.someclass {
/* code to style how we want it */
}
ul.someclass li {
/* code to style how we want it */
}
ul.someclass li a {
/* code to style how we want it */
}

Yes. this is somewhat of a tangent and a rant, but the point is, if we are going to inline a ul/li here *please* dont add a wrapper div with a class to do it. JUST USE THE ul.pagination OR ul.pager CLASS!!!! /RANT

Oh sorry I never meant an extra div. I completely agree wrapper divs are rubbish. I think a js fiddle is a good idea. I'll put one together with what I'm suggesting and a comparable span version then we can discus it better.

Hey all,

Here are the jsfiddles.

Span version: http://jsfiddle.net/BZDWz/1/
List version: http://jsfiddle.net/2FmyK/1/

List version has an extra 10 lines of css, but I'm suggesting we also use this inline-list class on our menus and breadcrumbs (on the ul, not a wrapper div).
The markup has equal lines of code. Span version has 4 extra characters per line.
My only issue with the span version at this point is it's readability suffers a little from the double span.

The inner span could be substituted with an <i> tag if no one objects.
It would certainly improve the readability and reduce the number of characters. eg...

<li><a href=""><i class="element-invisible">Go to page </i>3</a></li>
<span><a href=""><i class="element-invisible">Go to page </i>3</a></span>

The .pagination class is purely for presentational purposes and should be the only thing a themer needs to worry about.
The css meets basic pagination needs as outlined in #11.

Hopefully this will help :)

Rikki

Thanks Rikki. Nice work.

I'm suggesting we also use this inline-list class on our menus and breadcrumbs (on the ul, not a wrapper div).

I still stand against this, because now while we embrace responsive design, it is often the case that something that was "inline" for desktop is no longer inline for mobile. this is akin to adding a class="blue" to an element, for desktop, and then have to write special overrides for mobile that shows something like .blue { color: green; } for mobile. This illustrates the problem with mixing visual style, presentation (and layout) into the html.

/* from system.base.css to visually hide information */
.element-invisible {
  position: absolute !important;
  clip: rect(1px 1px 1px 1px); /* IE6, IE7 */
  clip: rect(1px, 1px, 1px, 1px);
  overflow: hidden;
  height: 1px;
}
/* generic .inline-list class, resuable on menu, breadcrumbs etc */
.inline-list {
  list-style: none;
  margin-left: 0;
  margin-bottom: 0;
}
.inline-list li,
.inline-list a {
  display:inline-block;
  *display:inline;
}

I suggest we forget about "inline-anything" and "element-invisible" classes, and just start writing simple, straight-forward (and opinionated) defaults for the specific case to which it pertains: Eg ul.pager { display: inline } and .pager i { display: none; /* or whatever */ }. This makes it trivially easy to override and customize for difference media queries without the need for stylesheet directives that seem to contradict themselves in our themes.

.pagination > span {
  margin-right: -4px; /* removes gap between items: http://css-tricks.com/fighting-the-space-between-inline-block-elements/ */
  color: #999;
}

I'm not sure about the negative margin solution myself. My suggestion would be to simply delete the whitespace between the LIs (this could be done in twig for either of the cases (span's or lis), and yes, it decreases readability. I *think* there is a patch somewhere from @sun that actually fixes this issue in core, by removing line-breaks after </li> in core (not sure).

Yeah I wasn't sold on the negative margin either, it was just the simplest one to add (and remove).

I think there is a case for and against abstracting out those repeated rules into their own class (OOCSS). It means less css as they don't need to be repeated on each element we need to inline or hide, and I think it's fairly safe to assume that no designer will ever want the text 'Go to page' visible. Where text is only ever added for accessibility purposes I think it's important to have a generic class to visually hide them.

I agree these elements (menus in particular) won't always be inline. Menu probably needs it's own solution. But pagination and breadcrumbs generally stay inline on smaller screens don't they?

It's also easy for a themer to take the class out of the template, that way a novice themer doesn't need to worry about how to inline a list, and an experienced front-end dev can quickly remove defaults and take full control.

My $0.02:

<li> pagers? -1

1. unnecessary css overrides / tag bloat
2. accessibility. From the css-tricks article you mention, criz, I think this about says it all:
   

   Stebner (blind) was tabbing through the far too many lists on mica.edu and they gave no clue as to where he was on the page and no information other than "List, 10 items. List, 6 items..." With divs and spans, the content is immediately available to the screen readers.

This list proudly powered by <ol>.

The accessibility point brought up in the css-tricks article is very interesting and convincing, but only on the condition that we do introduce a <nav> wrapper, to make the otherwise semantically "weak" series of spans, to regain some semantic karma points. Just wanted to point this out in writing because some of the previous examples in this thread didn't use the <nav> element, but the jsfiddle does...

http://jsfiddle.net/BZDWz/1/ wins?

Which means morten's original code would be pretty close except for the nagging issue of the extra spaces and negative margins. All the spans could appear printed out on the same line, mitigated by Twig whitepace cleanup controls.

Also we have no hierarchical elements in paginations, so we can safely use spans in my opinion.

I agree on that point.

One more point: it looks like HTML5.1 recommendation depicts that we should be putting a header element *inside* the <nav> not outside.

what if we change from:

<p>Pagination</p>
<nav>
  [...]
</nav>

to

<nav>
  <h3>Pagination</h3>
  [...]
</nav>

or something similar?

Added KnpPaginatorBundle to sources.

A handful of thoughts on this:

• The absolute last thing in the world we should be doing is basing our markup on CSS Frameworks. Period. Don't do it.
• As discussed in this article about accessible pagination patterns, screen readers do read the ordinals in <ol> lists, so we shouldn't use those
• Using lists for navigation potentially makes it harder for screen readers to navigate them, so I'm not necessarily jumping to use them
• Then again, there is an official example using lists taking advantage of the menubar, menu, and menuitem roles. That's for menus though, maybe not applicable for this.

With this all in mind, I'm in favor of the following:

• Pagination (really, all nav) should use the <nav> tag
• We should try and pull in one of our users who regularly uses, or at the very least regularly tests, with screen readers to get their opinion on whether or not we should use lists vs spans.
• No matter what we choose, we should absolutely not use <ol>
• We need to ensure that we apply the correct roles to our markup, no matter what method we choose. Currently, our pagers do not have any proper roles attached to them.

<nav class="pagination" role="navigation">

Why the change from classname "pager" to "pagination" ? IMO it's needless byte bloat, for questionable improvement on clarity, and introduces inconsistency with the existing php (now twig) theme template / function names.

I know I've made the point earlier, I agree with you in #16 where you state that the inline issue is a CSS architecture issue, but I still maintain that the layout in this case should be left up to the theme to implement, not done via a handy default layout ".l-inline" class. This is because, now with Twig and the Attributes object, it is now trivially easy to override the template file and an an extra CSS class to display the ul as inline if you so desire (which would automatically override any other default layout styles provided by Drupal, were we to just use the given semantic classes), however it is not so easy to *remove* such a class -- AFAIK, you still have to implement a PHP template preprocess and use PHP code (or deal with crappy over-qualified css workarounds like what I mentioned in #12). So, just as we wouldn't want to put a default grid-width class on the container element of the pager, I dont think we should put an inline. This argument alone, makes me really sway my vote towards the SPAN solution, unless we can agree to abandon the concept of "l-inline". Opinionated, yes. Wrong, maybe ;) Let me know what you think!

1. I'm not totally up to speed with all SMACSS techniques, but the following classes seem confusing.
   

   .pager__item--prev
   .pager__item--active
   .pager__item--ellipses
   .pager__item--next
   

   Could we maybe get away with placing these cleaner classes on an inner wrapper element (either an A tag, or a span) inside the outer li.pager__item element:

   .pager__prev
   .pager__active
   .pager__ellipses
   .pager__next
   

2. What is gained by wrapping the ellipsis with a link tag? Where would the href attribute go? This seems like something an individual theme should produce if they needed it, not for default markup produced by Drupal.

   <li class="[...] unavailable"><a href="">&hellip;</a></li>
   [...]
   <span class="[...] unavailable"><a href="">&hellip;</a></span>
   

   I understand the need to have another wrapper element between the .pager__item element, and the actual content, so i might propose something like this instead:

   <li class="pager__item"><a class="pager__link" href="">A regular link here</a></li>
   <li class="pager__item"><span class="pager__ellipses">&hellip;</span></li>
   

3. Similar argument for the current page... Strictly speaking, there is no need to make it a link:

   <li class="pager__item"><span class="pager__active"><span class="element-invisible">You are currently on page </span>2</span></li>
   

Code with changes from #26 + some of my changes.
'Pagination' is not a label, it's a header of pagination block, though invisible.
I wish we can move the arrows from code to css in pseudo ::before or ::after of the <span> or <a>elements. It makes more simple to change the text inside it or completely remove without any php work.

  <nav class="pager" role="navigation">
  <h4 class="pager__label element-invisible">Pagination</h4>
  <ul class="pager__list">
    <li class="pager__item pager__prev"><a href="" rel="prev"><span class="element-invisible">Go to the previous page </span></a></li>
    <li class="pager__item pager__active"><span class="element-invisible">You are currently on page </span>2</li>
    <li class="pager__item"><a href=""><span class="element-invisible">Go to page </span>3</a></li>
    <li class="pager__item"><a href=""><span class="element-invisible">Go to page </span>4</a></li>
    <li class="pager__item"><span class="pager__ellipses"></span></li>
    <li class="pager__item"><a href=""><span class="element-invisible">Go to page </span>12</a></li>
    <li class="pager__item"><a href="" ><span class="element-invisible">Go to page </span>13</a></li>
    <li class="pager__item pager__next"><a href="" rel="next"><span class="element-invisible">Go to the next page </span></a></li>
  </ul>
</nav>

Screen readers apparently skip the …. I agree with moving the html entity glyphs that pagers use into CSS before and after pseudo selectors, where it will be a heluva lot easier to remove (or change them) with just CSS and not have to dig into a template file.

But you didn't incorporate all of the changes I suggested. The problem with your version above is that now the current page is inconsistent with the other items in the list, which all have two elements (the LI and the A for links, and the LI and the SPAN for the ellipses). I think we need another SPAN around the active page text, and to move the pager__active to that SPAN. Also, the pager__first and pager__next, and pager__last should be moved to the anchor tag.

<nav class="pager" role="navigation">
  <h4 class="pager__label element-invisible">Pagination</h4>
  <ul class="pager__list">
    <li class="pager__item"><a class="pager__prev" href="" rel="prev"><span class="element-invisible">Go to the previous page</span></a></li>
    <li class="pager__item"><span class="pager__active"><span class="element-invisible">You are currently on page </span>2</span></li>
    <li class="pager__item"><a class="pager__link" href=""><span class="element-invisible">Go to page </span>3</a></li>
    <li class="pager__item"><a class="pager__link" href=""><span class="element-invisible">Go to page </span>4</a></li>
    <li class="pager__item"><span class="pager__ellipses"></span></li>
    <li class="pager__item"><a class="pager__link" href=""><span class="element-invisible">Go to page </span>12</a></li>
    <li class="pager__item"><a class="pager__link" href=""><span class="element-invisible">Go to page </span>13</a></li>
    <li class="pager__item"><a class="pager__next" href="" rel="next"><span class="element-invisible">Go to the next page</span></a></li>
    <li class="pager__item"><a class="pager__last" href=""><span class="element-invisible">Go to the last page</span></a></li>
  </ul>
</nav>

<nav class="pager" role="navigation">
  <h4 class="pager__label element-invisible">Pagination</h4>
  <span class="pager__item"><a class="pager__prev" href="" rel="prev"><span class="element-invisible">Go to the previous page</span></a></span>
  <span class="pager__item"><span class="pager__active"><span class="element-invisible">You are currently on page </span>2</span></span>
  <span class="pager__item"><a class="pager__link" href=""><span class="element-invisible">Go to page </span>3</a></span>
  <span class="pager__item"><a class="pager__link" href=""><span class="element-invisible">Go to page </span>4</a></span>
  <span class="pager__item"><span class="pager__ellipses"></span></span>
  <span class="pager__item"><a class="pager__link" href=""><span class="element-invisible">Go to page </span>12</a></span>
  <span class="pager__item"><a class="pager__link" href=""><span class="element-invisible">Go to page </span>13</a></span>
  <span class="pager__item"><a class="pager__next" href="" rel="next"><span class="element-invisible">Go to the next page</span></a></span>
  <span class="pager__item"><a class="pager__last" href=""><span class="element-invisible">Go to the last page</span></a></span>
</nav>

Can we just drop <li class="pager__item"> and add this class to a/span element ? The further we go - the less need i see in the li/span wrapper. Just having class on the <a> covers 90% use cases for the pager. Ispacially now that we don't support IE7 and can use display: inline-block which is just awesome.

I was trying to come up with a reason why we need the extra wrappers, but couldn't. #29++

I've come up with two models, the first one implements the feedback from #29:

<nav class="pager" role="navigation">
  <h4 class="pager__label element-invisible">Pagination</h4>
  <a class="pager__first" href=""><span class="element-invisible">Go to the first page</span></a>
  <a class="pager__prev" href="" rel="prev"><span class="element-invisible">Go to the previous page</span></a>
  <span class="pager__active"><span class="element-invisible">You are currently on page </span>2</span>
  <a class="pager__link" href=""><span class="element-invisible">Go to page </span>3</a>
  <a class="pager__link" href=""><span class="element-invisible">Go to page </span>4</a>
  <span class="pager__ellipsis"></span>
  <a class="pager__link" href=""><span class="element-invisible">Go to page </span>12</a>
  <a class="pager__link" href=""><span class="element-invisible">Go to page </span>13</a>
  <a class="pager__next" href="" rel="next"><span class="element-invisible">Go to the next page</span></a>
  <a class="pager__last" href=""><span class="element-invisible">Go to the last page</span></a>
</nav>

The second, is uber-minimalist, and uses an <i> for the ellipsis (hat tip rikki_iki, #14) and a <b> for the current page.

<nav class="pager" role="navigation">
  <h4 class="pager__label element-invisible">Pagination</h4>
  <a class="pager__first" href=""><span class="element-invisible">Go to the first page</span></a>
  <a class="pager__prev" href="" rel="prev"><span class="element-invisible">Go to the previous page</span></a>
  <b class="pager__active"><span class="element-invisible">You are currently on page </span>2</b>
  <a class="pager__link" href=""><span class="element-invisible">Go to page </span>3</a>
  <a class="pager__link" href=""><span class="element-invisible">Go to page </span>4</a>
  <i class="pager__ellipsis"></i>
  <a class="pager__link" href=""><span class="element-invisible">Go to page </span>12</a>
  <a class="pager__link" href=""><span class="element-invisible">Go to page </span>13</a>
  <a class="pager__next" href="" rel="next"><span class="element-invisible">Go to the next page</span></a>
  <a class="pager__last" href=""><span class="element-invisible">Go to the last page</span></a>
</nav>

And, of course, the accompanying CSS, for clarity of where we've moved some of the HTML Entity markup.

.pager__first::before {
  content: " « ";
}
.pager__prev::before {
  content: " ‹ ";
}
.pager__next::before {
  content: " › ";
}
.pager__last::after {
  content: " » ";
}
.pager__ellipsis::before {
  content: " … ";
}

Now that is some serious clean dream code there -- shaving bytes and splitting hairs.

The keen eye will also see that I changed pager__ellipses (plural) to pager__ellipsis (singular) to match singular form of pager__link.

Tagging for a11y review.

Heres a fiddle containing the uber-minimalist version in #30: http://jsfiddle.net/kZr7k/

I like this no list version now :)
I didn't realise you would need PHP to remove a class.
Couldn't the <i> tag wrap the 'go to page' text as well? http://jsfiddle.net/kZr7k/1/

@jwilson3, this minimalistic version is awesome! jwilson3++
It really covers 99% of needs of a themer and it's still can be easily customized for the remaining 1%.

Fro those who see the code still being gigantic, check the code without hidden elements:

<nav class="pager" role="navigation">
  <a class="pager__first" href=""></a>
  <a class="pager__prev" href="" rel="prev"></a>
  <b class="pager__active">2</b>
  <a class="pager__link" href="">3</a>
  <a class="pager__link" href="">4</a>
  <i class="pager__ellipsis"></i>
  <a class="pager__link" href="">12</a>
  <a class="pager__link" href="">13</a>
  <a class="pager__next" href="" rel="next"></a>
  <a class="pager__last" href=""></a>
</nav>

This is the smallest BEM version of pager I can think of :)
And even more - it becomes very stable because of only 1 level deep of DOM and class names.

Here is the minimum styles for it : http://jsfiddle.net/MZ9Qb/
It's almost the same as removing list-style, padding and margins from ul / li elements. But the markup is cleaner.

The only thing i think we should add a common class to first/last/prev/next, something like .pager__navigation.
It won't add a little bit of extra markup, but it will be easier to give those elements common styling.

i like @criz version.

- H4 just because it has default font size of 1em. The pager is not an important header at all. So making it h4 will print the same base page font size, only bold - which is ok for a pager.

- we don't need the   inside the ellipses - it will create an extra space. Empty is perfect - if we remove the :before of ellipses, the element will just count as empty. But with space you'll have to hide it.

I would love to stop on this version:

1. all elements have common class, independently of element type
2. all variations have their class name
3. Without having wrapper we can easily use flexbox in future for the pager
4. Arrows and ellipsis is fully customizible just from css
5. this markup provides excellent base.

About (5) - for exmple for breadcrumb we could exactly the same markup, only adding .breadcrumb class. http://jsfiddle.net/Uaa6M/ The only problem with that - arrows will be part of link, so it's a bit silly :D But it's still possible!

@criz - it's not clear yet. We need accessibility review - headers are seen in readers, if we could understand what we should show as headers in readers and what should show as labels or smth - it would be just great. for now just using header element )

You know, I got to thinking about the active item...

<span class="pager__item pager__item--active"><span class="element-invisible">You are currently on page </span>2</span>

Everywhere else in Drupal navigation (eg menu lists for example) a reference to the current page is rendered as a link, and an active class is added. Why do we make a special case for the pager?

Could we not also have the current page be a link, styled differently (or styled like Drupal styles active links by default)? Would this simplify and clean up the markup? Or is there a convincing argument for why we need to use plain text for the current page.

Self-follow-up for #40....

I just played with the fiddle from #37, and realized that with the current page as a link it would take *more* lines of css code to null out any a.pager__item:hover code you create for the active page (generally, you don't want to see any hover state changes when you hover the active page). So perhaps, that by itself, is reason enough to not do #40.

All good in the hood.

Code in #35 (and accompanying fiddle in #37) are good to go.

one of the basic principles in the TWIG template thinking is that we wanna build for the 80% not for 100% and that we want to keep the markup & css as small as possbile, so we add instead of removing (i know its crazy talk )

so imho we should remove the pager__item cause we can easy target the a with a .pager a
If your theme needs a little bit of pager__item love its easy to add, and a themer dosnt need to remove it

whatta ya say to this code:

<nav class="pager" role="navigation">
  <h4 class="element-invisible">Pagination</h4>
  <a class="pager__item--first" href=""><span class="element-invisible">Go to the first page</span></a>
  <a class="pager__item--prev" href="" rel="prev"><span class="element-invisible">Go to the previous page</span></a>
  <span class="pager__item--active"><span class="element-invisible">You are currently on page </span>2</span>
  <a href=""><span class="element-invisible">Go to page </span>3</a>
  <a href=""><span class="element-invisible">Go to page </span>4</a>
  <span class="pager__item--ellipsis" role="presentation">&nbsp;</span>
  <a href=""><span class="element-invisible">Go to page </span>12</a>
  <a href=""><span class="element-invisible">Go to page </span>13</a>
  <a class="pager__item--next" href="" rel="next"><span class="element-invisible">Go to the next page</span></a>
  <a class="pager__item--last" href=""><span class="element-invisible">Go to the last page</span></a>
</nav>

Does it make sense to have it like this?

<nav class="pager" role="navigation">
  <h4 class="element-invisible">Pagination</h4>
  <a class="pager__item--first" href=""><span class="element-invisible">Go to the first page</span></a>
  <a class="pager__item--prev" href="" rel="prev"><span class="element-invisible">Go to the previous page</span></a>
  <span class="pager__item--active"><span class="element-invisible">You are currently on page </span>2</span>
  <div class="numerical_items">
    <a href=""><span class="element-invisible">Go to page </span>3</a>
    <a href=""><span class="element-invisible">Go to page </span>4</a>
    <span class="pager__item--ellipsis" role="presentation">&nbsp;</span>
    <a href=""><span class="element-invisible">Go to page </span>12</a>
    <a href=""><span class="element-invisible">Go to page </span>13</a>
  </div>
  <a class="pager__item--next" href="" rel="next"><span class="element-invisible">Go to the next page</span></a>
  <a class="pager__item--last" href=""><span class="element-invisible">Go to the last page</span></a>
</nav>

@betz,
nono wrapper for elements, it can break using display:inline-block for example.
And.. oh there are cases, it's not the best idea and I'm not sure it will be used much.

@mortendk,
you are leading back to the Pitfalls described in the new CSS Architecture:
"Pitfall: Relying on HTML structure.
Mirroring a markup structure in our CSS selectors makes the resulting styles easy to break (with markup changes) and hard to reuse (because it’s tied to very specific HTML)."
I've seen you doing that for more issues related to markup. As you said - we are adding instead of removing. This rule also should be applied to class names.

Adding them I hope to keep the weight of any base component equal to [0 0 1 0] (1 for class name);
This makes it really easy to override if needed, and it stops being depended on html.

Does it make sense? :)

Just had a talk with vincenzo @falcon03 about removing the <ul><li>...
His recommendation is to keep the li's cause screenreaders works better with them (list amount of links etc)

so i guess its back to the drawing board ;)

Always better to have the more semantic option for accessibility. Screen reader users can pull meaning from well structured text.

Was an animated conversation, wish I was there for more of it - https://www.flickr.com/photos/mgifford/8763090948/

I’m down with @Snugug in #22. If we choose the right ARIA roles, we might be able to still announce item count (#45).

    <nav role="navigation">
      <h1 class="visually-hidden">Pagination</h1>
      <div class="pager" role="menu">
        <a class="pager__link" role="menuitem" href="">
          <i class="icon icon--go-first"><span class="visually-hidden">First page</span></i>
        </a>
        <a class="pager__link" role="menuitem" href="">
          <i class="icon icon--go-previous"><span class="visually-hidden">Previous page</span></i>
        </a>
        <a class="pager__link" role="menuitem" href="">
          <span class="visually-hidden">Page </span>1
        </a>
        <a class="pager__link is-active" role="menuitem" href="">
          <span class="visually-hidden">(Current) page </span>2
        </a>
        <a class="pager__link" role="menuitem" href="">
          <span class="visually-hidden">Page </span>3
        </a>
        <a class="pager__link" role="menuitem" href="">
          <i class="icon icon--go-next"><span class="visually-hidden">Next page</span></i>
        </a>
        <a class="pager__link" role="menuitem" href="">
          <i class="icon icon--go-last"><span class="visually-hidden">Last page</span></i>
        </a>
      </div>
    </nav>

General notes

- Since <nav> is a sectioning element, we may as well use <h1>.
- The icon component here is really a separate issue, but it means we don’t need special classes for the first/last next/prev links. See #1849712: Add theming template and prepare logic for rendering icons.

Accessibility notes

RE: the 'menu' role, the ARIA spec says:

A menu is often a list of common actions or functions that the user can invoke. The menu role is appropriate when a list of menu items is presented in a manner similar to a menu on a desktop application.

So I am not entirely sure it is appropriate. However, the 'list' role is for “A group of non-interactive list items” so menu seems to fit best.

I tested this in VoiceOver on Mac OS X. It reads out:
- 'Entering navigation landmark (menu 7 items)' when entering the nav
- 'First page (menu item)' on the first link
- 'Next page (menu item)' on the next link
- '(Current) page 2' on the active link
- etc.
- 'End of menu’ when navigating past the last item.

CSS architecture notes

The new CSS standards say to use design semantics to apply styles to elements. Since we can’t make assumptions about design in core, we have to be conservative about what classes we apply. That means few if any utility classes like 'clearfix' or 'l-inline' etc. As noted, they are useful in themes but problematic in core.

I’ve noticed a couple of common issues as folks come to grips with the new CSS standards. Some of these appear in various proposals here:

- Variant classes used without the base class. Since variants like .pager__item--first only include styles needed to modify .pager__item, variants must appear with the base class in the markup: class="pager__item pager__item--first". Yes, it’s verbose, but it’s the best choice we have until CSS supports some kind of module inheritance.

- Mixing up states and variants. 'pager__item--active' is not a variant of 'pager__item'; it’s a state. As such, the markup should be class="pager__item is-active" and the CSS would be .pager__item.is-active {}. I like to think of state classes as custom DOM states analogous to a:active.

I really don't know what the fuss with lists is. Taking @mortendk guide in #42, adding in lists is easier to understand and takes 9 characters '

' while adding 'role="menuitem"' to every item adds 14 characters. If the goal is "to keep the markup & css as small as possible", then list items aren't adding much.

In #22 @Snugug wasn't opposed to UL, but rather OL. He was arguing for use of ARIA roles & the NAV element.

Anyways, this is riffing off the previous code:

<nav class="pager" role="navigation">
  <h4 class="element-invisible" id="pagination">Pagination</h4>
  <ul aria-labelledby="pagination" role="navigation">
    <li class="pager__item--first" ><a href="#"><span class="element-invisible">Go to the first page</span></a></li>
    <li class="pager__item--prev"><a  href="#" rel="prev"><span class="element-invisible">Go to the previous page</span></a></li>
    <li class="pager__item--active"><a  href="#"><span span class="element-invisible">You are currently on page </span>2</span></li>
    <li><a href="#"><span class="element-invisible">Go to page </span>3</a></li>
    <li><a href="#"><span class="element-invisible">Go to page </span>4</a></li>
    <li class="pager__item--ellipsis" role="presentation">&nbsp;</li>
    <li><a href="#"><span class="element-invisible">Go to page </span>12</a></li>
    <li><a href="#"><span class="element-invisible">Go to page </span>13</a></li>
    <li class="pager__item--next"><a href="#" rel="next"><span class="element-invisible">Go to the next page</span></a></li>
    <li class="pager__item--last" ><a href="#"><span class="element-invisible">Go to the last page</span></a></li>
    </ul>
</nav>

I see a lot of "Go to page </span>X" here: the "Go to" is already implied by the link tag, and would become very repetitive. <span class="element-invisible">Page </span>X, Next<span class="element-invisible"> page</span> should be enough.

As for the lists vs divs/spans, historically, good accessible markup has used lists for navigation items, so many screenreader users will have become used to that convention (and may, for example, use a shortcut to skip through each list on a page, looking for a navigation control). You can also skip over a list easily, if you recognise it as a navigation control that you don't want. I'm not sure that's as easy with divs/spans. That said, I haven't done any significant a11y work with HTML5 elements, so I don't know how the <nav> element affects this. It'd be great to have some user-testing or feedback from screen-reader users around this.

Agreed on the shortening of the text. Good suggestion @manarth

Also agree for the need for greater user-testing.

I've been trying to rally folks behind this, in various institutions but so far I've had no real traction.

Here is the pager markup that I currently use:

<nav role="navigation">
  <h2 class="element-invisible">Pages</h2>
  <ul class="pager">
    <li class="pager__item pager__item--first">
      <a class="pager__link pager__link--first" href="#">
        <span class="element-invisible">First page</span>
      </a>
    </li>
    <li class="pager__item pager__item--previous">
      <a class="pager__link pager__link--previous" rel="prev" href="#">
        <span class="element-invisible">Previous page</span>
      </a>
    </li>
    <li class="pager__item pager__item--ellipsis" role="presentation">&hellip;</li>
    <li class="pager__item">
      <a class="pager__link" href="#">
        <span class="element-invisible">Page </span>8
      </a>
    </li>
    <li class="pager__item pager__item--current">
      <span class="element-invisible">Currently on page </span>9
    </li>
    <li class="pager__item">
      <a class="pager__link" href="#">
        <span class="element-invisible">Page </span>10
      </a>
    </li>
    <li class="pager__item pager__item--ellipsis" role="presentation">&hellip;</li>
    <li class="pager__item pager__item--next">
      <a class="pager__link pager__link--next" rel="next" href="#">
        <span class="element-invisible">Next page</span>
      </a>
    </li>
    <li class="pager__item pager__item--last">
      <a class="pager__link pager__link--last" href="#">
        <span class="element-invisible">Last page</span>
      </a>
    </li>
  </ul>
</nav>

We probably don't want all of those classes in the Stark theme, but they can be added to Bartik and Seven. I also add icons to the first, previous, next, and last items using :before in CSS.

we dont want any classes in the stark theme unless theres a really really really good reason for it ;)

Moving to core queue.

Could someone please update the issue summary with the current proposed syntax?

@criz can you assign this issue to yourself so that we know you are working on it?
Thanks!

@criz: Yes, we can do away with a class on the individual list items. I’d like to standardize on this pattern with @jenlampton and @mortendk: In most cases, for `ol` or `ul` we can add a class to the list and allow descendent items to be styled using a child selector (`>`). So I would add `.pager__items`to the list (I think a class name with “items” in it is a good convention barring anything else).

I also removed the pager__label class on the heading: since it’s hidden, it shouldn’t need to be styled. BTW, does anyone know why we have a hidden heading *and* an aria-label attribute? Won’t that cause aria-savvy screen readers to read out something like “Entering navigation; Pagination; Pagination List”. *If* that’s the case, it’d be a bit verbose. Also per #2052473: Add aria-label or aria-describedby attributes to all <nav> elements, I’ve moved the aria-label to the <nav> element. I also removed the navigation role on the `ul`, it seems weird to have nested navigation landmarks. Needs further a11y review.

<nav class="pager" role="navigation" aria-label="Pagination">
  <h4 class="visually-hidden">Pagination</h4>
  <ul class="pager__items">
    <li><a href="#"><i class="icon icon--go-first" aria-hidden="true"></i><span class="visually-hidden">First page</span></a></li>
    <li><a  href="#" rel="prev"><i class="icon icon--go-previous" aria-hidden="true"></i><span class="visually-hidden">Previous page</span></a></li>
    <li class="is-active"><a  href="#"><span span class="visually-hidden">Current page </span>2</span></li>
    <li><a href="#"><span class="visually-hidden">Page </span>3</a></li>
    <li><a href="#"><span class="visually-hidden">Page </span>4</a></li>
    <li role="presentation"><i class="icon icon--ellipsis" aria-hidden="true"></i></li>
    <li><a href="#"><span class="visually-hidden">Page </span>12</a></li>
    <li><a href="#" rel="next"><i class="icon icon--go-next" aria-hidden="true"></i><span class="visually-hidden">Next page</span></a></li>
    <li><a href="#"><i class="icon icon--go-last" aria-hidden="true"></i><span class="visually-hidden">Last page</span></a></li>
    </ul>
</nav>

Updated issue summary.

Since the CSS really is a separate issue, we’re in agreement.

Since we're using aria-label on the nav element, we can get rid of the h4.

Also, shouldn't we remove the <a> on the current item?

@dcmounard Makes sense to not have an empty link like that, although if it were disabled that might be sufficient. The alternative (which would also be fine) would be to replace the <a> with a <span> for the active item, and add a class so stylesheets can target the link/span independent of element type.

Also, I just noticed we have role="presentation" on the ellipsis item. Shouldn’t screen readers read that out? I imagine an unannounced jump from page 4 to page 12 would be confusing without it.

I would agree with removing role="presentation" from the ellipsis.

http://stackoverflow.com/questions/453189/what-do-screen-readers-do-with...

@dcmounard There's been a bunch of discussion about this here #2052473: Add aria-label or aria-describedby attributes to all <nav> elements & #2045039: Cleanup HTML heading structure

More discussion over on GDO:
https://groups.drupal.org/node/157959

Updating proposed version by comment #60

This is my very first contribution to Drupal, so please be nice.

This patch contains the following:

• Implemented the markup as per #1912608-60: Update pagination markup for new CSS standards and improved accessibility
• Moved the conditional and loop aspects of pagination to the twig file
• Removed the code from views.theme.inc that generated the array that in d7 was sent to the link function. My coding standards may not be up to par, but as I'm a n00b, please go ahead and make suggestions.
• Updated the css files for Seven and Bartik to match the new markup.

just a reroll.

In general I wonder how much in scope of this issue is to also change the mini pager of views.

Here are some small points in the review.

1. +++ b/core/includes/pager.inc
   @@ -224,192 +214,63 @@ function template_preprocess_pager(&$variables) {
   +    // If there is a better way to generate the URL, by all means go for it!
   +    $variables['current_path'] = $base_url . '/' . $current_path;
   

   \Drupal::request()->getRequestUri() should return you the value.

2. +++ b/core/modules/system/templates/pager.html.twig
   @@ -12,6 +12,37 @@
   +      ¶
   ...
   +      ¶
   ...
   +    ¶
   

   spaces all over the place

3. +++ b/core/modules/views/views.theme.inc
   @@ -62,7 +62,7 @@ function template_preprocess_views_view(&$variables) {
   -  $variables['title'] = !empty($view->views_ui_context) ? Xss::filterAdmin($view->getTitle()) : '';
   +  $variables['title'] = !empty($view->views_ui_context) ? filter_xss_admin($view->getTitle()) : '';
    
      if ($view->display_handler->renderPager()) {
        $exposed_input = isset($view->exposed_raw_input) ? $view->exposed_raw_input : NULL;
   @@ -203,7 +203,7 @@ function template_preprocess_views_view_fields(&$variables) {
   
   @@ -203,7 +203,7 @@ function template_preprocess_views_view_fields(&$variables) {
          }
    
          if (!empty($variables['options']['separator']) && $previous_inline && $object->inline && $object->content) {
   -        $object->separator = Xss::filterAdmin($variables['options']['separator']);
   +        $object->separator = filter_xss_admin($variables['options']['separator']);
          }
    
          $object->class = drupal_clean_css_identifier($id);
   @@ -453,7 +453,7 @@ function template_preprocess_views_view_summary_unformatted(&$variables) {
   
   @@ -453,7 +453,7 @@ function template_preprocess_views_view_summary_unformatted(&$variables) {
      foreach ($variables['rows'] as $id => $row) {
        // Only false on first time.
        if ($count++) {
   -      $variables['rows'][$id]->separator = Xss::filterAdmin($variables['options']['separator']);
   +      $variables['rows'][$id]->separator = filter_xss_admin($variables['options']['separator']);
        }
        $variables['rows'][$id]->attributes = array();
        $variables['rows'][$id]->link = $argument->summaryName($row);
   @@ -643,7 +643,7 @@ function template_preprocess_views_view_table(&$variables) {
   
   @@ -643,7 +643,7 @@ function template_preprocess_views_view_table(&$variables) {
              // Place the field into the column, along with an optional separator.
              if (!empty($column_reference['content'])) {
                if (!empty($options['info'][$column]['separator'])) {
   -              $column_reference['content'] .= Xss::filterAdmin($options['info'][$column]['separator']);
   +              $column_reference['content'] .= filter_xss_admin($options['info'][$column]['separator']);
                }
   

   Out of scope and totally wrong

Thanks for the feedback, I'll clean things up and take another shot at the patch.

I've debugged all of the errors from Simpletest and rerolled.

Note, I didn't use any of mgifford's work but instead continued from my original patch.

Sorry for the preview (and EMPTY) patch. This time with actual data plus interdiff!

Some of my earlier work doesn't play nice with the views pager, so going to keep on working on it.

A few rogue instances of filter_xss_admin were causing issues with the CommentBlockTest and GlossaryTest. This time skipping the interdiff because I think the other patches are a little messy and confusing. Still learning this whole process. :)

Somehow I had lost the necessary changes to the various tests (needed because the markup changed obviously). This patch includes those, fixes some oversights on how I implemented getRequestUri() originally, plus a CSS tweak to hide the pagination icons in views previewing.

1. +++ b/core/includes/pager.inc
   @@ -223,193 +213,72 @@ function template_preprocess_pager(&$variables) {
   +      ¶
   ...
   +      ¶
   ...
   +        $items['previous'] = $li_previous['page']; ¶
   ...
   +      ¶
   ...
   +      ¶
   ...
   +        ¶
   ...
   +      ¶
   ...
   +    ¶
   ...
   +    ¶
   ...
   +    ¶
   +    $variables['pager_current'] = $pager_current; ¶
   
   +++ b/core/modules/system/lib/Drupal/system/Tests/Pager/PagerTest.php
   @@ -91,20 +93,20 @@ protected function assertPagerItems($current_page) {
   -
   +    ¶
   
   +++ b/core/modules/system/templates/pager.html.twig
   @@ -12,6 +12,34 @@
   \ No newline at end of file
   

   Lots of trailing whitespace was introduced, as well as files with no newline at the eof. Please refer to the coding standards and if possible setup your editor to auto-fix these issues for you as you code.

2. +++ b/core/modules/system/templates/pager.html.twig
   @@ -12,6 +12,34 @@
   +        <li><a href="{{ current_path }}"><i class="icon icon--go-first" aria-hidden="true"></i><span class="visually-hidden">First page</span></a></li>
   ...
   +        <li><a href="{{ current_path }}?page={{ items.previous }}" rel="prev"><i class="icon icon--go-previous" aria-hidden="true"></i><span class="visually-hidden">Previous page</span></a></li>
   ...
   +          <li class="is-active"><a href="{{ current_path }}?page={{ key }}"></a><span class="visually-hidden">Current page</span>{{ item }}</span></li>
   ...
   +        <li><a href="{{ current_path }}?page={{ items.next }}" rel="next"><i class="icon icon--go-next" aria-hidden="true"></i><span class="visually-hidden">Next page</span></a></li>
   ...
   +        <li><a href="{{ current_path }}?page={{ items.last }}"><i class="icon icon--go-last" aria-hidden="true"></i><span class="visually-hidden">Last page</span></a></li>
   

   "First page", "Previous page", "Next page", "Last page", all need to be translatable (eg with {{ 'String'|t }}?)

3. +++ b/core/modules/system/templates/pager.html.twig
   @@ -12,6 +12,34 @@
   +      {% for key,item in items.pages %}
   

   http://twig.sensiolabs.org/doc/tags/for.html Suggests separating key and value with a comma and a space.

4. +++ b/core/modules/system/templates/pager.html.twig
   @@ -12,6 +12,34 @@
   +          <li><a href="{{ current_path }}?page={{ key }}"><span class="visually-hidden">Page </span>{{ item }}</a></li>
   

   <span>Page </span>{{ item }} in theory presents a problem for translatability, because the unit of text that needs to be translatable is "Page @count" (thinking in Drupal 7 terms).

5. +++ b/core/themes/bartik/css/style.css
   @@ -276,25 +276,48 @@ table ul.links li {
   +nav.pager {
   +  text-align: center;
   +}
   

   To my knowledge, this is overqualified, I might be wrong (if so, link?)

6. +++ b/core/themes/bartik/css/style.css
   @@ -276,25 +276,48 @@ table ul.links li {
   +.pager__items .icon--ellipsis:before {
   +  content: "...";
   
   +++ b/core/themes/seven/style.css
   @@ -543,28 +546,42 @@ div.submitted {
   +.pager__items .icon--ellipsis:before {
   +  content: "...";
   +}
   

   Why are we using three dots here instead of "\2026" ?

+++ b/core/includes/pager.inc
@@ -223,193 +213,72 @@ function template_preprocess_pager(&$variables) {
+    // Strip pager querystring because it gets added back in pager.html.twig but leave it otherwise for ajax calls
+    if (strpos($uri,'?page=') !== FALSE) {

Not 100% sure on this, but can't there be more than 1 pager on the url parameters? If so, sounds like we may need a test for this.

Thanks, jwilson3! I'm on it!!

My reroll included fixes for the following from #83

1. I set my editor (Komodo Edit) to remove the whitespace characters but I'm not 100% sure if it is working. I reviewed my files and don't see any at the moment, but I may have missed some. What editor/tools do you use, jwilson3?
2. Translate filter added to all text within pager.html.twig
3. Space added after comma
4. I used {{ trans }}translate me{{ endtrans }} here at it seems to be working
5. Changed nav.pager to .pager
6. Three dots changed to \2026

And re: #84, there is a testTwoPagers() function in CommentPageTest.php which passes, but that may not be testing for what jwilson3 was imagining.

Looks like the trailing whitespace issues are gone. Good! I use Sublime Text 3 with the TrailingSpaces package installed via Package Control.

+++ b/core/modules/system/templates/pager.html.twig
@@ -12,6 +12,34 @@
+          <li><a href="{{ current_path }}?page={{ key }}"><span class="visually-hidden">{% trans %}Page {{ item }}{% endtrans %}</span>{{ item }}</a></li>

Screen readers will speak this as "Page two two". le sigh. :-/ I suppose the former solution <span ...>{{ 'Page'|t }} </span>{{ item}} is the lesser of the two evils, but we would need a nod from i18n people first.

But there seems to be one more major problem with this implementation. The "?" in {{ current_path }}?page={{ key }} needs to have the ability to be an "&" in the case when the current_path already contains a query string with other URL parameters. Its hard to believe that all tests are passing the way this code is written and makes me thing we aren't testing this scenario well enough. I could be wrong about this.

Cool, now to move the simpletest files so that the patch applies.

We could go back to generating the pager paths in the preprocess function, which I believe would solve that problem. But it creates a new one: matching the paths with the appropriate list item in the twig template.

Rerolled with correct PSR-4 filepaths. We still need to iron out the two issues raised by jwilson3 in #1912608-88: Update pagination markup for new CSS standards and improved accessibility before this is RTBC.

Can we have some screenshots to ensure everything is still ok. (spark vs bartik vs seven)

Bartik needed a small CSS adjustment, hence the rerolled patch. Screenshots.zip contains screenshots for Bartik's and Seven's dblog and views preview pager. Other pagers available on request. I didn't make any changes to the Stark pager styles since there are, in fact, not any. Should there be?

Adding styles for Stark and removing double page number for screen readers as per Lewis.

Added styles for Stark (in system.theme.css) and removed double page number for screen readers. Still have a question for a views guru about the way I'm generating the request URI. Relevant lines are 258-265 in pager.inc.

Functionality seems to work which is great!

Some things I'd like to point:

1. +++ b/core/includes/pager.inc
   @@ -223,193 +213,59 @@ function template_preprocess_pager(&$variables) {
   +  //$current_path = current_path();
   

   This should be removed.

2. +++ b/core/includes/pager.inc
   @@ -223,193 +213,59 @@ function template_preprocess_pager(&$variables) {
   +      $items['first'] = true;
   ...
   +        $ellipsis['prev'] = true;
   ...
   +        $ellipsis['next'] = true;
   

   Constants should be uppercase.

3. +++ b/core/includes/pager.inc
   @@ -223,193 +213,59 @@ function template_preprocess_pager(&$variables) {
   +    // Get uri
   

   Comments should end with dot.

4. +++ b/core/includes/pager.inc
   @@ -223,193 +213,59 @@ function template_preprocess_pager(&$variables) {
   +    // Strip pager querystring because it gets added back in pager.html.twig but leave it otherwise for ajax calls
   

   In general, all lines of code should not exceed 80 characters.

5. +++ b/core/includes/pager.inc
   @@ -223,193 +213,59 @@ function template_preprocess_pager(&$variables) {
   +          $items['pages'][$i-1] = $query['page'];
   

   There should be white space before and after -

6. +++ b/core/includes/pager.inc
   @@ -223,193 +213,59 @@ function template_preprocess_pager(&$variables) {
   +    if (strpos($uri,'?page=') !== FALSE) {
   

   There should be white space after comma

Thanks, Lauri. I'll make your recommended changes.

This patch contains the following additions/changes from the previous one:

• Fixed the coding standards issues raised by Lauri
• Reverted back to original pager.inc because we need querytsring/paths functionality of l()
• Added dream markup into template_preprocess_pager()

Note: because "item_list__pager" is the theme suggestion, we still have <div class="item-list"> wrapping <ul class="pager__items">

We're green! I tested the patch by hand and it does what it should do. I'd suggest RTBC.

1. +++ b/core/modules/system/css/system.theme.css
   @@ -161,19 +161,37 @@ abbr.ajax-changed {
   +.is_active {
      font-weight: bold;
    }
   

   This seems to be an overly generic style. also, according to standard class naming conventions, it should be .is-active. It looks like elsewhere in the other stylesheets this is done as .pager__items .is-active

2. +++ b/core/modules/system/src/Tests/Pager/PagerTest.php
   @@ -97,14 +99,14 @@ protected function assertPagerItems($current_page) {
   +        $this->assertNoClass($element, 'is-active', "Item for page $page has no .is-active class.");
   +        //$this->assertClass($element, 'pager-item', "Item for page $page has .pager-item class.");
   ...
   -        $this->assertNoClass($element->a, 'active', "Link to page $page is not active.");
   +        //$this->assertNoClass($element, 'is-active', "Link to page $page is not active.");
   

   Please remove the lines of code that have been commented out.

This seems to be an overly generic style. also, according to standard class naming conventions, it should be .is-active. It looks like elsewhere in the other stylesheets this is done as .pager__items .is-active

Yes that's right, according toe SMACSS you usually chain the class like this .pager__items a.is-active or ideally .pager__item.is-active?

With excellent guidance from Mark Carver, I properly restored the dream markup to the twig template. John Albin also helped in guiding me towards proper class naming conventions.

103: 1912608-pagination-twig-103.patch queued for re-testing.

Found a small bug, fixed and ready for review.

Something in testActiveClass() is breaking here on d.o but it's fine when testing locally. Trying again. Also simplified ellipsis markup.

Rerolled to remove exceptions, but CommentBlockTest is failing on line 98. I'm a little at a loss as to why, but initial research makes me wonder if the URLs generated in the comment block are incorrect.

Rerolled.

For some reason I don't completely understand, the ellipsis character (…) was causing CommentBlockTest to fail. Using … fixed it.

That's the right way to go, but don't think it's related to #44987: [meta] Use the UTF-8 ellipsis character instead of "..." in the user interface

Moved the first/prev/next/last text from CSS files into pager.inc (so they are translatable).

1. +++ b/core/includes/pager.inc
   @@ -229,30 +229,26 @@ function template_preprocess_pager(&$variables) {
   -          'rel' => 'first',
   ...
              'rel' => 'prev',
   
   @@ -269,12 +266,11 @@ function template_preprocess_pager(&$variables) {
              'rel' => 'next',
   
   @@ -283,133 +279,80 @@ function template_preprocess_pager(&$variables) {
   -          'rel' => 'last',
   
   +++ b/core/modules/system/css/system.theme.css
   @@ -161,19 +161,31 @@ abbr.ajax-changed {
   +  content: "\00AB\00A0 first";
   ...
   +  content: "\003C\00A0 previous";
   ...
   +  content: "next \00A0\003E";
   ...
   +  content: "last \00A0\00BB";
   
   +++ b/core/themes/bartik/css/style.css
   @@ -277,25 +277,39 @@ table ul.links li {
   +  content: "\00AB\00A0 first";
   ...
   +  content: "\003C\00A0 previous";
   ...
   +  content: "next \00A0\003E";
   ...
   +  content: "last \00A0\00BB";
   
   +++ b/core/themes/seven/style.css
   @@ -546,28 +549,22 @@ div.submitted {
   +  content: "\00AB\00A0 first";
   ...
   +  content: "\003C\00A0 previous";
   ...
   +  content: "next \00A0\003E";
   ...
   +  content: "last \00A0\00BB";
   

   These strings, unfortunately, are not/cannot be translated because they are directly in the CSS.

   We should instead use the rel attribute to populate this or even probably more appropriately the title (although I am not entirely sure how the screen readers would react to a title being populated... data attribute mayhaps?):

   (first)

   content: "\00AB\00A0 " attr(rel);
   

2. +++ b/core/modules/system/src/Tests/Pager/PagerTest.php
   @@ -97,14 +97,12 @@ protected function assertPagerItems($current_page) {
   -        $this->assertFalse(isset($element->a), 'Item for current page has no link.');
   ...
   +        $this->assertTrue(isset($element->a), 'Item for current page has link.');
   
   +++ b/core/modules/views_ui/src/Tests/PreviewTest.php
   @@ -110,58 +110,58 @@ public function testPreviewWithPagersUI() {
   -    $this->assertFalse(isset($elements[0]->a), 'Element for current page has no link.');
   ...
   +    $this->assertTrue(isset($elements[0]->a), 'Element for current page has no link.');
   ...
   -    $this->assertFalse(isset($elements[3]->a), 'Element for current page has no link.');
   ...
   +    $this->assertTrue(isset($elements[3]->a), 'Element for current page has link.');
   

   Why are we changing this? This really falls outside of the scope of what should really be changed in this issue.

3. +++ b/core/modules/system/src/Tests/Pager/PagerTest.php
   @@ -97,14 +97,12 @@ protected function assertPagerItems($current_page) {
   -        $this->assertClass($element, 'pager-item', "Item for page $page has .pager-item class.");
   

   Shouldn't this be changed to look for .pager__item instead of removing it altogether?

4. +++ b/core/modules/system/templates/pager.html.twig
   @@ -12,6 +12,30 @@
   +  <nav class="pager" role="navigation" aria-label="Pagination">
   +    <h4 class="visually-hidden">{{ 'Pagination'|t }}</h4>
   

   Wouldn't having both an aria-label a visually-hidden <h4> cause a screen reader to announce "Pagination" twice?

   Furthermore, the aria-label is not translated either, should be aria-label="{{ 'Pagination'|t }}".

1. that CSS is gone already as per Joel. Were you looking at the interdiff?

2. When we added a link to the current item in the dream markup, of course those tests have to change.

3. In the latest patch that change is already made.

4. I don't know, but that was from the dream markup in comment 60 I think.

The issue summary is severely out of date.

I'm doublechecking now, but I really think we can get rid of the aria-label="Pagination" in #119.4 above. @Mark Carver - good catch on the CSS as well.

@scaragucc' - #60 is coming from #2052473: Add aria-label or aria-describedby attributes to all <nav> elements - It doesn't make sense though where there are duplicate titles for the heading.

This would add semantic value using ARIA to the navigation:

+++ b/core/modules/system/templates/pager.html.twig
@@ -12,6 +12,30 @@
+  <nav class="pager" role="navigation" aria-labelledby="pagination-heading">
+    <h4 id="pagination-heading" class="visually-hidden">{{ 'Pagination'|t }}</h4>

That is inline with other efforts to with navigation.

In #122 I was just looking at Mark's comment. Looking at the patch in #117 I can't find #119.1. @scaragucc' replied nicely to 2 & 3.

I've re-rolled the patch to address the concerns of duplication with #4.

#117 needs to have the extra span (the visible one) removed from the pager links. mark explained to me that we can add a data-label attribute and access that in the CSS content property. I will take that on as soon as the issue summary is updated and we know exactly what else needs to be done.

seven/style.css moved to seven/css/style.css

1. +++ b/core/includes/pager.inc
   @@ -185,11 +185,15 @@ function template_preprocess_pager(&$variables) {
   -    t('« first'),
   -    t('‹ previous'),
   -    '',
   -    t('next ›'),
   -    t('last »'),
   ...
   +    t('« first'),
   +    t('< prev'),
   +    t('next >'),
   +    t('last »'),
   

   Were going from using « and ‹ to using « and <, which looks horrible in most fonts. TBH,

   We should either:

   a) use the html encoded version of ‹, which is ‹ and its counterpart › instead of < and > respectively?

   OR

   b) Just revert to using the strings that are there now.

   I'm of a mind to revert, to lessen the need for new string retranslations.

2. +++ b/core/modules/system/css/system.theme.css
   @@ -163,17 +163,17 @@ abbr.ajax-changed {
   -.item-list .pager li {
   +.pager__items .pager__item {
   
   +++ b/core/themes/bartik/css/style.css
   @@ -277,25 +277,27 @@ table ul.links li {
   +.pager__items .pager__item {
   

   This looks to be overqualified. Would just .pager__item work there, or is the nature of this being in a ul structure causing weird inheritance issues?

3. +++ b/core/modules/system/css/system.theme.css
   @@ -163,17 +163,17 @@ abbr.ajax-changed {
   -.pager-current {
   +.pager__items .is-active {
   
   +++ b/core/themes/bartik/css/style.css
   @@ -277,25 +277,27 @@ table ul.links li {
   +.pager__items .is-active {
   

   Lewis pointed out above in #102 that the ideal format here would be to us chaining of the form .pager__item.is-active. Again, not sure about inheritance, but it certainly looks cleaner to do it that way.

#2033211: Move Seven CSS files into css directory was reverted, so #127 is no good any longer.

Ok, I think this gets those 3 points. And ya, annoying with the reversion, but... The css seems a bit messed up:

Patch is looking good.

I think we should use "‹ previous" and "next ›" for two reasons:

1) Drupal is UTF8, so there is no reason to not encode these entities in the source (this was an argument used in another issue from long ago when the arrows were first introduced, but i cannot find the issue to link it here...
2) as mentioned above in #129, switching to the HTML entities introduces needless string changes, impacting translators.

Couple nit picks with the tests:

1. +++ b/core/modules/system/src/Tests/Pager/PagerTest.php
   @@ -107,24 +105,24 @@ protected function assertPagerItems($current_page) {
   -      $this->assertClass($first, 'pager-first', 'Item for first page has .pager-first class.');
   +      $this->assertClass($first, 'pager__item-first', 'Item for first page has .pager__item-first class.');
   
   +++ b/core/modules/views_ui/src/Tests/PreviewTest.php
   @@ -106,58 +106,58 @@ public function testPreviewWithPagersUI() {
   +    $this->assertClass($elements[1], 'pager__item', "Element for page 2 has .pager__item class.");
   ...
   +    $this->assertClass($elements[2], 'pager__item', "Element for page 3 has .pager__item class.");
   ...
   +    $this->assertClass($elements[3], 'pager__item-next', "Element for next page has .pager__item-next class.");
   ...
   +    $this->assertClass($elements[4], 'pager__item-last', "Element for last page has .pager__item-last class.");
   

   Can we consistently use single quotes in all of the modified asserts in our tests? There is a mix of both single and double quotes. In some cases, this patch is already simplifying some of them to single quotes. Let's get them all.

2. +++ b/core/modules/views_ui/src/Tests/PreviewTest.php
   @@ -106,58 +106,58 @@ public function testPreviewWithPagersUI() {
   -    $this->assertClass($elements[1], 'pager-previous', "Element for previous page has .pager-previous class.");
   +    $this->assertClass($elements[1], 'pager__item-previous', 'Item for previous page has .pager__item-previous class.');
   ...
   -    $this->assertClass($elements[5], 'pager-next', "Element for next page has .pager-next class.");
   +    $this->assertClass($elements[5], 'pager__item-next', 'Item for next page has .pager__item-next class.');
   ...
   -    $this->assertClass($elements[6], 'pager-last', "Element for last page has .pager-last class.");
   +    $this->assertClass($elements[6], 'pager__item-last', 'Item for last page has .pager__item-last class.');
   

   In some of the test messages we're using "Element for X" and in others we're using "Item for X". I dont know if this is on purpose, if one means the actual link element, and the other means the wrapper list item, or what? In the cases above, we've switched from one to the other, but in other places the entire set uses "Element for X". Can we just use "Element for" instead of "Item for" everywhere for consistency?

And yeah, this needs another re-roll now that #2033211 is back in, and now that #2227907 is in too.

$ git checkout 8.0.x 
Switched to branch '8.0.x'
Your branch is up-to-date with 'origin/8.0.x'.

$ git pull
Current branch 8.0.x is up to date.

$ git apply 1912608-pagination-twig-131.patch
error: patch failed: core/includes/pager.inc:229
error: core/includes/pager.inc: patch does not apply
error: core/themes/seven/style.css: No such file or directory

+++ b/core/includes/pager.inc
@@ -229,30 +233,26 @@ function template_preprocess_pager(&$variables) {
-          'title' => t('Go to first page'),
...
-          'title' => t('Go to previous page'),

@@ -269,12 +270,11 @@ function template_preprocess_pager(&$variables) {
-          'title' => t('Go to next page'),

@@ -283,133 +283,80 @@ function template_preprocess_pager(&$variables) {
-          'title' => t('Go to last page'),
...
-                  'title' => t('Go to page @number', array('@number' => $i)),

One more thing: Screen-readers do not read text in the title attribute aloud. Removing this text in the links actually reduces usability for sighted users who will not see any helper text appear when the links are hovered with a mouse.

+++ b/core/includes/pager.inc
@@ -283,133 +283,80 @@ function template_preprocess_pager(&$variables) {
       for (; $i <= $pager_last && $i <= $pager_max; $i++) {
-        if ($i < $pager_current) {
...
-                  'interval' => ($pager_current - $i),
...
-        if ($i > $pager_current) {
...
-                  'interval' => ($i - $pager_current),

Found one more issue: the pager interval logic stopped working when $i > $pager_current. I propose to use absolute value.

I've addressed all the issues my feedback above. I've also fixed some styles that seemed to have been inadvertently removed from some of the stylesheets in previous patches, and made some further adjustments in the Seven stylesheet which wasn't adding enough whitespace below the pager.

Also, I've finally updated the issue summary to show where we are at today, updating the code snippets and added the twig template proposal, for easier review. What started off as very streamlined "Dream Markup" flowered into let's do this Aria support thing out of the box. I don't think the two are diametrically opposed, but adding all the Aria stuff has certainly taken us away from streamlined and simple markup.

I think the goal at this point would be to provide a solid foundation from where themers can easily swap out the tags in the twig template without losing much of the aria support, and this latest round of patches from mgifford and my subsequent cleanup in this patch gets us there. I'm sorry that there is not an interdiff, but the problem was again that the patch would not apply, so I make my own improvements as I went along re-applying the patch by hand, so I didn't have somethign to base an interdiff from.

There is real tangible improvement here, notably in the css, best summarized by this part of the patch:

 /**
  * Pagination.
- * The item-list CSS uses quite strong selectors, we have to match them here to
- * override.
  */

This followup fixes a couple test case string cleanups that I missed from #136.

Fixed last remaining failures from testTwoPagers() in CommentPagerTest.php where $this->clickLink('Next page'); fails to navigate between pager links because the Anchor tags no longer have plain text links, but use HTML spans for visually-hidden text inside the links.

Had to write a custom method: clickLinkWithXPath() (based on clickLink) which follows a link based on a provided xpath query that targets a specific anchor tag.

+++ b/core/modules/system/src/Tests/Pager/PagerTest.php
@@ -91,14 +91,13 @@ protected function assertPagerItems($current_page) {
+        $this->assertTrue(isset($element->a), 'Element for current page has link.');

Missed a minor cleanup.

Hrm. I just globally searched and replaced all the tests that were referring to pagers, without realizing we haven't done any work on views mini pager here -- which is where the current errors are coming from.

Because the classes and styles for the pagers in core themes are affected by this code, I think we may need rope in the mini pager into this issue. Fun :-/

Thanks for tracking this down.. Another case were tests reveal something we'd overlooked..

Mike, I'm at Drupalaton now and was hoping to work again on this ticket at the sprints. Shall I, or do you want to keep on working on it? Thanks, Adam

Please proceed @scaragucc' and thanks for asking. Sorry I didn't get to this sooner.

Should we take care of the views mini pager here or create a new issue for that? This issue is getting bigger and bigger all the time.

Getting back into the mix!

After discussion with @scaragucc' and @mortendk, I thought maybe we should create a follow-up issue for the views mini pager.

I also attached a patch where the mini pager test changes have been removed.

Sending to review.

+++ b/core/themes/seven/style.css
@@ -546,28 +549,22 @@ div.submitted {
+  content: "\00AB\00A0 first";
...
+  content: "\003C\00A0 previous";
...
+  content: "next \00A0\003E";
...
+  content: "last \00A0\00BB";

This seems a bit odd to me, it's not purely presentational nor translatable. Can you explain this? I see you have hidden translations for screen readers which is good. Though if I were to translate my site to any other language those English words would have to be swapped out in the theme generate the css file on the fly? Maybe you have a work around?

I also want to say, I like that template is filling out, more HTML in the template is looking great. Also love that this has more tests too. And BEMy class names too! Just because I'm against the words in CSS doesn't mean I don't like the patch. It's going in a good direction, I think just not that bit:) Nice work, keep it up!

@joelpittet: Im not sure if I'm missing something because I cannot find that CSS from #151 anywhere. Is that actually used somewhere?

@lauriii oh my bad, I think I was reading a different patch #115, Not sure how that happened... my only guess is I took the bottom one in the IS instead of the top.

Hiding older patches.

So there is the follow-up issue with the mini pager #2318341: Views mini pager markup

I just rolled up an instance here http://s4c72eb8a63bda03.s2.simplytest.me/admin/reports/dblog

Seems RTBC to me. Here's a screenshot.

More functionality can still be moved to Twig files.

@lauriii, This issue has been vetted by and has been worked on by a number of people now. The Twig file looks very complete. I'm not sure what else you would want in there that is not already there. If you can come up with a list, then we can collectively discuss and determine if any of it is valid, in order to change this back to Needs Work, or better yet, file as a followup.

Whoops. I totally missed that you *assigned yourself* to keep working on this. Sorry for overstepping there. Have a blast!

A list of what you're going to move into the template would be good, particularly if its a great departure from the current patch which imho is ready to go in as is, because its very clean.

@jwilson3 No problem! I was thinking of moving spans for the screen readers from prerender to Twig, so it's easier to override if there's a need for that.

Ok here's my patch. What do you think? Is this change good or should we go with the more preprocess driven solution?

Overall, I like this approach! Nice work!

At least some of the failures have to do with these lines:

+++ b/core/includes/pager.inc
@@ -227,187 +227,80 @@ function template_preprocess_pager(&$variables) {
+    $li_previous['attributes'] = new Attribute(array('title' => t('Go to previous page')));
...
+    $li_next['attributes'] = new Attribute(array('title' => t('Go to next page')));

They are missing 'rel' => 'prev' and 'rel' => 'next' attributes.

+++ b/core/includes/pager.inc
@@ -227,187 +227,80 @@ function template_preprocess_pager(&$variables) {
-        // Below is ignored by default, supplied to support hook_link_alter
-        // implementations.
-        'pager_context' => array(
-          'link_type' => 'first',
-          'element' => $element,
-        ),
...
-        // Below is ignored by default, supplied to support hook_link_alter
-        // implementations.
-        'pager_context' => array(
-          'link_type' => 'previous',
-          'element' => $element,
-        ),
...
-        // Below is ignored by default, supplied to support hook_link_alter
-        // implementations.
-        'pager_context' => array(
-          'link_type' => 'next',
-          'element' => $element,
-        ),
...
-        // Below is ignored by default, supplied to support hook_link_alter
-        // implementations.
-        'pager_context' => array(
-          'link_type' => 'last',
-          'element' => $element,
-        ),
...
-                // Below is ignored by default, supplied to support hook_link_alter
-                // implementations.
-                'pager_context' => array(
-                  'link_type' => 'item',
-                  'element' => $element,
-                  'interval' => ($pager_current - $i),
-                ),
...
-                // Below is ignored by default, supplied to support hook_link_alter
-                // implementations.
-                'pager_context' => array(
-                  'link_type' => 'item',
-                  'element' => $element,
-                  'interval' => ($i - $pager_current),
-                ),

One more thing, obviously we're not using a render function for the links here anymore, so the question is what, if anything, was using this hook_link_alter information, and why was that code added in the first place? You can probably check with git blame to figure out where/when it was introduced.

Added the rel attributes - test now passing locally.

Swapped 'previous' to 'prev'.

Re #163 - I don't think we need that anymore, as a themer can now easily override the markup using a template (unless I'm misunderstanding).

It looks like this was originally added here: https://www.drupal.org/node/1898474