#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 | Owned by: | 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)
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
@
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.
#6
@
3 weeks ago
- Owner set to SergeyBiryukov
- Resolution set to fixed
- Status changed from new to closed
In 59259:
@SergeyBiryukov commented on PR #7583:
3 weeks ago
#7
Thanks for the PR! Merged in r59259.
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.