Make WordPress Core

Opened 2 months ago

Closed 7 weeks ago

Last modified 7 weeks ago

#60524 closed defect (bug) (fixed)

Cropping site icon should preserve attachment properties

Reported by: afercia's profile afercia Owned by: joedolson's profile joedolson
Milestone: 6.5 Priority: normal
Severity: normal Version:
Component: Media Keywords: has-patch has-unit-tests commit
Focuses: accessibility Cc:

Description (last modified by afercia)

Follow-up to #37750 / [53027]
See #54370 / [57602]

In [53027], a bug was fixed where newly generated cropped images didn't copy over some meta data from the original image like the alt text and caption.

The code that fixed the bug was added in the wp_ajax_crop_image() function but it turns out it was added in the default case of the big switch statement in that function.

As such, the meta data are copied over only for the default case, which includes the cropping of the WP logo, but they aren't copied over for the site-icon case.

It could be argued that the site icon image doesn't need the alt text or the caption because it is used in the browsers' UI. However, it would be best to always copy over these data to any cropped image for at least two reasons:

  • Cropping an image will generate a new image that is added to the Media Library. It's good that the new image preserves all the data from the original image.
  • These meta data are useful in the admin interface. For example now that [57602] adds a site icon UI in the admin General Settings page, the UI needs the alt text of the site icon image to inform users what the currently selected image is. This would be even more important for screen reader users as the current implementation in [57602] doesn't provide any textual information about the currently selected image.

The inconsistency in copying over the meta data can be tested also in the Customizer or in the new UI from [57602].

To reproduce:

  • Set Twenty Twenty-One as active theme so that the Customizer is available.
  • Go to the Customizer > Site Identity.
  • Set a Logo image: make sure to select an image that is large enough to be cropped and that does have an alt text.
  • Once the image gets cropped and set as logo, open the Media Library again.
  • Observe there's a new cropped image.
  • Select the cropped image and observe in the right side of the media modal dialog that the original alt text (and caption if originally present) has been copied to the new image.
  • Close the media modal dialog.
  • In the Customizer set a Site Icon, repeating the steps above.
  • Once the image gets cropped and set as site icon, open the Media Library again.
  • Observe there's a new cropped image.
  • Select the new image and observe the original alt text (and caption if any) hasn't been copied to the new image.

Attachments (1)

60524.patch (1.8 KB) - added by rcreators 2 months ago.
I have extended class-wp-site-icon -> create_attachment_object to carry forward parent attachment title, alt text, caption and description as well. This will solve the issue.

Download all attachments as: .zip

Change History (27)

#1 @afercia
2 months ago

It's also important to note that some filters, for example wp_ajax_cropped_attachment_id run only for the default case and aren't available for the site-icon case.

Not sure that's ideal. I'd tend to think all the filters within wp_ajax_crop_image should be audited and made available for all cases, when that makes sense.
At the very least, the documentation should clarify that these filters don't run for 'all' the cropped images.

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

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


2 months ago

#3 @joedolson
2 months ago

  • Keywords needs-patch added
  • Owner set to rcreators
  • Status changed from new to assigned

#4 @jorbin
2 months ago

  • Milestone changed from Awaiting Review to 6.5

This could be very prevalent if the alt text is updated depending on the final direction of alt text and UI for #54370, therefore I think it would be good to get a fix in soon.

#5 @afercia
2 months ago

  • Description modified (diff)

@rcreators
2 months ago

I have extended class-wp-site-icon -> create_attachment_object to carry forward parent attachment title, alt text, caption and description as well. This will solve the issue.

#6 @rcreators
2 months ago

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

Instead of working on ajax-actions.php->wp_ajax_crop_image function, I worked on class-wp-site-icon.php->create_attachment_object function. I extended create_attachment_object() function to copy original file's meta data, title, desc, caption and alt text and attach it to new cropped image.

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


2 months ago

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


2 months ago

#9 @huzaifaalmesbah
2 months ago

Test Report

Patch tested: 60524.patch

Environment

  • WordPress: 6.5-beta1-57630-src
  • PHP: 8.2.15
  • Server: nginx/1.25.3
  • Database: mysqli (Server: 8.0.36 / Client: mysqlnd 8.2.15)
  • Browser: Chrome 121.0.0.0
  • OS: macOS
  • Theme: Twenty Twenty-One 2.1
  • MU Plugins: None activated
  • Plugins:
    • Gutenberg 17.7.0
    • Test Reports 1.1.0

Screenshots

Before Apply Patch Crop After Apply Patch Crop ✅
https://i.ibb.co/99XhsYW/Screenshot-2024-02-15-at-12-17-11-AM.pnghttps://i.ibb.co/VCHDHTR/Screenshot-2024-02-15-at-12-16-59-AM.png

#10 @joedolson
2 months ago

Thank you, @rcreators!

This does work, but I don't think that the method is optimal. It does the job, but it splits the logic so that identical logic is being applied in different places. This makes long-term code maintenance harder.

I think that a better path might be to create a helper function that copies the attachment data as needed. E.g.,

function wp_copy_parent_attachment_properties( $attachment, $parent ) {
    // Logic to check and modify attachment array.
    return $attachment; 
}

Then, all the logic is in one place, and we can call it as needed. Thoughts?

#11 @rcreators
2 months ago

Hello @joedolson,

Yep. Got your point. Do you have any specific file in mind to put this function in? I will do that Today. As it's related to attachment but there is no attachment class or just put in ajax-action file works?

This ticket was mentioned in PR #6128 on WordPress/wordpress-develop by @rcreators.


2 months ago
#12

As per the discussion over the ticket, Created the function wp_copy_parent_attachment_properties() in image.php file and called it in ajax-actions.php file.

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

#13 @shailu25
2 months ago

Test Report

Tested Patch: https://github.com/WordPress/wordpress-develop/pull/6128

Testing Environment:

OS: Windows
Web Server: Nginx
PHP: 8.1.23
WordPress: 6.5-beta1
Browser: Chrome
Theme: Twenty Seventeen
Active Plugins: None

Actual Results:

  • Issue Resolved With Patch.✅

Screenshots:

Before Patch: https://prnt.sc/NEhmi2yk7kgt
After Patch: https://prnt.sc/DvUZVXSGFVlV

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


2 months ago

#15 @rcreators
2 months ago

@joedolson, PR updated with custom header class as well. It was also cropping the image without copying parent attributes.

About removing the create_attachement_object function from the site icon class, I tried to find it on https://wpdirectory.net/, and found some matches but cannot be sure if they are using from the site icon or their own or from somewhere else. So right now we can leave them as it is.

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


2 months ago

#17 @joedolson
2 months ago

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

I think this looks good. However, I think it may be worthwhile to remove the create_attachment_object method. I looked over the directory results, and there are really only two plugins and one theme using this; everything else is people committing vendor directories, build tools, etc. Only one of those plugins has a significant installation base, and I think it's a pretty low bar to change this rather than keep an unused method in core.

But I'd appreciate second opinions on that; it would be removing core methods and breaking backwards compatibility, but only on a very small scale.

I think this is good for commit as is, just would like a second opinion about whether or not to remove the unused method.

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


2 months ago

#19 @rcreators
2 months ago

If that's the case, we can remove create_attachment_object method from the site icon class. Removing old code and making WordPress cleaner is also a good thing to proceed further.

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


8 weeks ago

#21 @swissspidy
7 weeks ago

  • Keywords needs-unit-tests added

The create_attachment_object method should be marked deprecated and not directly removed from core. Especially since it is public and used in the ecosystem.

By the way, that method has some unit tests, can this be used as a starting point to add some test coverage for this newly proposed wp_copy_parent_attachment_properties function as well?

Also, just noting that 6.5 RC1 is approaching soon, so this would need to land before that or else we should punt it to 6.6.

#22 @joedolson
7 weeks ago

  • Keywords has-unit-tests added; 2nd-opinion needs-unit-tests removed

Unit tests added and create_attachment_object deprecated.

#23 @swissspidy
7 weeks ago

  • Keywords commit added
  • Owner changed from rcreators to joedolson

#24 @joedolson
7 weeks ago

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

In 57755:

Media: Accessibility: Copy attachment properties on site icon crop.

Add parity between site icon, custom header, and default image crop behaviors. [53027] fixed a bug where alt text and caption were not copied on custom headers, but did not apply that change in any other context.

Deprecate the create_attachment_object method in the Wp_Site_Icon and Custom_Image_Header classes and replace that functionality with the new function wp_copy_parent_attachment_properties() to improve consistency.

Props afercia, rcreators, jorbin, joedolson, huzaifaalmesbah, shailu25, swissspidy, mukesh27.
Fixes #60524.

@joedolson commented on PR #6128:


7 weeks ago
#25

In r57755

#26 @desrosj
7 weeks ago

In 57771:

Coding standards: Apply some changes after composer format.

Follow up to [57565], [57627], [57755],

See #60233, #60506, #60524.

Note: See TracTickets for help on using tickets.