WordPress.org

Make WordPress Core

Opened 11 months ago

Closed 8 weeks ago

#44675 closed defect (bug) (fixed)

Update Media Trash Bulk Action, Modal Action, Publishing Action and Bulk Media Action to Move to Trash

Reported by: garrett-eclipse Owned by: pento
Milestone: 5.2 Priority: normal
Severity: normal Version: 5.2
Component: Media Keywords: has-patch commit
Focuses: ui, administration Cc:

Description

Hello,

While looking at the consistency for Bulk Actions I came across the Media list table uses 'MEDIA_TRASH' to enable it's trash functionality. It's trash action is labelled as 'Trash' with a context of 'verb' this is inconsistent with all other similar implementations of trash in the list tables as they all use 'Move to Trash' to remove the ambiguity. Aside from the consistency benefit we'll be able to remove the context as it's no longer needed to distinguish it from the Trash filter or page.

So replacing this line;
https://github.com/WordPress/WordPress/blob/8e01f9f99b9cc39bed623543f77752c819132699/wp-admin/includes/class-wp-media-list-table.php#L154

With - $actions['trash'] = __( 'Move to Trash' );

Will make it consistent with all other implementations;
Comments list table - https://github.com/WordPress/WordPress/blob/1b5d6c6971c158e38b2f7c4ac6acfa19276aa5df/wp-admin/includes/class-wp-comments-list-table.php#L351
Posts list table - https://github.com/WordPress/WordPress/blob/1b5d6c6971c158e38b2f7c4ac6acfa19276aa5df/wp-admin/includes/class-wp-posts-list-table.php#L413

This will also clear up confusion for translators as it seemed my local en_CA was replacing Trash with Delete in this case which isn't correct;
https://translate.wordpress.org/projects/wp/dev/en-ca/default?filters%5Bstatus%5D=either&filters%5Boriginal_id%5D=202108&filters%5Btranslation_id%5D=9657614
*I'm cleaning up the translations.

Thank you

Attachments (19)

44675.diff (570 bytes) - added by ianbelanger 11 months ago.
Patch to change verbiage from "Trash" to "Move to Trash"
44675.2.diff (4.0 KB) - added by garrett-eclipse 3 months ago.
Update Media actions (Bulk Action, Publishing Action, Media Modal Action, Bulk Media Action) to be consistent with other areas, left Row Actions as single term
Screen Shot 2019-03-29 at 11.40.44 PM.png (87.5 KB) - added by garrett-eclipse 3 months ago.
Original Bulk Media Action
Screen Shot 2019-03-29 at 11.40.31 PM.png (70.4 KB) - added by garrett-eclipse 3 months ago.
Original Media Publishing Action
Screen Shot 2019-03-29 at 11.43.03 PM.png (14.8 KB) - added by garrett-eclipse 3 months ago.
Original Modal Action
Screen Shot 2019-03-29 at 11.42.22 PM.png (23.5 KB) - added by garrett-eclipse 3 months ago.
Updated Bulk Action
Screen Shot 2019-03-29 at 11.42.40 PM.png (71.4 KB) - added by garrett-eclipse 3 months ago.
Updated Publishing Action
Screen Shot 2019-03-29 at 11.56.37 PM.png (17.4 KB) - added by garrett-eclipse 3 months ago.
Updated Modal Action
44675.2.selected.media.diff (4.1 KB) - added by garrett-eclipse 3 months ago.
Potential addition to cleanup the Bulk Media action buttons. Updating button verbiage to 'Move Selected Media to Trash', 'Restore Selected Media from Trash' and 'Delete Selected Media'
Screen Shot 2019-03-30 at 12.07.04 AM.png (93.0 KB) - added by garrett-eclipse 3 months ago.
Bulk Media Action buttons w/ updated Verbiage
Screen Shot 2019-03-30 at 12.07.18 AM.png (88.7 KB) - added by garrett-eclipse 3 months ago.
Trash view Bulk Media Action buttons w/ updated Verbiage
44675.2.selected.media.permanently.diff (4.1 KB) - added by garrett-eclipse 3 months ago.
Sorry last minor change, added 'Permanently' to the Bulk Media Action button to make it Delete Selected Media Permanently
44675.3.diff (4.3 KB) - added by ianbelanger 3 months ago.
Updates patch with design suggestions
44675.4.diff (16.4 KB) - added by garrett-eclipse 3 months ago.
Updated Patch to remove unneeded js.i10n strings and fix qunit string tests.
Screen Shot 2019-04-01 at 10.17.33 PM.png (119.3 KB) - added by garrett-eclipse 3 months ago.
Bulk Media string change w/o Media Trash
Screen Shot 2019-04-01 at 10.18.21 PM.png (111.6 KB) - added by garrett-eclipse 3 months ago.
Bulk Media Select w/ Media Trash
Screen Shot 2019-04-01 at 10.21.51 PM.png (89.8 KB) - added by garrett-eclipse 3 months ago.
Bulk Media Select w/ Trash - Restore
Screen Shot 2019-04-08 at 8.08.35 PM.png (57.8 KB) - added by garrett-eclipse 2 months ago.
From translate.wp.org it appears I missed removing a verb context which is causing a duplicate unnecessary string
44675.5.diff (792 bytes) - added by garrett-eclipse 2 months ago.
Patch to address the unnecessary and incorrect 'verb' context on 'Move to Trash'

Download all attachments as: .zip

Change History (51)

@ianbelanger
11 months ago

Patch to change verbiage from "Trash" to "Move to Trash"

#1 @ianbelanger
11 months ago

  • Keywords has-patch added

#2 @garrett-eclipse
11 months ago

  • Keywords good-first-bug removed

Thanks @ianbelanger that was quick. Looks good to me

#3 @swissspidy
11 months ago

  • Component changed from I18N to Media

#4 @afercia
5 months ago

  • Milestone changed from Awaiting Review to 5.2
  • Version trunk deleted

Makes sense. Not a new issue introduced in trunk though.

This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.


5 months ago

#6 @mikeschroder
5 months ago

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

#7 @johnbillion
3 months ago

Ref 44675.diff: this isn't the only place that _x( 'Trash', 'verb' ) is used, there are quite a few instances. Should they all be changed?

#8 @garrett-eclipse
3 months ago

  • Keywords needs-refresh added

Thanks @johnbillion IMHO all of the action labels such as the Bulk Actions and the Row Actions should all be 'Move to Trash'. And then 'Trash' should be left as is when referring to the View Filter.

The other instances I found that should be updated are;
Media Bulk Action - https://github.com/WordPress/WordPress/blob/56c162fbc9867f923862f64f1b4570d885f1ff03/wp-admin/includes/class-wp-media-list-table.php#L154
Media Row Action - https://github.com/WordPress/WordPress/blob/56c162fbc9867f923862f64f1b4570d885f1ff03/wp-admin/includes/class-wp-media-list-table.php#L680
Comments Row Action* - https://github.com/WordPress/WordPress/blob/56c162fbc9867f923862f64f1b4570d885f1ff03/wp-admin/includes/class-wp-media-list-table.php#L680
Comments Handle Row Action - https://github.com/WordPress/WordPress/blob/6fe48278eaa539f33bbfe86ccd24b2e7dd6fc83a/wp-admin/includes/class-wp-comments-list-table.php#L643
Posts Row Action - https://github.com/WordPress/WordPress/blob/99b929cbbc72171eabc095c2f089316cbc261dbb/wp-admin/includes/class-wp-posts-list-table.php#L1366
Dashboards Comments Row Action - https://github.com/WordPress/WordPress/blob/e901bc4c2bbf259258ce3aa3f3d82bcb8f7a6281/wp-admin/includes/dashboard.php#L658
Attachment Submit Meta Box - https://github.com/WordPress/WordPress/blob/6fe48278eaa539f33bbfe86ccd24b2e7dd6fc83a/wp-admin/includes/meta-boxes.php#L402
*These in GitHub show they are 'Trash' but looking them up in translate.wp.org they're Move to Trash so not sure why.
Please refer to any I've overlooked and we'll get this refreshed.

All the best

#9 follow-up: @afercia
3 months ago

Noting the same argumentation applies also to "Spam".

For some background, see #11258

It's a long discussion about the order of the Row Actions links in the Comments page and it touches also the "Trash" and "Spam" verb / noun ambiguity.

At some point it was considered to add "Move to" and "Mark as" to the Trash and Spam links. Then, decided against it because the links had the title attributes "tooltips" that clarified the context. See ticket:11258#comment:4 Not saying those title attributes were an effective solution.

In fact, in the last years, WordPress has been actively discouraging the use of title attributes and those title attributes were removed. So the ambiguity persists but I'd be cautious in adding words to the Row Actions links. While it makes perfectly sense to expand "Trash" to "Move to trash" where there's enough space e.g.:

http://cldup.com/588vFmf_gq.png

instead, adding more words to the Row Actions links would add some cognitive load and make the links less readable overall.

#10 in reply to: ↑ 9 @garrett-eclipse
3 months ago

Thanks @afercia

Replying to afercia:

For some background, see #11258
It's a long discussion about the order of the Row Actions links in the Comments page and it touches also the "Trash" and "Spam" verb / noun ambiguity.

I appreciate your raising those points and I agree with the comment lets KISS on the Row Actions to conserve space.

At some point it was considered to add "Move to" and "Mark as" to the Trash and Spam links. Then, decided against it because the links had the title attributes "tooltips" that clarified the context. See ticket:11258#comment:4 Not saying those title attributes were an effective solution.
In fact, in the last years, WordPress has been actively discouraging the use of title attributes and those title attributes were removed.

Do you have a recommendation on how to handle the Row Actions and provide the additional context? Is there a more accessible fashion to add the 'Move to Trash' & 'Mark as Spam' to the Row Actions?

So the ambiguity persists but I'd be cautious in adding words to the Row Actions links. While it makes perfectly sense to expand "Trash" to "Move to trash" where there's enough space e.g.:

http://cldup.com/588vFmf_gq.png

instead, adding more words to the Row Actions links would add some cognitive load and make the links less readable overall.

I agree, let's KISS on the Row Actions and keep the verbiage to a minimum. And we can go for the expanded verbiage on the Bulk Actions and Publish Actions.

@garrett-eclipse
3 months ago

Update Media actions (Bulk Action, Publishing Action, Media Modal Action, Bulk Media Action) to be consistent with other areas, left Row Actions as single term

@garrett-eclipse
3 months ago

Original Bulk Media Action

@garrett-eclipse
3 months ago

Original Media Publishing Action

@garrett-eclipse
3 months ago

Original Modal Action

@garrett-eclipse
3 months ago

Updated Publishing Action

#11 @garrett-eclipse
3 months ago

After doing a recon on 'Trash' strings I found, taking the thoughts from above comments into account, that the misuse of 'Trash' was limited to the Media Library.

The patch 44675.2.diff addresses this as the attached screenshots illustrate.

@garrett-eclipse
3 months ago

Potential addition to cleanup the Bulk Media action buttons. Updating button verbiage to 'Move Selected Media to Trash', 'Restore Selected Media from Trash' and 'Delete Selected Media'

@garrett-eclipse
3 months ago

Bulk Media Action buttons w/ updated Verbiage

@garrett-eclipse
3 months ago

Trash view Bulk Media Action buttons w/ updated Verbiage

#12 @garrett-eclipse
3 months ago

Also in 44675.2.diff I had updated the Bulk Media Actions to;
Move Selected to Trash
Restore Selected from Trash

But I felt it would be more clear if we added 'Media' so in 44675.2.selected.media.diff I updated the verbiage on Bulk Media Action Buttons to (as illustrated in above screenshots);
Move Selected Media to Trash
Restore Selected Media from Trash
Delete Selected Media

@garrett-eclipse
3 months ago

Sorry last minor change, added 'Permanently' to the Bulk Media Action button to make it Delete Selected Media Permanently

#13 follow-up: @afercia
3 months ago

Do you have a recommendation on how to handle the Row Actions and provide the additional context? Is there a more accessible fashion to add the 'Move to Trash' & 'Mark as Spam' to the Row Actions?

I know in the past there have been discussions around introducing "tooltips" in core and it was decided against it. Now that Gutenberg introduced accessible tooltips, maybe they could be reconsidered. However, I'm not sure it is worth it. While accessible tooltips are necessary for controls that use only icons, I'm not sure they're really necessary for controls that have some text. I'm afraid in this specific case they would just add unnecessary complexity.

#14 in reply to: ↑ 13 @garrett-eclipse
3 months ago

  • Keywords needs-design-feedback needs-testing added; needs-refresh removed
  • Summary changed from Media Trash action verbiage inconsistent and causes confusion to Update Media Trash Bulk Action, Modal Action, Publishing Action and Bulk Media Action to Move to Trash

Replying to afercia:

Do you have a recommendation on how to handle the Row Actions and provide the additional context? Is there a more accessible fashion to add the 'Move to Trash' & 'Mark as Spam' to the Row Actions?

I know in the past there have been discussions around introducing "tooltips" in core and it was decided against it. Now that Gutenberg introduced accessible tooltips, maybe they could be reconsidered. However, I'm not sure it is worth it. While accessible tooltips are necessary for controls that use only icons, I'm not sure they're really necessary for controls that have some text. I'm afraid in this specific case they would just add unnecessary complexity.

Thanks @afercia I appreciate that feedback and concur it's an unnecessary complexity.

I've updated this ticket to make it's focus on the Media Trash actions, all but the row Action were updated in my patches.
I've flagged needs-design-feedback to get their thoughts on the verbiage variants I provided for the Bulk Media Buttons.
As well as tagged for testing.

Any changes to the accessibility patterns on these actions we can revisit in another ticket, for now am going to have the focus just bringing Media Trash verbiage up to be consistent with the rest of post type actions.

Note to testers; To enable the Media trash place define( 'MEDIA_TRASH', true ); in your wp-config.php.

#15 @garrett-eclipse
3 months ago

  • Focuses ui administration added

*Adding ui/admin focuses.

#16 @ianbelanger
3 months ago

  • Keywords needs-testing removed

The patch looks good @garrett-eclipse. This can be tagged for commit pending design-feedback

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


3 months ago

#18 @estelaris
3 months ago

  • Keywords needs-design-feedback removed

As discussed with the #design team, we concluded that is best to maintain verbiage as short as possible without affecting the context. Short phrases are easier for screen readers and also for translations.

Cancel Selection -> Cancel
Restore Selected -> Restore from Trash
Delete Selected -> Delete Permanently
Move Selected -> Move to Trash

@ianbelanger
3 months ago

Updates patch with design suggestions

#19 @ianbelanger
3 months ago

  • Keywords needs-testing added

44675.3.diff builds on @garrett-eclipse's latest patch, updating the verbiage to what was suggested by @estelaris

Testing would be appreciated

#20 @Presskopp
3 months ago

I'm not sure about

'deleteSelected' => __( 'Delete Permanently' ),
'deletePermanently' => __( 'Delete Permanently' ),

related #45317

@garrett-eclipse
3 months ago

Updated Patch to remove unneeded js.i10n strings and fix qunit string tests.

@garrett-eclipse
3 months ago

Bulk Media string change w/o Media Trash

@garrett-eclipse
3 months ago

Bulk Media Select w/ Media Trash

@garrett-eclipse
3 months ago

Bulk Media Select w/ Trash - Restore

#21 @garrett-eclipse
3 months ago

Thanks for the refresh @ianbelanger.

@Presskopp good catch there, I've adjusted the patch w/ 44675.4.diff to remove the redundancy as well of that for the Cancel string.

Also in the attached patch is updated qunit tests to address the string changes.

Would love another once over from you both. I attached screens as well for @estelaris and the design team to give a final review of.

All the best,
Cheers
P.S. Also coming from that design chat I created #46757 and #46758 to address the other concerns with the Bulk Media Trash functionality.

#22 @ianbelanger
3 months ago

  • Keywords commit added; needs-testing removed

Patch looks good @garrett-eclipse and the qunit tests passed for me. Nice work!

#23 @garrett-eclipse
3 months ago

Thanks @ianbelanger I appreciate your help getting this to the goal line.

This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.


3 months ago

#25 @pento
2 months ago

  • Owner changed from mikeschroder to pento
  • Status changed from assigned to accepted

#26 @pento
2 months ago

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

In 45147:

Media: Update the media bulk action labels for consistency.

Props ianbelanger, garrett-eclipse, afercia, Presskopp.
Fixes #44675.

@garrett-eclipse
2 months ago

From translate.wp.org it appears I missed removing a verb context which is causing a duplicate unnecessary string

@garrett-eclipse
2 months ago

Patch to address the unnecessary and incorrect 'verb' context on 'Move to Trash'

#27 @garrett-eclipse
2 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened
  • Version set to trunk

Hi @pento thank you for the commit. Now that I see it percolate up to translate.wp.org I realize I've overlooked a verb context that should have been removed. My apologies. Anyway we can revisit that last string to remove the duplicate for translators?
I've uploaded 44675.5.diff to address this string.
Thank you

#28 @pento
2 months ago

In 45153:

Media: Remove an extra verb context missed in [45147].

Props garrett-eclipse.
See #44675.

#29 @pento
2 months ago

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

#30 @RogerTheriault
8 weeks ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

This change is somewhat detrimental to sites with a large number of posts in the wp_posts table. The use of get_available_post_mime_types in media.php is particularly problematic, it results in a complete table scan and would benefit from some caching.

In testing, we saw this query running on most wp-admin pages, especially post.php and edit.php, and adding at least a few seconds of page load time to any site with > 1 million rows in wp_posts. In some cases, it takes much much longer.

SELECT DISTINCT post_mime_type FROM wp_posts WHERE post_type = 'attachment'

Last edited 8 weeks ago by RogerTheriault (previous) (diff)

#31 @RogerTheriault
8 weeks ago

Possibly this actually stemmed from https://core.trac.wordpress.org/ticket/43658#comment:14 but I've reopened both issues...

#32 @pento
8 weeks ago

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

Thanks for the bug report, @RogerTheriault! It looks like #43658 is the culprit, so I'll re-close this one to keep discussion in the same place.

Note: See TracTickets for help on using tickets.