Problem/Motivation

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

Proposed resolution

  • Move as much markup as possible into the views-mini-pager.html.twig Twig template.
  • Follow Drupal 8 CSS Guidelines for CSS class nomenclature.
  • 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.

We have made some discussion regarding this here #1912608: Update pagination markup for new CSS standards and improved accessibility

Remaining tasks

Contributor tasks needed
Task Novice task? Contributor instructions Complete?
Create a patch Instructions
Update automated tests Instructions
Manually test the patch Novice Instructions
Review patch to ensure that it fixes the issue, stays within scope, is properly documented, and follows coding standards Instructions

User interface changes

-

API changes

-

Screenshots

Bartik

Before

After

Stark

Before

After

Seven

Before

After

Files: 
CommentFileSizeAuthor
#68 interdiff-2318341-62to66.txt3.28 KBmgifford
#66 drupal-mini_pager_markup-2318341-66.patch9.4 KBocastle
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,852 pass(es). View
#63 2014-10-17-000340_280x39_scrot.png3.2 KBtuutti
#63 2014-10-17-000359_290x52_scrot.png2.79 KBtuutti
#63 2014-10-17-000412_250x53_scrot.png3.19 KBtuutti
#57 Screen Shot 2014-10-05 at 10.23.26 AM.png16.7 KBmgifford
#57 Screen Shot 2014-10-05 at 10.22.08 AM.png171.53 KBmgifford
#52 interdiff.txt695 bytesrteijeiro
#52 drupal-mini_pager_markup-2318341-51.patch8.86 KBrteijeiro
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,860 pass(es). View
#40 interdiff-2318341-33-40.txt529 bytesJolidog
#40 mini_pager_markup-2318341-reroll-40.patch8.85 KBJolidog
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,840 pass(es), 7 fail(s), and 24 exception(s). View
#38 stark-before.png1.73 KBblazey
#38 stark-after.png1.46 KBblazey
#38 seven-before.png1.53 KBblazey
#38 seven-after.png1.25 KBblazey
#38 bartik-before.png1.56 KBblazey
#38 bartik-after.png1.42 KBblazey
#33 interdiff-2318341-30-33.txt948 bytesRade
#33 mini_pager_markup-2318341-33.patch8.88 KBRade
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,743 pass(es). View
#33 after.png4.69 KBRade
#33 before.png4.95 KBRade
#30 mini_pager_markup-2318341-30.patch8.87 KBlauriii
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,862 pass(es). View
#30 interdiff-2318341-26-30.txt7.05 KBlauriii
#9 mini_pager_markup-2318341-9.patch8.29 KBlauriii
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Repository checkout: failed to checkout from [git://git.drupal.org/project/drupal.git]. View
#9 interdiff-2318341-7-9.txt4.88 KBlauriii
#12 mini_pager_markup-2318341-12.patch8.09 KBlauriii
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,078 pass(es), 0 fail(s), and 32 exception(s). View
#12 interdiff-2318341-9-12.txt1.49 KBlauriii

Comments

lauriii’s picture

Assigned: Unassigned » lauriii
lauriii’s picture

Assigned: lauriii » Unassigned
Status: Active » Needs review
FileSize
7.23 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,254 pass(es), 11 fail(s), and 4 exception(s). View
scaragucc'’s picture

I'm not sure we need the indexes for the $tags array on line 1104. I'd leave them out and then call the values by the default numerical indexes.

lauriii’s picture

FileSize
6.96 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,599 pass(es), 8 fail(s), and 0 exception(s). View

I fixed some parts of the tests and changed tags keying system match the way its used in the main pager.

The last submitted patch, 2: mini_pager_markup-2318341-2.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 4: mini_pager_markup-2318341-3.patch, failed testing.

lauriii’s picture

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

I added keys for tags and added missing page text for the page number

dawehner’s picture

I really like that we move more into the template. Could we also put

t('Page @current', array('@current' => $pager_current));

and

    4 => t('Previous page'),
    5 => t('Next page'),

into the template? This is not something you can configure ...

lauriii’s picture

FileSize
8.29 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Repository checkout: failed to checkout from [git://git.drupal.org/project/drupal.git]. View
4.88 KB

Moved more functionality to template as @dawehner pointed in his comment #8.

dawehner’s picture

+++ b/core/modules/views/templates/views-mini-pager.html.twig
@@ -12,6 +12,44 @@
+              {% trans %}
+                Previous page
+              {% endtrans %}

No reason for this complex beast: {{ "Previous page."|trans }} works as well.

Status: Needs review » Needs work

The last submitted patch, 9: mini_pager_markup-2318341-9.patch, failed testing.

lauriii’s picture

FileSize
1.49 KB
8.09 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,078 pass(es), 0 fail(s), and 32 exception(s). View

That's true actually. I changed few trans blocks to shorter ones.

lauriii’s picture

Status: Needs work » Needs review

The last submitted patch, 12: mini_pager_markup-2318341-12.patch, failed testing.

lauriii’s picture

FileSize
8.19 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,152 pass(es). View
2.14 KB

Fixed missing attributes

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

lauriii++

+++ b/core/modules/views/templates/views-mini-pager.html.twig
@@ -12,6 +12,32 @@
+    <h4 id="pagination-heading" class="visually-hidden">{{ 'Pagination'|t }}</h4>
...
+            <span class="visually-hidden">{{ "Previous page"|trans }}</span>
...
+            <span class="visually-hidden">{{ "Next page"|trans }}</span>

We could unify on t or trans but I don't care. Showing the different possibilities we have is not a totally bad idea, if you ask me.

webchick’s picture

Status: Reviewed & tested by the community » Needs work

I think trans and t both do the same thing, in which case let's pick one and use it throughout for consistency.

jwilson3’s picture

Agreed. Sister-issue #1912608 uses t so lets go with that for consistency between the two pager templates.

lauriii’s picture

Status: Needs work » Needs review
FileSize
1.1 KB
3.72 MB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,110 pass(es). View

Changed trans to t for consistency!

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Thank you!

webchick’s picture

Status: Reviewed & tested by the community » Needs work

Um, something is very, very wrong with that diff. :)

lauriii’s picture

Status: Needs work » Needs review
FileSize
8.09 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,240 pass(es), 0 fail(s), and 32 exception(s). View

Hmm, fixed patch #19 from unnecessary stuff..

Status: Needs review » Needs work

The last submitted patch, 22: mini_pager_markup-2318341-12.patch, failed testing.

lauriii’s picture

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

Ups, uploaded wrong patch..

Cottser’s picture

Title: Mini pager markup » Views mini pager markup

I know there's no other mini pager, but I think adding 'Views' to the title makes it a bit clearer.

I diffed #15 and #24 and the only different was changing |trans to |t. Looks much better :)

+++ b/core/modules/views/templates/views-mini-pager.html.twig
@@ -12,6 +12,32 @@
+    <h4 id="pagination-heading" class="visually-hidden">{{ 'Pagination'|t }}</h4>
...
+            <span class="visually-hidden">{{ "Previous page"|t }}</span>
...
+            <span class="visually-hidden">{{ "Next page"|t }}</span>

This is so very minor but can we consistently use single quotes please? https://www.drupal.org/coding-standards#quotes

lauriii’s picture

FileSize
1.09 KB
8.19 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,862 pass(es). View

Changed all quotes to single quotes ;)

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Cool!

Cottser’s picture

Sorry/thank you :)

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/modules/views/views.theme.inc
    @@ -1111,48 +1111,28 @@ function template_preprocess_views_mini_pager(&$variables) {
       $li_previous = array();
    ...
       $li_next = array();
    
    @@ -1162,25 +1142,11 @@ function template_preprocess_views_mini_pager(&$variables) {
    +  $items['previous'] = $li_previous;
    +  $items['current'] = $pager_current;
    +  $items['next'] = $li_next;
    ...
    +  $variables['items'] = $items;
    

    I think we can remove all the unnecessary assignment here and just add these things to the $variables array as necessary. $items is a hangover from using item_list theme for this.

  2. +++ b/core/modules/views_ui/src/Tests/PreviewTest.php
    @@ -164,40 +164,35 @@ public function testPreviewWithPagersUI() {
         // Verify elements and links to pages.
         // We expect to find 3 elements: previous and current pages, with no link,
         // and next page with a link.
    -    $this->assertClass($elements[0], 'pager-previous', 'Element for previous page has .pager-previous class.');
    -    $this->assertFalse(isset($elements[0]->a), 'Element for previous page has no link.');
    

    The comment needs to change here and we should probably assert that the previous element is not there at all.

  3. +++ b/core/modules/views_ui/src/Tests/PreviewTest.php
    @@ -164,40 +164,35 @@ public function testPreviewWithPagersUI() {
    -    $this->assertClass($elements[1], 'pager-current', 'Element for current page has .pager-current class.');
    ...
    +    $this->assertClass($elements[0], 'pager__item-current', 'Element for current page has .pager__item-current class.');
    ...
    -    $this->assertClass($elements[2], 'pager-next', "Element for next page has .pager-next class.");
    ...
    +    $this->assertClass($elements[1], 'pager__item-next', 'Element for next page has .pager__item-next class.');
    

    pager-current and pager-next seem better than pager__item-current and pager__item-next how come?

lauriii’s picture

Status: Needs work » Needs review
FileSize
7.05 KB
8.87 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,862 pass(es). View

I fixed 1. and 3, and the documentation from 2 but I didnt create a test for that yet. Isn't it enough if we test that $elements[0] has class pager__item-current? Or would it be better to add test like this: $this->assertFalse(isset($elements[2]), 'No third element found');.

I also moved the attributes from preprocess to Twig. We have discussion about classes in the other issue here: #1912608: Update pagination markup for new CSS standards and improved accessibility.

Status: Needs review » Needs work

The last submitted patch, 30: mini_pager_markup-2318341-30.patch, failed testing.

Status: Needs work » Needs review
Rade’s picture

FileSize
4.95 KB
4.69 KB
8.88 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,743 pass(es). View
948 bytes

Pages with mini pager gave fatal error since there were callbacks to url(). Changed that to _url() now.

Also this patch results in some visual changes for the pager, in a positive way. This is what the mini pager looks like before the patch:

And here with the patch applied:

lauriii’s picture

Issue tags: +Needs screenshots

We need before/after screenshots from all Bartik, Seven, Stark and Classy ;)

lauriii’s picture

Issue tags: +Novice
blazey’s picture

Doing it.

blazey’s picture

Issue tags: +Amsterdam2014

Tagging

blazey’s picture

Attaching screenshots for bartik, stark and seven

Sutharsan’s picture

@blazey Please assign the issue to yourself while working on an issue. It will prevent others to start woking on the issue.
Ignore my comment. We have been instructed not to assign here during DrupalCon sprint.

Jolidog’s picture

FileSize
8.85 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,840 pass(es), 7 fail(s), and 24 exception(s). View
529 bytes

Had to reroll the patch. Added an interdiff.

vollepeer’s picture

The minipager is actually looking fine, but the when interacting with it, the user is send to the homepage (path is lost).

E.g.:
http://d8.local/articles?page=1 -> http://d8.local/?page=1

Status: Needs review » Needs work

The last submitted patch, 40: mini_pager_markup-2318341-reroll-40.patch, failed testing.

Jolidog’s picture

Checking, probably messed something up.

vollepeer’s picture

I fixed the current_path problem.

Jolidog’s picture

vollepeer, Why not just adding the variable $current_path = current_path(); back?

So,

$variables['items']['previous']['href'] = \Drupal::url('<current>', [], $options);
...
$variables['items']['next']['href'] = \Drupal::url('<current>', [], $options);

can go back to:

$variables['items']['previous']['href'] = _url($current_path, $options);
...
$variables['items']['next']['href'] = _url($current_path, $options);

Which in IMO is cleaner.

Jolidog’s picture

Status: Needs work » Needs review
FileSize
8.88 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,859 pass(es). View

Re-rolled again. correct urls are back. Let's see what the test bots have to say.

vollepeer’s picture

@Jolidog: see https://www.drupal.org/node/2345753 for the new special route names.

Jolidog’s picture

@vollepeer, didn't know that, not sure how I can make the test bots forget mine and go with your patch. Feel free to upload your patch again and thanks.

vollepeer’s picture

FileSize
8.87 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh. View

@Jolidog: no problem.

rteijeiro’s picture

Status: Needs review » Needs work

A little nitpick but important because we are getting rid of unneccesary ids and using classes instead.

+++ b/core/modules/views/templates/views-mini-pager.html.twig
@@ -11,7 +11,33 @@
+    <h4 id="pagination-heading" class="visually-hidden">{{ 'Pagination'|t }}</h4>

Change id pagination-heading to class. A little nitpick but important because we are getting rid of unneccesary ids and using classes instead.

The last submitted patch, 49: drupal-mini_pager_markup-2318341-49.patch, failed testing.

rteijeiro’s picture

Status: Needs work » Needs review
FileSize
8.86 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 78,860 pass(es). View
695 bytes

Let's see.

lauriii’s picture

Issue tags: +sprint

Tagging this to sprint since mini pager is atm in the HEAD visually broken

lauriii’s picture

Issue summary: View changes
mgifford’s picture

Status: Needs review » Needs work
lauriii’s picture

Status: Needs work » Needs review
mgifford’s picture

Not sure why I marked that Needs work earlier. Sorry.

Here's 2 screenshots from http://s8d797e2b25e91f7.s3.simplytest.me/ (which will still be good for 5 more hours if folks want to look at the view there)

ocastle’s picture

FileSize
9.44 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch drupal-mini_pager_markup-2318341-58.patch. Unable to apply patch. See the log in the details link for more information. View

We need to update the ajax_view.js file too with the updated css classes, otherwise its going to break ajax pagination.

Noted in https://www.drupal.org/node/2351753#comment-9249871

I've updated the patch based on #51

So we will still need to add to this patch based on the comments from @mgifford on #57.

Status: Needs review » Needs work

The last submitted patch, 58: drupal-mini_pager_markup-2318341-58.patch, failed testing.

mgifford’s picture

@ocastle this does seem to offer some improvements so was worth nudging along.

ocastle’s picture

@mgifford Okay, I'll try and sort out the re-roll this evening.

ocastle’s picture

Status: Needs work » Needs review
FileSize
9.45 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 79,504 pass(es). View

Okay, I've re-rolled the patch from #58.

tuutti’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
3.19 KB
2.79 KB
3.2 KB

Tested with Stark, Bartik and Seven (ajax enabled and disabled).

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/modules/views/templates/views-mini-pager.html.twig
    @@ -11,7 +11,33 @@
    +            <span aria-hidden="true">{{ items.previous.text|default('‹‹') }}</span>
    ...
    +            <span aria-hidden="true">{{ items.next.text|default('››') }}</span>
    
    +++ b/core/modules/views/views.theme.inc
    @@ -1027,87 +1027,31 @@ function template_preprocess_views_mini_pager(&$variables) {
    -  $tags += array(
    -    1 => t('‹‹'),
    -    3 => t('››'),
    -  );
    

    The default is no longer translatable.

  2. +++ b/core/modules/views/templates/views-mini-pager.html.twig
    @@ -11,7 +11,33 @@
    +        <li class="pager__item pager__item--current">
    

    In the full pager in the system module it does <li class="pager__item{{ current == key ? ' is-active' : '' }}"> perhaps we should follow that lead?
    Now the full pager styling is applying to the views mini pager where before it was not - i think we should use the same classes. One concern is it possible to style them differently?

  3. +++ b/core/modules/views_ui/src/Tests/PreviewTest.php
    @@ -165,40 +165,35 @@ public function testPreviewWithPagersUI() {
    -    $this->assertClass($elements[1], 'pager-current', 'Element for current page has .pager-current class.');
    -    $this->assertFalse(isset($elements[1]->a), 'Element for current page has no link.');
    +    // We expect to find current pages element with no link, next page element
    +    // with a link, and not to find previous page element.
    +    $this->assertClass($elements[0], 'pager__item--current', 'Element for current page has .pager__item--current class.');
    ...
    -    $this->assertClass($elements[1], 'pager-current', 'Element for current page has .pager-current class.');
    -    $this->assertFalse(isset($elements[1]->a), 'Element for current page has no link.');
    +    $this->assertClass($elements[1], 'pager__item--current', 'Element for current page has .pager__item--current class.');
    

    Why have we dropped testing that this does not have a link?

droplet’s picture

  1. +++ b/core/modules/views/js/ajax_view.js
    @@ -100,7 +100,7 @@
    +    this.$view.find('ul.pager__items > li > a, th.views-field a, .attachment .views-summary a')
    

    ul.pager__items -> .pager__items ?

  2. +++ b/core/modules/views/templates/views-mini-pager.html.twig
    @@ -11,7 +11,33 @@
    +    <h4 class="pager__heading visually-hidden">{{ 'Pagination'|t }}</h4>
    ...
    +            <span class="visually-hidden">{{ 'Previous page'|t }}</span>
    ...
    +            <span class="visually-hidden">{{ 'Next page'|t }}</span>
    

    'Previous page'|t -> "double quote" ?

ocastle’s picture

FileSize
9.4 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,852 pass(es). View

@alexpott I've made these adjustments to the patch.

@droplet Following the system pager template this uses single quote rather than the double quote.. So i've left this as is for the moment.

ocastle’s picture

Status: Needs work » Needs review
mgifford’s picture

Ok, here's an interdiff. I think this patch matches all of @alexpott's concerns. The patch still applies nicely. We've got screenshots up in #63, but not sure if we need new ones since we're now using is-active. @ocastle's response to @droplet makes sense to me.

So, this is pretty darn close to being RTBC again... What else do we need?

jsacksick’s picture

Status: Needs review » Reviewed & tested by the community

I was working on the D8 port of http://www.drupal.org/project/entityreference_view_widget when I noticed the views ajax pager bug, the patch in #66 fixed it.

jsacksick’s picture

After a discussion with alexpott on IRC, here are some details about the bug I encountered :
The pager markup of the view looks like the following :

  <nav class="pager" role="navigation" aria-labelledby="pagination-heading">
    <h4 id="pagination-heading" class="visually-hidden">{{ 'Pagination'|t }}</h4>
    <ul class="pager__items">
    [....]

The ajax behavior for the pager wasn't attached because of a wrong Jquery selector (this.$view.find('ul.pager > li > a, th.views-field a, .attachment .views-summary a')), as mentioned in my previous comment, the patch in #66 is fixing the issue. (nav.pager could have worked too).

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

This issue is a prioritized change as per #2350615: [policy, no patch] What changes can be accepted during the Drupal 8 beta phase? since it is changing mainly markup and it's benefits outweigh any disruption. It also happens to fix a javascript bug.

Committed c997eee and pushed to 8.0.x. Thanks!

  • alexpott committed c997eee on 8.0.x
    Issue #2318341 by lauriii, ocastle, Jolidog, vollepeer, rteijeiro, Rade...

Status: Fixed » Closed (fixed)

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