WordPress.org

Make WordPress Core

Opened 11 months ago

Closed 10 months ago

Last modified 10 months ago

#39516 closed defect (bug) (fixed)

Media grid upload errors display below the grid.

Reported by: joemcgill Owned by: afercia
Milestone: 4.7.3 Priority: normal
Severity: normal Version: 4.6
Component: Media Keywords: has-patch fixed-major
Focuses: ui, accessibility, javascript Cc:

Description

In the media grid, whenever an error occurs we display that error in a media sidebar.

https://cldup.com/EGD14_07QK-3000x3000.png

Since [37610] (see #36909) the display order has changed so that the side bar is moved after the uploader and attachments list, which puts these errors after the grid, where they're not visible.

Attachments (7)

media-grid-errors.39516.diff (204.7 KB) - added by codegeass 11 months ago.
re-position the media grid sidebar
media-grid-errors.39516.2.diff (204.7 KB) - added by codegeass 11 months ago.
39516.diff (2.0 KB) - added by MatheusGimenez 11 months ago.
I've fixed the bug using conditionals according to the model.id of the object, if it's library, load the sidebar first, if not, load the uploader and attachments first.
uploader.mp4 (1.0 MB) - added by MatheusGimenez 11 months ago.
Working example video
39516.2.diff (2.0 KB) - added by MatheusGimenez 11 months ago.
Change code standards
39516.3.diff (1.8 KB) - added by afercia 11 months ago.
39516.4.diff (1.8 KB) - added by joemcgill 10 months ago.

Download all attachments as: .zip

Change History (42)

#1 @joemcgill
11 months ago

  • Component changed from General to Media

@codegeass
11 months ago

re-position the media grid sidebar

#2 follow-up: @SergeyBiryukov
11 months ago

Hi @codegeass, thanks for the patch!

There's no need to patch the minified or RTL files, those are generated automatically by the build script.

Also a note, attachments don't trigger notifications, so it's helpful to add a comment after uploading a patch.

#3 in reply to: ↑ 2 @codegeass
11 months ago

Hi @SergeyBiryukov;

So thanks, I was searching how to edit the minified files :)

Okey i will care about that.

Best,

Last edited 11 months ago by codegeass (previous) (diff)

#4 @joemcgill
11 months ago

#39569 was marked as a duplicate.

#5 @afercia
11 months ago

My bad 😬 There are good accessibility reasons to keep the current order in the source, see #36909. I'd recommend to don't revert the change. Looking for the simplest way to solve this bug, there's probably the need to distinguish two cases:

  • in the media modal: the sidebar is still displayed on the right (even if it's after the attachments in the source order) so, visually, everything works as expected (see screenshot below)
  • in the media Library though, the "sidebar" is displayed at the end of the attachments

Error messages are handled by wp.media.view.UploaderStatus and wp.media.view.UploaderStatusError and displayed inside the "sidebar". I guess trying to change the way the UploaderStatus is tied to the sidebar wouldn't be so easy.

As far as I see, in the Library view the "sidebar" is only used to display error messages so maybe one option could be to just create and place the sidebar conditionally:

  • if it's the Library view: create the sidebar before the attachments
  • otherwise: create the sidebar after the attachments

Any thoughts welcome.

Screenshot: error messages in the media modal sidebar:

https://cldup.com/xc4JU-50QX.png

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


11 months ago

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


11 months ago

#8 @afercia
11 months ago

  • Milestone changed from Awaiting Review to 4.7.2
  • Owner set to afercia
  • Status changed from new to assigned

Since this is a regression, I guess it should be fixed as soon as possible. Proposing for the next minor release.

@MatheusGimenez
11 months ago

I've fixed the bug using conditionals according to the model.id of the object, if it's library, load the sidebar first, if not, load the uploader and attachments first.

@MatheusGimenez
11 months ago

Working example video

#9 @MatheusGimenez
11 months ago

  • Keywords has-patch needs-testing added; needs-patch removed

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


11 months ago

#11 follow-up: @afercia
11 months ago

Uploading a few screenshots made without the patch applied. They're meant as reference to check where the error messages should appear in different context. Some testing would be nice, the patch should not change any visual part of the UI, just the placement of elements in the source.

https://cldup.com/Qj-4mCHeHk.png

https://cldup.com/Yrtqc0_6Ak.png

https://cldup.com/OZ0ziIcW26.png

#12 follow-up: @adamsilverstein
11 months ago

@MatheusGimenez Thanks for the patch, it looks good!

A couple of code comments: for the model id check you should use a strict equality check and also switch the order to use Yoda conditions.

#13 in reply to: ↑ 11 @adamsilverstein
11 months ago

Replying to afercia:

Uploading a few screenshots made without the patch applied.

Thanks for the screenshots @afercia that's really helpful.

@MatheusGimenez
11 months ago

Change code standards

#14 in reply to: ↑ 12 @MatheusGimenez
11 months ago

Replying to adamsilverstein:

@MatheusGimenez Thanks for the patch, it looks good!

A couple of code comments: for the model id check you should use a strict equality check and also switch the order to use Yoda conditions.

Hi, i applied the code standards in the last patch.

Thanks for the feedback! :)

@afercia
11 months ago

#15 @afercia
11 months ago

I'd propose a slightly different approach, see refreshed patch.

Looking at the doc description for [options.sidebar=true], it Accepts true, false, and 'errors'. By default it's true and, as far as I see, it is set to 'errors' just once, for the media grid.

In the previous approach instead, the model id can have several values. Basically checking for the 'options.sidebar' value should be more reliable and the same check is already used a few lines below in browser.js.

Also, I'd propose to avoid nested ifs.

Worth mentioning the code could be simplified a bit, for example the multiple checks for 'errors' === this.options.sidebar could be avoided maybe, and the CSS classes could be set in just one go. But I didn't want to touch anything else ;)

#16 @afercia
11 months ago

Worth noting this could change again if the change proposed in #37188 will be approved.

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


11 months ago

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


10 months ago

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


10 months ago

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


10 months ago

@joemcgill
10 months ago

#21 @joemcgill
10 months ago

@afercia The approach in 39516.3.diff looks good, but let's use the state of the view instead of the value of the sidebar property, which is more likely to change. 39516.4.diff is exactly the same but checks for:

'library' === this.options.state

#22 @afercia
10 months ago

  • Keywords needs-refresh added

@joemcgill OK thanks :) I've used the sidebar property value just because it is already used a few lines below, for basically the same purpose (see below) so it seemed consistent to me. Either ways it's fine! I think 39516.4.diff missed to build media-views.js though.

https://cldup.com/6EP_Xc2QiA.png

#23 @joemcgill
10 months ago

  • Keywords commit added; needs-testing needs-refresh removed

@afercia good catch on the missed build.

I missed the use of this.options.sidebar earlier, and on second thought we should probably check that this.options.sidebar is not falsey before trying to render the sidebar anyway, so 39516.3.diff is good.

#24 @afercia
10 months ago

Thanks @joemcgill.
About the error when uploading a new image from the uploader:

https://cldup.com/Yrtqc0_6Ak.png

I see something changed from 4.5 to 4.6. In 4.5, when there is an error, the uploader view automatically switches to the grid view and the error gets displayed in the right column. Couldn't find the related change though. Any thoughts?

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


10 months ago

#26 @afercia
10 months ago

To recap: 39516.3.diff restores the errors displaying to what they looked like in 4.6. Wasn't able to track down the previous change from 4.5 to 4.6 though.

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


10 months ago

#28 @jnylen0
10 months ago

Per Slack discussion above, it looks like this is ready to go for 4.7.3, but the issue in comment:24 needs a separate ticket.

#29 @afercia
10 months ago

I think the issue in comment:24 is not because of changes in the media views JS, but it's because of [37727]. The related ticket #14244 omitted to highlight the (unintended?) change in the UI behaviour. Not a big deal I guess, just a bit difficult to trace down.

#30 @afercia
10 months ago

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

In 40126:

Media: Restore correct upload errors displaying after [37610].

Props codegeass, MatheusGimenez, joemcgill.
Fixes #39516.

#31 @afercia
10 months ago

  • Keywords fixed-major added; commit removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for 4.7.3 consideration.

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


10 months ago

#33 @joemcgill
10 months ago

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

In 40132:

Media: Restore correct upload errors displaying after [37610].

Props codegeass, MatheusGimenez, joemcgill.
Merges [40126] to the 4.7 branch.
Fixes #39516.

#34 @SergeyBiryukov
10 months ago

#39979 was marked as a duplicate.

#35 @joemcgill
10 months ago

#39989 was marked as a duplicate.

Note: See TracTickets for help on using tickets.