Make WordPress Core

Opened 6 years ago

Closed 4 years ago

Last modified 4 years ago

#47215 closed defect (bug) (fixed)

The media that is "Unattached" is different in list mode vs grid mode

Reported by: pbiron's profile pbiron Owned by: antpb's profile antpb
Milestone: 5.6 Priority: normal
Severity: normal Version:
Component: Media Keywords: has-screenshots
Focuses: Cc:

Description

When in list mode (on Media > Library), images that have the post_states "Header Image" and "Site Icon" are shown as "Unattached"; whereas in grid mode they are not.

I think the grid mode behavior is preferable, since to me, "unattached" means "is not used anywhere" and clearly "Header Images" and the "Site Icon" are used (assuming the theme supports them).

Attachments (1)

47215.png (41.4 KB) - added by afercia 4 years ago.

Download all attachments as: .zip

Change History (16)

#1 @pbiron
6 years ago

After several hours tracking down another problem involving grid mode, I discovered why grid mode doesn't show images with post_states as unattached...and I don't like the reason :-(

Grid mode intentionally does not show any image (whether it is attached or not) with a non-empty value in the _wp_attachment_context postmeta of the attachment post. (see src/js/media/models/attachmetns.js, lines 155-161). That "feature" was introduced in #21819.

Why don't I like this? Because it is not just grid mode in the media library that excludes those images...it is anything that uses /wp-includes/js/media-models.js. One such user is the Customizer, for setting header images.

One consequence of this is the following:

  1. set the active theme to any theme that supports custom-header
  2. launch the customizer and select one or more images as header images
  3. publish that customizer changeset
  4. now, set a different theme that supports custom-header as the active theme
  5. launch the customer and try to set the same header images. You'll find that you can't, because wp.media.model.Attachments strips them out :-(

Why would you want to have the same header images in 2 different themes? In my case, the 1st theme is what is currently used on a given site and the 2nd theme is an in-dev complete rewrite of the 1st theme that has the same basic page layout (including the area where the header image is displayed)...and I need to switch back-and-forth between the 2 themes to make sure that the revised theme hasn't broken things that work in the 1st theme.

Another possibly unintended consequence of this "feature" is that it prevents you from inserting a header image (or site icon, etc) into post content...because the image inserter (in both gutenberg and class) uses the media modal (and hence media-models.js). Why would you want to do that? Suppose you're writing a blog post about how you built the current site. You give detailed/step-by-step instructions of various things like: setting header images, site icons, etc in the customizer. It would be perfectly normal to include those images in the content of that post...but you can't!

@joemcgill since you worked on #21819 do you have any comments? That ticket was about hiding crops of a given image from the media modal, but the solution implemented actually hides things that may not have been cropped. For instance, suppose my theme does:

<?php
add_theme_support( 'custom-header', array( 'width' => 1000, 'height' => 150 ) );

and the header images I upload in the customizer are exactly those dimensions...then the media modal stripping them out because 'context' === 'custom-header' is overkill, since they are not crops, they are originals.

Maybe the the work that @azaozz is undertaking on derived images for 5.3 could be leveraged to make the media modal's stripping of crops less greedy (i.e., don't strip an image just because it is "contextual", but only if it a crop/derivation of another image)?

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


5 years ago

#3 @afercia
4 years ago

  • Milestone changed from Awaiting Review to 5.5

Related: #50410

I'd wholeheartedly agree with all the above. See also #50410 where there's some more context on how this affects the work to remove infinite scrolling from the Media Grid, see #50105.

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


4 years ago

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


4 years ago

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


4 years ago

#7 @davidbaumwald
4 years ago

  • Milestone changed from 5.5 to Future Release

This was discussed during today's scrub. With 5.5 RC1 approaching and the status of the related ticket, this is being moved to Future Release. If any maintainer or committer feels this can be resolved in time, or wishes to assume ownership during a specific cycle, please update the milestone accordingly.

#8 follow-up: @Mista-Flo
4 years ago

I am a bit confused about the original ticket description. An image can be "attached" only to post types, and media_states are not. I don't see how you could implement it. It's already shown as a Media state in List view, so it seems pretty obvious.

I am not sure what do you mean by 'whereas in grid mode they are not.' Can you provide a screenshot?

That said, I have worked on these two tickets that are meant to display the attached (uploaded to) and media state value on edit attachment popup screen.

https://core.trac.wordpress.org/ticket/42063
https://core.trac.wordpress.org/ticket/46866

It would also probably be better to open another ticket for the issue you mention on your first comment.

Last edited 4 years ago by Mista-Flo (previous) (diff)

#9 in reply to: ↑ 8 @afercia
4 years ago

  • Keywords has-screenshots added

Replying to Mista-Flo:

I am a bit confused about the original ticket description. An image can be "attached" only to post types, and media_states are not. I don't see how you could implement it. It's already shown as a Media state in List view, so it seems pretty obvious.

The original ticket description asks to remove the text "Unattached" for images that are actually _used_ (whether they're used as site icon, header image, etc.).

to me, "unattached" means "is not used anywhere"

For example, see the screenshot below, where an image used as site icon is also marked as "unattached".

There are also other important considerations in the second part of this ticket that go a bit beyond simple labelling issues.

@afercia
4 years ago

#10 @pbiron
4 years ago

@Mista-Flo In a very real sense the original ticket description is asking for a (backwards incompatible) change to what unattached means for media.

I'm not 100% convinced that is a good idea, so I'm very much open to other opinions on the subject.

Yes, attached for media (currently) means has post_parent !== 0. And I think that is still a valuable concept. Perhaps what is needed is a new "filter" such as Unused that would be in addition to Unattached.

It would also probably be better to open another ticket for the issue you mention on your first comment.

It may not be clear, but comment:1 was added 2 weeks after this ticket was opened, and the issues mentioned in that comment are tangential to the original issue description...but as @afercia says, they come into play on other related tickets, such as linked to in comment:3.

Hope that clarifies things.

#11 @pbiron
4 years ago

The other thing I'll mention is that finding which media are unused is not an easy thing to do...so maybe the additional filter I just suggested isn't feasible...it was just a suggestion, after all :-)

#12 @Mista-Flo
4 years ago

Thanks for the explanation guys, I have a better understanding now.

Hum, yes I agree, that seems a bit touchy. I am a bit scare of adding more complication here

#13 @Mista-Flo
4 years ago

@pbiron @afercia I dropped a patch in https://core.trac.wordpress.org/ticket/46127 to remove the filter that prevents attachments with context to be shown.

After some tests, it now displays context attachments as well in grid mode in the upload.php view. But it also display them in post edit page (media popup) when you want to add a media that has a context. So it seems to fix all the issue around here. Tell me if it's okay for you

#14 @antpb
4 years ago

  • Owner set to antpb
  • Resolution set to fixed
  • Status changed from new to closed

In 48989:

Media: Allow contextually generated images to show in Media Library grid view.
In grid view, contextually generated or cropped media is filtered out causing page numbering to be offset and incorrect. This also impacted any media utilizing media-models.js.
Props webmandesign, audrasjb, afercia, pbiron, mista-flo.
Fixes #46127, #50410, #47215.

#15 @desrosj
4 years ago

  • Milestone changed from Future Release to 5.6
Note: See TracTickets for help on using tickets.