Make WordPress Core

Opened 2 months ago

Last modified 24 hours ago

#42968 accepted defect (bug)

Media: Grid View: new upload, file is in the wrong position in the grid until after upload is complete

Reported by: lancewillett Owned by: adamsilverstein
Milestone: 4.9.5 Priority: normal
Severity: normal Version: 4.9.1
Component: Media Keywords: has-patch commit
Focuses: ui, javascript Cc:


Steps to reproduce

  1. Log in to your WordPress site.
  2. Go to wp-admin/upload.php (or click Media link in the wp-admin menu)
  3. Take a look at the grid view of images and files, if you have any there.
  4. Now, add a new file.
  5. Pay attention to where the new file goes in the grid during the upload. This is more obvious on a slow host with a huge file like a 1GB video.

What I expected
I expected the new file to appear at the top left position in the grid during upload and processsing.

What happened instead
The file appears inside the grid, not in the position I expected. The result is — as an author — that I feel like the upload didn't work because my eyes are looking for the new placeholder at the top left of the grid.

Note: the upload does work. And it does appear at the position I expect — but only after upload is complete and the file is saved.

Browser / OS version
Tested on OS X 10.12.6 with Safari, Chrome, and Firefox (all latest stable).

Screenshot / Video
https://cloudup.com/cHuVsO0OQd1 -- animated GIF

Attaching 2 screenshots: step 1, you'll see the new image file is in-between the 2 existing files. After upload, it moves to the left as expected.

Context / Source
Real life usage on WordPress.com; tracked it back to a vanilla WP install, version 5.0-alpha-42125-src. Two plugins: Debug Bar and Debug Bar console. Default theme.

Attachments (7)

step-1.png (41.4 KB) - added by lancewillett 2 months ago.
Step 1 of upload
step-2.png (58.7 KB) - added by lancewillett 2 months ago.
Step 2 of upload
upload-grid-bug.gif (1.1 MB) - added by lancewillett 2 months ago.
Animated GIF to show the bug as I upload a large video file
Screen Shot 2017-12-22 at Fri Dec 22 1.04.17 PM.png (2.9 MB) - added by designsimply 8 weeks ago.
42968.diff (894 bytes) - added by adamsilverstein 3 weeks ago.
42968.2.diff (3.0 KB) - added by adamsilverstein 10 days ago.
42968.3.diff (3.2 KB) - added by adamsilverstein 10 days ago.

Change History (25)

2 months ago

Step 1 of upload

2 months ago

Step 2 of upload

2 months ago

Animated GIF to show the bug as I upload a large video file

#1 @designsimply
8 weeks ago

Tested and confirmed that pending uploads in the Media Library appear in the 2nd to last position during upload instead of the top left position as expected. I also found that if you upload multiple images, they appear as duplicates until the entire set has completed uploading.

Video: 1m58s

Seen at /wp-admin/upload.php using WordPress 4.9.1, Safari 11.0.1, macOS 10.13.2.

#2 @adamsilverstein
8 weeks ago

  • Focuses javascript added
  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 4.9.2
  • Owner set to adamsilverstein
  • Status changed from new to accepted
  • Version changed from trunk to 4.9.1

@lancewillett Thanks for the excellent bug report.

Using git bisect, I tracked this down: it was introduced in r41937 (on github)

#3 @adamsilverstein
7 weeks ago

cc: @joemcgill, @westonruter

#4 @dd32
5 weeks ago

  • Milestone changed from 4.9.2 to 4.9.3

Bumping to 4.9.3 due to 4.9.2s release

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

4 weeks ago

#6 @SergeyBiryukov
3 weeks ago

  • Milestone changed from 4.9.3 to 4.9.4

#7 @adamsilverstein
3 weeks ago

In 42968.diff:

  • remove call to this._filterContext when a model is added.

This corrects the reported issue in my testing. I don't see any side effects, however I'm not certain what this line of code is intended to do - appreciate any feedback/help from @joemcgill or @westonruter.

@lancewillett can you confirm this patch fixes the issue for you?

#8 @lancewillett
3 weeks ago

Yes, I can confirm the patch fixes the issue (running latest trunk). With the patch applied, the file upload is correctly positioned as expected.

#9 @Junaidkbr
3 weeks ago

  • Keywords has-patch added; needs-patch removed

I can confirm too. Latest trunk, patch applied. Works as expected, new processing upload is placed as first item.

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

2 weeks ago

#11 @dd32
11 days ago

  • Milestone changed from 4.9.4 to 4.9.5

Bumping, 4.9.4 has been released.

#12 @adamsilverstein
10 days ago

I spent some time reviewing #21819 where this issue was introduced. The purpose of the filtering was to "Filter out contextually created attachments (e.g. headers, logos, etc.)". My previous proposed patch broke that.

In 42968.3.diff I take a different approach, preventing these items from ever being added to the collection in Attachments.validator which is called by Attachments.validate. This prevents the resort caused by the previous approach and the misplaced imaged we were seeing.

@Junaidkbr @lancewillett can you give this new patch another test?

#13 @Junaidkbr
10 days ago

Tested on a clean build. The new patch works and resolves the initial issue.

#14 @lancewillett
10 days ago

Confirmed 42968.3.diff also fixes the issue, with latest trunk. Looks good.

#15 @adamsilverstein
8 days ago

@joemcgill could you take a look at this and confirm this still fixes the original issue?

#16 @joemcgill
8 days ago

@adamsilverstein this approach seems much cleaner than my previous attempt and I can confirm that it works as intended. Nice one!

#17 @adamsilverstein
8 days ago

  • Keywords commit added

Great, thanks for testing! Let's get this in so it can get some wider testing.

#18 @adamsilverstein
24 hours ago

#43337 was marked as a duplicate.

Note: See TracTickets for help on using tickets.