Make WordPress Core

Opened 4 years ago

Last modified 7 months ago

#52990 new defect (bug)

Account for single quoted attributes when lazy loading images and iframes.

Reported by: peterwilsoncc's profile 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.

Follow up to #50756, #52768.

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

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]

pyronaur commented on PR #1358:


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?

ocean90 commented on PR #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

pyronaur commented on PR #1358:


3 years ago
#5

Thanks @ocean90

Curious - there are tests that assert that single quotes should fail 🤔 .

#6 follow-up: @pyronaur
3 years ago

https://github.com/WordPress/wordpress-develop/blob/260f13ab5795edce707cfd84570e021954dc0e7d/tests/phpunit/tests/media.php#L2882-L2891

The tests explicitly mention testing for regressions, so this might need some input from @azaozz and @flixos90

#7 in reply to: ↑ 6 @azaozz
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 an inline <script> (and the double quotes broke the js). This seems to be very uncommon, but given the size of WordPress...

Last edited 3 years ago by azaozz (previous) (diff)

#8 @azaozz
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.

Last edited 3 years ago by azaozz (previous) (diff)
Note: See TracTickets for help on using tickets.