WordPress.org

Make WordPress Core

Opened 3 weeks ago

Last modified 5 days ago

#50410 new defect (bug)

Media grid: hiding the "contextually created attachments" breaks the collection length count

Reported by: afercia Owned by:
Milestone: 5.5 Priority: normal
Severity: normal Version:
Component: Media Keywords: has-screenshots needs-patch
Focuses: accessibility, javascript Cc:

Description

After [41732], [41937], and [42739] the "contextually created attachments" are filtered out from the Media grid view.

"Contextually created attachments" are, for example, the ones created when cropping an image for the site logo, site header, etc. In this cases, the attachments are marked with a context property to distinguish them from user-uploaded attachments.

While I do understand the arguments on the related tickets #21819 and #42968, there is a problem with this implementation.

Actually, the attachments collection is filtered after it is fetched from the server. This breaks the expected collection length, which is supposed to be 40 attachments "per page".

Also, it makes impossible to build logic based on the collection length as necessary for #50105 so this issue is a blocker for #50105.

Additionally, I'm still not fully sure if this impacts also the "per page" setting and the collection hasMore() built-in method. This would require further investigation.

Basically, filtering the collection "after the fact" (o.e. after it gets fetched) artificially alters the model length property which is still 40 (for example) while the actual attachments displayed may be less than 40.

Ideally, the filtering should happen when fetching the collection. I do realize the attachments context is based on a not-indexed meta and querying for that would cause performance issues. However, filtering after the fetch doesn't seem ideal for many reasons.

Attachments (4)

cropped images attachment count 03.png (567.9 KB) - added by afercia 3 weeks ago.
cropped images attachment count 01 site logo.jpg (129.9 KB) - added by afercia 3 weeks ago.
Sice logo cropped images displayed in the list mode
custom header image skip cropping.jpg (67.5 KB) - added by afercia 3 weeks ago.
Custom header image displayed in the list mode: note that when skipping the crop, it does create a "copy" anyways
cropped images attachment count 02.jpg (165.0 KB) - added by afercia 3 weeks ago.

Download all attachments as: .zip

Change History (16)

#1 @afercia
3 weeks ago

  • Keywords has-screenshots added

Some more context.

Given the media grid fetches new attachments depending on the viewport height and scroll position, on medium / large screens it usually fetches at least twice. This makes spotting the issue not that easy. To reproduce, open your browser's dev tools and make the viewport height as small as possible. See screenshot below. Then:

  • to to the Media Library page in "grid mode"
  • check there was only one AJAX request to fetch attachments
  • select the grid unordered list element and get the li items count in your console
  • in my case (see screenshot below) I had 3 attachments coming from "cropped" images
  • the grid displays only 37 items
  • expected: 40 items

#2 @afercia
3 weeks ago

From an user experience perspective:

The Media Library in "list mode" does display images that come from crop actions. See attached screenshots.

However, when users switch from "list mode" to "grid mode", these attachments suddenly disappear. Not sure this is the best user experience.

@afercia
3 weeks ago

Sice logo cropped images displayed in the list mode

@afercia
3 weeks ago

Custom header image displayed in the list mode: note that when skipping the crop, it does create a "copy" anyways

#3 @afercia
3 weeks ago

As said, the above images suddenly disappear in the "grid mode". There's one more point yet about user experience. For example:

  • go to the customizer
  • add a site logo
  • the UI offers to crop the logo
  • as a user, I may want to carefully craft a cropped image for art direction and to represent my brand at best
  • once I'm OK, I crop the image and I can see my nice, well cropped, logo on my website
  • at some point I may want to switch to another theme
  • once I set a new theme, I need to set the logo again
  • I click "Select logo" in the customizer
  • I expect to find my logo there, the one I previously cropped at best of my abilities
  • the logo is gone: the media library UI doesn't display my logo
  • I'm forced to upload a new image and start the process from scratch

Not sure this is the best user experience.

#4 @afercia
3 weeks ago

Last consideration:

See screenshot below, with the patch from #50105 applied.

See the highlighted text "Media items 37 of 84".

Expected: "Media items 40 of 84".

Actually, the collection length is altered because in the example above there are 3 attachments that come from cropped images. This breaks the "per page" setting and makes any logic based on the attachment length / per_page (and maybe hasMore()) unreliable.

Note that after pressing "Load more" a few times and all available attachments are displayed, the counter text will display "Media items 81 of 84" thus breaking the count.

Last edited 2 weeks ago by afercia (previous) (diff)

#5 @afercia
3 weeks ago

I'm not sure what is the best way to solve this issue. While I partly understand the reasoning behind the attachments being filtered, I'm not sure the current implementation is ideal.

At the very least, the implementation is incomplete. I don't see a good reason why these attachments are filtered in the grid view and are not filtered in the list view. This is confusing for users.

On the other hand, since there are technical concerns in querying the meta, I'd lean towards reconsidering the filtering and remove it. After all, cropped images are first-citizens attachments. Opting to not displaying them seems based on assumptions to me. Actually, there are valid cases where cropped images should be displayed and be available to users.

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


2 weeks ago

#7 @afercia
2 weeks ago

Related: #46127

#8 @pbiron
2 weeks ago

Related: #47215

#9 @afercia
11 days ago

  • Keywords needs-patch added

Considering all the above and considering the feedback on:

  • #46127 created 17 months ago and still no feedback from the Media team
  • #47215 created 14 months ago and still no feedback from the Media team

I'd definitely lean towards removing the attachments filtering. It breaks many users expectations, produces a different behavior of the two Media library modes, and makes the collection behavior unreliable. I'm not fully sure the arguments behind the introduction of the filter were a good reason for that change to start with.

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


10 days ago

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


10 days ago

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


5 days ago

Note: See TracTickets for help on using tickets.