Don't swallow spaces in a srcset attribute

[joostdecock]

Spaced in a srcset attribute are removed

I'm using a remark plugin to add responsive images. It adds image tags with a srcset attribute like this:

srcset="/mdx_files/basting-1000.jpg, /mdx_files/basting-2000.jpg 2x"

MDX picks up the attribute and converts it to a prop:

{
  srcset: [
    "/mdx_files/basting-1000.jpg", 
    "/mdx_files/basting-2000.jpg 2x"
  ]
}

MDX then feeds that to React.CreateElement which will concat the array and render this:

srcset="/mdx_files/basting-1000.jpg,/mdx_files/basting-2000.jpg 2x"

The problem is that it's dropping the space, and as a comma is a valid character in a URL, the browser reads this as a single entry in the srcset: /mdx_files/basting-1000.jpg,/mdx_files/basting-2000.jpg

Your environment

• OS: Linux
• Packages: "@mdx-js/react": "^1.6.22", "@mdx-js/runtime": "^1.6.22"
• Env: Node 14.16.0 (lts/fermium), browser independent

Steps to reproduce

I didn't setup a repo for this because I felt it was pretty obvious what's happening.

Expected behaviour

What we need is to not trim the splitted parts of the string, or simply prefixed each srcset entry with a space:

{
  srcset: [
    " /mdx_files/basting-1000.jpg", 
    " /mdx_files/basting-2000.jpg 2x"
  ]
}

Leading whitespace is ok in the srcset attribute.

Actual behaviour

The whitespace after the comma in the srcset list is swallowed by splitting it into an array and then joining it.

[ChristianMurphy]

@joostdecock this looks like a URL?
If so the plugin should probably url encode the content, would become %20

[ChristianMurphy]

It may also be worth checking to see how MDX 2 handles this #1041 as version 2 includes a number of parser fixes and improvements

[joostdecock]

If so the plugin should probably url encode the content, would become %20

The srcset attribute holds a list of comma+whitespace seperated URLs. Encoding the space would turn them into one long URL, which is the problem we have.

The spec is rather explicit about this:

the following image candidate string, if there is one, must begin with one or more ASCII whitespace.

(The image candidate string is the url.)

So urlencoding is no solution here as the space is not part of the URL, but precisely there to signal where one URL ends and the next one starts.

[wooorm]

Seconding @joostdecock here re the spec. This behavior landed in GH-1039 here, I checked that it works in xdm, so it most likely also works on the next branch here for v2.

[joostdecock]

Thanks @wooorm & @ChristianMurphy I'll give @next a go to confirm whether or not it's resolved.

In the meanwhile, for those facing the same issue (or landing here from a search) you can side-step the problem by explicitly setting a pixel density descriptor. So instead of this:

srcset="/mdx_files/basting-1000.jpg,/mdx_files/basting-2000.jpg 2x"

Use this:

srcset="/mdx_files/basting-1000.jpg 1x,/mdx_files/basting-2000.jpg 2x"

The space before the 1x removes all ambiguity that these are two separate image candidate strings.

[joostdecock]

Upgraded to @next (2.0.0-next.9) but the problem persists.

I will try to setup a minimal repo to reproduce this as I've got next-mdx-remote in the mix.

[joostdecock]

I have setup a minimal repo to reproduce this problem: https://github.com/joostdecock/mdx-issue
Also published at: https://mdx-issue.netlify.app/

It has two pages, one using next-mdx-remote (which is what I'm using) and the other using the official @next/mdx.
The problem is the same in both cases.

What I've published uses @latest. I have also tried with @next and the problem persists with next-mdx-remote whereas with @next/mdx the image is not there, I presume because the plugin I'm using to generate the responsive images is not happy.

However, given that the problem is still there when using next-mdx-remote and that it's not not there 😂 with @next/mdx, I hope this warrants a closer look by somebody who has a better understanding of how we end up rendering to React.

[wooorm]

This seems to be because the plugin in question, @fec/remark-responsive-images, builds a string of HTML. mdx-js/mdx@1 tried to allow that but didn’t always do a good job, hence this issue.
For mdx-js/mdx@2, it by default doesn’t understand / even throws an error on unknown nodes (e.g., Error: Cannot handle unknown node `raw`).

It might be able to work with mdx-js/mdx@2 if rehypeRaw is used, and it does work with xdm

xdm

import path from 'node:path'
import remarkResponsiveImages from '@fec/remark-responsive-images'
import rehypeRaw from 'rehype-raw'
import {compile} from 'xdm'

main()

async function main() {
  const compiled = await compile(
    '![This is an image](/example.jpg "This is the image title")',
    {
      remarkPlugins: [
        [
          remarkResponsiveImages,
          {
            srcDir: path.resolve('.', 'public'),
            targetDir: path.resolve('.', 'public'),
            blurredBackground: false,
            imageSizes: [500, 1000, 1500],
            resolutions: [1, 2, 3]
          }
        ]
      ],
      rehypePlugins: [rehypeRaw]
    }
  )
  console.log(String(compiled))
}

But I would recommend that the plugin were changed. Or a new plugin written. It’s operating in the markdown phase and injecting HTML, which is not logical. There’s an HTML phase specifically for changing HTML (so a rehype plugin could do it cleaner)

[joostdecock]

Thank you @wooorm for your help in getting to the bottom of this, much appreciated. 🙏

I will close this and see if @florianeckerstorfer would be willing to update the plugin 🤞

If not, I might fork it 🤔

Keep up the great work! ♥️