Make WordPress Core

Opened 5 years ago

Closed 4 years ago

Last modified 4 years ago

#50105 closed defect (bug) (fixed)

Remove infinite scrolling behavior from the Media grid

Reported by: afercia's profile afercia Owned by: joedolson's profile joedolson
Milestone: 5.8 Priority: normal
Severity: normal Version:
Component: Media Keywords: has-screenshots needs-testing needs-dev-note
Focuses: ui, accessibility, javascript Cc:

Description

Splitting this out from #40330.

This ticket aims to discuss technical implementation details for the removal of infinite scrolling from the Media grid. For general discussion on the infinite scrolling behavior, please head over to the main ticket #40330.

Attachments (23)

50105.diff (16.6 KB) - added by afercia 5 years ago.
50105.jpg (90.8 KB) - added by afercia 5 years ago.
50105.2.diff (18.6 KB) - added by afercia 5 years ago.
50105 buttons spinner.png (152.2 KB) - added by afercia 5 years ago.
50105.3.diff (18.6 KB) - added by joedolson 4 years ago.
Refreshed version of 50105.2.diff
50105.5.diff (19.8 KB) - added by adamsilverstein 4 years ago.
Screen Recording 2021-02-05 at 03.43.53 PM.mp4 (2.8 MB) - added by adamsilverstein 4 years ago.
50105.6.diff (21.7 KB) - added by joedolson 4 years ago.
Updated patch. Adds focus handling for last collection, design update.
50105.7.diff (18.9 KB) - added by joedolson 4 years ago.
Refreshed patch
media-load-more-initial.png (343.5 KB) - added by joedolson 4 years ago.
Media load more initial view
media-load-more-post-with-highlight.png (1.6 MB) - added by joedolson 4 years ago.
Media load more after loading more; highlight showing first added graphic.
50105.8.diff (20.2 KB) - added by joedolson 4 years ago.
Updated patch - language changes, highlight new items.
50105-new-language.png (323.6 KB) - added by joedolson 4 years ago.
Screenshot showing new language
Screen Shot 2021-05-02 at 16.28.52.png (15.1 KB) - added by francina 4 years ago.
Total conter doesn't update
50105.9.diff (20.2 KB) - added by joedolson 4 years ago.
Updated patch - language change, resolve jshint issues.
50105.10.diff (5.1 KB) - added by adamsilverstein 4 years ago.
50105.11.diff (5.2 KB) - added by adamsilverstein 4 years ago.
50105.12.diff (5.1 KB) - added by adamsilverstein 4 years ago.
50105.13.diff (5.1 KB) - added by joedolson 4 years ago.
Handles returned data that isn't an XHR object & creates load more view when switching tabs in modal
50105.14.diff (469 bytes) - added by audrasjb 4 years ago.
rwd_media.PNG (130.0 KB) - added by Boniu91 4 years ago.
media_collection.PNG (362.9 KB) - added by Boniu91 4 years ago.
weird_collection.PNG (545.7 KB) - added by Boniu91 4 years ago.

Change History (160)

@afercia
5 years ago

@afercia
5 years ago

#1 @afercia
5 years ago

  • Keywords has-patch has-screenshots added

50105.diff is a proof of concept meant for initial testing.

Still to do:

  • update the number of total attachments when adding / removing attachments
  • more testing

It would be great to run extended testing on different installations, with a varying mount of attachments stored in the media library. I'd suggest to focus on:

  • general behavior
  • whether the default "per page" of 40 attachments should be increased
  • filtering by type and date, keeping in mind there are existing bugs when filtering see for example #50025.

Suggestions for testing:

  • styling of the button / spinner / etc. is temporary, please ignore it for now
  • throttle the network via your browser's dev tools: this helps in observing the UI behavior e.g. the spinner visibility
  • test without and with media trash enabled (by setting define( 'MEDIA_TRASH', true ); in your wp-config.php file)
  • test the filter add_filter( 'media_library_infinite_scrolling', '__return_true' ); to restore the infinite scrolling behavior
  • test the Media Library
  • test the Media modal with Classic Editor (Add Media, featured image)
  • test the Media modal with Gutenberg (image, gallery, featured image...)
  • optionally: test the Media modal with plugins that implement custom frames to upload / select attachments
  • inspect the queries array of cached collections

Accessibility:
For now, clicking the "load more" sets focus on the first added attachment. I think it would be best to explore other options, as moving focus unexpectedly should preferably be avoided.

Questions:

1
The patch changes the data returned by the WP_Query used to retrieve attachments. Previously, the response only held the array of attachments. Now, it holds 2 items:

  • the array of attachments
  • the number of total attachments, that is: found_posts

Later, the response is parsed so that it passes the attachments array to the views and sets a new property for the number of total attachments. Basically, the response and the cached queries use a different format. An interesting question in this regard is: should the old format be kept when the media_library_infinite_scrolling filter restore infinite scrolling? The media grid works regardless but I guess this should be explored for potential backwards compatibility issues?

2
One more interesting question for anyone familiar with the history of the media views is what are the various "compat" methods meant for? This part is a bit obscure to me. See things like:

  • Attachment.saveCompat
  • the AJAX call save-attachment-compat and related wp_ajax_save_attachment_compat
  • the attachment_fields_to_edit things (I think this was used for the old upload method?)

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


5 years ago

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


5 years ago

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


5 years ago

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


5 years ago

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


5 years ago

#8 @afercia
5 years ago

  • Owner set to afercia
  • Status changed from new to assigned

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


5 years ago

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


5 years ago

#11 @afercia
5 years ago

Related: #50410 which is a blocker for this ticket, at least for the part concerning the text with the media items count / total.

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


5 years ago

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


5 years ago

@afercia
5 years ago

#14 @afercia
5 years ago

50105.2.diff refreshes the patch and:

  • adds a "Jump to first new item" button
  • the button is only displayed on focus for keyboard users
  • uses more wp.i18n

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


5 years ago

#16 @audrasjb
5 years ago

  • Keywords needs-dev-note needs-testing added

Looks really good to me!
Adding needs-dev-note and needs-testing keywords.

#17 @afercia
5 years ago

See screenshot above. To clarify the behavior:

  • the "Load more" button is only visible when there are more items to load
  • the spinner is only visible when items are being fetched
  • the "Jump to first new item" button is only visible on focus for keyboard users, after new items have been added

Still to do [Edited]:

  1. avoid focus loss when the "Load more" button disappears because there are no new items to load
  2. scrolling issues in the media modal
  3. update the number of total attachments when adding / removing attachments: an alternative option could be hiding the counts after attachments add/removal
  4. consider whether the default "per page" of 40 attachments should be increased
  5. what to do with other long-standing issues made more apparent by this patch, for example: when filtering attachments, see #50025 (Filters don't show new uploads)
Last edited 5 years ago by afercia (previous) (diff)

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


5 years ago

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


5 years ago

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


5 years ago

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


5 years ago

#22 @afercia
5 years ago

Here's a list of existing tickets that are blockers for this proposed change:
#50410, #50025, #46127, and #47215.

Note: these are all existing buggy behaviors in the Media views that exist since forever.

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


5 years ago

#24 @davidbaumwald
5 years ago

@afercia The dependent tickets are still open. Is this still a viable option in 5.5 at this point?

#25 @afercia
5 years ago

  • Keywords needs-dev-note removed

@davidbaumwald I don't think this ticket can make it for 5.5. I didn't punt it yet because I wanted the component maintainers to be aware of this ticket.

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


5 years ago

#27 @desrosj
5 years ago

  • Milestone changed from 5.5 to 5.6

Since this has been discussed at a media in depth, it's safe to say the Media team is aware of it and just has not had time to circle back. Going to punt due to lack of movement so far.

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


4 years ago

#29 @afercia
4 years ago

  • Milestone changed from 5.6 to Future Release

This ticket was discussed during today's accessibility bug-scrub: given the Beta 1 period is approaching fast and considering this ticket would need greater visibility and a good amount of time for in depth testing, the teams agreed to punt it to future release.

#30 @afercia
4 years ago

  • Owner afercia deleted

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


4 years ago

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


4 years ago

#33 @antpb
4 years ago

  • Milestone changed from Future Release to 5.7

As mentioned here: https://core.trac.wordpress.org/ticket/50025#comment:13

#50025 is a blocker for this so we should prioritize it before this ticket, but both seem well in scope of 5.7 as good focuses. Moving to the milestone to maintain visibility. :)

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


4 years ago

#35 @ryokuhi
4 years ago

  • Owner set to joedolson

#36 @joedolson
4 years ago

  • Keywords needs-refresh added

@joedolson
4 years ago

Refreshed version of 50105.2.diff

#37 @joedolson
4 years ago

  • Keywords needs-refresh removed

Patch refreshed. This is unchanged from the last stages that Andrea was working on; I think that there are a couple things that could be improved, and the 'todo' items from https://core.trac.wordpress.org/ticket/50105#comment:17 are still to be done.

Noting: 'Jump to first new item' is only in tab sequence when moving forward, and not when moving backwards from the media item sidebar. It's only meaningful right after new items have been loaded, however, so it's a legitimate question whether it should even continue to be present once it's used.

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


4 years ago

#39 @hellofromTonya
4 years ago

  • Keywords needs-testing-info added

In preparation for Testing Scrub scheduling, please provide more information for the testers:

  • What are the steps to reproduce the problem?
  • Are there any testing dependencies such as a plugin or script?
  • What is the expected behavior after applying the patch?

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


4 years ago

#41 @ricjcs
4 years ago

After the feature was implemented, it would be good to be able to turn it on or off.
There are users who prefer the infinite scrolling.

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


4 years ago

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


4 years ago

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


4 years ago

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


4 years ago

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


4 years ago

#47 @adamsilverstein
4 years ago

50105.5.diff refresh against trunk.

#49 @adamsilverstein
4 years ago

@ricjcs

After the feature was implemented, it would be good to be able to turn it on or off.
There are users who prefer the infinite scrolling.

This is achievable with the patch using a filter (media_library_infinite_scrolling); one code line or a mini plugin can be used to re-enable this.

#51 @adamsilverstein
4 years ago

This is close; the ticket still needs a little work to be completely ready though, see https://core.trac.wordpress.org/ticket/50105#comment:37

Since we are in 5.7 beta, we need to get this in soon or punt to 5.8.

#52 @adamsilverstein
4 years ago

avoid focus loss when the "Load more" button disappears because there are no new items to load

What is the desired behavior? should the focus be set to the last media item or to the "Jump to first new item" button?

scrolling issues in the media modal

Not certain what these are, any more details?

update the number of total attachments when adding / removing attachments: an alternative option could be hiding the counts after attachments add/removal

Ideally these counts should stay synced with the collection total.

consider whether the default "per page" of 40 attachments should be increased

40 feels ok for now. We can make sure this is filterable. It would be nice if this were user controllable like posts per page, or terms per page are from the screen options tab. We would need to integrate this into the modal itself though since it operates in numerous contexts.

@joedolson
4 years ago

Updated patch. Adds focus handling for last collection, design update.

#53 @joedolson
4 years ago

Updated patch:

1) Changes button style from hero to normal size
2) Places buttons on a single line
3) Makes 'jump to first new item' visible by default.

This fixes a scrolling issue where navigating to the 'Jump' link could cause an unnecessary change in the view position, and makes the 'Jump' link available to sighted mouse users. Since the first added item is not obvious, this is useful for all users.

4) Visual highlight on first newly added item, to make easier to see what's new.

What is the desired behavior?

In my opinion, move focus to the 'Jump to first new item'; this is more predictable in my opinion.

5) Moves focus to 'Jump to first new item' button when Load more button does not exist in final screen.

6) Updates @since references to 5.7.0, optimistically

Update the number of total attachments.

This will require some kind of adaptation to getTotalAttachments. Seems like the model doesn't refresh when items are added or removed, though the collection does.

40 feels ok for now.

Agreed. Possibly a filter now, then explore adding a UI enhancement to support changing it later?

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


4 years ago

#55 @antpb
4 years ago

  • Keywords early added
  • Milestone changed from 5.7 to 5.8

It was agreed in the recent Media component meeting by participants that it might be too late in the 5.7 cycle to feel comfortable changing the media library experience. It was determined that this is better fit as an early candidate for 5.8 and should be committed asap.

Still pending as @joedolson noted in the meeting The major outstanding items: update attachments count, review design, add filter for attachment number.

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


4 years ago

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


4 years ago

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


4 years ago

#59 @lukecarbis
4 years ago

  • Keywords needs-refresh added

@joedolson Do you think you'll have time to take another look at this for 5.8?

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


4 years ago

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


4 years ago

@joedolson
4 years ago

Refreshed patch

@joedolson
4 years ago

Media load more initial view

@joedolson
4 years ago

Media load more after loading more; highlight showing first added graphic.

#62 @joedolson
4 years ago

  • Keywords needs-refresh removed

Patch refreshed & new screenshots loaded showing new version of load more design & visual indicator of first new loaded attachment.

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


4 years ago

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


4 years ago

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


4 years ago

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


4 years ago

@joedolson
4 years ago

Updated patch - language changes, highlight new items.

#67 @joedolson
4 years ago

Updated patch accomplishes four things:

1) Language to 'Jump' link changed based on Media meeting.
2) Language describing what's currently shown changed.
3) Mis-alignment of 'Jump to first loaded' button when all items displayed fixed.
4) Added highlight on all items added to help visual users perceive the insertion point and change.

@antpb I didn't find a way to parse over the newly added items, but I did find that I could add a class to all the items already loaded, and use a :not selector to invert the selection for highlighting.

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

@joedolson
4 years ago

Screenshot showing new language

#68 @joedolson
4 years ago

Still needed: update total collection size if items are added or deleted from the collection.

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


4 years ago

#70 @francina
4 years ago

Tested

  • MacOS 11.3
  • Safari 14.1
  • WordPress 5.8-alpha-50427-src
  • Twenty Twenty-One 1.3

After applying it, you have to runnpm run dev to get the expected behavior, might be worth adding it to the testing instructions.

The patch applies almost cleanly. The total counter doesn't update during the upload of the images in grid mode. Only after you refresh the page or do another upload. I am going to attach the image.

That said, I have some feedback:

  • UX/UI - There are no screen options in the grid view, so you can not change the number of images loaded in one go
  • Copy - The instruction "Jump to first loaded" is not clear. First what? The image of course, but could be clearer. First when? In the last batch. What if I want to jump to the first image loaded in the penultimate batch?
  • UX/UI - Have you considered pagination? If you have a large number of images, this patch solves the infinite scrolling issues but doesn't address the issue of separating the images in smaller batches.

Thanks!

@francina
4 years ago

Total conter doesn't update

#71 @joedolson
4 years ago

Thanks, @francina!

In a media meeting, we discussed that making the number of images a setting would probably be a future release issue, as there are a number of UI questions to resolve for that. That's also true for pagination, as implementing pagination into the media library would be a much more substantial change to the way it works.

The total counter updating is still an unresolved issue; it's the major outstanding item to figure out.

The language for the 'jump to' is a hard problem. It can't be 'image', since the media library contains attachments of all types. It could be 'attachment', I suppose; I'm not sure how readily understood that would be. But there's a question of how verbose it makes sense to be, e.g. "Jump to first attachment loaded in this request" seems awfully verbose.

I have no particular bias about the specific language; happy to hear suggestions! The aim is certainly to reach something that will be easily understood, but preferably also concise.

Can you tell me what in the patch didn't apply cleanly for you? This patch should be clean; it was fresh yesterday, so I'm not sure why it wouldn't apply cleanly.

I'm not sure what npm run dev does; it's not a command I've ever used.

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


4 years ago

#73 @francina
4 years ago

Thanks @joedolson!
Makes sense to ship this as an MVP and then iterate with pagination or any other solution that makes this accessibile, easly understandable and usable 🙌

Jump to first attachment loaded in this request" seems awfully verbose.

I'll take awfully verbose over unclear anytime, but this is only my opinion of course. "Request" is a technical term, end-users will not relate to it probably.

"Jump to the first file uploaded in the previous XX" where XX could be "operation, load, command" perhaps?
I am also trying to think about how that would sound in Italian "Vai al primo file caricato dopo l'ultima operazione".
I am adding a reminder to myself to ping copywriters and polyglots to come up with a reasonable text :)


Regarding the patch, it applies cleanly. What I meant by "almost" referred to known bug of the total counter. But I wasn't clear on communicating that, my bad.


And finally the npm command. This ticket doesn't have testing instructions, hence the needs-testing-info keyword added.
Since WordPress 5.6, the release squad has a testing focus lead: it was observed that not many tickets have clear testing instructions added, in the form of a list of steps needed.
I did figure out what to do and noticed that when you use wordpress-develop you need an additional npm run devafter the patch is applied. But it is only for people that test with that specific environment.

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


4 years ago

#75 @antpb
4 years ago

It was discussed in the recent Media component meeting that saying "Jump to first loaded item" is a great way to make this not focus on images or any other specific media type and still be clear. I agree that some folks might be confused and miss the "loaded" part of that phrase but it may be the clearest option to start with. This is also a very low impacting button to press as it keeps you on the same page and only changes your focus. Folks will learn pretty quickly what it does.

@hellofromTonya mentioned in the meeting on the pending count issue, "I think it would be a better experience for users if we could solve it before 5.8 beta. That said, it would be good to get the patch committed and then work on that specific issue as a follow-up."

It was agreed by multiple participants in the meeting that landing this sooner and iterating is ideal. :)

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


4 years ago

@joedolson
4 years ago

Updated patch - language change, resolve jshint issues.

#77 @joedolson
4 years ago

  • Keywords commit added; needs-testing-info removed

Updated patch for commit.

Until commit, this will require you to build WordPress after applying the patch.

Testing:

Testing preconditions: rquires a minimum of 41 items in media to interact with these controls.

  • throttle the network via your browser's dev tools: this helps in observing the UI behavior e.g. the spinner visibility
  • test without and with media trash enabled (by setting define( 'MEDIA_TRASH', true ); in your wp-config.php file)
  • test the filter add_filter( 'media_library_infinite_scrolling', 'return_true' ); to restore the infinite scrolling behavior
  • test the Media Library
  • test the Media modal with Classic Editor (Add Media, featured image)
  • test the Media modal with Gutenberg (image, gallery, featured image...)
  • optionally: test the Media modal with plugins that implement custom frames to upload / select attachments
  • inspect the queries array of cached collections

#78 @joedolson
4 years ago

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

In 50829:

Media: Remove infinite scroll from media library and modal.

Replace infinitely autoloading behavior on scroll with a user-controlled load more button. Fix a long standing accessibility issue in the media library. Infinite scroll poses a wide range of problems for accessibility, usability, and performance.

This change modifies the library to load 40 items in the initial view, with a load more button to load the next 40 items and a button to move focus from the load more region to the first of the most recently added items.

The text for communicating the jump target was broadly discussed, agreeing that the text incorporated here would most concisely and clearly convey the purpose of the button, and any further detail is learnable from use.

Props afercia, adamsilverstein, joedolson, audrasjb, francina
Fixes #50105. See #40330.

#79 @joedolson
4 years ago

See #53171 for continuation concerning the total attachment count issue.

#80 @SergeyBiryukov
4 years ago

In 50831:

Docs: Correct @since tags for new properties and functions related to infinite scrolling in Media Library.

Follow-up to [50829].

See #50105, #40330, #52628.

#81 @spacedmonkey
4 years ago

This change has broken in the Unsplash plugin. It is likely this change will break other plugins that extend the media library. Is there anyway of doing this, without breaking integrations?

#82 @joedolson
4 years ago

We'd need some information about what the breakage is to know; can you provide any documentation on what has broken?

The plugin author should be able to restore infinite scroll if they need to by using the new filter for that:

add_filter( 'media_library_infinite_scrolling', 'return_true' )

But if there's anything we can do to ensure plugins still work, happy to work on that.

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


4 years ago

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


4 years ago

#85 @spacedmonkey
4 years ago

The issue here is the change of the output from the function wp_ajax_query_attachments

        $result = array(
                'attachments'      => $posts,
                'totalAttachments' => $query->found_posts,
        );

        wp_send_json_success( $result );

The output admin ajax call is now different. I think changing the format of the response here will likely break other plugin as this admin ajax call is used in other plugin, not just core. I have used in a couple of plugins I have used for clients.

Instead of changing the output of the admin ajax call, not just use the REST API for these views. If you add the _envelope param to the rest api call, then you would get the same data. See https://make.wordpress.org/core/wp-json/wp/v2/media?_envelope=true

#86 @johnbillion
4 years ago

  • Keywords commit removed

Good spot. However, switching to the REST API for these views will also break plugins that override this output, for example Asset Manager Framework.

How about adding the totalAttachments data as an HTTP header and leaving the body response unchanged?

#87 @johnbillion
4 years ago

  • Keywords commit added

Whoops, I didn't see that this had already been committed :)

#88 @johnbillion
4 years ago

  • Keywords needs-dev-note 2nd-opinion added; has-patch needs-testing early commit removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for further discussion of the above. There are several plugins that override this Ajax response.

If this change stays as-is then this will need a dev note about the breaking change. If not then I think delivering the total via an HTTP header makes sense, but this might also mean that core cannot trust that the header is in place if a plugin overrides the Ajax handler.

#89 @spacedmonkey
4 years ago

There maybe other plugins out there that are using these reusing underscore views but do not override the admin ajax calls. In the Unsplash plugin for example, we have a custom REST API endpoint, the replicates the format of the admin ajax call.

Currently the view does this.

        parse: function( response, xhr ) {
                if ( ! _.isArray( response.attachments ) ) {
                        response = [ response.attachments ];
                }

                this.totalAttachments = parseInt( response.totalAttachments, 10 );

However, as documented above, the properties attachments and totalAttachments may not exist. The code should check first to see if these properties exist, if, they do not fall back to the previous behavior. If totalAttachments does not exist, infinite scroll should also be re-enabled.

Something like this might work.

        parse: function( response, xhr ) {
                if ( response.hasOwnProperty('attachments') && ! _.isArray( response.attachments ) ) {
                        response = [ response.attachments ];
                }

                if ( response.hasOwnProperty('totalAttachments') ) {
                    this.totalAttachments = parseInt( response.totalAttachments, 10 );
                }

#90 @spacedmonkey
4 years ago

If we use http headers, I believe we should follow the headers in the REST API.

Example

        $response->header( 'X-WP-Total', (int) $total_posts );
        $response->header( 'X-WP-TotalPages', (int) $max_pages );

#91 @joedolson
4 years ago

I think it makes sense to check for the existence of values specific to this update and make the change conditional on their existence. If any plug-in is overriding the data, then I think we should assume that plug-in is taking full responsibility for how that data is fetched and displayed.

That makes a lot of sense to me, at least.

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


4 years ago

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


4 years ago

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


4 years ago

This ticket was mentioned in PR #1335 on WordPress/wordpress-develop by adamsilverstein.


4 years ago
#95

  • Keywords has-patch added

Trac ticket:

#96 @adamsilverstein
4 years ago

  • Keywords has-patch removed

I looked at this a little bit (PR in progress: https://github.com/WordPress/wordpress-develop/pull/1335) and found the xhr object is not passed back to fetch as expected here, so I was unable to extract the post count from the header. Perhaps we are overriding fetch somewhere else?

Appreciate any help getting the header here, I agree moving this data to the header will be best to avoid breaking anything. Another option would be to introduce a new endpoint for the request.

#97 @joedolson
4 years ago

There's a lot of overriding happening in \wp\api.js; that's where the X-WP-Nonce header is set and fetched.

It's interesting that the XHR object documented there is not, in fact, an XHR object, and I can't see anything that's overriding fetch on the model. Sync is being extended, but that shouldn't be changing anything about the fetch.

Might be able to follow the path used for adding nonces to get this header. I'm currently looking at that.

#98 @joedolson
4 years ago

OK. Here's what I'm seeing, I think.

The Media library overrides backbone.sync in media\models\query.js.

Backbone uses .fetch to get data using .sync, so this is overriding the default Backbone return value, using wp.media.ajax instead. wp.media.ajax (an alias of wp.ajax.send) doesn't return an XHR object, but instead returns a promise.

We could probably pass an option into wp.media.ajax that gets the XHR object, though I'm not at all sure what additional consequences that could have.

In my quick test, I was able to get an XHR object, but I didn't see the header showing up, but I ran out of time today to try to track down why.

#99 @spacedmonkey
4 years ago

I wondered not getting the xhr object back was related to the jQuery update and something changing under the hood there but that doesn't seem to be the case.

Once we get headers, I still recommend using the same headers as the REST API .

X-WP-Total
X-WP-TotalPages

Not adding new response headers, will help compatibility with those using firewalls and reserve proxies. New headers have to added to allowed lists in these cases. Reusing an existing header, avoids this.

#100 @joedolson
4 years ago

@adamsilverstein's incomplete patch uses X-WP-TotalPages, so that's covered @spacedmonkey. I don't see any reason we'd change that.

#101 @adamsilverstein
4 years ago

Updated, functional patch: https://github.com/WordPress/wordpress-develop/pull/1335, attaching here as well.

#102 @adamsilverstein
4 years ago

In 50105.10.diff:

  • Pass attachment totals back in headers, restoring original response data shape.
  • Use a customized fetch handler to capture the returned X-WP-Total header.
  • Ensure spinner hides after load completes.
  • Use approach from rest endpoint to calculate correct X-WP-Total & X-WP-TotalPages headers.

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


4 years ago

#104 @antpb
4 years ago

We are leaving this open through Beta 1 as this is partially merged or related to a partially merged feature. All that is needed is a review of the above patch and potentially refinement. If the patch can't be merged before Beta 3 we'll need to revert to allow time for the revert to be tested before Release Candidate.

#105 @adamsilverstein
4 years ago

  • Keywords needs-testing added

@spacedmonkey can you confirm the latest patch fixes your use case in the Unsplash plugin?

@johnbillion in the latest patch, i reverted the ajax callback data return shape so that it should match what existed in 5.7, putting the post and page count information in the header as you suggested. Can you confirm this would restore compatibility for Asset Manager Framework?

@joedolson Can you please review the latest patch and give it a test? This biggest potentially breaking change is setting the xhr as context so we have access to the headers. I also wound up changing how the spinner hiding is handled to fix the behavior.

Once we confirm this works well we should commit before beta 2 to get wider testing, especially with potentially affected plugins.

Last edited 4 years ago by adamsilverstein (previous) (diff)

#106 @joedolson
4 years ago

This seems to work fine for core.

For the Unsplash plug-in, it partially works - Unsplash is replacing the returned object, however, so the XHR object isn't where we expect it to be. We can easily work around that for this one use case, since they retain the original response in _more:

if ( this.hasOwnProperty( 'getResponseHeader' ) ) {
	collection.totalAttachments = parseInt( this.getResponseHeader( 'X-WP-Total' ), 10 );
} else {
	collection.totalAttachments = parseInt( this._more.getResponseHeader( 'X-WP-Total' ), 10 );
}

Probably not practical for core, though.

I also tried setting a default value if we couldn't get the total attachments.

if ( this.hasOwnProperty( 'getResponseHeader' ) ) {
	collection.totalAttachments = parseInt( this.getResponseHeader( 'X-WP-Total' ), 10 );
} else {
	collection.totalAttachments = 0;
}

That worked really well for Unsplash, and I can imagine it might be a good general solution, though we'd only find that out in real-world testing.

I did find a bug that occurs when changing tabs - on the initial load of the Media Library tab, the load more button appears, but then it's gone if you switch away (e.g. to the Upload files tab or to a custom tab, like Unsplash.) Exploring this.

#107 @joedolson
4 years ago

Ah - the load more button isn't gone, it's just hidden.

Problem is that the updateLoadMoreView is only loaded on the 'add remove reset' events, and those aren't fired when switching tabs.

updateLoadMoreView needs to fire when the view is shown, as well.

This is fixed by running updateLoadMoreView on the same expectations as updateContent, which makes sense to me.

New patch coming shortly.

@joedolson
4 years ago

Handles returned data that isn't an XHR object & creates load more view when switching tabs in modal

#108 @joedolson
4 years ago

New patch handles a couple more edge cases; could still use feedback from @spacedmonkey and @johnbillion

#109 @spacedmonkey
4 years ago

I am currently doing some testing. I am not seeing the load more button, so I will hold off.

The unsplash intergration is going to be updated, as there is a new wrapping div and the css has changed. I think changing to use the headers, does not things a lot easier for us to update. We also have to map the header.

There needs to be some error handling to check if the header exists. If it doesn't exist fallback to the old behaviour.

#110 @joedolson
4 years ago

Test the most recent patch, if you're not already.

The load more button should not show up in Unsplash.

#111 @spacedmonkey
4 years ago

After doing some more testing, I got it working. I am going to need to do a small release to disable infinite scroll and make some css tweaks, but the latest patch work well!

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


4 years ago

#113 @joedolson
4 years ago

Regarding the new wrapper - that was needed in order to group the media items and the load more interface. I think that is needed, but it predates the point where I was working on this issue, so I can't say for sure that it's the only path to do this.

But it sounds like you can work around it pretty easily, which really should generally be true.

Noting that the additional wrapper should go into the dev note on this.

#114 @joedolson
4 years ago

  • Keywords commit added; 2nd-opinion needs-testing removed

I've done some continuing testing, and I think this is ready for commit. It clearly fixes issues with the existing implementation, creates a path for plugins to better adapt for the changes, and as far as I can find, creates no new issues.

#115 @joedolson
4 years ago

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

In 51145:

Media: Restore AJAX response data shape in media library.

Restore the original shape of the AJAX response data in the media library after removing infinite scroll, and pass total number of attachments in the response headers X-WP-Total and X-WP-TotalPages.

Improve backwards compatibility for plugins intercepting the ajax response. Headers match the structure and count calculation used in REST API responses.

Fix an issue with hiding the spinner after the load is completed and ensure that the load more view is created when changing tabs in the media library modal.

Follow up to [50829].

props adamsilverstein, spacedmonkey, joedolson.
Fixes #50105.

#116 @desrosj
4 years ago

In 51147:

Coding Standards: Apply some alignment fixes.

Follow up to [51145].
See #50105.

#117 @desrosj
4 years ago

In 51222:

Docs: Remove inaccurate @since tag.

Follow up to [50829,50831,51145,51147].

Props johnbillion.
Fixes #53461. See #50105.

#118 @desrosj
4 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

It looks like [51145] introduced an undefined variable. $query_args should be $query in wp_ajax_query_attachments().

I double checked with @adamsilverstein just to be sure, and it seems this does need to be corrected.

#119 @audrasjb
4 years ago

  • Keywords needs-patch added; commit removed

Indeed, $query_args is not defined, and $query should be used instead.

@audrasjb
4 years ago

#120 @audrasjb
4 years ago

  • Keywords has-patch added; needs-patch removed

#121 @joedolson
4 years ago

  • Keywords commit added

#122 @joedolson
4 years ago

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

In 51224:

Media: Correct undefined variable in wp_ajax_query_attachments.

Fix a misnamed variable introduced in [51145]. Change $query_args to correctly defined variable $query.

Follow-up to [51145].

props desrosj, audrasjb.
Fixes #50105.

#123 @ricjcs
4 years ago

Personally I didn't really like the implementation of this feature and the usability. To scroll through multiple images I have to press the button many times, it's not as fast as before. I think it should be an optional feature.

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


4 years ago

#125 @wildworks
4 years ago

  • Keywords needs-testing added; needs-dev-note has-patch commit removed

I used the media_library_infinite_scrolling filter in WordPress 5.8-RC3, but infinite scroll is not enabled.

Here is the code.

add_filter( 'media_library_infinite_scrolling', '__return_true' );

And I found a javascript error on the Media Library page.
https://i.gyazo.com/31de01f6d254557758bdf774068c4baf.png

#126 @joedolson
4 years ago

  • Keywords needs-dev-note added

Hi, @wildworks

How did you implement the filter? I just tested using the Media Library Enable Infinite Scroll plug-in, and it seemed to work as expected.

I was able to reproduce the JS error when media_library_infinite_scrolling is enabled, however, and I'll look into that.

#127 @joedolson
4 years ago

JS error is pretty straightforward; will get that fixed shortly. Created new ticket: #53672

#128 @wildworks
4 years ago

@joedolson
Thank you for the quick response.
I will test it again as soon as the new version is released.

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


4 years ago

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


4 years ago

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


4 years ago

@Boniu91
4 years ago

#132 @Boniu91
4 years ago

Hey! I do have some questions after testing this ticket.

  1. It looks like RWD when adding a image from the post editing view is not that good:


  1. I'm not sure what's the purpose of the grey background under newly loaded images, is this expected?


  1. I experienced situation when the first and second newly uploaded images are not highlighted (note that the first image is in the dotted frame)


#133 @grapplerulrich
4 years ago

40 feels ok for now. We can make sure this is filterable. It would be nice if this were user controllable like posts per page, or terms per page are from the screen options tab.

@adamsilverstein A filter was not added for 5.8? If not would it make sense to open a trac ticket for it? I am personally most interested in a filter, though a UI would be nice for normal users.

#134 @joedolson
4 years ago

I think that @antpb was going to do that (or at least open a ticket for discussing changes to the perpage number for the media gallery) following up from the media meeting last week, but it hasn't been opened yet. No filter was added in 5.8, so I think you could go ahead and open that ticket.

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


4 years ago

#136 @herregroen
4 years ago

The fix for this ticket seems to have introduced a backwards-incompatible change to the query-attachments ajax action.

Previously it wasn't required to send a posts_per_page variable. Now not sending this causes a division by 0 to happen.

Note that not passing it causes it to fallback to the global option but this is done on the query_vars property of the query and not on the query property which would still contain the originally passed parameters.

In this fix the query property is used to determine the total pages which may not have the posts_per_page set at all.

#137 @desrosj
4 years ago

@herregroen could you open a new ticket to address this please?

Note: See TracTickets for help on using tickets.