Make WordPress Core

Opened 7 years ago

Closed 7 years ago

#38768 closed defect (bug) (fixed)

Custom logo is missing meaningful alt attribute

Reported by: samikeijonen's profile sami.keijonen Owned by: samikeijonen's profile sami.keijonen
Milestone: 4.8 Priority: normal
Severity: normal Version:
Component: Themes Keywords: has-patch commit has-unit-tests
Focuses: accessibility Cc:

Description

In 38812 default alt fallbacks was removed to improve accessibility.

Therefore custom logo now have empty alt attribute. Should it have meaningful alt attribute by default for better accessibility?

There is background info in Slack archive.

Attachments (7)

38768.diff (920 bytes) - added by sstoqnov 7 years ago.
Add logo alt attribute
38768.2.diff (903 bytes) - added by afercia 7 years ago.
38768.3.diff (1.9 KB) - added by afercia 7 years ago.
Update related test.
38768.4.diff (2.4 KB) - added by flixos90 7 years ago.
38768.5.diff (2.4 KB) - added by flixos90 7 years ago.
38768.6.diff (3.5 KB) - added by flixos90 7 years ago.
38768.7.diff (3.6 KB) - added by afercia 7 years ago.

Download all attachments as: .zip

Change History (37)

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


7 years ago

#2 follow-up: @joedolson
7 years ago

I'm inclined to think that the ideal default alt attribute for this case would be the site title. That's likely to be appropriate in most cases - even if it's not a formal match for the text on the alt attribute, it would probably convey the same general information.

#3 @sami.keijonen
7 years ago

Agreed that site title would be nice default alt.

Did some more testing.

  1. Go to the Customizer >> Site Identity.
  2. Click Select logo.
  3. Pick image and write alt text.
  4. Click Select.
  5. Crop image.
  6. Save and Publish.
  7. Cropped image doesn't inherit the alt text that I just wrote in step 3.
  8. If I go back to Media library and add alt text to cropped image, it does show in up in custom logo.

This is probably how it should work but step 7. still feels weird to me. I'd expect that alt text would be in logo image because I just wrote it.

I was testing in Twenty Seventeen.

Also note that if I use image that I don't have to crop, alt text is naturally there.

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


7 years ago

#5 @afercia
7 years ago

  • Milestone changed from Awaiting Review to Future Release
  • Type changed from enhancement to defect (bug)

#6 @afercia
7 years ago

  • Milestone changed from Future Release to 4.8

In 4.6. the alt attribute is just the image filename, and should be improved to have a meaningful value. In 4.7 is going to be an empty alt attribute because the fallback is going to be removed, see [38812]. Either ways, this should be fixed, hopefully during 4.8.

@sstoqnov
7 years ago

Add logo alt attribute

#7 @sstoqnov
7 years ago

  • Keywords has-patch needs-testing added

I agree with you that the site title is a meaningful value for alt attribute, but I think that the user should have control over that value. That's why I think it's better to check if the image has an alt attribute first and then we can add the site title if it's empty.
What do you think? @afercia @sami.keijonen

#8 @nobremarcos
7 years ago

Just tested @sstoqnov path and as he describes it right now there is no checking if the user has added an "Alt Text" to the logo image and it applies the site title to the alt even if the user entered a custom "Alt Text" to it.

#9 @gma992
7 years ago

@nobremarcos

The check in the patch is here:

		if ( empty( $image_alt ) ) {
			$image_alt = get_bloginfo( 'name' );
		}

I've just tested it and works fine when adding a new logo.

If the alt field is empty, is populated automatically with the site title, otherwise is populated with the custom alt text added into the field. Mind to clarify the steps to reproduce why is not working on your end? Maybe I misunderstood the issue you're seeing?

On the other hand, as @sami.keijonen pointed out, when we crop the image, it does not grab the customized alt text, but populates it by an empty alt text by default or with the site title after being patched.

This is the normal behavior thought (but not the expected one) as when we crop the logo, we're creating a new image on the media gallery, and that new image has no alt text yet.

#10 in reply to: ↑ 2 @LiamMcArthur
7 years ago

I agree. I personally think it's overkill to have another input box for the user to specify a custom alt tag for their logo. I think the site title (or even tagline, if a site title isn't set) should suffice.

Replying to joedolson:

I'm inclined to think that the ideal default alt attribute for this case would be the site title. That's likely to be appropriate in most cases - even if it's not a formal match for the text on the alt attribute, it would probably convey the same general information.

#11 @jjcomack
7 years ago

  • Keywords needs-testing removed

Tested this patch by going through the following steps:

  1. Uploaded an image to use as a logo (700px x 700px) and applied an alt-tag.
  2. Was prompted to crop said image, which I did, resulting in the alt-tag changing to the site title.
  3. Uploaded a different image (250px x 250px) and applied an alt-tag.
  4. Was given the option to skip cropping, which I did. Original alt-tag remains.

Clearly the issue where the alt-tag gets removed post-cropping (and thus replaced thanks to this patch) needs to be addressed as this leads to inconsistent behavior in the user experience. Will probably be best to create a new issue for this.

#12 @sstoqnov
7 years ago

@jjcomack Thanks for testing.
I agree with you that the cropped image should keep the main image attributes, because rarely you will have cropped images with different alt attributes than the main image. If you still need different attribute, you can change it from editor.

Actually there is a ticket for this issue already and I will try to provide a patch when I have some more free time #37750

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


7 years ago

@afercia
7 years ago

#14 @afercia
7 years ago

Refreshed patch, escapes the alt attribute and makes it filterable. Since the issue with cropped images is already tracked in #37750, I'd say this could go. Some final testing would be nice.

Last edited 7 years ago by afercia (previous) (diff)

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


7 years ago

#16 @afercia
7 years ago

Haven't had the chance to check what happens on multisite, some testing would be nice :)

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


7 years ago

#18 @afercia
7 years ago

  • Owner set to sami.keijonen
  • Status changed from new to assigned

#19 @sami.keijonen
7 years ago

I tested the patch also in multisite and default alt is the site title (for sub site and main site) as it should.

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


7 years ago

#21 @afercia
7 years ago

  • Keywords needs-refresh needs-unit-tests added

Related test Tests_General_Template::test_the_custom_logo needs to be updated.

@afercia
7 years ago

Update related test.

#22 @afercia
7 years ago

  • Keywords needs-refresh needs-unit-tests removed

Refreshed patch to update a related test.

@flixos90
7 years ago

#23 @flixos90
7 years ago

  • Keywords commit added

38768.4.diff adjusts the previous test fix, so that _set_custom_logo() is called before accessing the logo's alt text, to make sure it is set prior. It furthermore adds a similar fix to another multisite-specific test that was failing before.

This should be good to go.

@flixos90
7 years ago

#24 @flixos90
7 years ago

38768.5.diff removes the esc_attr() calls, as the attributes should be escaped late as it happens in wp_get_attachment_image().

@flixos90
7 years ago

#25 @flixos90
7 years ago

  • Keywords has-unit-tests added

38768.6.diff makes the new code in get_custom_logo() and its unit tests more precise, by only passing the alt attribute if it is the site name. This ensures the trim( strip_tags( ... ) ) logic in wp_get_attachment_image() still fires correctly for the original alt attribute.

Furthermore it adds a new unit test for a custom logo that has an alt text provided already, to provide better coverage.

I added commit a bit early, but now I actually think it's ready. :)

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


7 years ago

@afercia
7 years ago

#27 follow-up: @afercia
7 years ago

38768.7.diff clarifies a bit the inline comments and cleans-up a test. However, this doesn't work well when updating and already existing custom logo in the customizer, i.e. press "chance logo", select the same image, update the alt attribute: the image get inserted with the previous alt attribute unchanged. /cc @westonruter

#28 in reply to: ↑ 27 @westonruter
7 years ago

Replying to afercia:

However, this doesn't work well when updating and already existing custom logo in the customizer, i.e. press "chance logo", select the same image, update the alt attribute: the image get inserted with the previous alt attribute unchanged.

This would be expected because the control only re-renders the image when the setting is changed. The setting just contains the attachment ID, so if you select the same image then there is no detected change, as the attachment props aren't being listened to for changes. One way to go about that would be to manually trigger a setting change event when the media library is closed after a selection has been made.

But the problem is deeper than that actually: the media control doesn't ever print out values for the alt attributes. See https://github.com/WordPress/wordpress-develop/blob/5ea3c91d57c17fdadbd0ef910b65c4ff8891ff87/src/wp-includes/customize/class-wp-customize-media-control.php#L160-L187

So a couple things would need to change here, out of scope from improving the Custom Logo image's alt on the frontend.

#29 @flixos90
7 years ago

So a couple things would need to change here, out of scope from improving the Custom Logo image's alt on the frontend.

I agree with @westonruter. I think this ticket can move forward without the Customizer handling it perfectly, this can happen in a separate ticket (although it's probably going to be rather low-priority).

#30 @afercia
7 years ago

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

In 40817:

Themes: Improve the theme Custom Logo accessibility.

Uses the Site title as fallback value for the Custom Logo alt attribute when the original alt attribute is empty.

Props sami.keijonen, joedolson, sstoqnov, nobremarcos, gma992, LiamMcArthur, jjcomack.
Fixes #38768.

Note: See TracTickets for help on using tickets.