WordPress.org

Make WordPress Core

Opened 10 months ago

Last modified 5 days ago

#50105 assigned defect (bug)

Remove infinite scrolling behavior from the Media grid

Reported by: afercia Owned by: joedolson
Milestone: 5.8 Priority: normal
Severity: normal Version:
Component: Media Keywords: has-patch has-screenshots needs-testing needs-testing-info early
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 (8)

50105.diff (16.6 KB) - added by afercia 10 months ago.
50105.jpg (90.8 KB) - added by afercia 10 months ago.
50105.2.diff (18.6 KB) - added by afercia 9 months ago.
50105 buttons spinner.png (152.2 KB) - added by afercia 9 months ago.
50105.3.diff (18.6 KB) - added by joedolson 8 weeks ago.
Refreshed version of 50105.2.diff
50105.5.diff (19.8 KB) - added by adamsilverstein 5 weeks ago.
Screen Recording 2021-02-05 at 03.43.53 PM.mp4 (2.8 MB) - added by adamsilverstein 5 weeks ago.
50105.6.diff (21.7 KB) - added by joedolson 4 weeks ago.
Updated patch. Adds focus handling for last collection, design update.

Change History (66)

@afercia
10 months ago

@afercia
10 months ago

#1 @afercia
10 months 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.


10 months ago

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


10 months ago

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


10 months ago

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


10 months ago

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


9 months ago

#8 @afercia
9 months ago

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

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


9 months ago

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


9 months ago

#11 @afercia
9 months 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.


9 months ago

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


9 months ago

@afercia
9 months ago

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


9 months ago

#16 @audrasjb
9 months ago

  • Keywords needs-dev-note needs-testing added

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

#17 @afercia
9 months 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 8 months ago by afercia (previous) (diff)

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


9 months ago

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


9 months ago

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


8 months ago

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


8 months ago

#22 @afercia
8 months 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.


8 months ago

#24 @davidbaumwald
8 months ago

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

#25 @afercia
8 months 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.


8 months ago

#27 @desrosj
8 months 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.


5 months ago

#29 @afercia
5 months 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 months ago

  • Owner afercia deleted

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


3 months ago

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


3 months ago

#33 @antpb
3 months 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.


2 months ago

#35 @ryokuhi
2 months ago

  • Owner set to joedolson

#36 @joedolson
2 months ago

  • Keywords needs-refresh added

@joedolson
8 weeks ago

Refreshed version of 50105.2.diff

#37 @joedolson
8 weeks 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.


8 weeks ago

#39 @hellofromTonya
7 weeks 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.


7 weeks ago

#41 @ricjcs
6 weeks 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.


6 weeks ago

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


6 weeks ago

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


6 weeks ago

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


5 weeks ago

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


5 weeks ago

#47 @adamsilverstein
5 weeks ago

50105.5.diff refresh against trunk.

#49 @adamsilverstein
5 weeks 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 weeks 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 weeks 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 weeks ago

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

#53 @joedolson
4 weeks 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 weeks ago

#55 @antpb
4 weeks 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 weeks ago

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


12 days ago

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


5 days ago

Note: See TracTickets for help on using tickets.