Make WordPress Core

Opened 8 years ago

Closed 2 years ago

Last modified 2 years ago

#37750 closed defect (bug) (fixed)

Cropping custom logo should preserve attachment properties

Reported by: flixos90's profile flixos90 Owned by: joedolson's profile joedolson
Milestone: 6.0 Priority: normal
Severity: normal Version: 4.5
Component: Media Keywords: has-patch has-unit-tests has-screenshots has-testing-info commit
Focuses: accessibility Cc:

Description

I just noticed that, when uploading and cropping a custom logo, the original attachment's properties won't be migrated to the cropped image. I specifically added an "alt" description of the image, but on the website it only showed the file name for the alt tag, since the new cropped image does not preserve any of the data provided into the uploaded image. I think this is unexpected behavior and should be fixed.

I think the data manually changed by the user should be migrated to the cropped image to prevent this.

Change History (39)

#1 @Clorith
8 years ago

  • Component changed from Themes to Media

I'm not sure I agree (just to give a counterpoint), if I manually crop an image, it is to put emphasis on a part of it, making it no longer relevant to the meta data added to the full image.

Scenario:

  • I upload a picture of a bunch of football players, I give it an alt label of "The whole football team in action"
  • I crop the image, I only want the part of the image where a player kicks the ball, I now have "Player McGee in action", as such it's expected that a new Alt title be put in, if we auto-populate the field then this will just be ignored.

I think a much better approach would be if the cropping tool actually provided the meta sidebar like the other media management windows does.
This would allow the meta data to be manipulated there and then, without running the risk of being forgotten or ignored, thoughts?

#2 @flixos90
8 years ago

Good point. Maybe a combination of both would do it. I think we should cover both scenarios - both of our proposals will ignore either one or the other. In my opinion a good solution would be to show the meta sidebar on the cropped image like you said, but fill in the values the user entered for the original image (as defaults). Many times these will apply to the cropped image as well, and if not, they could easily change it.

#3 @afercia
7 years ago

  • Focuses accessibility added

Related: #38768

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


7 years ago

#5 @afercia
7 years ago

  • Milestone changed from Awaiting Review to 4.8.1
  • Owner set to afercia
  • Status changed from new to assigned

#6 @afercia
7 years ago

#36868 was marked as a duplicate.

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


7 years ago

#8 @afercia
7 years ago

  • Milestone changed from 4.8.1 to 4.9

See also #30155

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


7 years ago

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


7 years ago

#11 @afercia
7 years ago

  • Owner changed from afercia to joemcgill

Agreed to re-assign ownership.

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


7 years ago

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


7 years ago

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


7 years ago

#15 @melchoyce
7 years ago

  • Milestone changed from 4.9 to 4.9.1

#16 @johnbillion
6 years ago

  • Milestone changed from 4.9.1 to 5.0

#17 @flixos90
6 years ago

  • Milestone changed from 5.0 to 5.1

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


5 years ago

#19 @pento
5 years ago

  • Milestone changed from 5.1 to Future Release

#20 @dlh
5 years ago

#46185 was marked as a duplicate.

#21 @jarledb
5 years ago

Why is this being ignored? It is an actual accessibility issue - since it takes away the ability to set proper alt text on the site logo.

Its also a very frustrating bug to come across when trying to set alt text for the site image (when its been cropped).

The very least it should be documented in the WordPress Codex so people can find it and understand that alt text (and other attachment data) is not available for the image after being cropped in the customizer.

#22 @joedolson
2 years ago

  • Milestone changed from Future Release to 6.0
  • Owner changed from joemcgill to joedolson
  • Status changed from assigned to accepted

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


2 years ago

#24 @antonvlasenko
2 years ago

I'm able to reproduce this bug.
I will try to work on a patch to see if I can move this issue forward.

This ticket was mentioned in PR #2441 on WordPress/wordpress-develop by anton-vlasenko.


2 years ago
#25

  • Keywords has-patch has-unit-tests added; needs-patch removed

This PR aims to implement preserving attributes when cropping a custom logo image.

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

#26 @antonvlasenko
2 years ago

Steps to test this PR:

  1. Activate a non-block theme like Twenty Twenty-One.
  2. Go to the Media Library.
  3. Upload a new image to the media library.
  4. Set attachment properties like Alternative Text, Title, Caption, Description.
  5. Go to the Appearance -> Customize page.
  6. Go to the Site Identity, click on the Select Logo.
  7. Set a new cropped logo for the theme using the image you uploaded.
  8. Go to the Media Library.
  9. Open the last (cropped) image.
  10. Make sure that the original attachment properties (i.e. Alternative Text, Title, Caption, Description) have been copied over to the cropped image.
  11. Repeat steps 2-3.
  12. Reset all attachment properties (leave the fields empty).
  13. Repeat steps 5-9.
  14. Make sure that the Title field contains name of the cropped file.
  15. Make sure that the Description field contains url of the cropped file.
  16. Alternative Text and Caption properties have to be empty.
Last edited 2 years ago by antonvlasenko (previous) (diff)

#27 @ironprogrammer
2 years ago

  • Keywords has-screenshots added

Thank you for the patch, @antonvlasenko!

Test Report

For PR 2441. Sincere apologies for stomach grumbles caused by these test images 😋

Env

  • WordPress 6.0-alpha-52448-src
  • Chrome 99.0.4844.74
  • macOS 12.3 (Monterey)
  • Theme: Twenty Twenty-One
  • Gutenberg DISABLED 🔴

Steps to Test

  1. Activate a non-block theme like Twenty Twenty-One.
  2. Navigate to Media > Library.
  3. Upload a new image to the media library, at least 512x512 pixels in size.
  4. Set attachment properties for Alternative Text, Title, Caption, and Description.
  5. Go to the Appearance > Customize.
  6. Select Site Identity, and under the Logo section, click "Select Logo".
  7. Select the image just uploaded.
  8. Click the "Select" button to open the crop tool.
  9. Select the region to crop, and click the "Crop image" button.
  10. Click "Publish"* and close the Customizer.
  11. Navigate to Media > Library.
  12. Select the latest image added (the cropped logo).
  13. Observe that the Alt Text, Title, Caption, and Description previously set are populated in the Attachment Details sidebar. ✅
  14. Repeat steps 2-3.
  15. Reset all attachment properties (clear all fields).
  16. Go to the Appearance > Customize.
  17. Select Site Identity, and under the Logo section, click "Change Logo".
  18. Repeat steps 7-12.
  19. Observe that Title contains the name of the cropped file, and that Description contains the URL of the cropped file. The other two fields remain empty. ✅

(* NB: you do not need to publish for this test, but it avoids popups and revision notices in subsequent steps.)

Expected Results (✅)

  • Attachment Details attributes are preserved when set on the source image.✅
  • When attributes are blank, the Title and Description attributes are automatically set to the cropped file name and URL, respectively.✅

Screenshots

https://cldup.com/RUxY0udTup.png
Original uploaded image (Step 3).

https://cldup.com/3zwaAryoSI.png
Cropped logo image retains attributes (Step 13).

https://cldup.com/KEjaIbr00c.png
Image with cleared attributes (Step 15).

https://cldup.com/DkvvJMfaay.png
Cropped logo image defaults Title and Description to name and URL (Step 19).

Additional Notes

This PR preserves attachment details for the Customizer's Logo crops, but crops to the Site Icon do not retain those attributes -- the system exhibits the same behavior as in Step 19 above. This is out of scope for the reported issue, but worth noting here.

https://cldup.com/A6Fmgaq4Hw.png
Crop for Site Icon does not retain attributes from original image (Additional Notes).

#28 @antonvlasenko
2 years ago

Thank you so much for the test reports, @ironprogrammer!
I appreciate it.

crops to the Site Icon do not retain those attributes

I'd implement this fix in the scope of another trac issue as it will require implementing new unit tests and test reports. Thank you for reporting.

Last edited 2 years ago by antonvlasenko (previous) (diff)

#29 follow-up: @antonvlasenko
2 years ago

Just FYI: this PR has some breaking changes.
Currently, WordPress always overwrites Title and Description properties (with name and url of the image file respectively) when we crop an image.
If this PR gets merged, these properties will only be populated (overwritten) if Title and Description properties of the original image are empty.
I'm not sure if that's desired behavior, but I can't think of a better implementation.

Last edited 2 years ago by antonvlasenko (previous) (diff)

#30 in reply to: ↑ 29 ; follow-up: @hellofromTonya
2 years ago

Replying to antonvlasenko:

Just FYI: this PR has some breaking changes.
Currently, WordPress always overwrites Title and Description properties (with name and url of the image file respectively) when we crop an image.
If this PR gets merged, these properties will only be populated (overwritten) if Title and Description properties of the original image are empty.
I'm not sure if that's desired behavior, but I can't think of a better implementation.

PR 2441 does pre-populate a new cropped image's properties from the original image's properties - including the Title and Description. The Title though doesn't compare if the user edited it though - proposing that change in the code review.

The PR seems to align to the intent proposed by @flixos90

fill in the values the user entered for the original image (as defaults)

But yes, both the Title and Description properties existing behavior / values are changed by the PR.

What about the original intent? Looking at the existing data and [33280] / #29211 that added this functionality:

  • there's no discussion about the properties.
  • and thus, both Title and Description seem to be generic placeholders to create the new cropped image.

To me, as a user, I'd expect to retain the original image's properties and be given the ability to modify them when cropping. If the properties sidebar is also exposed as @Clorith proposed, then the user is given that ability.

Hey @flixos90 @Clorith what do you think today (as this one is an oldie)? Any concerns about changing the existing behavior for the Title and Description? Or does it still make sense today to pre-populate with the original image's properties and show the properties sidebar?

#31 @hellofromTonya
2 years ago

  • Keywords has-testing-info added

Test Report

Patch tested: PR 2441

Env:

  • OS: macOS Big Sur
  • Localhost: wp-env
  • WordPress: trunk
  • Browser: Firefox and Edge
  • Plugins: none
  • Theme: Twenty Twenty

Testing Steps

  1. Activate Twenty Twenty or any other non-block theme.
  2. Go to Media > Library.
  3. Upload at least one image. (In my case, I uploaded 4 different images with different metadata to pre-populate the properties.)
  4. For certain tests, set the attachment properties for alt test, title, caption, and/or description.
  5. Go to the Appearance > Customize > Site Identity.
  6. In the Logo section, click "Select Logo".
  7. Select an image.
  8. Click the "Select" button. Expected behavior: Crop modal opens.
  9. Select a region to crop.
  10. Click the "Crop image" button.
  11. Click "Publish"* and close the Customizer.
  12. Go to Media > Library.
  13. Click on the cropped logo image to open its properties screen. Test: Check that the properties are properly populated. Expected behavior:
    • Title:
      • Is cropped-[filename] when the original image's title is the filename (meaning the image did not have a title in its metadata when uploaded or the title property wasn't manually changed in step 4).
      • Is the same as the original image's title when the original title is not the filename (meaning the image either has a title in its metadata or the title property was manually changed in step 4).
    • Alt text: matches the original image's alt text property.
    • Caption: matches the original image's caption property.
    • Description:
      • Cropped image's URL when the original image's description property is empty.
      • Else, matches the original image's description property.

Test Results

Scenarios:

Original Title Original Alt Original Caption Original Description Cropped Correct?
filename
filename alt
filename caption
filename description
filename alt caption
filename alt description
filename alt caption description
title
title alt
title caption
title description
title alt caption
title alt description
title alt caption description

Summary

  • Able to reproduce the reported issue ✅
  • After applying the patch and tested each of the scenarios, patch works as expected ✅

#32 @hellofromTonya
2 years ago

PR 2441 LGTM for pre-populating cropped file properties from the original image. Approved the PR.

commit is pending feedback from @flixos90 and @Clorith about changing the title and description's existing behavior (see comment 29 above).

#33 in reply to: ↑ 30 @flixos90
2 years ago

Replying to hellofromTonya:

Hey @flixos90 @Clorith what do you think today (as this one is an oldie)? Any concerns about changing the existing behavior for the Title and Description? Or does it still make sense today to pre-populate with the original image's properties and show the properties sidebar?

This still makes sense to me today, I think it's a UX flaw that this information isn't taken over when cropping the image. I also just reviewed the PR and left a few minor comments, but it's pretty much good to go from my end.

Thank you @antonvlasenko and everyone involved in getting this fixed after all these years! :)

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


2 years ago

#35 @Clorith
2 years ago

This makes sense from my end as well, once you are done cropping the interface also now shows the cropped version and its data so that covers my original concerns as well, so looks good!

(And can i say thank you for those amazing test scenarios and the details they provide, I am loving them!)

#36 @joedolson
2 years ago

  • Keywords commit added

Awesome! Let's get this moving! I'm marking for commit; I'll do a final review and take care of this soon.

#37 @joedolson
2 years ago

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

In 53027:

Media: Preserve attachment properties on cropping custom logo.

Migrate the alternative text, title, description, and caption of an image over to the cropped copy of the image after cropping. Ensure that characteristics added to an image prior to cropping are not lost.

Props flixos90, Clorith, afercia, antonvlasenko, ironprogrammer, hellofromTonya.
Fixes #37750.

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


2 years ago

Note: See TracTickets for help on using tickets.