WordPress.org

Make WordPress Core

Opened 8 months ago

Last modified 5 days ago

#49587 new defect (bug)

Add error handling for the media manager Ajax response

Reported by: 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 (2)

media-modal loading.png (17.1 KB) - added by ibdz 6 weeks ago.
media-modal error.png (19.1 KB) - added by ibdz 6 weeks ago.

Download all attachments as: .zip

Change History (17)

#1 @joemcgill
7 months 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
7 months ago

  • Keywords needs-design added

#3 @johnbillion
6 months ago

  • Keywords needs-ux removed

#4 @estelaris
4 months 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.


3 months ago

#6 @antpb
3 months 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
3 months 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
3 months 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.


6 weeks ago

#10 @ibdz
6 weeks 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.


5 weeks ago

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


3 weeks ago

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


8 days ago

#14 @hedgefield
6 days 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
5 days 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!

Note: See TracTickets for help on using tickets.