Make WordPress Core

Opened 16 months ago

Closed 16 months ago

Last modified 16 months ago

#56969 closed defect (bug) (fixed)

decoding="async" breaks my site

Reported by: rodricus's profile rodricus Owned by: adamsilverstein's profile adamsilverstein
Milestone: 6.1.1 Priority: normal
Severity: normal Version: 6.1
Component: Media Keywords: reporter-feedback has-patch has-unit-tests dev-feedback commit fixed-major
Focuses: performance Cc:

Description

Like the title says: decoding="async" attribute in my img elements breaks my site. WordPress 6.1 brings this attribute to my img elements. It generates a javascript error: Uncaught SyntaxError: Unexpected token 'async'

The javascript functionality breaks on my website. This needs to be fixed as soon as possible. You can see the problem here: https://bouma.diepcreative.nl/

Attachments (2)

56969.diff (1.8 KB) - added by adamsilverstein 16 months ago.
56969.2.diff (1.7 KB) - added by adamsilverstein 16 months ago.

Download all attachments as: .zip

Change History (38)

#1 @rodricus
16 months ago

The problem was residing in my Slick Carousel initialisation:

$('.section-service-slider .services').slick({

autoplay: false,
speed: 500,
infinite: true,
slidesToShow: 6,
slidesToScroll: 1,
prevArrow: "<button type='button' class='navigation-arrow slick-prev'><img decoding='async' src='" + arrow_url + "' /></button>",
nextArrow: "<button type='button' class='navigation-arrow slick-next'><img decoding='async' src='" + arrow_url + "' /></button>",
dots: false,
appendDots: false,
accessibility: false,
mobileFirst: false,

WordPress somehow injected " decoding='async' " in the img element. If you change that to explicitly " decoding='sync' ", then the javascript-errors are resolved and the site-page works again.

Version 0, edited 16 months ago by rodricus (next)

#2 @TimothyBlynJacobs
16 months ago

Hi @rodricus,

Thanks for the ticket!

Is that JavaScript code in your post content? Or is it coming from somewhere else like your theme or a widget?

#3 @TimothyBlynJacobs
16 months ago

  • Component changed from General to Media
  • Focuses performance added
  • Keywords reporter-feedback added
  • Severity changed from critical to normal

#4 @rodricus
16 months ago

Hello @TimothyBlynJacobs, sorry for being unclear. Yes that's javascript-code. It's the initialisation code for the Slick Slide plugin.

#5 @TimothyBlynJacobs
16 months ago

Thanks @rodricus. What I mean, is are you inserting that JavaScript code into one of your posts or pages? As in the actual content of the page with perhaps the HTML Block?

#6 @joelmadigan
16 months ago

I'd like to chime in with an instance of the same issue in a slightly different context.

In my case, NinjaForms apparently uses JSON+JavaScript to load their forms and the method used by WordPress to inject the decode="async" attribute doesn't take the image possibly being in a double-quoted string into account.

{
  "objectType": "Field",
  "objectDomain": "fields",
  "editActive": false,
  "order": 14,
  "idAttribute": "id",
  "label": "HTML",
  "type": "html",
  "default": "<p><img decoding="async " style=\"width: 604px;\" src=\"https:\/\/www.lewislandscape.com\/wp-content\/uploads\/2022\/05\/CheckImage.png\"><br><\/p>",
  "container_class": "",
  "element_class": "",
  "key": "html_1652099495998",
  "drawerDisabled": false,
  "id": 28,
  "beforeField": "",
  "afterField": "",
  "value": "<p><img decoding="async " style=\"width: 604px;\" src=\"https:\/\/www.lewislandscape.com\/wp-content\/uploads\/2022\/05\/CheckImage.png\"><br><\/p>",
  "label_pos": "above",
  "parentType": "html",
  "element_templates": ["html", "input"],
  "old_classname": "",
  "wrap_template": "wrap"
},

As we can see in this snippet, "default" and "value" have the string terminated early causing the JavaScript parser to abend with Unexpected Keyword: async.

I fixed it with

<?php
add_filter('wp_img_tag_add_decoding_attr', '__return_false');

in theme functions.php, but it's not ideal.

I suspect this could be fixed by escaping the double quotes (for the lazy loading injection too), which shouldn't bother the HTML parser.

#7 @rodricus
16 months ago

@TimothyBlynJacobs Yes that's true. The javascript is loaded in a html section element after the HTML-code itself. The section is then loaded in the page if a user selects that block in the editor (the section is an ACF Gutenberg-block).

@joelmadigan Thank you for your information.

#8 @mw108
16 months ago

Same issue here as @joelmadigan - This new feature breaks NinjaForms when you have img tags in your fields.

The "magic" happens in wp-includes/media.php in function wp_filter_content_tags( ... ):

Forcing the decoding="async" attribute by using preg_match_all into all img tags is a bad solution. This has been discussed many times on StackOverflow: You should not parse HTML using regular expressions. Parse the DOM properly.

#9 @TimothyBlynJacobs
16 months ago

  • Milestone changed from Awaiting Review to 6.1.1

Moving to 6.1.1 for visibility.

#10 follow-ups: @adamsilverstein
16 months ago

I will take a look at this one to see how we can best avoid breaking image tags inside scripts or json. I would expect the lazy loading attribute to cause the same issue, is that not the case @rodricus?

#11 @adamsilverstein
16 months ago

  • Owner set to adamsilverstein
  • Status changed from new to assigned

#12 in reply to: ↑ 10 @joelmadigan
16 months ago

Replying to adamsilverstein:

I will take a look at this one to see how we can best avoid breaking image tags inside scripts or json. I would expect the lazy loading attribute to cause the same issue, is that not the case @rodricus?

They are implemented in the same way, so they absolutely can cause the same issue. The lazy loading part, however, has a limiting factor that it also checks for width & height attributes so it may not always affect the same images that the decoding one will.

#13 @flixos90
16 months ago

@adamsilverstein I wonder if we can find a reliable way to ignore img elements that appear in a script.

#14 @adamsilverstein
16 months ago

They are implemented in the same way, so they absolutely can cause the same issue. The lazy loading part, however, has a limiting factor that it also checks for width & height attributes so it may not always affect the same images that the decoding one will.

Ah, good point.

@adamsilverstein I wonder if we can find a reliable way to ignore img elements that appear in a script.

Yes, that was the direction I was thinking of heading. I'll give that a try to start.

#15 in reply to: ↑ 10 @rodricus
16 months ago

Replying to adamsilverstein:

I will take a look at this one to see how we can best avoid breaking image tags inside scripts or json. I would expect the lazy loading attribute to cause the same issue, is that not the case @rodricus?

Regarding the website I mentioned earlier, lazy loading attribute did not do any harm, only decoding="async".

#16 @flixos90
16 months ago

After further investigation with @adamsilverstein, we found what is probably the most straightforward and reliable fix here:

  • The lazy-loading implementation is slightly different most importantly in the sense that it also checks for src=", which means it'll only touch img tags that use regular double quotes, i.e. no single quotes or escaped double quotes.
  • This ensures it doesn't break any JS code where e.g. img tags may use other single quotes or escaped quotes.

So the best fix for this bug would probably be to add a similar check before adding any decoding attribute: If there is no src=" in the img tag, don't add the attribute. (If there is no src attribute, it's probably some special case that core shouldn't handle anyway.)

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


16 months ago
#17

  • Keywords has-patch has-unit-tests added

#18 @adamsilverstein
16 months ago

  • Keywords dev-feedback added

I'd like to chime in with an instance of the same issue in a slightly different context.

This new feature breaks NinjaForms when you have img tags in your fields.

Thanks @joelmadigan & @mw108 this helped me get to:

### Steps to reproduce ###

  1. Install Ninja Forms
  2. Create a form with single HTML field, inserting an image in the Default Value field
  3. Insert the form on a page and view

So the best fix for this bug would probably be to add a similar check before adding any decoding attribute: If there is no src=" in the img tag, don't add the attribute. (If there is no src attribute, it's probably some special case that core shouldn't handle anyway.)

I did just this in https://core.trac.wordpress.org/ticket/56969

Also added some tests to cover the JSON case and the single quote case, both of which fail before the patch.

#19 @adamsilverstein
16 months ago

56969.diff adds the fix discussed above along with tests, mirroring changes in the PR.

#20 follow-up: @adamsilverstein
16 months ago

@rodricus @mw108 @rodricus - can you please give the uploaded patch a test to verify it corrects the issue in your cases?

#21 @flixos90
16 months ago

  • Keywords commit added

I reviewed the PR, this looks good to go from my end. Just two quick nit-picks to address prior to commit.

#22 in reply to: ↑ 20 @mw108
16 months ago

Replying to adamsilverstein:

... - can you please give the uploaded patch a test to verify it corrects the issue in your cases?

Tested it and can confirm it works. Thanks. :)

@desrosj commented on PR #3586:


16 months ago
#23

Applied some of the more straightforward suggestions. @adamsilverstein would you be able to get this committed at some point today?

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


16 months ago

@adamsilverstein commented on PR #3586:


16 months ago
#25

Yes, will do.

@adamsilverstein commented on PR #3586:


16 months ago
#26

This seems Ok as temporary fix but as single quotes are a perfectly legitimate form of HTML, it would be good to follow up with something more elegant.

WordPress itself uses the quote types interchangeably and it seems to be fairly common in plugins too, especially when generating the markup from a set of variables.

https://github.com/WordPress/wordpress-develop/blob/55d26534df75bbd13cc36a9e4e0881c5f8cc223c/src/wp-includes/pluggable.php#L2890

I agree it would be nice to have a more robust solution; this one seems ok for now - especially since the lazy attribute callback already has an identical check.

#27 @adamsilverstein
16 months ago

56969.2.diff includes all the latest changes/feedback from the PR.

@adamsilverstein commented on PR #3586:


16 months ago
#28

Applied some of the more straightforward suggestions. @adamsilverstein would you be able to get this committed at some point today?

@desrosj I'll get this committed and backported first thing tomorrow if you don't beat me to it.

#29 @peterwilsoncc
16 months ago

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

In 54802:

Media: Prevent decoding attribute corrupting JSON data.

Workaround wp_img_tag_add_decoding_attr() potentially breaking JavaScript and JSON data by limiting the addition of the decoding attribute to image tags using unescaped double quoted attributes src attributes.

Props rodricus, TimothyBlynJacobs, joelmadigan, mw108, adamsilverstein, flixos90, desrosj, mukesh27.
Fixes #56969.

#31 @peterwilsoncc
16 months ago

  • Keywords fixed-major added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for merging in to the 6.1 branch.

#32 @desrosj
16 months ago

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

In 54807:

Media: Prevent decoding attribute corrupting JSON data.

Workaround wp_img_tag_add_decoding_attr() potentially breaking JavaScript and JSON data by limiting the addition of the decoding attribute to image tags using unescaped double quoted attributes src attributes.

Props rodricus, TimothyBlynJacobs, joelmadigan, mw108, adamsilverstein, flixos90, desrosj, mukesh27, peterwilsoncc.
Merges [54802] to the 6.1 branch.
Fixes #56969.

#33 @TobiasBg
16 months ago

@peterwilsoncc: As it's polyfilled already in Core, should str_contains() be used instead of that strpos() check, "to eat our own dogfood"?

@adamsilverstein commented on PR #3586:


16 months ago
#34

thanks @peterwilsoncc!

#35 @peterwilsoncc
16 months ago

@TobiasBg Good point, as WordPress 6.1.1 is currently in RC, I think it best to update this in WordPress 6.2 as part of the coding standards ticket, #56791.

Do you have the time to make up a quick patch/pull request and upload it to that ticket? If so, please mention me or @adamsilverstein when you upload it to bring it to my attention, this will help ensure you get props for the change.

Note: See TracTickets for help on using tickets.