WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#30174 closed enhancement (fixed)

Twenty Fifteen: paginated posts navigation (page break) accessibility

Reported by: afercia Owned by: iandstewart
Milestone: 4.1 Priority: normal
Severity: normal Version: 4.1
Component: Bundled Theme Keywords: has-patch
Focuses: ui, accessibility Cc:

Description

Page links need some accessibility improvements. See screenshot below: when tabbing, those links are read out just as:

2 (link)
3 (link)

Thoughts and patch welcome :)

https://cldup.com/FPt7dT50Ft.png

Attachments (2)

30174.patch (2.8 KB) - added by afercia 3 years ago.
30174.alt.patch (3.3 KB) - added by afercia 3 years ago.

Download all attachments as: .zip

Change History (17)

#1 @GrahamArmfield
3 years ago

  • Resolution set to duplicate
  • Status changed from new to closed

The paginate_link() function does contain the facility for adding context to the links for screen readers. There was a ticket early in 2014 about this and it was provided in WP3.9. See http://codex.wordpress.org/Function_Reference/paginate_links#Improving_Accessibility and #24709

#2 @DrewAPicture
3 years ago

  • Milestone Awaiting Review deleted

#3 @afercia
3 years ago

  • Resolution duplicate deleted
  • Status changed from closed to reopened

Reopening because this is not about archive pagination or comments pagination in core.
It's about wp_link_pages() and specifically for Twenty Fifteen, should be managed in the theme's:
content.php
content-link.php
content-page.php
image.php

The pagination links you can see in the screenshot are used when you split a single post in multiple pages, inserting a "Page break" tag in the post content.

I called them "page links" because they use a CSS class page-links, happy to use a better name for them if there's one.

#4 @ocean90
3 years ago

  • Milestone set to Awaiting Review

#5 @afercia
3 years ago

  • Keywords has-patch needs-testing added

See proposed patch.
I would go for keeping it simple and, after some investigation, I don't think this group of links should have any special semantic or ARIA role, it's not a "main" navigation. Any thoughts and different opinions are welcome.

I would ask the accessibility team's feedback for a better wording for "Pages:".
Please notice this "title" could also be hidden for screen readers and a better "screen-reader-text" used instead.

Tested with Firefox and NVDA, now it reads out loud (e.g. on the first page):

Pages: (pause)
Page 1 (pause)
link Page 2 (pause)
link Page 3

P.S.
Attached also an alternate version that make screen readers read out:

*Current* Page 1

but the markup ends up being a bit inelegant.

@afercia
3 years ago

@afercia
3 years ago

This ticket was mentioned in Slack in #core-themes by iandstewart. View the logs.


3 years ago

#7 follow-up: @davidakennedy
3 years ago

I tested this (the first patch) and it works as expected. It adds some nice additional context for screen reader users.

One point of feedback: I don't think we need to include a separator here:

'separator'   => '<span class="screen-reader-text">, </span>'
  • Most screen readers won't read the punctuation by default anyway.
  • It's not shown visually and serves no additional benefit (unless I'm missing something?).

Other than that, go with it. :)

#8 @davidakennedy
3 years ago

  • Keywords needs-testing removed

#9 in reply to: ↑ 7 @afercia
3 years ago

Replying to davidakennedy:

One point of feedback: I don't think we need to include a separator here:

  • Most screen readers won't read the punctuation by default anyway.

Indeed they won't read it but they will *pause*. The comma is intentionally added to make screen readers pause. If you try without the comma, you will hear the difference.

This ticket was mentioned in Slack in #core-themes by iandstewart. View the logs.


3 years ago

#11 follow-up: @iamtakashi
3 years ago

I've visually reviewed the patch 30174.alt.patch. I was going to adjust adjust the spacing around .page-links a and .page-links > span in #30301 but this will changes the markup so let's get this in first. And I'll update #30301 later.

#12 in reply to: ↑ 11 @afercia
3 years ago

Replying to iamtakashi:

I've visually reviewed the patch 30174.alt.patch.

hi @iamtakashi, @obenland mentioned on Slack the ".alt" patch is not translatable, not sure why, maybe ask @obenland for feedback or stick with the first patch?

#13 @afercia
3 years ago

I think I got it why the ".alt" patch isn't good, it's not about translation but about i18n, providing a translatable phrase split in 2 parts isn't good for localisation and many languages may need a different word order. See for example the very, very long discussion Mr. Armfield will remember for sure :) about Post/Page edit links starting from this comment.
So I would recommend to go with the first patch. Providing more information about the "Current" item should be handled by the core function.

#14 @iandstewart
3 years ago

  • Owner set to iandstewart
  • Resolution set to fixed
  • Status changed from reopened to closed

In 30314:

Twenty Fifteen: adding context to page links for screenreaders.

Props afercia, fixes #30174.

#15 @SergeyBiryukov
3 years ago

  • Milestone changed from Awaiting Review to 4.1
Note: See TracTickets for help on using tickets.