WordPress.org

Make WordPress Core

Opened 3 years ago

Closed 3 months ago

#37305 closed defect (bug) (fixed)

Custom Logo: output invalid according to the W3C

Reported by: henry.wright Owned by: SergeyBiryukov
Milestone: 5.2 Priority: normal
Severity: normal Version: 4.5
Component: Themes Keywords: has-patch commit
Focuses: Cc:

Description

The W3C Validator throws an error when the_custom_logo() is used without specifying an itemtype:

The itemprop attribute was specified, but the element is not a property of any item.

Should itemprop="url" be removed from get_custom_logo()?

Attachments (4)

37305.diff (732 bytes) - added by henry.wright 3 years ago.
37305.1.patch (644 bytes) - added by tfrommen 19 months ago.
37305.2.patch (900 bytes) - added by tfrommen 19 months ago.
Include yet another occurrence
37305.2.diff (2.8 KB) - added by afercia 3 months ago.

Download all attachments as: .zip

Change History (21)

#1 @swissspidy
3 years ago

  • Component changed from General to Themes
  • Summary changed from the_custom_logo() output invalid according to the W3C to Custom Logo: output invalid according to the W3C

#2 @henry.wright
3 years ago

If removing the property isn't an option, perhaps we could recommend theme authors use something like itemscope itemtype="http://schema.org/WebPage" on the <body> element.

#3 @henry.wright
3 years ago

Alternatively, and I think this is the most appropriate solution, we could wrap the logo markup:

<div itemscope itemtype="http://schema.org/Brand">
    <!-- Logo markup here -->
</div>

#4 @karmatosed
3 years ago

I'm not super keen we force or ask theme authors to put anything extra in their body element. If this is needing to be fixed, lets do this in the function once over every theme.

#5 @henry.wright
3 years ago

@karmatosed in that case the fix would be wrap the property with an itemtype or we could simply remove the microdata completely. Which would be preferred?

#6 @henry.wright
3 years ago

The incoming patch wraps the property with the necessary microdata markup. Custom logo markup should now pass the W3C Validator.

@henry.wright
3 years ago

#7 @henry.wright
3 years ago

  • Keywords has-patch added

#8 @ocean90
3 years ago

  • Version set to 4.5

#9 @markcallen
3 years ago

I think making assumptions about the output is inappropriate. There are circumstances where

  • itemptype "Brand" is inappropriate. It may be marking up for "Organization" for example.
  • It is inappropriate to wrap it in anything. The rest of the schema for a type may be included already - this is afterall, just one part of the "Brand" type.

It would be more appropriate to remove the itemprop attributes from the link and img tag and provide a facility for developers to add them (filters and/or function args).

In any event, the get_custom_logo filter that is in place at present provides the ability to wrap the output of the function in the "Brand" (or anything else) markup if that is appropriate for a theme developer.

#10 @henry.wright
3 years ago

@markcallen removing the microdata completely would indeed solve the invalid markup error.

@tfrommen
19 months ago

#11 follow-up: @tfrommen
19 months ago

What needs to be done to get this fixed for core, @ocean90 and @swissspidy?

I just uploaded a new patch, refreshed for current trunk, and with just the removal of the itemprop attribute, as suggested and what makes the most sense to me.

Quite interesting that none of the SEO-affine people find this troublesome...

#12 in reply to: ↑ 11 ; follow-up: @zodiac1978
19 months ago

Replying to tfrommen:

I just uploaded a new patch, refreshed for current trunk, and with just the removal of the itemprop attribute, as suggested and what makes the most sense to me.

Hi Thorsten, there is another itemprop for the logo here:
https://github.com/WordPress/WordPress/blob/aaf99e691391cfceb004d848450dbbf3344b1bee/wp-includes/general-template.php#L897

Replying to markcallen:

It would be more appropriate to remove the itemprop attributes from the link and img tag and provide a facility for developers to add them (filters and/or function args).

I think this would be the best way. If you are a theme developer and you know that you have added itemscope itemtype="http://schema.org/WebPage" to the html element for example, you can use the filter or the parameter to add this feature. But the default should be to not add this attribute.

@tfrommen
19 months ago

Include yet another occurrence

#13 in reply to: ↑ 12 @tfrommen
19 months ago

Replying to zodiac1978:

Replying to tfrommen:

I just uploaded a new patch, refreshed for current trunk, and with just the removal of the itemprop attribute, as suggested and what makes the most sense to me.

Hi Thorsten, there is another itemprop for the logo here:
https://github.com/WordPress/WordPress/blob/aaf99e691391cfceb004d848450dbbf3344b1bee/wp-includes/general-template.php#L897

Ah, good catch! I just took the original patch as example, and didn't see this somewhat hidden second definition. :)

Updated patch uploaded.

#14 @SergeyBiryukov
4 months ago

  • Milestone changed from Awaiting Review to 5.2
  • Owner set to SergeyBiryukov
  • Status changed from new to reviewing

#15 @afercia
3 months ago

Asked to a few SEO experts and the argumentations from @markcallen and @zodiac1978 seem the most appropriate. Core should not make assumption on the output. Also, core is far from supporting schema so this seems more plugins / themes territory and should be removed. Noting also that today, JSON-LD is preferred.
+1 for removal.

@afercia
3 months ago

#16 @afercia
3 months ago

  • Keywords commit added

37305.2.diff updates the patch and related tests.

#17 @SergeyBiryukov
3 months ago

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

In 45028:

Themes: Remove itemprop="url" from get_custom_logo() output.

Making incorrect assumptions about microdata used in a theme results in invalid markup.

The attribute can still be added using the get_custom_logo or wp_get_attachment_image_attributes filter, but it should not be added by default.

Props henry.wright, tfrommen, afercia, markcallen, zodiac1978.
Fixes #37305.

Note: See TracTickets for help on using tickets.