Make WordPress Core

Opened 7 years ago

Closed 4 years ago

#43656 closed defect (bug) (fixed)

Media attachment filter does not work after switching list mode to grid for image attachment filter

Reported by: itzmekhokan's profile itzmekhokan Owned by: adamsilverstein's profile adamsilverstein
Milestone: 5.5 Priority: normal
Severity: major Version: 4.9.4
Component: Media Keywords: has-screenshots has-patch commit
Focuses: Cc:

Description

On media library if you switch to List mode and try to filter by image type attachment filter, it's perfectly filtered by image type in list mode.
Now at this time try to switch media mode to Grid, you will see all the attachments filtered by image type. And now if you try to filter by any attachments type filter in grid mode, it does not work. Only faced this issue for image type filter for switching mode List to Grid. Rest of attachments filters works perfectly in above case.

Attachments (4)

media-attachment-filter-doesnt-work.mp4 (2.0 MB) - added by itzmekhokan 7 years ago.
After filter by image type from media attachment filter and then after switching mode list to grid attachment filter doesn't work in grid filter
Bildschirmfoto 2019-10-24 um 10.29.22.png (138.6 KB) - added by zodiac1978 5 years ago.
Used filter "Documents" in list view (showing just Documents correctly)
Bildschirmfoto 2019-10-24 um 10.29.41.png (74.7 KB) - added by zodiac1978 5 years ago.
Switching to Grid view - the dropwdown shows "All media" but the output is still just "Documents" because of attachment-filter in URL
43656.diff (603 bytes) - added by adamsilverstein 4 years ago.

Change History (19)

@itzmekhokan
7 years ago

After filter by image type from media attachment filter and then after switching mode list to grid attachment filter doesn't work in grid filter

#1 @itzmekhokan
7 years ago

  • Keywords has-screenshots added

#2 @netsurfer2705
6 years ago

  • Severity changed from normal to major

I can confirm this bug!
It is caused by the presence of the "attachment-filter" in the URL, if it has any value other than empty.

The href attribute of the "view-switch" icon (id:'view-switch-grid') holds all the query parameters, which are then (falsely) used for the ajax query.
And if the first query for grid view does have a value for "attachment-filter", then you'll never be able to see "all" media!

BTW: The ajax query is also totally "buggy". Normally one should be able to change the $query parameter byy using the 'ajax_query_attachments_args' filter. But this will have no effect for most of the query parameters, because some of them are hardcoded elsewhere in core!!!

So there is no easy fix for this. Because to get it right, I would also expect the mime-type selection drop-down to be set to choosen value from the list view (and of course to work for "all").

Also one should think about the trash option (if enabled), where all other filters should be ignored.

@zodiac1978
5 years ago

Used filter "Documents" in list view (showing just Documents correctly)

@zodiac1978
5 years ago

Switching to Grid view - the dropwdown shows "All media" but the output is still just "Documents" because of attachment-filter in URL

#3 @zodiac1978
5 years ago

  • Keywords needs-patch dev-feedback added

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


5 years ago

#5 @zodiac1978
4 years ago

We could remove the filter in the view_switcher function in /wp-admin/inludes/class-wp-list-table.php

<?php
                        printf(
                                "<a href='%s' class='%s' id='view-switch-$mode'$aria_current><span class='screen-reader-text'>%s</span></a>\n",
                                esc_url( remove_query_arg( 'attachment-filter', add_query_arg( 'mode', $mode ) ) ),
                                implode( ' ', $classes ),
                                $title
                        );

This would at least solve the mismatch between the dropdown and the display of media.

As an alternative we could convert the filter to the correct dropdown setting. But this converting would need some JS, I think.

Last edited 4 years ago by zodiac1978 (previous) (diff)

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


4 years ago

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


4 years ago

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


4 years ago

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


4 years ago

#10 follow-up: @adamsilverstein
4 years ago

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

@zodiac1978 I like your suggestion to remove the parameter- the other idea to explore would be to properly select the filtered item in the grid mode when switching (assuming that fixes the behavior you noted).

#11 in reply to: ↑ 10 @zodiac1978
4 years ago

Replying to adamsilverstein:

[...] the other idea to explore would be to properly select the filtered item in the grid mode when switching (assuming that fixes the behavior you noted).

Hi @adamsilverstein - exactly, this is what I meant by this:

As an alternative we could convert the filter to the correct dropdown setting. But this converting would need some JS, I think.

I will prepare a patch for the first idea!

This ticket was mentioned in PR #318 on WordPress/wordpress-develop by Zodiac1978.


4 years ago
#12

  • Keywords has-patch added; needs-patch removed

Remove attachment-filter on switch between grid/list
Fixes https://core.trac.wordpress.org/ticket/43656

Trac ticket: https://core.trac.wordpress.org/ticket/43656

#13 @adamsilverstein
4 years ago

Excellent, thank you @zodiac1978 - I will review the PR.

I experimented a bit with preserving the selection when switching to grid, but selecting All still did not work correctly. Removing the parameter seems like the best approach.

#14 @adamsilverstein
4 years ago

  • Keywords commit added; dev-feedback removed
  • Milestone changed from Awaiting Review to 5.5

43656.diff from @zodiac1978's PR works as expected - nice work!

I want to make sure the unit tests pass, then I'll get this committed.

#15 @adamsilverstein
4 years ago

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

In 47915:

Media: remove any attachment-filter query param when switching views.

Fix an issue where the media attachment filter no longer worked after switching to list view, filtering by a media type then switching back to the grid view.

Props itzmekhokan, netsurfer2705, zodiac1978.
Fixes #43656.

Note: See TracTickets for help on using tickets.