WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 3 months ago

#38221 closed defect (bug) (fixed)

Incorrect page id/name search blocks future searches

Reported by: bencroskery Owned by: tomdude
Milestone: 5.5 Priority: normal
Severity: normal Version: 4.4.2
Component: Media Keywords: has-patch 2nd-opinion
Focuses: accessibility, javascript Cc:

Description

After an incorrect page id or name search in the media library, ex upload.php?p=123 or upload.php?name=noexist, future searches get blocked in js.

Steps to reproduce:

  1. go to upload.php?p=123 (or use another id which doesn't exist)
  2. should see 'No media files found.'
  3. submit a search in the search box (nothing will happen)

Submitting the form is blocked by wp-admin/js/media.js:90, preventing the submit for (an unrelated?) search #find-posts.

Attachments (4)

38221.diff (672 bytes) - added by tomdude 3 years ago.
38221.1.diff (825 bytes) - added by audrasjb 4 months ago.
Media: Prevent incorrect previous queries to block media search input
38221.2.diff (845 bytes) - added by afercia 4 months ago.
38221.3.diff (1.7 KB) - added by SergeyBiryukov 3 months ago.

Download all attachments as: .zip

Change History (20)

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


3 years ago

#2 @desrosj
3 years ago

  • Keywords needs-patch good-first-bug added

Welcome to trac, @bencroskery! I was able to reproduce this, but it did take one change. This only happens in the list view in the media library, not the grid view. To reproduce, you must add mode=list to the URL.

upload.php?p=123&mode=list

Are you interested in tackling this @bencroskery?

@tomdude
3 years ago

#3 @tomdude
3 years ago

  • Keywords needs-patch removed

Created patch to fix this. Adds check if any posts exist for preventDefault conditional.

#4 @tomdude
3 years ago

  • Keywords has-patch added

#5 @DrewAPicture
3 years ago

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

Assigning to mark the good-first-bug as "claimed".

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


6 months ago

#7 @SergeyBiryukov
6 months ago

  • Milestone changed from Awaiting Review to 5.5

@audrasjb
4 months ago

Media: Prevent incorrect previous queries to block media search input

#8 @audrasjb
4 months ago

  • Focuses accessibility added
  • Keywords commit added

Hi there,

Adding accessibility focus as this is a blocker for the search input’s accessibility.

The proposed patch perfectly fixes the issue. I tested it and it works fine.

I refreshed the patch since JS dependencies were completely moved.

I think 38221.1.diff is clearly good to go, so marking this for commit.

Cheers,
Jb

#9 follow-up: @afercia
4 months ago

  • Keywords 2nd-opinion added; good-first-bug commit removed

Thanks for the patches. Looking a bit into this, I'm not sure I understand what is the scenario where, using the admin interface, parameters like ?p=123 or ?name=noexist get added to the query string. Maybe I'm missing something but I couldn't find such a case.

Appending manually random parameters may break many other admin screens as well. Clarifying whether there are actual cases where these parameters are added while using the UI may help. Otherwise I'm not sure this can be considered an real bug.

Regardless, I'm not sure it should be fixed on the JavaScript side. The root cause of this odd behavior is that there are multiple "submit" buttons in the same form element, e.g.:

  • the "Filter" button
  • the "Select" button inside the "Attach to existing content" modal dialog: though it's visually hidden and appears only when clicking the "attach" link, it's in the same form and brings some JavaScript into play

When manually appending ?p=123 or ?name=noexist, the "Filter" button is not printed out. Thus, the "Select" button is the one that browsers consider the form submit button and triggers a click event when submitting a search by pressing Enter in the search field. The "Select" button has some JavaScript that prevents the default action when none of the radio buttons representing the posts within the modal is checked so the search submission is prevented.

A more proper fix would be making sure the "Filter" button is always printed out. It isn't because the ! is_singular() check originally introduced in [7251] fails: this is very old code and I'm not sure I understand whether it still needs to be there.

Worth noting this may happen also on other list tables where the same ! is_singular() check is used, it would just need a URL parameter that makes WP_Query consider the page as singular.

I'd like to ask for a second opinion from some experienced contributor with some historical knowledge of this ! is_singular() conditional implementation.

@afercia
4 months ago

#10 @afercia
4 months ago

38221.2.diff is a proof of concept that makes the "Filter" submit button always visible and thus fixes the search.

Still, I'd lean towards a wontfix if there are no actual cases where those URL parameters are added by using the admin interface.

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


3 months ago

#12 @afercia
3 months ago

Another approach could be using the form attribute on the button so that the button is instructed to submit a specific form. No support in IE though. https://developer.mozilla.org/en-US/docs/Web/HTML/Element/button#Browser_compatibility

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


3 months ago

#14 in reply to: ↑ 9 @SergeyBiryukov
3 months ago

Replying to afercia:

Thanks for the patches. Looking a bit into this, I'm not sure I understand what is the scenario where, using the admin interface, parameters like ?p=123 or ?name=noexist get added to the query string. Maybe I'm missing something but I couldn't find such a case.

I also could not find such a case. Thinking about it, I went back and forth for a bit between trying to fix it and closing as wontfix, since adding arbitrary parameter to the URL doesn't seem like a common use case.

That said, I think the is_singular() check can be removed here:

  • The instance in class-wp-posts-list-table.php was added in [7247].
  • The instance in class-wp-media-list-table.php was added in [7251].

As far as I can tell, both instances are now redundant and don't serve any purpose:

  • wp-admin/edit.php?p=123 redirects to wp-admin/edit-comments.php?p=123
  • wp-admin/upload.php?p=123 displays a single media item (assuming the ID exists), but without the filter.

I think it would make sense to always show the filter here, regardless of the is_singular() check. If that also fixes the issue reported here, sounds great :)

See 38221.3.diff.

Last edited 3 months ago by SergeyBiryukov (previous) (diff)

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


3 months ago

#16 @SergeyBiryukov
3 months ago

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

In 48240:

Administration: Always show the filters on media and post list tables.

Previously, the filters were hidden for single posts or attachments, which could only be achieved by editing the URL manually.

The is_singular() check was added long before the list tables were introduced, and appears to no longer serve any purpose in the current code.

As a side effect, this resolves an issue where a non-existing attachment ID in the URL would block further search in Media Library.

Props afercia, tomdude, audrasjb, bencroskery, desrosj, SergeyBiryukov.
Fixes #38221.

Note: See TracTickets for help on using tickets.