Make WordPress Core

Opened 3 weeks ago

Closed 3 weeks ago

Last modified 3 weeks ago

#62246 closed defect (bug) (fixed)

wp_print_font_faces() prints an HTML tag with an id property despite being able to be used more than once.

Reported by: mmaattiiaass's profile mmaattiiaass Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 6.7 Priority: normal
Severity: normal Version: trunk
Component: Editor Keywords: has-patch has-unit-tests commit
Focuses: Cc:

Description

wp_print_font_faces() prints an HTML tag with an id property despite being able to be used more than once, so the document can end up with multiple tags with the same id attribute

<style id='wp-fonts-local'></style>

The id global attribute defines an identifier (ID) which must be unique in the whole document. Reference: https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/id

Change History (7)

#1 @mmaattiiaass
3 weeks ago

We could probably use class instead of ID as a solution.I'm wondering about retrocompatibility. There's nothing in core or Gutenberg depending on this, but an extender could have added some functionality referencing this ID. Is this type of attribute ok to change? Still if it's not changed WordPress would be rendering invalid HTML so I think it needs to be fixed.

This ticket was mentioned in PR #7583 on WordPress/wordpress-develop by @mmaattiiaass.


3 weeks ago
#2

  • Keywords has-patch has-unit-tests added

Replace id by class in the wp-fonts-local style tag printer by wp_print_font_faces()

#3 @peterwilsoncc
3 weeks ago

  • Component changed from General to Editor
  • Milestone changed from Awaiting Review to 6.7

@mmaattiiaass I think it's fine to change. Although it's a technical back-compat break the selector is currently broken so there isn't much that extenders can do with it at the moment. It could do with a paragraph in the miscellaneous changes dev-note

@mmaattiiaass commented on PR #7583:


3 weeks ago
#4

Thanks for the review, @peterwilsoncc!

Having a number doesn't seem to be very useful for extenders. The tags can be selected by the _arbitrary_ index very easily with any script.

A simple JS example:

const fontsTags = document.querySelectorAll('style.wp-fonts-local');
const firstTag  = styleTags[0];
const secondTag = styleTags[1];

As the id suggested only consists of an arbitrary index that the extenders can't customize (for example, to identify a particular list of fonts rendered in the page), I think that printing the id attribute is not necessary because it doesn't seem to add extra value.

#5 @peterwilsoncc
3 weeks ago

  • Keywords commit added

#6 @SergeyBiryukov
3 weeks ago

  • Owner set to SergeyBiryukov
  • Resolution set to fixed
  • Status changed from new to closed

In 59259:

Editor: Replace id attribute with class in WP_Font_Face::get_style_element().

wp_print_font_faces() prints an HTML tag that can be used more than once, so the document could end up having multiple tags with the same id attribute.

The id global attribute defines an identifier (ID) which must be unique in the whole document.

Reference: MDN Web Docs: id.

Follow-up to [56500].

Props mmaattiiaass, peterwilsoncc.
Fixes #62246.

@SergeyBiryukov commented on PR #7583:


3 weeks ago
#7

Thanks for the PR! Merged in r59259.

Note: See TracTickets for help on using tickets.