WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

Last modified 4 years ago

#34678 closed defect (bug) (fixed)

Responsive Images: Check if content images already have a sizes attribute

Reported by: jaspermdegroot Owned by: azaozz
Milestone: 4.4 Priority: normal
Severity: normal Version: 4.4
Component: Media Keywords: has-patch
Focuses: Cc:

Description

If someone adds a custom sizes attribute in the post editor but no srcset attribute, the image will get filtered because we only check if there already is a srcset attribute in wp_make_content_images_responsive() before calling wp_image_add_srcset_and_sizes(). As a result the image will get two sizes attributes. We should check if the image already has a sizes attribute before adding it.

I have created a fix for this in RICG Responsive Images plugin repo. It's currently being reviewed. I will attach a patch here shortly.

Attachments (3)

34678.diff (1.7 KB) - added by jaspermdegroot 4 years ago.
34678.1.diff (2.0 KB) - added by jaspermdegroot 4 years ago.
Unit test for 34678
34678.2.diff (3.2 KB) - added by azaozz 4 years ago.

Download all attachments as: .zip

Change History (21)

#1 @joemcgill
4 years ago

  • Owner set to joemcgill
  • Status changed from new to accepted

#2 @joemcgill
4 years ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 4.4

@jaspermdegroot
4 years ago

#3 @jaspermdegroot
4 years ago

In 34678.diff:

  • wp_image_add_srcset_and_sizes(): Check if the image already has a sizes attribute before adding one
  • wp_make_content_images_responsive(): Make the srcset check work with both double and single quotes

#4 @joemcgill
4 years ago

  • Keywords has-patch needs-unit-tests added; needs-patch removed

The patch looks good to me.

Note that wp_image_add_srcset_and_sizes() doesn't contain any checks for the existence of a srcset attribute either. However, we do check for the existence of a srcset attribute in wp_make_content_images_responsive() before passing the image element off to wp_image_add_srcset_and_sizes().

While it's possible that someone could pass and <img> element containing a srcset attribute directly to this function, it seems unlikely and adding a second check would add some unnecessary performance weight to the wp_make_content_images_responsive() display filter, so I'm not in favor of adding an additional check for srcset attributes here.

@jaspermdegroot
4 years ago

Unit test for 34678

#5 @jaspermdegroot
4 years ago

In 34678.1.diff: Addition to 34678.diff. Adds a unit test for images in content that already have a sizes attribute.

#6 @jaspermdegroot
4 years ago

  • Keywords needs-unit-tests removed

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


4 years ago

#8 @wonderboymusic
4 years ago

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

In 35678:

Media: when making images responsive, check if they already have a sizes attribute.

Adds unit test.

Props jaspermdegroot.
Fixes #34678.

#9 @azaozz
4 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

If someone adds a custom sizes attribute in the post editor but no srcset attribute...

Can the sizes attribute be used without having srcset attribute? As far as I see it cannot. Why should we support bad/broken HTML and let it override the core functionality?

In my opinion this should be reverted and all the sizes checks should be removed. If we are generating srcset, we should be generating the sizes to go with it.

If somebody wants to change the sizes attribute, they can use the filter. Then we are in better position to ensure backwards compatibility one day. If somebody adds invalid/inconsistent attributes, they will get invalid HTML.

Alternatively we can check for the presence of sizes and srcset, and if either is present can skip generating both for that image.

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

This ticket was mentioned in Slack in #feature-respimg by azaozz. View the logs.


4 years ago

#11 follow-up: @jaspermdegroot
4 years ago

@azaozz

First of all, I wouldn't recommend inserting the sizes attribute in the image markup in the post editor, but there might be situations where it is the best (least worst) solution. The limitation of the filter is that you don't know anything about the context. So if a specific layout is used somewhere which warrants a custom sizes value, you can only use the filter if the combination of image ID/URL and size is unique for that situation.

Having a sizes attribute without a srcset attribute is indeed useless. That would be bad HTML, but not broken/invalid as far as I understood from the specs. Only a srcset attribute with w descriptors and no sizes attribute is invalid. However, when users add a custom sizes attribute they know of course that their final markup will also contain the srcset and I suppose what matters is if the final markup is good or not.

Alternatively we can check for the presence of sizes and srcset, and if either is present can skip generating both for that image.

That would be better than not checking at all, to prevent two sizes attributes in the final markup. Otherwise we would have to document somewhere that you are not allowed to add that attribute.

This would make it more difficult though to add a simple custom sizes attribute since you would also have to manually create the srcset attribute. So I think we should decide if there are good use cases for inserting a custom sizes attribute in the image markup instead of using the filter, and if so, I suggest we don't make that harder by requiring a srcset as well.

#12 in reply to: ↑ 11 @azaozz
4 years ago

Replying to jaspermdegroot:

We chatted few months ago that the next logical stage of "responsive images" should be adding srcset and sizes to post_content either directly in TinyMCE or on saving a post. That will bring high-res images in the editor and will eliminate the use of the display filter for new posts. It's likely to happen in 4.6-4.7, after the specs and the browser implementations (including in contentEditable) have completely "solidified".

I think it's important to strongly discourage hard-coding sizes in post_content for this initial implementation:

  • It's hard-coded :)
  • It's useless without srcset.
  • It relies on the display filter to add the srcset which can be changed by plugins or by core in the future.
  • It may make future back-compat a pain.

I think we should decide if there are good use cases for inserting a custom sizes attribute in the image markup instead of using the filter, and if so, I suggest we don't make that harder by requiring a srcset as well.

Don't think it is good to add hard-coded attribute that depends on a dynamically generated one. Also, this will not display properly in TinyMCE which can be pretty disorienting for the users (the image "magically" changes size when viewing the post). Perhaps we can support such cases in the next stage if we implement adding srcset and sizes in TinyMCE.

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

@azaozz
4 years ago

#13 @azaozz
4 years ago

In 34678.2.diff:

  • Change the behavior of wp_make_content_images_responsive() to skip images that have either srcset or sizes attributes.
  • Revert the sizes attribute check in wp_image_add_srcset_and_sizes(). The function name suggests that it will add the two attributes.
  • Change the test added in r35678 to check for no changes to the <img> tag when the sizes attribute is present.

#14 follow-up: @joemcgill
4 years ago

I second everything @jaspermdegroot already mentioned. The main concern addressed in r35678 was to ensure we didn't add an additional sizes attribute whenever someone had already added a custom sizes attribute in the post editor.

I agree with @azaozz that discouraging users from saving sizes attributes in the editor gives us the most flexibility going forward, but am not convinced that we should actively restrict this behavior. I also don't have strong feelings against restricting for now and giving ourselves some time to watch what issues come up over the next few months.

That said, if restricting the use of standalone sizes attributes is the goal, would we be better off stripping those sizes attributes when the post is saved?

#15 in reply to: ↑ 14 @azaozz
4 years ago

Replying to joemcgill:

I'm also not 100% on what is best but tend to prefer that we treat the srcset and sizes attributes as a group.

On one side sizes is a standard attribute for <img> tags and we shouldn't restrict the users that want to use it. On the other side it is redundant without srcset, i.e. it can be considered a user error to have sizes but not srcset in an <img> tag.

The main concern addressed in r35678 was to ensure we didn't add an additional sizes attribute whenever someone had already added a custom sizes attribute in the post editor.

This is also addressed in 34678.2.diff but instead of accepting the standalone sizes attr, it bypasses the image tag and doesn't add srcset. I.e. keeps the content exactly as the user has entered it.

That said, if restricting the use of standalone sizes attributes is the goal, would we be better off stripping those sizes attributes when the post is saved?

Don't think we should remove standalone sizes attributes as they are not technically "invalid", they are just "useless". There shouldn't be a reason to add them without adding srcset in the first place.

My main concern is that a (popular) plugin can start using the new core functionality in this (hacky) way. Then that code will be copied by other plugins and themes and we will end up with a fairly large amount of useless attributes hard-coded in post_content.

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

This ticket was mentioned in Slack in #feature-respimg by joemcgill. View the logs.


4 years ago

#17 @joemcgill
4 years ago

  • Owner changed from joemcgill to azaozz
  • Status changed from reopened to assigned

While I agree that it's not a good idea to add sizes attributes directly into the post editor—particularly without also pairing it with a srcset attribute—I don't think that we should actively restrict site owners who want to do this from being able to. If we find that a lot of people are using this "hack" to custom set sizes attributes, we should probably take that as a sign that our filter hooks need to be improved.

On the other hand, if there is another reason to change the behavior introduced in r35678, (e.g., performance or other back-compat concerns) I think the approach in 34678.2.diff is reasonable.

#18 @azaozz
4 years ago

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

Right. After the above chat in #feature-respimg in Slack, agree it's probably best to leave it as-is for now. If plugins and themes add lone sizes attributes, they will need to ensure the proper srcset is added by the display filter.

This may change when we add support for srcset and sizes to images inserted in the editor.

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