Meta issue: #1980004: [meta] Creating Dream Markup

Problem/Motivation

We need better markup for pagination in terms of semantics and accessibility. Also, we need to follow our Drupal 8 style guide.

Proposed resolution

Much of this issue involves a discussion about removing the list from the markup. The version using list-items has more advocates than the version without lists, it is what we are currently using, and is what many other frameworks and CMSs also use. Therefore, the proposal at this point is

  1. Stop using theme_item_list; move as much markup as possible into the pager.html.twig Twig template.
  2. Follow Drupal 8 CSS Guidelines for CSS class nomenclature.
  3. Use Drupal's own visually-hidden class in combination with Aria attributes like aria-labelledby and aria-hidden to improve the way pagers are read aloud by screen-readers.

Current Drupal 8 pagination markup:

<div class="item-list">
  <ul class="pager">
    <li class="pager-first"><a href="/node" title="Go to first page">« first</a></li>
    <li class="pager-previous"><a href="/node?page=4" title="Go to previous page" rel="prev">‹ previous</a></li>
    <li class="pager-ellipsis">…</li>
    <li class="pager-item"><a href="/node?page=1" title="Go to page 2">2</a></li>
    <li class="pager-item"><a href="/node?page=2" title="Go to page 3">3</a></li>
    <li class="pager-item"><a href="/node?page=3" title="Go to page 4">4</a></li>
    <li class="pager-item"><a href="/node?page=4" title="Go to page 5">5</a></li>
    <li class="pager-current">6</li>
    <li class="pager-item"><a href="/node?page=6" title="Go to page 7">7</a></li>
    <li class="pager-item"><a href="/node?page=7" title="Go to page 8">8</a></li>
    <li class="pager-item"><a href="/node?page=8" title="Go to page 9">9</a></li>
    <li class="pager-item"><a href="/node?page=9" title="Go to page 10">10</a></li>
    <li class="pager-ellipsis">…</li>
    <li class="pager-next"><a href="/node?page=6" title="Go to next page" rel="next">next ›</a></li>
    <li class="pager-last"><a href="/node?page=12" title="Go to last page">last »</a></li>
  </ul>
</div>

Proposed pagination markup:

(Last updated to match comment #202)

<nav class="pager" role="navigation" aria-labelledby="pagination-heading">
  <h4 id="pagination-heading" class="visually-hidden">Pagination</h4>
  <ul class="pager__items">
    <li class="pager__item pager__item--first">
      <a href="/node?status=All&type=All&title=" title="Go to first page">
        <span class="visually-hidden">First page</span>
        <span aria-hidden="true">« first</span>
      </a>
    </li>
    <li class="pager__item pager__item--previous">
      <a href="/node?status=All&type=All&title=" title="Go to previous page" rel="prev">
        <span class="visually-hidden">Previous page</span>
        <span aria-hidden="true">‹ previous</span>
      </a>
    </li>
    <li class="pager__item ">
      <a href="/node?status=All&type=All&title=" title="Go to page 1">
        <span class="visually-hidden">Page</span>
        1
      </a>
    </li>
    <li class="pager__item is-active">
      <a href="/node?status=All&type=All&title=&page=1" title="Current page">
        <span class="visually-hidden">Current page</span>
        2
      </a>
    </li>
    <li class="pager__item ">
      <a href="/node?status=All&type=All&title=&page=2" title="Go to page 3">
        <span class="visually-hidden">Page</span>
        3
      </a>
    </li>
    <li class="pager__item pager__item--next">
      <a href="/node?status=All&type=All&title=&page=2" title="Go to next page" rel="next">
        <span class="visually-hidden">Next page</span>
        <span aria-hidden="true">next ›</span>
      </a>
    </li>
    <li class="pager__item pager__item--last">
      <a href="/node?status=All&type=All&title=&page=2" title="Go to last page">
        <span class="visually-hidden">Last page</span>
        <span aria-hidden="true">last »</span>
      </a>
    </li>
  </ul>
</nav>

This markup is based on feedback from various comments, and includes the following changes:

  • Replace <div class="item-list"> with <nav> wrapper element.
  • Add a visually hidden <h4> element for announcing the section title, link it to the nav wrapper element via a aria-labelledby attribute.
  • Add class pager__items to the unordered list element, following Drupal 8 CSS Style guidelines.
  • Add pager__item classes to all <li> elements, following Drupal 8 CSS Style guidelines.
  • Make current page a link, for more consistent markup, making all elements easier to theme.
  • Change from pager-current class to is-active state, for consistency with Drupal 8 navigation active link state.

The proposed twig template will look like this:

{% if items %}
  <nav class="pager" role="navigation" aria-labelledby="pagination-heading">
    <h4 id="pagination-heading" class="visually-hidden">{{ 'Pagination'|t }}</h4>
    <ul class="pager__items">
      {# Print first item if we are not on the first page. #}
      {% if items.first %}
        <li class="pager__item pager__item--first">
          <a href="{{ items.first.href }}" title="{{ 'Go to first page'|t }}"{{ items.first.attributes }}>
            <span class="visually-hidden">{{ 'First page'|t }}</span>
            <span aria-hidden="true">{{ items.first.text|default('« first'|t) }}</span>
          </a>
        </li>
      {% endif %}
      {# Print previous item if we are not on the first page. #}
      {% if items.previous %}
        <li class="pager__item pager__item--previous">
          <a href="{{ items.previous.href }}" title="{{ 'Go to previous page'|t }}" rel="prev"{{ items.previous.attributes }}>
            <span class="visually-hidden">{{ 'Previous page'|t }}</span>
            <span aria-hidden="true">{{ items.previous.text|default('‹ previous'|t) }}</span>
          </a>
        </li>
      {% endif %}
      {# Add an ellipsis if there are further previous pages. #}
      {% if ellipses.previous %}
        <li class="pager__item pager__item--ellipsis" role="presentation">&hellip;</li>
      {% endif %}
      {# Now generate the actual pager piece. #}
      {% for key, item in items.pages %}
        <li class="pager__item {{ current == key ? 'is-active' : '' }}">
          {% if current == key %}
            {% set title = 'Current page'|t %}
          {% else %}
            {% set title %}
              {%- trans %}Go to page {{ key }}{% endtrans -%}
            {% endset %}
          {% endif %}
          <a href="{{ item.href }}" title="{{ title }}"{{ item.attributes }}>
            <span class="visually-hidden">
              {{ current == key ? 'Current page'|t : 'Page'|t }}
            </span>
            {{ key }}
          </a>
        </li>
      {% endfor %}
      {# Add an ellipsis if there are further next pages. #}
      {% if ellipses.next %}
        <li class="pager__item pager__item--ellipsis" role="presentation">&hellip;</li>
      {% endif %}
      {# Print next item if we are not on the last page. #}
      {% if items.next %}
        <li class="pager__item pager__item--next">
          <a href="{{ items.next.href }}" title="{{ 'Go to next page'|t }}" rel="next"{{ items.next.attributes }}>
            <span class="visually-hidden">{{ 'Next page'|t }}</span>
            <span aria-hidden="true">{{ items.next.text|default('next ›'|t) }}</span>
          </a>
        </li>
      {% endif %}
      {# Print last item if we are not on the last page. #}
      {% if items.last %}
        <li class="pager__item pager__item--last">
          <a href="{{ items.last.href }}" title="{{ 'Go to last page'|t }}"{{ items.last.attributes }}>
            <span class="visually-hidden">{{ 'Last page'|t }}</span>
            <span aria-hidden="true">{{ items.last.text|default('last »'|t) }}</span>
          </a>
        </li>
      {% endif %}
    </ul>
  </nav>
{% endif %}

Remaining tasks

  • Review code.

User interface changes

  • Minimal *visual* changes; the current proposal introduces the current page as a *link*, but it will be visually styled to not appear as a link.
  • An improved page navigation experience for the visually impaired via screen-readers. Page navigation section will be announced by screen-readers as the "Pagination" section, and individual items will be read aloud with natural language such as "Next page" and "Previous page" as opposed to "next" and "prev"; similarly, the current page will be read aloud as "Current Page Six" as opposed to just "Six", and numbered pages will be read aloud as "Page Seven", "Page Eight", "Page Nine".

API changes

Currently, the proposal will change the unwritten "link label tags" API by adding the natural language version of the pagination items that are read aloud by screen-readers, so that they may be altered.


Original report by @criz

View the original report here (the summary here is too long for inclusion at this point).

Sources:

Files: 
CommentFileSizeAuthor
#233 interdiff-1912608-229-230.txt340 byteslauriii
#233 1912608-230.patch31.08 KBlauriii
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,608 pass(es). View
#229 1912608-229-seven-after.png7.26 KBCottser
#229 1912608-229-seven-before.png7.24 KBCottser
#229 1912608-229-stark-after.png13.41 KBCottser
#229 1912608-229-stark-before.png13.87 KBCottser
#229 1912608-229-bartik-after.png6.71 KBCottser
#229 1912608-229-bartik-before.png6.63 KBCottser
#229 interdiff.txt417 bytesCottser
#229 1912608-229.patch31.02 KBCottser
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,452 pass(es). View
#222 pagination_markup-1912608-222.patch30.98 KBlauriii
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,253 pass(es). View
#200 pagination_markup-1912608-200.patch31.07 KBlauriii
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,591 pass(es). View
#200 interdiff-1912608-197-200.txt9.78 KBlauriii

Comments

criz’s picture

Maybe the title attribute would make sense too of course (like we have it now):

<ol role="navigation" class="pagination">
  <li class="prev" rel="prev" title="Go to previous page"><a href="">&laquo;</a></li>
  <li class="active" title="Current page"><a href="">2</a></li>
  <li><a href="" title="Go to page 3">3</a></li>
  <li><a href="" title="Go to page 4">4</a></li>
  <li class="ellipses unavailable"><a href="">&hellip;</a></li>
  <li><a href="" title="Go to page 12">12</a></li>
  <li><a href="" title="Go to page 13">13</a></li>
  <li class="next" rel="next" title="Go to next page"><a href="">&raquo;</a></li>
</ol>

But maybe we can get rid of the "Go to last page" and the "Go to the first page" list items.

jwilson3’s picture

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

mortendk’s picture

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

jwilson3’s picture

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.

rikki_iki’s picture

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.

jwilson3’s picture

#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?

rikki_iki’s picture

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.

mortendk’s picture

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.

rikki_iki’s picture

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...

mortendk’s picture

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

jwilson3’s picture

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).
  4. Other than these, what other criteria am I missing?

jwilson3’s picture

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

rikki_iki’s picture

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.

rikki_iki’s picture

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

jwilson3’s picture

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).

rikki_iki’s picture

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.

criz’s picture

thanks for the jsfiddles, this is really helpful!

@ list vs. spans
I am not sure what would be the best solution here.

My thoughts:
- All pager elements don't need to be ordered (though mostly they are, depending on the styleguide). So at least ol makes no sense. On the other side I see no difference to menu items here. Is there any intention to get rid of the lists also for menus?
- I see the issue with the .inline-list class. But this is a general css architecture issue: http://drupal.org/node/1887918 Imho we shouldn't make assumptions how pagination should look like. So when using lists I am in favour of not such a class -> some more css lines needed. spans would be much better here.
- It is a point that every other cms/framework is using list items for pagination. Probably it would be more convenient for people coming from outside when we use lists.

Maybe we can summarize this issue in the following question:
Do we want to introduce new markup patterns for navigation in Drupal?

There are some points, but maybe the world is not ready yet for this. Read the following article and its comments: http://css-tricks.com/navigation-in-lists-to-be-or-not-to-be/

criz’s picture

Project: » Drupal core
Issue summary: View changes

Updated issue summary.

criz’s picture

Issue summary: View changes

Adding some ressources mentioned in discussion.

steveoliver’s picture

Project: Drupal core »

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>.

jwilson3’s picture

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.

criz’s picture

Another notable point:
- UL or OL seems not be allowed to use role="navigation": http://www.w3.org/html/wg/drafts/html/master/dom.html#wai-aria

As it would be really good to get rid of any wrapping element this is a strong argument for using no lists.
Also we have no hierarchical elements in paginations, so we can safely use spans in my opinion.

See this summary of above mentioned css-tricks article: http://codepen.io/chriscoyier/details/ztxvw

jwilson3’s picture

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?

jwilson3’s picture

Project: » Drupal core
Issue summary: View changes

Added KnpPaginatorBundle to sources.

Snugug’s picture

Project: Drupal core »

A handful of thoughts on 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.
criz’s picture

Okay, trying to summarize all thoughts into 2 versions using Drupal 8 coding standards.

List version:

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

Span version:

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

Some notes:

  • We can't use IDs, simple by the fact that there can be multiple paginations per document.
  • Aria-labelledby seems to need an ID as value: http://www.w3.org/TR/wai-aria/states_and_properties#aria-labelledby So I eliminated it for now. Any ideas? Are we going to introduce this also in Drupal forms?
  • Both versions have more markup than my personal dream-markup had originally. But for the sake of accessibility, reusability and maintainability I think it is okay. Span version looks better imho.
  • Are there any other roles we should use here?
  • For the span version we could use also "pagination--element" instead of "pagination--item", but this would introduce more characters.
jwilson3’s picture

<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!

criz’s picture

Ah yes, some good points. The l-inline class is really bogus, let's remove it. As I understand it now the l-classes should only be used for containers, like "l-header" or "l-footer". My fault.

Was also thinking about using .pager, as it saves some characters, means the same and it is the current approach.

I also talked to fubhy about all this and he is against having another hierarchy with .pager-list, so the list items would just get a .pager-item class. He is right of course. Makes the list version much nicer.

So, here the updated versions:

List version:

<nav class="pager" role="navigation">
  <p class="pager__label element-invisible">Pagination</p>
  <ul class="pager__list">
    <li class="pager__item pager__item--prev"><a href="" rel="prev"><span class="element-invisible">Go to the previous page </span>&laquo;</a></li>
    <li class="pager__item pager__item--active"><a href=""><span class="element-invisible">You are currently on page </span>2</a></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 pager__item--ellipses unavailable"><a href="">&hellip;</a></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__item--next"><a href="" rel="next"><span class="element-invisible">Go to the next page </span>&raquo;</a></li>
  </ul>
</nav>

Span version:

<nav class="pager" role="navigation">
  <p class="pager__label element-invisible">Pagination</p>
  <span class="pager__item pager__item--prev"><a href="" rel="prev"><span class="element-invisible">Go to the previous page </span>&laquo;</a></span>
  <span class="pager__item pager__item--active"><a href=""><span class="element-invisible">You are currently on page </span>2</a></span>
  <span class="pager__item"><a href=""><span class="element-invisible">Go to page </span>3</a></span>
  <span class="pager__item"><a href=""><span class="element-invisible">Go to page </span>4</a></span>
  <span class="pager__item pager__item--ellipses unavailable"><a href="">&hellip;</a></span>
  <span class="pager__item"><a href=""><span class="element-invisible">Go to page </span>12</a></span>
  <span class="pager__item"><a href="" ><span class="element-invisible">Go to page </span>13</a></span>
  <span class="pager__item pager__item--next"><a href="" rel="next"><span class="element-invisible">Go to the next page </span>&raquo;</a></span>
</nav>

Any more ideas? When we agree on a version we should try to get some feedback by screen reader users, as Snugug proposed.

jwilson3’s picture

  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>
    
oresh’s picture

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>
jwilson3’s picture

Screen readers apparently skip the &hellip;. 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>
oresh’s picture

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.

jwilson3’s picture

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.

jwilson3’s picture

Tagging for a11y review.

jwilson3’s picture

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

rikki_iki’s picture

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/

oresh’s picture

@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.

criz’s picture

great, the more minimalistic the better ;)

A few things:

  1. If we use a header element, why exactly h4?
  2. Wouldn't be span better than <i> for the ellipses? Semantics should go over readability I guess and <i> is meant for text, also in html5: "Represents some text in an alternate voice or mood" http://www.w3.org/International/questions/qa-b-and-i-tags.en. Same for the <b>.
  3. Empty tags are ugly, at least a &nbsp; should be present, isn't it? And for the sake of accessibility I think we should add some meaningful (invisible) text in html OR/AND use role="presentation".
  4. And oresh is right that we should use a common class for all pager elements. When I understand it right the next-link is definitely a varation of the sub-component "pager__item" or "pager__link", so I would add a class like "pager__item--next". What we definelity shouldn't use (or need to use) is some css selectors like ".pager > a".
<nav class="pager" role="navigation">
  <h4 class="pager__label element-invisible">Pagination</h4>
  <a class="pager__item pager__item--first" href=""><span class="element-invisible">Go to the first page</span></a>
  <a class="pager__item pager__item--prev" href="" rel="prev"><span class="element-invisible">Go to the previous page</span></a>
  <span class="pager__item pager__item--active"><span class="element-invisible">You are currently on page </span>2</span>
  <a class="pager__item" href=""><span class="element-invisible">Go to page </span>3</a>
  <a class="pager__item" href=""><span class="element-invisible">Go to page </span>4</a>
  <span class="pager__item pager__item--ellipsis" role="presentation">&nbsp;</span>
  <a class="pager__item" href=""><span class="element-invisible">Go to page </span>12</a>
  <a class="pager__item" href=""><span class="element-invisible">Go to page </span>13</a>
  <a class="pager__item pager__item--next" href="" rel="next"><span class="element-invisible">Go to the next page</span></a>
  <a class="pager__item pager__item--last" href=""><span class="element-invisible">Go to the last page</span></a>
</nav>
criz’s picture

About removing classes and ellipses and arrows and stuff see morten's twig template in #3. Done like this it would be easy.

oresh’s picture

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 &nbsp; 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’s picture

Great. By the way, the #dreammarkup metaissue says:

Don't use headers for elements that are by default hidden. For these purposes use span elements. [? needs review by accessibility team]

oresh’s picture

@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 )

jwilson3’s picture

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.

jwilson3’s picture

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.

mortendk’s picture

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>
betz’s picture

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>
oresh’s picture

@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? :)

mortendk’s picture

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 ;)

mgifford’s picture

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/

ry5n’s picture

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.

mgifford’s picture

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>
    manarth’s picture

    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.

    mgifford’s picture

    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.

    dcmouyard’s picture

    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.

    mortendk’s picture

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

    Cottser’s picture

    Project: » Drupal core
    Version: » 8.x-dev
    Component: Markup or variable cleanup (front-end branch) » markup
    Category: feature » task

    Moving to core queue.

    jenlampton’s picture

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

    criz’s picture

    @jenlampton: Yeah, will have a try...

    jenlampton’s picture

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

    criz’s picture

    Assigned: Unassigned » criz

    sure.

    criz’s picture

    Issue summary: View changes

    meta issue added

    criz’s picture

    Here is an issue summary.

    Current version using lists:

    Seems to have more advocates right now (e.g. mgifford and some other a11y guys) than the version without lists.

    <nav class="pager" role="navigation">
      <h4 class="pager__label visually-hidden">Pagination</h4>
      <ul aria-label="Pagination" role="navigation">
        <li class="pager__item" ><a href="#"><i class="icon icon--go-first" aria-hidden="true"></i><span class="visually-hidden">First page</span></a></li>
        <li class="pager__item"><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="pager__item is-active"><a  href="#"><span span class="visually-hidden">Current page </span>2</span></li>
        <li class="pager__item"><a href="#"><span class="visually-hidden">Page </span>3</a></li>
        <li class="pager__item"><a href="#"><span class="visually-hidden">Page </span>4</a></li>
        <li class="pager__item" role="presentation"><i class="icon icon--ellipsis" aria-hidden="true"></i></li>
        <li class="pager__item"><a href="#"><span class="visually-hidden">Page </span>12</a></li>
        <li class="pager__item"><a href="#"><span class="visually-hidden">Page </span>13</a></li>
        <li class="pager__item"><a href="#" rel="next"><i class="icon icon--go-next" aria-hidden="true"></i><span class="visually-hidden">Next page</span></a></li>
        <li class="pager__item" ><a href="#"><i class="icon icon--go-last" aria-hidden="true"></i><span class="visually-hidden">Last page</span></a></li>
        </ul>
    </nav>
    

    Markup based on comment #48.
    Changes:
    - Removing "id=pagination" in h4 (as there could be more than 1 pagers on a page this could be conflicting), changing to aria-label
    - Adding "pager__item" and "pager__label" css classes as pointed out in comment #47
    - Adding shorter hidden texts (comment #49)
    - Changing from active-class to state (comment #47)
    - Adding icon markup as proposed here: https://drupal.org/node/1849712 (comment #47); But I am not 100% sure that we should assume that everybody needs this icon markup here.
    - Changing class "element-invisible" (d7) to "visually-hidden" (d8) https://drupal.org/node/1849712 (comment #47)

    Current version without lists:

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

    Markup based on comment #35.
    Changes:
    - Adding shorter hidden texts (comment #49)
    - Removed &nbsp; inside of ellipses (comment #37)
    - Adding role="menuitem" (comment #47)
    - Changing from active-class to state (comment #47)
    - Adding icon markup as proposed here: https://drupal.org/node/1849712 (comment #47); But I am not 100% sure that we should assume that everybody needs this icon markup here.
    - Changing class "element-invisible" (d7) to "visually-hidden" (d8) https://drupal.org/node/1849712 (comment #47)

    criz’s picture

    Issue summary: View changes

    Updated issue summary.

    criz’s picture

    Issue summary: View changes

    Adding problem/motivation.

    criz’s picture

    Another thing: When adding the icon markup instead of the .pager__item--variant classes we maybe don't need the .pager__item classes itself also. Maybe ry5n can confirm?

    This would save some characters and would make the markup for the version using list-items looking like this:

    <nav class="pager" role="navigation">
      <h4 class="pager__label visually-hidden">Pagination</h4>
      <ul aria-label="Pagination" role="navigation">
        <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>
    
    ry5n’s picture

    @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>
    
    ry5n’s picture

    Issue summary: View changes

    Updated issue summary.

    criz’s picture

    Thx! Updated the issue summary.

    I think we even can go with .pager__items li instead of child selectors then, as the breadcrumb navigation will never be a multi-level-list (list in a list). We should be careful about introducing selectors like .pager__items > li.

    ry5n’s picture

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

    dcmouyard’s picture

    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?

    ry5n’s picture

    @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.

    jwilson3’s picture

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

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

    mgifford’s picture

    mgifford’s picture

    Issue summary: View changes

    Updating proposed version by comment #60

    mortendk’s picture

    Title: Pagination markup idea » Pagination markup
    Issue tags: +Twig
    scaragucc&#039;’s picture

    Assigned: criz » scaragucc'
    scaragucc&#039;’s picture

    Assigned: scaragucc' » Unassigned
    Status: Active » Needs review
    FileSize
    16.47 KB
    FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,602 pass(es), 39 fail(s), and 54 exception(s). View

    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.

    Status: Needs review » Needs work

    The last submitted patch, 69: 1912608-pagination-twig.patch, failed testing.

    mgifford’s picture

    Status: Needs work » Needs review
    FileSize
    16.47 KB
    FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details.. View

    just a reroll.

    Status: Needs review » Needs work

    The last submitted patch, 71: 1912608-pagination-twig-71.patch, failed testing.

    dawehner’s picture

    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

    scaragucc&#039;’s picture

    Assigned: Unassigned » scaragucc'

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

    scaragucc&#039;’s picture

    Assigned: scaragucc' » Unassigned
    Status: Needs work » Needs review
    FileSize
    0 bytes
    PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,162 pass(es). View

    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.

    scaragucc&#039;’s picture

    FileSize
    29.46 KB
    FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,608 pass(es), 86 fail(s), and 16 exception(s). View
    13.87 KB

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

    Status: Needs review » Needs work

    The last submitted patch, 76: 1912608-pagination-twig-76.patch, failed testing.

    scaragucc&#039;’s picture

    Assigned: Unassigned » scaragucc'

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

    scaragucc&#039;’s picture

    Assigned: scaragucc' » Unassigned
    Status: Needs work » Needs review
    Issue tags: +drupalcampfi
    FileSize
    14.16 KB
    FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,540 pass(es), 45 fail(s), and 57 exception(s). View

    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. :)

    Status: Needs review » Needs work

    The last submitted patch, 79: 1912608-pagination-twig-79.patch, failed testing.

    scaragucc&#039;’s picture

    Assigned: Unassigned » scaragucc'
    scaragucc&#039;’s picture

    Assigned: scaragucc' » Unassigned
    Status: Needs work » Needs review
    FileSize
    26.29 KB
    PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,721 pass(es). View

    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.

    jwilson3’s picture

    Status: Needs review » Needs work
    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" ?

    jwilson3’s picture

    +++ 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.

    scaragucc&#039;’s picture

    Assigned: Unassigned » scaragucc'

    Thanks, jwilson3! I'm on it!!

    scaragucc&#039;’s picture

    Assigned: scaragucc' » Unassigned
    Status: Needs work » Needs review
    FileSize
    25.98 KB
    FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 1912608-pagination-twig-86.patch. Unable to apply patch. See the log in the details link for more information. View

    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.

    Status: Needs review » Needs work

    The last submitted patch, 86: 1912608-pagination-twig-86.patch, failed testing.

    jwilson3’s picture

    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.

    scaragucc&#039;’s picture

    Assigned: Unassigned » scaragucc'

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

    scaragucc&#039;’s picture

    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.

    scaragucc&#039;’s picture

    Assigned: scaragucc' » Unassigned
    Status: Needs work » Needs review
    FileSize
    25.73 KB
    PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,622 pass(es). View

    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.

    aspilicious’s picture

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

    scaragucc&#039;’s picture

    FileSize
    25.69 KB
    PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,590 pass(es). View
    661.6 KB

    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?

    scaragucc&#039;’s picture

    Assigned: Unassigned » scaragucc'
    Status: Needs review » Needs work
    Issue tags: +frontend

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

    scaragucc&#039;’s picture

    Assigned: scaragucc' » Unassigned
    Status: Needs work » Needs review
    FileSize
    26.68 KB
    PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,758 pass(es). View

    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.

    lauriii’s picture

    Status: Needs review » Needs work

    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

    scaragucc&#039;’s picture

    Assigned: Unassigned » scaragucc'

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

    scaragucc&#039;’s picture

    Assigned: scaragucc' » Unassigned
    Status: Needs work » Needs review
    FileSize
    25.37 KB
    PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,706 pass(es). View

    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">

    lauriii’s picture

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

    jwilson3’s picture

    Status: Needs review » Needs work
    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.

    scaragucc&#039;’s picture

    Assigned: Unassigned » scaragucc'
    LewisNyman’s picture

    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?

    scaragucc&#039;’s picture

    Assigned: scaragucc' » Unassigned
    Status: Needs work » Needs review
    FileSize
    26.03 KB
    FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,695 pass(es), 1 fail(s), and 1 exception(s). View

    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.

    Status: Needs review » Needs work

    The last submitted patch, 103: 1912608-pagination-twig-103.patch, failed testing.

    scaragucc&#039;’s picture

    Status: Needs work » Needs review

    Status: Needs review » Needs work

    The last submitted patch, 103: 1912608-pagination-twig-103.patch, failed testing.

    scaragucc&#039;’s picture

    Status: Needs work » Needs review
    FileSize
    26.03 KB
    FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,705 pass(es), 1 fail(s), and 1 exception(s). View

    Found a small bug, fixed and ready for review.

    Status: Needs review » Needs work

    The last submitted patch, 107: 1912608-pagination-twig-107.patch, failed testing.

    scaragucc&#039;’s picture

    Status: Needs work » Needs review
    FileSize
    25.64 KB
    FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,717 pass(es), 10 fail(s), and 2 exception(s). View

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

    Status: Needs review » Needs work

    The last submitted patch, 109: 1912608-pagination-twig-109.patch, failed testing.

    scaragucc&#039;’s picture

    Status: Needs work » Needs review
    FileSize
    25.65 KB
    FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 1912608-pagination-twig-111.patch. Unable to apply patch. See the log in the details link for more information. View

    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.

    Status: Needs review » Needs work

    The last submitted patch, 111: 1912608-pagination-twig-111.patch, failed testing.

    scaragucc&#039;’s picture

    Status: Needs work » Needs review
    FileSize
    25.51 KB
    FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,832 pass(es), 10 fail(s), and 0 exception(s). View

    Rerolled.

    Status: Needs review » Needs work

    The last submitted patch, 113: 1912608-pagination-twig-113.patch, failed testing.

    scaragucc&#039;’s picture

    Status: Needs work » Needs review
    FileSize
    25.52 KB
    PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,839 pass(es). View

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

    mgifford’s picture

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

    scaragucc&#039;’s picture

    FileSize
    24.87 KB
    FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,827 pass(es), 7 fail(s), and 3 exception(s). View
    4.7 KB

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

    Status: Needs review » Needs work

    The last submitted patch, 117: 1912608-pagination-twig-117.patch, failed testing.

    markcarver’s picture

    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 }}".

    scaragucc&#039;’s picture

    Assigned: Unassigned » scaragucc'

    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.

    markcarver’s picture

    The issue summary is severely out of date.

    mgifford’s picture

    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.

    mgifford’s picture

    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.

    mgifford’s picture

    Status: Needs work » Needs review
    FileSize
    24.9 KB
    FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,973 pass(es), 7 fail(s), and 3 exception(s). View

    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.

    scaragucc&#039;’s picture

    #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.

    Status: Needs review » Needs work

    The last submitted patch, 124: 1912608-pagination-twig-124.patch, failed testing.

    mgifford’s picture

    Status: Needs work » Needs review
    FileSize
    24.92 KB
    FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 74,267 pass(es), 7 fail(s), and 0 exception(s). View

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

    Status: Needs review » Needs work

    The last submitted patch, 127: 1912608-pagination-twig-127.patch, failed testing.

    jwilson3’s picture

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

      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 &lsaquo; and its counterpart &rsaquo; 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.

    jwilson3’s picture

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

    mgifford’s picture

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

    Pagination with messed up css.

    jwilson3’s picture

    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?

    jwilson3’s picture

    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
    
    jwilson3’s picture

    +++ 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.

    jwilson3’s picture

    +++ 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.

    jwilson3’s picture

    Assigned: scaragucc' » Unassigned
    Issue summary: View changes
    Status: Needs work » Needs review
    Issue tags: -Needs issue summary update
    Parent issue: » #1980004: [meta] Creating Dream Markup
    FileSize
    27.33 KB
    FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,245 pass(es), 26 fail(s), and 20 exception(s). View

    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.
      */
    

    Status: Needs review » Needs work

    The last submitted patch, 136: pagination_markup-1912608-136.patch, failed testing.

    jwilson3’s picture

    FileSize
    28.57 KB
    10.5 KB

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

    jwilson3’s picture

    Status: Needs work » Needs review
    FileSize
    30.04 KB
    FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,242 pass(es), 13 fail(s), and 26 exception(s). View
    2.72 KB

    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.

    jwilson3’s picture

    FileSize
    30.03 KB
    FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,318 pass(es), 13 fail(s), and 26 exception(s). View
    776 bytes
    +++ 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.

    The last submitted patch, 139: pagination_markup-1912608-139.patch, failed testing.

    Status: Needs review » Needs work

    The last submitted patch, 140: pagination_markup-1912608-140.patch, failed testing.

    jwilson3’s picture

    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 :-/

    mgifford’s picture

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

    scaragucc&#039;’s picture

    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

    mgifford’s picture

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

    lauriii’s picture

    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.

    scaragucc&#039;’s picture

    Assigned: Unassigned » scaragucc'
    Issue tags: +Drupalaton 2014

    Getting back into the mix!

    lauriii’s picture

    Related issues: +#2318341: Views mini pager markup
    FileSize
    26.64 KB
    PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,065 pass(es). View

    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.

    scaragucc&#039;’s picture

    Assigned: scaragucc' » Unassigned
    Status: Needs work » Needs review

    Sending to review.

    joelpittet’s picture

    +++ 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?

    joelpittet’s picture

    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!

    lauriii’s picture

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

    joelpittet’s picture

    @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.

    mgifford’s picture

    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.

    lauriii’s picture

    Assigned: Unassigned » lauriii
    Status: Needs review » Needs work

    More functionality can still be moved to Twig files.

    jwilson3’s picture

    Status: Needs work » Reviewed & tested by the community

    @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.

    jwilson3’s picture

    Status: Reviewed & tested by the community » Needs work

    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.

    lauriii’s picture

    @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.

    lauriii’s picture

    Assigned: lauriii » Unassigned
    Status: Needs work » Needs review
    FileSize
    9.39 KB
    28.64 KB
    FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,078 pass(es), 14 fail(s), and 0 exception(s). View

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

    Status: Needs review » Needs work

    The last submitted patch, 160: pagination_markup-1912608-161.patch, failed testing.

    jwilson3’s picture

    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.

    jwilson3’s picture

    +++ 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.

    jamesquinton’s picture

    Assigned: Unassigned » jamesquinton
    jamesquinton’s picture

    FileSize
    28.67 KB
    PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,099 pass(es). View

    Added the rel attributes - test now passing locally.

    jamesquinton’s picture

    Status: Needs work » Needs review
    jamesquinton’s picture

    FileSize
    28.67 KB
    PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,103 pass(es). View

    Swapped 'previous' to 'prev'.

    jamesquinton’s picture

    Assigned: jamesquinton » Unassigned

    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

    jwilson3’s picture

    Status: Needs review » Needs work

    @jamesquinton, you're exactly right thanks for looking that up. I even commented a bit on that issue, and particularly comment #77 from Issue #1898474 is relevant here. Quoting myself, because I can:

    In an ideal world a Twig themer might just implement the pager.html.twig file in his theme, and do any changes there.

    with this patch, ideal world is now the real world :D.

    -- FEEDBACK REMOVED --

    jwilson3’s picture

    Status: Needs work » Reviewed & tested by the community

    I edited my previous post to remove some feedback.... I had been thinking it might make sense to also move the title text and rel attributes straight into the template, and not even use the Attributes object, but I changed my mind about that because it reduces the flexibility, and is inconsistent with other templates. RTBC.

    jwilson3’s picture

    Core reviewers, please also see #2318341: Views mini pager markup, which goes hand in hand with this issue.

    jwilson3’s picture

    Status: Reviewed & tested by the community » Needs review
    +++ 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'), 'rel' => 'prev'));
    ...
    +    $li_next['attributes'] = new Attribute(array('title' => t('Go to next page'), 'rel' => 'next'));
    

    Please split these onto multiple lines. The text goes over 80 characters, and doing this will also be more consistent with the patch on comment #15 of the mini pager issue.

    jwilson3’s picture

    Status: Needs review » Needs work
    lauriii’s picture

    Status: Needs work » Needs review
    FileSize
    28.71 KB
    PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,135 pass(es). View
    1.07 KB

    Thank you for your comments @jwilson3!

    I uploaded a patch where I split the attributes to multiple lines.

    jwilson3’s picture

    Status: Needs review » Reviewed & tested by the community

    Thanks for the fix @lauriii; back to RTBC.

    jwilson3’s picture

    Issue tags: +novice documentation

    I spoke too soon. We still need to improve the Twig template documentation for all of the available variables. (This is one of the few last outstanding tasks mentioned in issue summary).

    I'm adding Novice tags, in case someone wants to help out to write the twig documentation.

    jwilson3’s picture

    Status: Reviewed & tested by the community » Needs work
    jwilson3’s picture

    +++ b/core/modules/comment/src/Tests/CommentPagerTest.php
    @@ -329,22 +330,53 @@ function testTwoPagers() {
    +   * Will click the first link found with this link text by default, or a later
    +   * one if an index is given. Match is case sensitive with normalized space.
    

    This needs to read "Will click the first link found with the given xpath query by default, or a later one if an index is given."

    joelpittet’s picture

    @jamesquinton re: #167 I didn't see an interdiff for your patches so it's hard to see what you changed between the patches and just going on your comment, why is Previous becoming prev? prev isn't a word, which makes it tricky to translate.

    +++ b/core/includes/pager.inc
    @@ -186,7 +186,7 @@ function template_preprocess_pager(&$variables) {
    -    t('‹ previous'),
    +    t('‹ prev'),
    

    Why is this prev?

    joelpittet’s picture

    Here's the documentation on interdiffs for anybody looking: https://drupal.org/documentation/git/interdiff
    It's the diff between the last two commits. The short hand is git show > interdiff.txt
    And I have a simpler workflow http://pittet.ca/drupal/sprint/patch if that helps.

    jamesquinton’s picture

    Hi Joel,

    I was referring to:

    +      'title' => t('Go to previous page'),
    +      'rel' => 'prev',
    

    I'm not sure where the other 'prev' comes from. But yes, not very clear without an interdiff - sorry!

    jwilson3’s picture

    FileSize
    1.28 KB
    631 bytes
    2.56 KB
    29.57 KB
    PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,183 pass(es). View

    It looks like"‹ prev'" was introduced between #115 and #117 when first/next/last/previous links were moved from CSS back to the PHP side, and my guess is that it was just t a copy/paste mistake. This patch removes that, and also addresses #176 and #178.

    As a courtesy, I've also created an interdiff between the patches since #160 that were missing.

    jwilson3’s picture

    Status: Needs work » Needs review
    jwilson3’s picture

    Status: Needs review » Needs work

    A couple issues and questions came up while writing the documentation in #182:

    1) The items.current item is a magical unicorn special case, because it is the only one in the list that is a simple integer. I'm on the fence about even bringing this up, or if its worth fixing at all, but it definitely makes the documentation less clear.

    2) Similarly items.ellipsis is a special cases, because it also does not follow the structure of the other items. When you actually think about the semantics of the structure, the the proper name for the element should be "ellipses" since it is an array with two sub-elements. This works nicely for the for loop in our template, but it just doesnt seem clean. Dunno... since this is really debatable, again, perhaps better to leave it alone.

    3) Should other defined variables such as element, parameters, quantity, or tags be mentioned in the template documentation if they are not used there? These four variables are defined but are primarily used in preprocess and are documented there as well. What is the rest of Drupal doing in this case?

    4) The preprocess structure creates many (needless?) temporary variables that refer to template implementation. Looking at you $li_first, $li_last, $li_previous, $li_next. This could be simplified, by just directly working on the $items array, eg $items['first']['href'] = url($current_path, $options);

    5) The preprocess code logic gate if ($pager_total[$element] > 1) { seems unnecessary because there is an earlier statement that breaks preprocessing if $pager_total[$element] <= 1. I might be reading this wrong, so I'd appreciate another set of eyes on this.

    6) The $pager_total[$element] is used throughout the template_preprocess_pager function, even when we've already set the value to a temporary variable $pager_max. It doesnt seem like the value of $pager_total[$element] ever changes through the flow of the code, so we could just use the $pager_max everywhere for consistency and readability.

    jwilson3’s picture

    Status: Needs work » Needs review
    FileSize
    4.8 KB
    30.11 KB
    PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,183 pass(es). View

    This patch addresses items 4,5, and 6 on #184... let's see what testbot turns up.

    Items 1, 2, and 3, are just observations and questions, not really actionable without another dev's feedback.

    lauriii’s picture

    FileSize
    30.07 KB
    PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,221 pass(es). View
    5.11 KB

    Very good clean up at #185! I looked the template_preprocess_pager() and it looks a lot cleaner now!

    I changed 1 and 2 to be own standalone variables because I was thinking about that few weeks ago also. I also wasn't sure if it would be worth fixing but probably it makes it at least a bit more cleaner.

    I'm not sure what is the policy for 3 but I looked pager.html.twig and it has all the available variables listed in documentation which kind of makes sense.

    Edit: In my patch I also changed all the quotes to be single quotes.

    jwilson3’s picture

    Status: Needs review » Needs work
    FileSize
    23.12 KB
    861 bytes
    30.26 KB
    PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,224 pass(es). View
    +++ b/core/modules/system/templates/pager.html.twig
    @@ -12,18 +12,18 @@
    + *   Sub-sub elements:
      *   items.first, items.previous, items.next, items.last and each item inside
      *   items.pages contain the following elements:
    ...
    + * - ellipses: If there are more pages than the quantity allows, then an
    + *   ellipsis before or after the listed pages may be present.
    

    Ellipses sub-items need documentation (fixed in patch below).

    +++ b/core/modules/system/templates/pager.html.twig
    @@ -12,6 +31,64 @@
    +          {% if current == key %}
    +            {% set title = 'Current page'|t %}
    +          {% else %}
    +            {% set title %}
    +              {% trans %}Go to page {{ key }}{% endtrans %}
    +            {% endset %}
    +          {% endif %}
    

    Two issues here:

    1) Seems out of place b/c the logic for setting titles for all other items is in preprocess. However, it is stupid to separate the "Current Page" string in two different places (preprocess for title, and template for visually hidden text) so I'm on the fence about what to do. Should we bring the title text into the template for next/previous/first/last too, or move this back to preprocess?

    2) Whitespace around "Go to page X" is visible when you hover the links see attached screenshot (https://www.drupal.org/files/issues/pager-item-title-whitespace.png).

    Leaving this in needs review to trigger test bot, but really its CNW for these two issues ^

    jwilson3’s picture

    Status: Needs work » Needs review

    NR to trigger testbot.

    lauriii’s picture

    FileSize
    675 bytes

    1) The reason why I left those in preprocess is that its possible to override those values e.g. from Views UI. We could implement that logic also in Twig but for me it made more sense to leave them into preprocess.

    2) I applied a fix for that. Do you think using trim is good solution or not because {% spaceless %} doesn't seem to work in that case.

    lauriii’s picture

    FileSize
    30.26 KB
    PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,227 pass(es). View

    For some reason the patch didn't make it through. Uploading it now.

    joelpittet’s picture

    Status: Needs review » Needs work
    +++ b/core/modules/system/templates/pager.html.twig
    @@ -12,6 +34,64 @@
    +          <a href="{{ item.href }}" title="{{ title|trim }}"{{ item.attributes|title }}>
    

    Just realized this little bug it seems. Did you mean to Title Case the item attributes?
    @see http://twig.sensiolabs.org/doc/filters/title.html

    Trim filter is fine, but you can also use the whitespace modifiers as well like {%-
    @see http://twig.sensiolabs.org/doc/templates.html#whitespace-control

    I agree with @jwilson3 in #187.1 It should be consistent all in preprocess for titles or all in template. You can use the default filter if you want to allow variable overrides in the template with sane defaults... consider {{ item.title|default('Current Page'|t) }}
    @see http://twig.sensiolabs.org/doc/filters/default.html

    lauriii’s picture

    Status: Needs work » Needs review
    FileSize
    30.62 KB
    PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,225 pass(es). View
    5.45 KB

    1) I don't actually even know where that came from :D

    2) I changed for those whitespace modifiers because its cleaner.

    3) I moved those default link texts to template and thought why do we have any attributes in preprocess and moved them also to template.

    ckrina’s picture

    Assigned: Unassigned » ckrina

    Starting with that

    mortendk’s picture

    Issue tags: +frontendunited
    rteijeiro’s picture

    Issue tags: -frontendunited +FUDK

    It seems official tag is FUDK (kinda weird, BTW)

    ckrina’s picture

    Assigned: ckrina » Unassigned
    FileSize
    31.57 KB
    FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch pagination_markup-1912608-196.patch. Unable to apply patch. See the log in the details link for more information. View
    2.31 KB

    There's a patch adding some comments in pager.html.twig file.

    ckrina’s picture

    FileSize
    31.04 KB
    PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,550 pass(es). View
    2.31 KB

    Uploaded correct patch.

    The last submitted patch, 196: pagination_markup-1912608-196.patch, failed testing.

    mortendk’s picture

    Status: Needs review » Needs work

    looks good i found a little thing with the class naming where it dosnt follow the css documentation its the use of modiefier classes (.foo--modifier)
    sorry settings it back to needs work

    1. +++ b/core/modules/system/templates/pager.html.twig
      @@ -12,6 +34,71 @@
      +        <li class="pager__item pager__item-first">
      

      if we uses bem naming: pager__item-first should be pager__item--first as its a modifer for pager__item

    2. +++ b/core/modules/system/templates/pager.html.twig
      @@ -12,6 +34,71 @@
      +        <li class="pager__item pager__item-previous">
      

      should use the modifier "--" so: pager__item--previes

    3. +++ b/core/modules/system/templates/pager.html.twig
      @@ -12,6 +34,71 @@
      +        <li class="pager__item pager__item-ellipsis" role="presentation">&hellip;</li>
      

      pager__item--ellipsis

    4. +++ b/core/modules/system/templates/pager.html.twig
      @@ -12,6 +34,71 @@
      +        <li class="pager__item pager__item-next">
      

      pager__item--next

    5. +++ b/core/modules/system/templates/pager.html.twig
      @@ -12,6 +34,71 @@
      +        <li class="pager__item pager__item-last">
      

      pager__item--last

    lauriii’s picture

    Status: Needs work » Needs review
    FileSize
    9.78 KB
    31.07 KB
    PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,591 pass(es). View

    Fixed the classes to follow css documentation.

    lauriii’s picture

    Issue summary: View changes
    mortendk’s picture

    updates the tags: we needs screenshots & issue summary update (and change notice when we get there)

    lauriii’s picture

    Issue summary: View changes
    lauriii’s picture

    Issue summary: View changes
    lauriii’s picture

    Cottser’s picture

    This is a review of most of the patch, I didn't look too long at the CSS changes. Not my strong point anyway :)

    I mostly found minor documentation issues. This patch is looking great overall and I think template_preprocess_pager() is much easier to follow now.

    1. +++ b/core/modules/comment/src/Tests/CommentPagerTest.php
      @@ -329,22 +330,52 @@ function testTwoPagers() {
      +   * @param $xpath
      +   *   An xpath query that targets an anchor tag, or set of anchor tags.
      +   * @param $index
      +   *   Link position counting from zero.
      +   *
      +   * @return
      +   *   Page contents on success, or FALSE on failure.
      

      We should document the data types for the @params and @return: https://www.drupal.org/node/1354#types

    2. +++ b/core/modules/system/templates/pager.html.twig
      @@ -5,6 +5,28 @@
      + *   - pages: List of pages, keyed by page numer.
      

      s/numer/number/

    3. +++ b/core/modules/system/templates/pager.html.twig
      @@ -5,6 +5,28 @@
      + *   - pages: List of pages, keyed by page numer.
      + *
      + *   Sub-sub elements:
      + *   items.first, items.previous, items.next, items.last and each item inside
      + *   items.pages contain the following elements:
      + *     - href: URL with appropriate query parameters for the item.
      + *     - attributes: A keyed list of HTML attributes for the item.
      + *     - text: The visible text used for the item link, such as "‹ previous"
      + *       or "next ›".
      + *
      + * - current: The page number of the current page.
      

      I think documentation standards wise there are a few issues here:

      a. I don't think the sub-sub list can have blank lines around it according to https://www.drupal.org/node/1354#lists.

      b. The list items are indented by two spaces too many.

      c. (minor) There should be an (Oxford) comma after items.last.

    4. +++ b/core/modules/system/templates/pager.html.twig
      @@ -5,6 +5,28 @@
      + *   - next: Present if the visibile list of pages ends before the last page.
      

      s/visibile/visible/

    5. +++ b/core/modules/system/templates/pager.html.twig
      @@ -12,6 +34,71 @@
      +        <li class="pager__item {{ current == key ? 'is-active' : '' }}">
      

      To avoid extra whitespace being printed, put the {{ right next to pager__item and use ' is-active' to insert the additional class.

    Cottser’s picture

    (sorry to add yet another comment for this…)

    After a tiny bit of manual testing it appears we have an issue. Or maybe two issues (one of them being that this happens whether twig_debug is on or off). This isn't really the fault of this patch but likely needs to be fixed before this can get in.

    lauriii’s picture

    Status: Needs work » Needs review
    FileSize
    2.89 KB
    31.08 KB
    PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,672 pass(es). View

    Thanks @cottser for a review! Fixed points from #207. Is there already issue for the issue on #208.

    Cottser’s picture

    I am not aware of an issue for #208, I think part of the problem is just in the way that the translation debug info is printed that may need to be changed. But it's weird that it shows even when twig_debug is off unless I was doing something weird. But all the other twig_debug info. was gone.

    Interdiff looks good!

    mgifford’s picture

    @lauriii pointed out this related link. I'm just connecting it here. It looks like making the active link item on pager a link actually simplifies the code too....

    Still it should be done in consideration with this larger patch (or incorporated).

    Cottser’s picture

    I think it'd be nice for this one to go in first if possible, either way we can reroll them. But I don't think we should smoosh that issue into this one, things are already complex enough here.

    lauriii’s picture

    FileSize
    31.07 KB
    PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,815 pass(es). View
    607 bytes

    After discussion on IRC we found workaround with @Cottser for #208. Patch is attached here and I filed a issue for that: #2332991: Twig debug and trans fails on HTML attributes.

    For #210 @Cottser also filed a new issue and it can be found here: #2332989: Twig trans tag debug is still hooked up to Settings.

    Cottser’s picture

    +++ b/core/modules/system/templates/pager.html.twig
    @@ -64,7 +64,7 @@
                 {% set title = 'Current page'|t %}
               {% else %}
                 {% set title %}
    -              {%- trans %}Go to page {{ key }}{% endtrans -%}
    +              {{- 'Go to page @key'|t({'@key': key}) -}}
                 {% endset %}
               {% endif %}
    

    I suppose you could also do {% set title = 'Go to page @key'|t({'@key': key}) %} to avoid the set block + whitespace modifiers.

    lauriii’s picture

    FileSize
    31.03 KB
    FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch pagination_markup-1912608-215.patch. Unable to apply patch. See the log in the details link for more information. View
    689 bytes

    True that!

    lauriii’s picture

    FileSize
    31 KB
    FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch pagination_markup-1912608-216.patch. Unable to apply patch. See the log in the details link for more information. View

    Reroll

    The last submitted patch, 215: pagination_markup-1912608-215.patch, failed testing.

    Status: Needs review » Needs work

    The last submitted patch, 216: pagination_markup-1912608-216.patch, failed testing.

    mgifford’s picture

    Issue tags: +Needs reroll
    lauriii’s picture

    Status: Needs work » Needs review
    FileSize
    30.98 KB
    PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,253 pass(es). View

    Rerolled

    lauriii’s picture

    Issue tags: -Needs reroll
    Cottser’s picture

    Status: Needs review » Needs work

    I went through everything again, I think this is looking really really good. However there may be too many visual changes, I think the CSS could probably use some more work since this issue should be about changing the markup. Not saying it needs to be pixel perfect but I think it's probably changed too much and the text underline in Stark looks a bit weird in particular.

    Here are some screenshots:

    Bartik before/after:

    Stark before/after:

    Seven before/after:

    Cottser’s picture

    Ho hum, accidentally hit submit. Attaching screenshots to this comment to embed into my last comment.

    Cottser’s picture

    Not that it was really a goal here but I'm happy to report that it looks like this is quite a decent improvement for backend performance :)

    Scenario: Default frontpage view displaying 10 nodes and pager like:
    1 2 3 4 5 6 7 8 9 … next › last »

    === 8.0.x..8.0.x compared (541de429e5ab1..541de55978b9a):
    
    ct  : 188,122|188,122|0|0.0%
    wt  : 988,128|986,192|-1,936|-0.2%
    cpu : 967,578|966,131|-1,447|-0.1%
    mu  : 54,414,272|54,414,272|0|0.0%
    pmu : 54,504,440|54,504,440|0|0.0%
    

    http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=541de429e5ab1&...

    === 8.0.x..pager-markup-1912608-222 compared (541de429e5ab1..541de5cbd937a):
    
    ct  : 188,122|186,551|-1,571|-0.8%
    wt  : 988,128|978,960|-9,168|-0.9%
    cpu : 967,578|959,332|-8,246|-0.9%
    mu  : 54,414,272|54,533,800|119,528|0.2%
    pmu : 54,504,440|54,624,736|120,296|0.2%
    

    http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=541de429e5ab1&...

    joelpittet’s picture

    I'd agree with @Cottser.

    The name changes for the CSS are fine with me, just the margin/padding/bold changes help a great deal and may bikeshed. Maybe just pair those back a bit so there is less visual regression?

    Also yes my site will be 7-9ms faster! woot:P But seriously anything that isn't a regression in performance is awesome in my books:)

    lauriii’s picture

    Status: Needs work » Needs review
    Issue tags: -FUDK
    FileSize
    1.62 KB
    31 KB
    PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,600 pass(es). View

    I fixed visual changes on Bartik, Seven and Stark

    Cottser’s picture

    Title: Pagination markup » Update pagination markup for new CSS standards and improved accessibility
    Issue summary: View changes
    Status: Needs review » Reviewed & tested by the community
    FileSize
    31.02 KB
    PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,452 pass(es). View
    417 bytes
    6.63 KB
    6.71 KB
    13.87 KB
    13.41 KB
    7.24 KB
    7.26 KB

    That looks much better, thanks @lauriii! Re-tested this on all three core themes, screenshots at the end of my comment. Stark changes a bit but I think that's fine (it should have minimal CSS after all), and is actually an improvement IMO :)

    I noticed this comment in Seven's pager.css that should be removed since we're now using nicer selectors so I made that minor change here. From my standpoint this is ready and RTBC!

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

    Bartik before/after:

    Stark before/after:

    Seven before/after:

    Cottser’s picture

    Was trying a new technique for taking the screenshots, sorry for the bad quality. Anyway you get the idea :)

    mgifford’s picture

    Issue tags: +accessibility
    mgifford’s picture

    Why on focus/over is there an underline on the numbers, but not in the first/previous in
    « first ‹ previous 1 2

    It's not a big deal, but a bit of a anomaly I'd think.

    lauriii’s picture

    FileSize
    31.08 KB
    PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,608 pass(es). View
    340 bytes

    Fixed one very small CSS issue with Bartik. As you can see from the screenshot, the pager has moved a bit right and is not centered anymore. This patch fixes this.

    I needed to use pretty strong selector since there is styles attached for .region-content ul. Since this is a very minor change, I'll leave this to RTBC.

    webchick’s picture

    Status: Reviewed & tested by the community » Fixed

    Wow, thanks so much for all of the thorough testing on this!

    Committed and pushed to 8.x. Thanks!

    • webchick committed ddbd7f6 on 8.0.x
      Issue #1912608 by Cottser, ckrina, jamesquinton, lauriii, jwilson3,...
    Cottser’s picture

    Status: Fixed » Closed (fixed)

    Automatically closed - issue fixed for 2 weeks with no activity.