WordPress.org

Make WordPress Core

#42968 closed defect (bug) (fixed)

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 fixed-major
Focuses: ui, javascript Cc:

Description

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 15 months ago.
Step 1 of upload
step-2.png (58.7 KB) - added by lancewillett 15 months ago.
Step 2 of upload
upload-grid-bug.gif (1.1 MB) - added by lancewillett 15 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 15 months ago.
42968.diff (894 bytes) - added by adamsilverstein 14 months ago.
42968.2.diff (3.0 KB) - added by adamsilverstein 14 months ago.
42968.3.diff (3.2 KB) - added by adamsilverstein 14 months ago.

Change History (35)

@lancewillett
15 months ago

Step 1 of upload

@lancewillett
15 months ago

Step 2 of upload

@lancewillett
15 months ago

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

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

cc: @joemcgill, @westonruter

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


14 months ago

#6 @SergeyBiryukov
14 months ago

  • Milestone changed from 4.9.3 to 4.9.4

#7 @adamsilverstein
14 months 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
14 months 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
14 months 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.


14 months ago

#11 @dd32
14 months ago

  • Milestone changed from 4.9.4 to 4.9.5

Bumping, 4.9.4 has been released.

#12 @adamsilverstein
14 months 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
14 months ago

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

#14 @lancewillett
14 months ago

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

#15 @adamsilverstein
14 months ago

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

#16 @joemcgill
14 months ago

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

#17 @adamsilverstein
14 months ago

  • Keywords commit added

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

#18 @adamsilverstein
13 months ago

#43337 was marked as a duplicate.

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


13 months ago

#20 @adamsilverstein
13 months ago

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

In 42739:

Media: grid view - correct placeholder positioning during uploads.

Preventing contextually created attachments from being added to the grid collection in Attachments.validator. Remove the previous filtering introduced in [41937] which caused the placement issue.

Props lancewillett, Junaidkbr, designsimply, joemcgill.
Fixes #42968.

#21 @adamsilverstein
13 months ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for consideration for the 4.9.5 branch.

#22 @adamsilverstein
13 months ago

Amended in [42740] removing a trailing comma to address a jshint error.

#23 @SergeyBiryukov
13 months ago

  • Keywords fixed-major added

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


13 months ago

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


13 months ago

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


12 months ago

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


12 months ago

#28 @ocean90
12 months ago

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

In 42848:

Media: Grid view - correct placeholder positioning during uploads.

Preventing contextually created attachments from being added to the grid collection in Attachments.validator. Remove the previous filtering introduced in [41937] which caused the placement issue.

Merge of [42739-42740] to the 4.9 branch.

Props lancewillett, Junaidkbr, designsimply, joemcgill.
Fixes #42968.

Note: See TracTickets for help on using tickets.