WordPress.org

Make WordPress Core

Opened 4 months ago

Closed 6 hours ago

Last modified 6 hours ago

#53765 closed defect (bug) (fixed)

Media Library shows only the selected image

Reported by: benitolopez Owned by: hellofromTonya
Milestone: 5.9 Priority: normal
Severity: normal Version: 5.8
Component: Media Keywords: has-patch
Focuses: javascript Cc:

Description

I don't know if this is a bug or if it is by design. But since WordPress 5.8, when you open the media libray that has a selection, only the selected image(s) is shown and other medias are not there. To show again the other images included in the library you need to filter the medias.

Seems that it happens only when using the classic editor. To reproduce the issue, you can set a featured image to a product of WooCommerce (for example). Or to any post type that is using the classic editor.

Previous versions of WordPress don't have this problem.

Attachments (7)

53765-reproduced-issue.gif (4.1 MB) - added by hellofromTonya 4 months ago.
Testing: reproduced when upgrading from 5.7.2 to 5.8.0
53765-reproduced-issue2.gif (6.8 MB) - added by hellofromTonya 4 months ago.
Testing: additional reproduced testing
53765.diff (1.3 KB) - added by joedolson 3 weeks ago.
Converts notes by szaqal21 into patch.
53765.2.diff (909 bytes) - added by joedolson 2 weeks ago.
New patch
screen-capture.webm (1.3 MB) - added by szaqal21 2 weeks ago.
Test scenario
screen-capture (1).webm (685.8 KB) - added by szaqal21 2 weeks ago.
posts_per_page = 40
no_load_more.png (90.5 KB) - added by szaqal21 13 days ago.
Noticed something else, lowering posts_per_page to 40 caused Load more button dissapear.

Change History (55)

@hellofromTonya
4 months ago

Testing: reproduced when upgrading from 5.7.2 to 5.8.0

#1 @hellofromTonya
4 months ago

Testing Report

Testing Environment:

  • OS: macOS Big Sur 11.4
  • Localhost: Local
  • WordPress: 5.7.2 and the 5.8.0
  • Activated plugins: Classic Editor plugin
  • Theme: Twenty Twenty-One
  • Browser: Chrome Version 91.0.4472.114, Firefox 89.0.2 (64-bit), Safari 14.1.1

Testing steps to reproduce:

  • Install and active the Classic Editor plugin
  • Switch to 5.7.2 (Local testing: use the Core Rollback plugin.)
  • Add multiple images to the Media Library (at least 2)
  • Create a new post
  • Set the Featured Image
  • Save the post
  • Click on the image in the Featured image field to reopen the modal -> notice all of the items in the media library including the selected one are displayed
  • Upgrade to 5.8.0
  • Go back to that post
  • Click on the featured image again to reopen the modal -> notice only the selected image is displayed

Results:

  • Reproduced issue when using the Classic Editor as shown in 53765-reproduced-issue.gif in each browser ❌
  • No errors in browser console ✅
  • error.log is empty ✅
  • Confirmed the issue does not happen with block editor, when Classic Editor plugin is not activated ✅
Last edited 4 months ago by hellofromTonya (previous) (diff)

#2 @hellofromTonya
4 months ago

  • Milestone changed from Awaiting Review to 5.8.1

Hello @benitolopez,

Welcome to WordPress Core Trac! Thank you for reporting this.

I was able to reproduce the issue. It does look like a regression. Moving it into 5.8.1 for further investigation.

@hellofromTonya
4 months ago

Testing: additional reproduced testing

#3 @hellofromTonya
4 months ago

  • Keywords needs-patch added

I was curious what happens when deselecting or removing the featured image. Are users able to get access to the rest of the media library? 53765-reproduced-issue2.gif shows the results of the testing.

Yes, users can regain access but only when following these steps:

  • Remove the featured image
  • Save or update
  • Select to add a feature image to reopen the media library modal

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


4 months ago

#5 @joedolson
4 months ago

Testing report:

Testing Environment:

OS: Windows 10
Localhost: no
WordPress: 5.7.2 and then 5.8.0 nightly 7/27/2021
Activated plugins: Core Rollback, WordPress Reset, Beta Tester
Theme: Twenty Twenty-One
Browser: Chrome 92.0.4515.107 (Official Build), Firefox 90.0.2

Could not reproduce issue in either of these environments.

Note: not a valid test, as it was performed using the block editor.

Last edited 4 months ago by joedolson (previous) (diff)

#6 @benitolopez
4 months ago

I just tried it with Windows to make sure it wasn't related to a different OS. I'm still having the issue: Windows 10, WordPress 5.8 and Chrome (92.0.4515.107).

Same with Big Sur 11.5.1, WordPress 5.8 and Chrome (92.0.4515.107).

Again, this happens only when using the classic editor. If the post type uses the Block Editor, the featured image works correctly.

#7 follow-up: @joedolson
4 months ago

Ah; I followed the testing instructions, but didn't notice that the original ticket specified classic editor.

@hellofromTonya Can you edit the testing instructions to specify that the Classic Editor needs to be activated?

#8 in reply to: ↑ 7 @hellofromTonya
4 months ago

Replying to joedolson:

Ah; I followed the testing instructions, but didn't notice that the original ticket specified classic editor.

@hellofromTonya Can you edit the testing instructions to specify that the Classic Editor needs to be activated?

Thanks for the ping @joedolson! Testing report and steps are updated. Sorry for missing the information in the report 🤦‍♀️.

Retested with and without the Classic Editor plugin activated. Confirmed it only happens in the classic mode and not with the block editor. Testing report updated.

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


4 months ago

#10 @joedolson
4 months ago

  • Owner set to joedolson
  • Status changed from new to accepted

#11 @peterwilsoncc
3 months ago

I did a bisect today, it appears to have been introduced in [50829].

While testing today, it happens in:

  • classic editor featured image (per image above)
  • classic editor inline image (click edit then replace buttons)
  • block editor classic block inline image (click edit then replace buttons)

With infinite scrolling enabled via add_filter( 'media_library_infinite_scrolling', '__return_true' ); the issue doesn't happen so I suspect the get-attachment action is confusing the logic and the load more button isn't displaying as it doesn't have the data.totalAttachments property.

I think when replacing an image there ought to be two initial ajax requests, one to get the selected attachment and one to get the first page of attachments via the query-attachments action.

#12 @circlecube
3 months ago

To be considered for the next release (5.8.1) this will need a patch and testing before RC1 which is scheduled for Wednesday, Sept 1.

#13 @desrosj
3 months ago

  • Milestone changed from 5.8.1 to 5.8.2

With 5.8.1 RC due out in less than 24 hours, I'm going to punt this one to 5.8.2.

#14 @danielbachhuber
8 weeks ago

I'm experiencing a similar issue in WordPress 5.8.1 (and all of the way down to WordPress 5.3?) It manifests in a couple different ways.

  • If you assign a featured image, close the modal, and then immediately re-open the featured image modal, no image is selected.
  • If you assign a featured image, close the modal, save the post, refresh the page, assign a new featured image, close the modal, and then immediately re-open the modal, the prior featured image will appear as selected.

Here's a Loom video documenting the behavior: https://www.loom.com/share/38df2347bd5648d2ac13c2904f55b301

My two problems happen with the Classic Editor plugin activated and deactivated.

#15 @PieWP
5 weeks ago

@joedolson not sure wether it helps but prior to founding out it was a core bug I encountered the following in debugging.

wp-includes/media-views.js:2410, this.collection.length returns 1 therefore the this.collection.more() ( line 2412 ) is no longer called and no additional media is being loaded.

Happy to see the workaround by @peterwilsoncc is working in the meantime.

#16 @desrosj
4 weeks ago

  • Milestone changed from 5.8.2 to Future Release

This one still needs a patch, and 5.8.2 RC is due out tomorrow. Moving to Future Release.

#17 @sabernhardt
3 weeks ago

#54403 was marked as a duplicate.

#18 @sabernhardt
3 weeks ago

  • Focuses javascript added

@szaqal21 forces loading more images on initialization, as a workaround (#54403). Could that help here?

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


3 weeks ago

#20 @joedolson
3 weeks ago

  • Milestone changed from Future Release to 5.9

#21 @szaqal21
3 weeks ago

I've made a lot of debug work and results are:

Gutenberg - lines from 487 to 493 of /wp-includes/js/dist/media-utils.js show what exactly is going on when frame opens, selection is being setup after attachments.more() is done

attachments.more().done(function () {
      var _attachments$models;

      if (isGallery && attachments !== null && attachments !== void 0 && (_attachments$models = attachments.models) !== null && _attachments$models !== void 0 && _attachments$models.length) {
        selection.add(attachments.models);
      }
    });

Classic editor - there is no call for loading more attachments on frame activation in /wp-includes/js/media-views.js lines 4047 - 4052

activate: function() {
		this.updateSelection();
		this.frame.on( 'open', this.updateSelection, this );

		Library.prototype.activate.apply( this, arguments );
	},

only updateSelection() (called twice, don't know why) which fetches only featured image (if set) so library has only one attachment and more() isn't called as @PieWP mentioned.

I've made those code changes:

includes/js/media-views.js lines 4047 - 4052 - call updateSelection() only on frame open event

activate: function() {
		this.frame.on( 'open', this.updateSelection, this );

		Library.prototype.activate.apply( this, arguments );
	},

added call of more() to updateSelection() in includes/js/media-views.js lines 4066 - 4077

updateSelection: function() {
		var selection = this.get('selection'),
			library = this.get('library'),
			id = wp.media.view.settings.post.featuredImageId,
			attachment;

		if ( '' !== id && -1 !== id ) {
			attachment = Attachment.get( id );
			attachment.fetch();
		}

		selection.reset( attachment ? [ attachment ] : [] );

		library.more();
	}

and includes/js/media-views.js lines 7470 - 7475

updateSelection: function() {
		var selection = this.get('selection'),
			library = this.get('library'),
			attachment = this.image.attachment;

		selection.reset( attachment ? [ attachment ] : [] );
		
		library.more();
	}

didn't noticed any unexpected behaviour in Gutenberg and Classic editor and everything works fine now. Perhaps it would be more elegant and intuitive to move more() call to activate() of ReplaceImage and FeatureImage views.

#22 @joedolson
3 weeks ago

Thanks for this work @szaqal21! This will be very helpful in moving this towards resolution.

@joedolson
3 weeks ago

Converts notes by szaqal21 into patch.

#23 @joedolson
3 weeks ago

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

These changes appear to work well in all relevant contexts. I added wrappers to verify library.hasMore() before calling .more(), but this patch is otherwise unchanged.

#24 @joedolson
2 weeks ago

  • Keywords commit added; needs-testing removed

#25 @joedolson
2 weeks ago

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

In 52167:

Media: Featured image modal loads only selected image.

Fix bug introduced in [50829] that caused media modal to only load the selected image. Executes .more() when loading the modal to ensure that the media collection is available.

Props benitolopez, hellofromTonya, peterwilsoncc, danielbachhuber, PieWP, sabernhardt, szaqal21.
Fixes #53765.

#26 @szaqal21
2 weeks ago

There's an update needed, after some more tests I've found some optimizations and one bug in my approach. wp.media.view.Attachments view calls scroll() on ready() if infinite scroll is enabled:

ready: function() {
		if ( this.options.infiniteScrolling ) {
			this.scroll();
		}
	},

so updateSelection() of wp.media.controller.ReplaceImage and wp.media.controller.FeaturedImage should be updated to conditionally call more() (only when infinite scroll if disabled):

FeaturedImage

updateSelection: function() {
		var selection = this.get('selection'),
			library = this.get('library'),
			id = wp.media.view.settings.post.featuredImageId,
			infinite_scrolling = wp.media.view.settings.infiniteScrolling,
			attachment;

		if ( '' !== id && -1 !== id ) {
			attachment = Attachment.get( id );
			attachment.fetch();
		}

		selection.reset( attachment ? [ attachment ] : [] );

		if ( ! infinite_scrolling && library.hasMore() ) {
			library.more();
		}
	}

and ReplaceImage

updateSelection: function() {
		var selection = this.get('selection'),
			library = this.get('library'),
			infinite_scrolling = wp.media.view.settings.infiniteScrolling,
			attachment = this.image.attachment;

		selection.reset( attachment ? [ attachment ] : [] );

		if ( ! infinite_scrolling && library.getTotalAttachments() == 0 && library.hasMore() ) {
			library.more();
		}
	}

there's library.getTotalAttachments() == 0 check in updateSelection() of ReplaceImage controller because activate() is being called every time user hits Replace button in edit image view if user would hit Replace button then hit Back and again hit Replace he would get another portion of attachments loaded.

#27 @hellofromTonya
2 weeks ago

  • Keywords commit removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for #comment:26.

#28 @joedolson
2 weeks ago

@szagal21 Can you provide testing steps for replicating this issue? I'm not figuring out how to replicate the issue you're fixing. The changes seem totally reasonable, and they make sense, but without being able to confirm the bug and the fix, I have no way to demonstrate that it works.

I can confirm that it doesn't break anything new that I can see.

@joedolson
2 weeks ago

New patch

#29 @joedolson
2 weeks ago

  • Keywords reporter-feedback needs-testing added

New patch adds additional changes from @szaqal21

I haven't been able to identify a way to test these yet.

@szaqal21
2 weeks ago

Test scenario

#30 @joedolson
2 weeks ago

Thanks, @szaqal21!

Is this an accurate description of the issue?

1) Test prerequisites: a media library with more than 80 items.
2) Add an image to a post in the classic visual editor.
3) Click through to edit the image.
4) Choose the 'Replace' option. Scroll down and note 80 images with a load more button.
5) Exit the replace dialog.
6) Return to the Replace dialog. Scroll down and note all images loaded with no load more.

#31 @szaqal21
2 weeks ago

Yes exactly. If there would be more attachments (lets say 300) they would be loaded more and more (80 -> 160 -> 240 -> ...). This bug doesn't exist when posts_per_page is lower than 80, example attached with posts_per_page = 40

add_filter( 'ajax_query_attachments_args', 'change_posts_per_page', 10, 1 );

function change_posts_per_page($args)
{
	$args['posts_per_page'] = 40;
	
	return $args;
}

@szaqal21
2 weeks ago

posts_per_page = 40

@szaqal21
13 days ago

Noticed something else, lowering posts_per_page to 40 caused Load more button dissapear.

#32 @joedolson
13 days ago

Anything related to changing posts_per_page would be useful information on #53787; currently that's not expected to work correctly, because the media library uses a hard-coded value.

#33 @szaqal21
13 days ago

@joedolson thanks for clearing that out, lets forget about posts_per_page for now and focus on second fix it should be all fine now. Checking infitineScrolling prevents more() call which is already called by ready() method of Attachments view if infiniteScrolling is enabled:

ready: function() {
		if ( this.options.infiniteScrolling ) {
			this.scroll();
		}
	},

and checking if library.getTotalAttachments() equals 0 (when Replace image view is activated) ensures that more() is called only once when library is empty.

This ticket was mentioned in Slack in #core-test by boniu91. View the logs.


5 days ago

#35 @dariak
4 days ago

Test Report

Env

  • MacOS Big Sur
  • Chrome 96.0.4664.55
  • Localhost: Local
  • WordPress: 5.8.2 and 5.9-alpha-52250
  • Activated plugins: Classic Editor plugin and WordPress Beta Tester
  • Theme: Twenty Twenty One

Steps to test

  1. Use WordPress 5.8.2: add multiple images to the Media Library (128 in my case)
  2. Create a new post using Classic editor.
  3. Set the Featured Image.
  4. Save the post.
  5. Click on the image in the Featured image field to reopen the modal -> only selected image is shown.
  6. Upgrade to WordPress 5.9-alpha-52250.
  7. Re-open the same post from the step 2 in Classic editor.
  8. Click on the image in the Featured image field to reopen the modal -> first 80 items in the media library including the selected one are displayed.
  9. Create one more post using Classic editor.
  10. Set the Featured Image.
  11. Save the post.
  12. Click on the image in the Featured image field to reopen the modal -> first 80 items in the media library including the selected one are displayed.

So initial issue is fixed.

The "Load more" case currently works as follows:

  1. Create a new post using Classic editor.
  2. Add image to a post from Media Library (the Media Library has 128 images).
  3. Edit image.
  4. Click Replace -> first 80 images are shown, scroll down -> Load more button is shown.
  5. Click Back.
  6. Click Replace.

Indeed all images are shown without Load more button.

This ticket was mentioned in Slack in #core-test by dariak. View the logs.


4 days ago

#37 @szaqal21
16 hours ago

@joedolson any commit comming for latest patch?

This ticket was mentioned in Slack in #core-test by boniu91. View the logs.


12 hours ago

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


11 hours ago

#40 @hellofromTonya
11 hours ago

  • Keywords commit added; reporter-feedback needs-testing removed

Marking 53765.2.diff ready for commit.

#41 @hellofromTonya
8 hours ago

  • Owner changed from joedolson to hellofromTonya
  • Status changed from reopened to reviewing

Per @joedolson in Slack earlier today:

No concerns I'm aware of; but I can't do it myself before beta 1.

Self-assigning for commit to include it in 5.9 Beta 1.

#42 @hellofromTonya
7 hours ago

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

In 52287:

Media: Featured image modal loads only selected image when infinite scroll is disabled.

Follow-up to [52167], which partially fixed a bug introduced in [50829] that caused media modal to only load the selected image.

This commit adds additional checks to ensure infinite scroll is disabled.

Follow-up to [50829], [52167].

Props dariak, joedolson, szaqal21.
Fixes #53765.

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


7 hours ago

This ticket was mentioned in PR #1981 on WordPress/wordpress-develop by hellofromtonya.


7 hours ago

Follow-up to 52287 which brings the global infinite scroll setting into the functions and changes from snake_case to camelCase for consistency and JS coding standards.

Trac ticket: https://core.trac.wordpress.org/ticket/53765

#45 @hellofromTonya
7 hours ago

  • Keywords commit removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening to address the undefined infinite_scrolling variable. PR 1981 brings the global setting into the respective functions and changes the variable name to camelCase.

#46 @hellofromTonya
6 hours ago

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

In 52288:

Media: Use infiniteScrolling global setting in js/media/controllers/featured-image.js and js/media/controllers/replace-image.js.

Follow-up to [52287] which added an undefined variable. The variable should have been the global wp.media.view.settings.infiniteScrolling. This commit brings that global setting into each of the functions and renames the variable using camelCase to comply with JS coding standards.

Follow-up to [52287].

Props SergeyBiryukov.
Fixes #53765.

#47 @hellofromTonya
6 hours ago

@szaqal21 can you test trunk to verify the changeset resolves the issue you noted? If no, please reopen this ticket.

Note: See TracTickets for help on using tickets.