![(please configure the [header_logo] section in trac.ini)](/chrome/site/your_project_logo.png){kind=logo}
#37750 closed defect (bug) (fixed)
Cropping custom logo should preserve attachment properties
| Reported by: | flixos90 | Owned by: | joedolson |
|---|---|---|---|
| Priority: | normal | Milestone: | 6.0 |
| Component: | Media | Version: | 4.5 |
| Severity: | normal | Keywords: | has-patch has-unit-tests has-screenshots has-test-info commit |
| Cc: | Focuses: | accessibility |
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 (40)
#2
@
10 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.
This ticket was mentioned in Slack in #core by sami.keijonen. View the logs.
9 years ago
This ticket was mentioned in Slack in #accessibility by afercia. View the logs.
9 years ago
This ticket was mentioned in Slack in #accessibility by afercia. View the logs.
9 years ago
This ticket was mentioned in Slack in #core-media by afercia. View the logs.
9 years ago
This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.
9 years ago
This ticket was mentioned in Slack in #accessibility by afercia. View the logs.
9 years ago
This ticket was mentioned in Slack in #core by melchoyce. View the logs.
9 years ago
This ticket was mentioned in Slack in #core-media by antpb. View the logs.
8 years ago
#21
@
7 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.
This ticket was mentioned in Slack in #core-media by boniu91. View the logs.
5 years ago
#24
@
4 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.
4 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
@
4 years ago
Steps to test this PR:
- Activate a non-block theme like
Twenty Twenty-One. - Go to the
Media Library. - Upload a new image to the media library.
- Set attachment properties like
Alternative Text,Title,Caption,Description. - Go to the
Appearance -> Customizepage. - Go to the
Site Identity, click on theSelect Logo. - Set a new cropped logo for the theme using the image you uploaded.
- Go to the
Media Library. - Open the last (cropped) image.
- Make sure that the original attachment properties (i.e.
Alternative Text,Title,Caption,Description) have been copied over to the cropped image. - Repeat steps 2-3.
- Reset all attachment properties (leave the fields empty).
- Repeat steps 5-9.
- Make sure that the
Titlefield contains name of the cropped file. - Make sure that the
Descriptionfield contains url of the cropped file. Alternative TextandCaptionproperties have to be empty.
#27
@
4 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
- Activate a non-block theme like Twenty Twenty-One.
- Navigate to Media > Library.
- Upload a new image to the media library, at least 512x512 pixels in size.
- Set attachment properties for Alternative Text, Title, Caption, and Description.
- Go to the Appearance > Customize.
- Select Site Identity, and under the Logo section, click "Select Logo".
- Select the image just uploaded.
- Click the "Select" button to open the crop tool.
- Select the region to crop, and click the "Crop image" button.
- Click "Publish"* and close the Customizer.
- Navigate to Media > Library.
- Select the latest image added (the cropped logo).
- Observe that the Alt Text, Title, Caption, and Description previously set are populated in the Attachment Details sidebar. ✅
- Repeat steps 2-3.
- Reset all attachment properties (clear all fields).
- Go to the Appearance > Customize.
- Select Site Identity, and under the Logo section, click "Change Logo".
- Repeat steps 7-12.
- 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
Original uploaded image (Step 3).
Cropped logo image retains attributes (Step 13).
Image with cleared attributes (Step 15).
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.
Crop for Site Icon does not retain attributes from original image (Additional Notes).
#28
@
4 years ago
Thank you so much for the test reports, @ironprogrammer!
I appreciate it.
crops to the Site Icon do not retain those attributes
IMO it's better to implement this fix in the scope of another trac issue as it will require implementing new unit tests. Thank you for reporting.
#29
follow-up:
↓ 30
@
4 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.
#30
in reply to: ↑ 29
;
follow-up:
↓ 33
@
4 years ago
Replying to antonvlasenko:
Just FYI: this PR has some breaking changes.
Currently, WordPress always overwritesTitleandDescriptionproperties (withnameandurlof the image file respectively) when we crop an image.
If this PR gets merged, these properties will only be populated (overwritten) ifTitleandDescriptionproperties 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
TitleandDescriptionseem 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
@
4 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
- Activate Twenty Twenty or any other non-block theme.
- Go to Media > Library.
- Upload at least one image. (In my case, I uploaded 4 different images with different metadata to pre-populate the properties.)
- For certain tests, set the attachment properties for alt test, title, caption, and/or description.
- Go to the Appearance > Customize > Site Identity.
- In the Logo section, click "Select Logo".
- Select an image.
- Click the "Select" button. Expected behavior: Crop modal opens.
- Select a region to crop.
- Click the "Crop image" button.
- Click "Publish"* and close the Customizer.
- Go to Media > Library.
- 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).
- Is
- 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.
- Title:
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
@
4 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
@
4 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
TitleandDescription? 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.
4 years ago
#35
@
4 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
@
4 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.
This ticket was mentioned in Slack in #core-media by joedolson. View the logs.
4 years ago
hellofromtonya commented on PR #2441:
4 years ago
#39
Committed via changeset https://core.trac.wordpress.org/changeset/53027.
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 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?