Make WordPress Core

Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#32253 closed defect (bug) (fixed)

List table: posts navigation links pointing to same page — at Version 35

Reported by: rianrietveld's profile rianrietveld Owned by: afercia's profile afercia
Milestone: 4.3 Priority: normal
Severity: normal Version: 4.1
Component: Administration Keywords: has-patch commit
Focuses: ui, accessibility Cc:

Description (last modified by rianrietveld)

In for example wp-admin/edit.php the links inside tablenav-pages can point to the displayed page itself.
This is the case when you are on the first page and on the last page of the items list.
The items show in a different colour but are links to the page itself.
http://www.rianrietveld.com/wp-content/uploads/2015/05/page-nav.jpg

Suggestion: remove that links and replace them with text only.

Code in
class WP_List_Table
function pagination

related: #32028

Tracking label: #a11y-color

Change History (41)

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


9 years ago

@joedolson
9 years ago

Remove links if point to current patch.

#2 @joedolson
9 years ago

First pass at patch.

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


9 years ago

#4 @joedolson
9 years ago

  • Milestone changed from Awaiting Review to 4.3
  • Owner set to afercia
  • Status changed from new to assigned

This ticket was mentioned in Slack in #core by joedolson. View the logs.


9 years ago

#6 @joedolson
9 years ago

  • Keywords commit has-patch added

To test: verify that the prev/first links are not linked when viewing first page of results or default view, verify that next/last links are not linked when viewing last page of results; verify that all other views show both sets of links.

@afercia
9 years ago

#7 @afercia
9 years ago

  • Keywords ui-feedback added

The refreshed patch tries to improve a bit, handling also the previous and next links. There's a bit of repetition in the code but maybe better for clarity and readability. Any improvements welcome. About the visual part, will ask the UI team to choose what to display when the links are not printed out: just an empty square, nothing (with visibility; hidden to avoid "jumps"), or ... ?
Some screenshots:

https://cldup.com/YGkLnq92hc.png

https://cldup.com/MQfUCoPkx6.png

https://cldup.com/JBDvcU_GKJ.png

#8 @afercia
9 years ago

Was thinking that instead of an empty square, yes we could still display the arrows (without link) and completely hide them from assistive technologies with aria-hidden=true. The visual difference between a "disabled" arrow and an enabled one should be very, very, clear though. Will try to refresh the patch.

#9 @afercia
9 years ago

One special case to consider is the Comments screen where, currently, a CSS class 'disabled' is toggled via JavaScript on the pagination links. JavaScript does other things too, like updating the "displaying items" and "total pages" numbers in addition to a bunch of hidden fields values being updated. Please notice there are several quirks there, and it doesn't work correctly in many cases. See also #32362.
This patch doesn't touch anything in edit-comments.js but maybe we should remove the CSS class toggling and/or create a new ticket for the Comments JavaScript part? Any thoughts more than welcome.

@afercia
9 years ago

#10 @afercia
9 years ago

In the refreshed patch, no more empty squares. When there's no link, arrows are displayed just as a "visual clue" but there's no semantic markup at all (that's intentional) and they're completely hidden from assistive technologies.
While I think this UI part would need some major re-thinking, any relevant CSS change is out of the scope of this ticket. I would recommend just a couple of changes, related to the links usability and readability:

  • make clear the difference between a link and a disabled "square with arrow"
  • for better readability, make all the text (i.e. "58 items 1 of nn") aligned on the same baseline
  • reduce font variants

Note: this patch should be coordinated with the one in #32028

Screenshot with the last patch applied:

https://cldup.com/Th3db2fQ76.png

#11 @obenland
9 years ago

  • Keywords commit removed

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


9 years ago

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


9 years ago

This ticket was mentioned in Slack in #core by davidakennedy. View the logs.


9 years ago

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


9 years ago

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


9 years ago

@afercia
9 years ago

#17 @afercia
9 years ago

Refreshed patch after r32693 also adds a "Current Page" screen-reader-text in the bottom pagination using the already existing string.

This ticket was mentioned in Slack in #design by helen. View the logs.


9 years ago

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


9 years ago

#20 @rianrietveld
9 years ago

Patch 32253.4 tested on june 18, still applies, works and looks well.

#21 @wonderboymusic
9 years ago

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

In 32948:

List tables: when post navigation links point to the current page, use <span>s and text instead of <a>s.

Props afercia.
Fixes #32253.

This ticket was mentioned in Slack in #core by netweblogic. View the logs.


9 years ago

#23 in reply to: ↑ 22 @netweblogic
9 years ago

Replying to slackbot:

This ticket was mentioned in Slack in #core by netweblogic. View the logs.

This patch seems to break the display when using paginate_links in the admin area. For example:

$return = '<div class="tablenav"><div class="tablenav-pages">';
$base = !empty($base) ? $base:esc_url_raw(add_query_arg( 'pno', '%#%' ));
$nav = paginate_links( array(
	'base' => $base,
	'total' => ceil($total / $limit),
	'current' => $page,
	'add_args' => $vars
));
$return .= sprintf( '<span class="displaying-num">Displaying %s&#8211;%s of %s </span>%s',
	number_format_i18n( ( $page - 1 ) * $limit + 1 ),
	number_format_i18n( min( $page * $limit, $total ) ),
	number_format_i18n( $total ),
	$nav
);
$return .= '</div></div>';

ends up with this now:

https://www.dropbox.com/s/oy5a6oa9xkg6eh9/07-08-2015%2022-18-52.png

might not be a core issue, but maybe something worth considering, anyone else using this similarly may experience breakages in their UI.

For my situation the fix is quite straightforward, the newly added width is the main problem causer, overriding that with an auto value for my paginations solves the issue:

.tablenav .tablenav-pages a,
.tablenav-pages-navspan {
	display: inline-block;
	width: 7px;
	...

#24 @SergeyBiryukov
9 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

#25 follow-up: @azaozz
9 years ago

Seems r32948 doesn't take this use into account. It's not just the width: display, padding, font-size are all wrong then there is text in there.

@netweblogic can you add an example that is easy to reproduce/test. Can even be in a form of a (small) plugin.

#26 in reply to: ↑ 25 ; follow-up: @afercia
9 years ago

Replying to azaozz:

when there is text in there.

Not arguing :) but should core take this into account? Core uses arrows there. If a plugin is going to change content (and markup) I would expect the plugin to adjust the CSS accordingly and update it when there are UI changes.

#27 in reply to: ↑ 26 @azaozz
9 years ago

Replying to afercia:

Yep, that's why I asked for usage example :)

@netweblogic
9 years ago

POC for styling issues when using em_paginate

#28 @netweblogic
9 years ago

Done, uploaded paginate-test.php

Enable the plugin, there's a Pagination Test menu item in the settings menu.

@afercia, one issue (as is in my case) is that the default text includes Previous/Next. Another potential solution is changing that. Also if you check my demo and screenshots, when you start reaching double digits in page numbers the number overflows the box.

Last edited 9 years ago by netweblogic (previous) (diff)

#29 follow-up: @ocean90
9 years ago

  • Keywords needs-patch added; has-patch ui-feedback removed

Can we change width into min-width?

#30 in reply to: ↑ 29 @afercia
9 years ago

  • Keywords has-patch added; needs-patch removed

Replying to ocean90:

Can we change width into min-width?

Yes :) It's a bit difficult here having the arrows perfectly centered and at the same time making the "squares" have the same size. Different font rendering in different platforms would also matter here and probably we can't have a "pixel perfect" alignment. We should try to make the squares have the same size though.
In the proposed patch min-width is now bigger than 1em and padding adjusted accordingly. In the screenshot below: the Pagination Test plugin in 4.2 and in trunk. As you can see even "before" the plugin should provide some styling...

https://cldup.com/aBwbq71ysG.png

Last edited 9 years ago by afercia (previous) (diff)

#31 @obenland
9 years ago

I think 32253.pagination-test.diff is a reasonable fix. @azaozz, what is your opinion?

#32 @ocean90
9 years ago

  • Keywords commit added

32253.pagination-test.diff looks good to me.

#33 @azaozz
9 years ago

Yep, 32253.pagination-test.diff fixes the worst breakage when text or numbers are used for the buttons. Seems good enough for 4.3, +1 for commit.

#34 @obenland
9 years ago

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

In 33608:

After [32948]: Account for the use of paginate_links() with table navigation markup.

Props afercia.
Fixes #32253.

#35 @rianrietveld
9 years ago

  • Description modified (diff)
Note: See TracTickets for help on using tickets.