WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#29026 closed defect (bug) (fixed)

Media Grid and List form labels

Reported by: afercia Owned by: wonderboymusic
Milestone: 4.0 Priority: normal
Severity: normal Version: 4.0
Component: Media Keywords: has-patch needs-testing
Focuses: accessibility, javascript Cc:

Description

I've noticed some form labels missing in the Media Library, in both grid and list mode, I'd like to try to help.

In grid mode:

  • currently only the search field and mime types select have a label
  • mime types label text is a bit too generic ("Select"), should provide more context
  • added labels to date filter and bulk actions selects
  • moved the spinner so now respects priority (hopefully)

Please consider I'm not so familiar with backbone.js, sorry @wonderboymusic! Be nice with me :)

In list mode:

  • added a label to date filter select

Changed or added some labels text for 2 reasons:

  • try to avoid duplicated strings to translate (capitalized version, non capitalized version... see e.g. 'Select Bulk Action' in #28867)
  • for consistency and parity between list and grid mode

Attached a screenshot where I made the grid mode form labels *temporarily* visible, just to have an idea about the changes.

Attachments (3)

29026.patch (5.8 KB) - added by afercia 6 years ago.
labels for Media Grid and List
media-grid-form-labels.png (8.5 KB) - added by afercia 6 years ago.
29026.2.patch (1.6 KB) - added by afercia 6 years ago.
adds check for grid mode and moves back setting of the spinner in its original position

Download all attachments as: .zip

Change History (22)

@afercia
6 years ago

labels for Media Grid and List

#1 @afercia
6 years ago

  • Keywords has-patch needs-testing added

#2 @wonderboymusic
6 years ago

  • Focuses accessibility javascript added
  • Milestone changed from Awaiting Review to 4.0

Nice work, looks harmless

#3 @ocean90
6 years ago

I played a bit with a wp.media.view.Label view, which should be used to add labels to our input fields, see https://gist.github.com/ocean90/244470a43e4f5be77e16. The issue is, that this.$el.before doesn't work. :(

#4 @sumobi
6 years ago

Tested 29026.patch looks good to me

This ticket was mentioned in IRC in #wordpress-dev by wonderboymusic. View the logs.


6 years ago

#6 @wonderboymusic
6 years ago

  • Owner set to wonderboymusic
  • Resolution set to fixed
  • Status changed from new to closed

In 29428:

Media Grid: add screen reader labels with more context to filters and bulk actions. Ditto for list view.

Props afercia.
Fixes #29026.

#7 @afercia
6 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Sorry looks like my patch breaks something when opening media frame from edit post pages.
New attached patch adds a check for grid mode and moves back setting of the spinner in its original position. Should be ok now, though the spinner now doesn't respect priority.

@afercia
6 years ago

adds check for grid mode and moves back setting of the spinner in its original position

#8 @azaozz
6 years ago

Thinking to revert [29428] for now. I may be missing something but why are the labels "hacked" from JS/jQiery and not part of a template?

Actually it seems the whole "filters bar" (media.view.Toolbar and/or media.view.PriorityList) should be able to use a template.

Last edited 6 years ago by azaozz (previous) (diff)

#9 @wonderboymusic
6 years ago

There was precedent for the this.$el.before( whatever ) so I just rolled with it. If the labels will always match the context of the things they are preceding, then yes, they should be in the Underscore templates.

#10 @azaozz
6 years ago

Yeah, seems this.$el.before( whatever ) sometimes fails as $el is not attached to the DOM yet. We can use append()/prepend() as the node exists, but not before()/after().

Last edited 6 years ago by azaozz (previous) (diff)

#11 @wonderboymusic
6 years ago

In 29432:

Revert [29428] to avoid errors with Add Media for now.

See #29026.

#12 @wonderboymusic
6 years ago

Reverted, this needs a reboot

#13 @wonderboymusic
6 years ago

In 29433:

Add screen reader text to list table date dropdown.

See #29026.

This ticket was mentioned in IRC in #wordpress-dev by wonderboymusic. View the logs.


6 years ago

#15 @wonderboymusic
6 years ago

In 29434:

Create a new Backbone view, media.view.Label, which can be used as a subview in things like toolbars, which need screen reader text.

Make an initial instance of it for the label for Bulk Actions in Media Grid.

See #29026.

#16 @wonderboymusic
6 years ago

In 29435:

Media Grid: add screen reader text in a subview on the toolbar, a media.view.Label instance before media.view.DateFilter.

See #29026.

#17 @wonderboymusic
6 years ago

In 29437:

Media Grid: use a media.view.Label instance in the toolbar for the screen reader text for Search.

See #29026.

#18 @wonderboymusic
6 years ago

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

In 29438:

Media Grid:

  • Use a media.view.Label instance in the toolbar for the screen reader text for the "all" dropdown.
  • Adjust the spinner's priority.

Fixes #29026.

#19 @wonderboymusic
6 years ago

In 29440:

Mark WP_List_Table::bulk_actions()'s argument $which as optional for BC.

Fixes #29026.

Note: See TracTickets for help on using tickets.