Opened 4 years ago
Last modified 6 months ago
#52990 new defect (bug)
Account for single quoted attributes when lazy loading images and iframes.
Reported by: | peterwilsoncc | Owned by: | |
---|---|---|---|
Milestone: | Awaiting Review | Priority: | normal |
Severity: | normal | Version: | 5.5 |
Component: | Embeds | Keywords: | needs-unit-tests needs-patch |
Focuses: | performance | Cc: |
Description
wp_iframe_tag_add_loading_attr()
and wp_img_tag_add_loading_attr()
includes a check to ensure the elements have a src
, width
and height
attribute before adding the lazy loading attribute.
In both functions the check only considers attributes using double quotes so the lazy loading tag is not added for elements using single quoted attributes. img ref, iframe ref.
As WordPress does not normalize attribute tags to use double quotes, single quoted attribute tags ought to be considered.
Image version 5.5, iframe version 5.7.
Change History (8)
This ticket was mentioned in PR #1358 on WordPress/wordpress-develop by pyronaur.
3 years ago
#1
- Keywords has-patch added; needs-patch removed
3 years ago
#2
Looked at the broken unit tests - it seems that they might be broken at a repository level and nothing to do with the patch?
#3
@
3 years ago
Suggested a patch via GitHub: https://github.com/WordPress/wordpress-develop/pull/1358
3 years ago
#4
Hi @pyronaur, there are actually two failing tests. Here's the direct link: https://github.com/WordPress/wordpress-develop/pull/1358/checks?check_run_id=2782515804#step:20:274
3 years ago
#5
Thanks @ocean90
Curious - there are tests that assert that single quotes should fail 🤔 .
#6
follow-up:
↓ 7
@
3 years ago
The tests explicitly mention testing for regressions, so this might need some input from @azaozz and @flixos90
#7
in reply to:
↑ 6
@
3 years ago
Replying to pyronaur:
Right, this comes from https://core.trac.wordpress.org/ticket/44427#comment:100 edge case where a loading
attribute was added to img tag that's inside js (and broke the js). This seems to be very uncommon, but given the size of WordPress...
#8
@
3 years ago
- Keywords needs-patch added; has-patch removed
The initial code in that conditional was without the double quote, false === strpos( $image, ' src=' )
. That made it compatible with all attribute "styles" (double, single and no quotes). However that broke some inline js, see above comment.
There was a patch to support single quotes, see https://core.trac.wordpress.org/attachment/ticket/44427/44427.10.diff (scroll to the middle, ~line 1715 in the patched src/wp-includes/media.php
). It can be adapted to implement that support now.
On the other hand the WordPress editor (both the block editor and TinyMCE) normalize HTML attributes to always use double quotes. The "template functions" also output HTML with double quotes.
Yes, the conditional where the quote is checked can have false positives. Its purpose is to bypass adding of the loading
attribute, i.e. if src
, width
, or height
are missing or in case of a false positive it falls back to the "safe" option of not changing the tag.
At the time #44427 was implemented it seemed that the slowdown of getting the quote style for each image would have larger negative impact than not adding the loading
attribute when the tags attributes are with single quotes or no quotes.
Now that iframes are also lazy-loaded it may make sense to look for single quotes and take the slowdown. Perhaps that can be only for iframes as the overall improvements there are quite more significant, and iframes are quite rare in post_content compared to images.
Trac ticket: https://core.trac.wordpress.org/ticket/52990
This PR accounts for attributes using single quotes in images and iframes to address the issue described in [ https://core.trac.wordpress.org/ticket/52990 Trac:52990]