WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 9 months ago

Last modified 9 months ago

#41859 closed defect (bug) (fixed)

Add aria-current to the paginated posts links output by wp_link_pages()

Reported by: afercia Owned by: afercia
Milestone: 5.1 Priority: normal
Severity: normal Version:
Component: Themes Keywords: aria-current
Focuses: accessibility Cc:

Description

When using the <!--nextpage--> tag one or multiple times in post, wp_link_pages() generates pagination links for the post. In the set of pagination links, aria-current would help assistive technologies users orientate themselves and understand which the current active "page" is.

See also #40833 that introduced aria-current for in paginate_links(), typically used on archive pages for posts navigation.

The actual links markup gets generated by _wp_link_page(). Worth noting this markup could be improved also for other things, for example there's no "current" CSS class used on the active paeg number.

Themes usually add their own markup around the links to improve things a bit. For example, Twenty Seventeen uses this:

wp_link_pages( array(
	'before'      => '<div class="page-links">' . __( 'Pages:', 'twentyseventeen' ),
	'after'       => '</div>',
	'link_before' => '<span class="page-number">',
	'link_after'  => '</span>',
) );

However, some improvements to the default output would be a nice improvement.

Attachments (4)

41859.patch (2.2 KB) - added by antonioeatgoat 22 months ago.
Improved default parameters and highlighted current item
41859.2.patch (2.5 KB) - added by antonioeatgoat 21 months ago.
New patch, following suggestions of #comment:7. Unit tests are still missing.
41859.diff (4.7 KB) - added by afercia 21 months ago.
41859.2.diff (4.8 KB) - added by afercia 21 months ago.

Download all attachments as: .zip

Change History (25)

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


22 months ago

#2 @afercia
22 months ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 5.0

@antonioeatgoat
22 months ago

Improved default parameters and highlighted current item

#3 @antonioeatgoat
22 months ago

  • Keywords dev-feedback added

The patch uses by default some of the parameters used by Twenty Seventeen, however this upsets the theme, because there is a repeated nested span with the same class. So likely not the best way to achieve it, maybe changing the class name used by default in the function would be a solution.

Then it forces the usage of a class to wrap the numbers, as wp_link_pages() does. I am not a big fun of the solution str_replace( '<a href="', '<a class="page-number" href="', $link ); , however it's the only way that I see for now with the current structure of the function. It would be very nice to improve consistency between paginate_links() and wp_link_pages() because they use parameters and CSS class positions/names in a quite different way, despite they do basically the same thing.

Last edited 22 months ago by antonioeatgoat (previous) (diff)

#4 @afercia
22 months ago

  • Owner set to afercia
  • Status changed from new to assigned

#5 @afercia
22 months ago

  • Keywords has-patch added; needs-patch removed

#6 @alexstine
22 months ago

I just tested the patch with the Twenty Seventeen theme on WP 4.9. It's working fine. The pagination is more accessible this way.

#7 @afercia
22 months ago

  • Keywords dev-feedback removed

@antonioeatgoat thanks for the patch and @alexstine thanks for testing!

A few things:

  • there should be a @since notation for the new aria_current argument, as done for example in https://core.trac.wordpress.org/changeset/41371
  • I wouldn't change the default paragraph tag in a div, this might produce some layout/css breakage for themes that expect a paragraph
  • to avoid str_replace(), what about to add the CSS class in the private function _wp_link_page()? Seems the simplest solution to me, then add the same CSS class in the new span element
  • I'd use a page-link name for the CSS class, to avoid conflicts
  • maybe use === instead of == to compare the index with the page number?
  • in tests/phpunit/tests/post/template.php there are a few tests to update, this can also be done later :)

@antonioeatgoat
21 months ago

New patch, following suggestions of #comment:7. Unit tests are still missing.

#8 @antonioeatgoat
21 months ago

@afercia, to add the CSS class to _wp_link_page() is definitely the best solution. I've actually made confusion, because I was sure that that function was used also in other places, for this reason I avoided to edit it. In addition _wp_link_page() is used many times there and I missed it, so str_replace() was really a terrible solution. My bad, I apologize.

Just uploaded the new patch following your suggestions, only the unit tests are still missing.

Last edited 21 months ago by antonioeatgoat (previous) (diff)

@afercia
21 months ago

#9 @afercia
21 months ago

41859.diff updates the related tests.

@afercia
21 months ago

#10 @afercia
21 months ago

Thinking at slightly better CSS class names. For analogy with the paginated links for the archive pages ("nav-links" and "page-numbers ") I'd say to use "post-nav-links" and "post-page-number/s". Updated in 41859.2.diff.

#11 @afercia
21 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 42440:

Accessibility: use aria-current for the paginated post links output by wp_link_pages().

Continues the introduction in core of the aria-current attribute after [41683], [41359], and [41371].

  • changes the wp_link_pages() (see the nextpage quicktag) output to use an aria-current attribute on the current item
  • adds post-nav-links and post-page-numbers CSS classes to help themes style these links
  • updates the related tests

Props antonioeatgoat, alexstine.
Fixes #41859.

#12 @audrasjb
12 months ago

  • Keywords fixed-major added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for 4.9.9 consideration as seen during accessibility team bug-scrub meeting.

#13 @SergeyBiryukov
12 months ago

  • Milestone changed from 5.0 to 4.9.9

#14 @pento
12 months ago

  • Milestone changed from 4.9.9 to 5.0.1

The phpdoc in [42440] will need to be updated.

#15 @afercia
11 months ago

  • Owner afercia deleted
  • Status changed from reopened to assigned

#16 @pento
9 months ago

  • Milestone changed from 5.0.1 to 5.0.2

#17 @pento
9 months ago

  • Keywords needs-patch added; has-patch fixed-major removed
  • Milestone changed from 5.0.2 to 5.1

Moving to 5.1, the php docs will need to be updated.

#18 @afercia
9 months ago

Noting this is already merged in trunk and waiting since 12 months to be released. Whether it's 5.0.3 or 5.1, at this point I'm not sure about the process. The phpdoc in [42440] will need to be updated. /Cc @desrosj

#19 @desrosj
9 months ago

@afercia Feel free to commit the since update in trunk any time and close out for 5.1. This isn’t a major or editor related bug, so it doesn’t fit the scope of 5.0.3.

#20 @afercia
9 months ago

  • Owner set to afercia
  • Resolution set to fixed
  • Status changed from assigned to closed

In 44411:

Accessibility: Update since annotation for wp_link_pages().

Updates the since annotation for the aria_current argument introduced in #42440.

Fixes #41859.

#21 @afercia
9 months ago

  • Keywords needs-patch removed
Note: See TracTickets for help on using tickets.