Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(gatsby-source-wordpress): Set empty default alt tag for inline images #38341

Merged
merged 2 commits into from
Jul 6, 2023

Conversation

LekoArts
Copy link
Contributor

@LekoArts LekoArts commented Jul 5, 2023

Description

WordPress seems to set undefined for the alt tag of an image if it's empty. gatsby-plugin-image requires an alt tag so this PR uses nullish coalescing to set an empty string as a default.

Tests

No tests added

Related Issues

Fixes #38259

@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Jul 5, 2023
@LekoArts LekoArts added topic: source-wordpress Related to Gatsby's integration with WordPress topic: source-plugins Relates to the Gatsby source plugins (e.g. -filesystem) and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels Jul 5, 2023
@LekoArts LekoArts requested a review from TylerBarnes July 5, 2023 06:14
Copy link
Contributor

@TylerBarnes TylerBarnes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Lennart! 🙌

@TylerBarnes TylerBarnes marked this pull request as ready for review July 5, 2023 14:26
@TylerBarnes TylerBarnes merged commit e5126f9 into master Jul 6, 2023
@TylerBarnes TylerBarnes deleted the wordpress-nullish-alt branch July 6, 2023 02:48
@therealgilles
Copy link

therealgilles commented Jul 6, 2023

Thank you for the fix!!

To satisfy my own curiosity, how does gatsby-source-wordpress get the alt image prop when querying images from WordPress? I know GraphQL exposes the prop as altText but I could not find any assignment from altText to alt anywhere (maybe that's another bug...?).

@pieh
Copy link
Contributor

pieh commented Jul 6, 2023

@therealgilles I think altText is provided by wp-graphql ( https://github.com/wp-graphql/wp-graphql/blob/b0d78131d63424501d19f5ffda61cb93a3595dd1/src/Model/Post.php#L809-L810 ) and that field is preserved as-is when querying for media items via Gatsby's Graphql

This PR was addressing handling of images embeded in html content (for example in a post main content) to create responsive variants of it, so while both are dealing with images - those are 2 different/separate things. We do get content as html "snippet" and we parse that html to find images, so that's why we deal with actual html attributes (alt) here and not the graphql field name.

@therealgilles
Copy link

@pieh Oh interesting! The warning seemed to occur from a MediaItem query so I assumed it was from wp-graphql. If you are parsing HTML directly, then all the more reasons not to assume that the alt prop won't necessarily be there. And I confirm that WP does not bother adding the tag if empty. A WP bug I guess.

Declaring the prop as mandatory is a bit of joke though, considering how HTML has been working for years, i.e. images still display without it. The official directive that it has to be there but can be empty is non-sensical in my book. I'm all for accessibility but it has to start with the tools.

Anyway, thanks again for fixing this and for taking the time to explain.

@pieh
Copy link
Contributor

pieh commented Jul 10, 2023

I don't know all the details on how things are connected here in this change as wordpress is not really my area in Gatsby context. I just tried doing few quick greps/code searches for altText to maybe provide some explanations (as shallow as they were). I noticed cheerioImg naming which I remembered being used to parse html snippets (https://www.npmjs.com/package/cheerio) as same library is used for finding <img> tags in our markdown handling + some (probably outdated) information about wordpress that I had from the past. You very well may be on to something, but I can't currently dive any deeper into the topic

@therealgilles
Copy link

@pieh I appreciate the honesty and it all sounds logical. I'll finish with it'd be helpful if Gatsby (really gatsby-source-wordpress) was a bit more verbose about what it does internally. Something like Parsing ... html to get image data for ... would probably have helped root cause the issue a bit quicker. Thanks for all the work :)

This was referenced Sep 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: source-plugins Relates to the Gatsby source plugins (e.g. -filesystem) topic: source-wordpress Related to Gatsby's integration with WordPress
Projects
None yet
4 participants