WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 3 years ago

#35577 closed defect (bug) (fixed)

Menus screen: improve the "View All" tab panels pagination links

Reported by: afercia Owned by: afercia
Milestone: 4.7 Priority: normal
Severity: normal Version:
Component: Menus Keywords: has-screenshots has-patch commit
Focuses: ui, accessibility, javascript Cc:

Description

In the Menus screen, the accordions on the left allow to quickly add items to the menus. When there are many pages, posts, categories, tags, etc. the related "View All" tab displays the available items paginated with some pagination links:

https://cldup.com/47n7slPv8Z.png

These links are a bit too small and I'd say extremely hard to activate, especially on small screens. Also their semantics could be improved, actually they're announced by screen readers just as "1, 2, ... right double angle bracket". They should probably be a list of links with aria-label attributes or screen-reader-text to expand them.

Attachments (5)

35577.diff (460 bytes) - added by xavortm 3 years ago.
35577.2.diff (6.2 KB) - added by cwpnolen 3 years ago.
Patch update - aria-label and updated unit tests
35577.3.diff (6.3 KB) - added by cwpnolen 3 years ago.
35577.4.diff (3.9 KB) - added by afercia 3 years ago.
35577.5.diff (4.0 KB) - added by afercia 3 years ago.

Download all attachments as: .zip

Change History (24)

#1 @afercia
4 years ago

  • Keywords ui-feedback added

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


3 years ago

#3 @afercia
3 years ago

  • Keywords good-first-bug added
  • Milestone changed from Awaiting Review to 4.7

Let's try this for 4.7 :)

@xavortm
3 years ago

#4 @xavortm
3 years ago

There is also similar list of numbers in here wp-admin/includes/media.php L2441

The stylings in the diff affect only the ones in the ticket as the wrappers are different between the two files. The change for now is only more active clickable area. I tried to wrap the next/prev arrows with aria labels, but that caused the whole page to reload instead of instantly change the pages. I am not sure what is causing this :S

http://i.imgur.com/rpZ3jYB.png

Making them bigger really looks weird to me though..

#5 @afercia
3 years ago

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

#6 @afercia
3 years ago

Thanks @xavortm looking back at this, worth considering when there are a lot of posts, pages, categories, these links can become something like this:

https://cldup.com/wbQHSA1GI5.png

As far as I see the pagination is set to display 50 items "per page", so having a large number of links it's not so easy to reproduce unless temporarily altering the hardcoded value.

Some UI/design feedback would be greatly appreciated, if there are no new ideas I'm afraid the only thing that can be done here is adding a very few pixels padding.

I tried to wrap the next/prev arrows with aria labels, but that caused the whole page to reload instead of instantly

Aria labels are HTML attributes, for example:
<a href="somelink" aria-label="some text here">link text that will be overridden by the aria label</a>

@cwpnolen
3 years ago

Patch update - aria-label and updated unit tests

#7 @cwpnolen
3 years ago

  • Keywords has-patch added; needs-patch removed

Added to the patch provided by @xavortm

paginate_links() now applies aria-labels to all generated links that are ready for translation.

Also updated the unit tests to account for the aria-labels.

Note: this is is my first core contribution, let me know if anything should be done differently.

#8 @xavortm
3 years ago

@cwpnolen thanks for the updates and welcome I think that the wording can be improved a little. Instead of "Go to the previous page" it could be "Previous page" and instead of "Link to page %s" to be "Page %s". More feedback on that can be given though, I am not sure what is the exact wording in other places.

Last edited 3 years ago by xavortm (previous) (diff)

@cwpnolen
3 years ago

#9 @cwpnolen
3 years ago

Thanks @xavortm. These strings and unit tests have been updated.

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


3 years ago

#11 @afercia
3 years ago

I'm not sure 'paginate_links()' can be changed this way since it is often used by themes too, via the_posts_pagination() / get_the_posts_pagination() and the aria-label will override any text provided by theme authors. Instead, I'd try to make use of the existing arguments like, for example, Twenty Sixteen does.

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


3 years ago

@afercia
3 years ago

#13 @afercia
3 years ago

  • Focuses javascript added
  • Keywords ui-feedback good-first-bug removed

Refreshed patch with a new approach. Uses the existing arguments to add some screen reader text and aria labels. A similar approach is used, for example, on the bundled Themes and works pretty well. The CSS part increases a bit the links clickable area, see in the screenshot below (the background color is just for testing purposes):

https://cldup.com/5OQJa9iXi9.png

There are also 3 JavaScript issues to solve.

1
Since the links now contain a <span> element, the JS part can't rely on the click event target and needs to be changed.

2
Pre existing issue: when clicking on the current page number (the one without a link) there's no href attribute to replace, thus a JS error:

https://cldup.com/KJpbDM2NrC.png

3
Pre existing issue: when clicking on the pagination links, the whole list below gets rebuilt and inserted in a new <div> element. For each click, a new div gets inserted and after several clicks there's a proliferation of <div>s:

https://cldup.com/n3m9BbG4Qu.png

I'd propose to simplify this part: while using DOM methods is nicer, jQuery can do this with a single line of code. I'd greatly appreciate a JS review to be sure the patch is not breaking anything :) /cc @azaozz

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

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


3 years ago

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


3 years ago

This ticket was mentioned in Slack in #core-editor by afercia. View the logs.


3 years ago

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


3 years ago

@afercia
3 years ago

#18 @afercia
3 years ago

  • Keywords commit added

Refreshed patch with a few adjustments.

#19 @afercia
3 years ago

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

In 38981:

Accessibility: Improve the Menus post type meta boxes pagination links.

  • adds hidden text to the pagination links
  • slightly increases the links clickable area
  • fixes a JS error when clicking on the current page number
  • avoids to generate nested <div> elements at each click

Props xavortm, cwpnolen, afercia.

Fixes #35577.

Note: See TracTickets for help on using tickets.