Make WordPress Core

Opened 4 years ago

Last modified 3 years ago

#49587 new defect (bug)

Add error handling for the media manager Ajax response

Reported by: johnbillion's profile johnbillion Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 3.5
Component: Media Keywords: needs-patch
Focuses: javascript, administration Cc:

Description

When the media manager modal performs an Ajax request to query attachments, if the Ajax call responds with an error then the user is presented with an eternal loading spinner and no indication there has been a problem.

I think there are two places which need a fail() handler for the Ajax promise:

  • wp.media.model.Attachments.more()
  • wp.media.model.Query.more()

I haven't done any testing yet though to confirm this.

Needs a decision around how best to present an error to a user. Probably needs UI work; maybe one of the UI pieces such as the upload failure message can be reused.

Attachments (3)

media-modal loading.png (17.1 KB) - added by ibdz 4 years ago.
media-modal error.png (19.1 KB) - added by ibdz 4 years ago.
Screenshot 2021-02-04 at 10.04.45.png (78.1 KB) - added by johnbillion 3 years ago.
Screenshot of existing error handler for drag and drop upload

Download all attachments as: .zip

Change History (21)

#1 @joemcgill
4 years ago

  • Milestone changed from Awaiting Review to 5.5

Thanks for the report @johnbillion, let's add this to 5.5 to confirm and get some design feedback on the best user experience for this.

#2 @johnbillion
4 years ago

  • Keywords needs-design added

#3 @johnbillion
4 years ago

  • Keywords needs-ux removed

#4 @estelaris
4 years ago

  • Keywords needs-screenshots added

This ticket was discussed in Design triage, we would like to see the error in a screenshot and see the "upload failure message" that could be used to solve the UI display.

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


4 years ago

#6 @antpb
4 years ago

  • Milestone changed from 5.5 to 5.6

thanks @estelaris! @johnbillion what part of the modal do you see this happening in? I'm happy to get a screenshot of the error but I think the error here is that there is no error message to begin with.

I can force one in the place you're thinking if that would be helpful for the Design team. I think the confusion on my end is just where this is happening in the media modal. I'm not super confident this is a 5.5 task given we are in Beta 1. Going to move this to 5.6. Feel free to change back if that is incorrect.

#7 @johnbillion
4 years ago

The errors above occur when fetching the list of attachments via Ajax, either for the first load or for subsequent loads as part of infinite scrolling.

You can force this by calling wp_send_json_error() in wp_ajax_query_attachments().

You're right that the idea behind this ticket is that there is no error handling at all at the moment, and the user will see an eternally spinning spinner.

#8 @johnbillion
4 years ago

This also applies if there's a network problem and there's no response, or there's a fatal error during the Ajax call, etc.

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


4 years ago

#10 @ibdz
4 years ago

  • Keywords needs-design-feedback added; needs-design needs-screenshots removed

I did some design for error message showing in Media Modal. When there's an error, the spinner should be stopped (replaced) and show the error message instead. The message might need more elaborate.

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


4 years ago

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


3 years ago

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


3 years ago

#14 @hedgefield
3 years ago

  • Keywords needs-design-feedback removed

This looks like a fine solution to me @idbz. It fits nicely in the space where the spinner appears as well, with a clear and concise message. I'm not sure "HTTP error" is all that helpful, the thing is that for the user, the "request failed". The reason is less important, except maybe for troubleshooting if there are more ways in which the request can fail. But that probably appears in the console?

Either way, I'd say this design with "Request failed, try again" as copy is good to move ahead to dev. Is there a dev available who can write this patch?

#15 @johnbillion
3 years ago

  • Milestone changed from 5.6 to Future Release

Thanks for the feedback @ibdz @hedgefield !

At the code level we can pass through whatever error message comes from the failure, so we ought to be able to avoid generic messages such as HTTP error here and possibly append a suffix such as Try again, although depending on the error it might not be possible to try again.

I don't have time to write a patch for this so I'm bumping it for now. If someone else wants to pick it up, great!

@johnbillion
3 years ago

Screenshot of existing error handler for drag and drop upload

#16 @johnbillion
3 years ago

I discovered today there is already UI for an error in this view. If you attempt to drag and drop a file while you're on the Media Library tab and there is an error from the upload, the error message appears in the sidebar. Screenshot above.

We can reuse this existing UI for the error message, we just need to connect it up to the response from the Ajax call for attachment queries.

This ticket was mentioned in PR #1065 on WordPress/wordpress-develop by johnbillion.


3 years ago
#17

  • Keywords has-patch added; needs-patch removed

#18 @johnbillion
3 years ago

  • Keywords needs-patch added; has-patch removed

I started working on this in https://github.com/WordPress/wordpress-develop/pull/1065 but I forgot how difficult it is to figure out the media manager views. If someone wants to pick this up and carry on with it that would be great.

Note: See TracTickets for help on using tickets.