fix(css): should not wrap with double quote when the url rebase feature bailed out (#20068)

Author: sapphi-red

packages/vite/src/node/plugins/css.ts

@@ -1901,10 +1901,15 @@ type CssUrlResolver = (
 ) =>
   | [url: string, id: string | undefined]
   | Promise<[url: string, id: string | undefined]>
+/**
+ * replace URL references
+ *
+ * When returning `false`, it keeps the content as-is
+ */
 type CssUrlReplacer = (
-  url: string,
-  importer?: string,
-) => string | Promise<string>
+  unquotedUrl: string,
+  rawUrl: string,
+) => string | false | Promise<string | false>
 // https://drafts.csswg.org/css-syntax-3/#identifier-code-point
 export const cssUrlRE =
   /(?<!@import\s+)(?<=^|[^\w\-\u0080-\uffff])url\((\s*('[^']+'|"[^"]+")\s*|[^'")]+)\)/
@@ -2034,12 +2039,12 @@ async function rewriteCssImageSet(
     return url
   })
 }
-function skipUrlReplacer(rawUrl: string) {
+function skipUrlReplacer(unquotedUrl: string) {
   return (
-    isExternalUrl(rawUrl) ||
-    isDataUrl(rawUrl) ||
-    rawUrl[0] === '#' ||
-    functionCallRE.test(rawUrl)
+    isExternalUrl(unquotedUrl) ||
+    isDataUrl(unquotedUrl) ||
+    unquotedUrl[0] === '#' ||
+    functionCallRE.test(unquotedUrl)
   )
 }
 async function doUrlReplace(
@@ -2050,16 +2055,20 @@ async function doUrlReplace(
 ) {
   let wrap = ''
   const first = rawUrl[0]
+  let unquotedUrl = rawUrl
   if (first === `"` || first === `'`) {
     wrap = first
-    rawUrl = rawUrl.slice(1, -1)
+    unquotedUrl = rawUrl.slice(1, -1)
+  }
+  if (skipUrlReplacer(unquotedUrl)) {
+    return matched
   }
 
-  if (skipUrlReplacer(rawUrl)) {
+  let newUrl = await replacer(unquotedUrl, rawUrl)
+  if (newUrl === false) {
     return matched
   }
 
-  let newUrl = await replacer(rawUrl)
   // The new url might need wrapping even if the original did not have it, e.g. if a space was added during replacement
   if (wrap === '' && newUrl !== encodeURI(newUrl)) {
     wrap = '"'
@@ -2083,16 +2092,22 @@ async function doImportCSSReplace(
 ) {
   let wrap = ''
   const first = rawUrl[0]
+  let unquotedUrl = rawUrl
   if (first === `"` || first === `'`) {
     wrap = first
-    rawUrl = rawUrl.slice(1, -1)
+    unquotedUrl = rawUrl.slice(1, -1)
+  }
+  if (skipUrlReplacer(unquotedUrl)) {
+    return matched
   }
-  if (isExternalUrl(rawUrl) || isDataUrl(rawUrl) || rawUrl[0] === '#') {
+
+  const newUrl = await replacer(unquotedUrl, rawUrl)
+  if (newUrl === false) {
     return matched
   }
 
   const prefix = matched.includes('url(') ? 'url(' : ''
-  return `@import ${prefix}${wrap}${await replacer(rawUrl)}${wrap}`
+  return `@import ${prefix}${wrap}${newUrl}${wrap}`
 }
 
 async function minifyCSS(
@@ -2392,14 +2407,24 @@ const makeModernScssWorker = (
     return resolved ?? null
   }
 
+  const skipRebaseUrls = (unquotedUrl: string, rawUrl: string) => {
+    const isQuoted = rawUrl[0] === '"' || rawUrl[0] === "'"
+    // matches `url($foo)`
+    if (!isQuoted && unquotedUrl[0] === '$') {
+      return true
+    }
+    // matches `url(#{foo})` and `url('#{foo}')`
+    return unquotedUrl.startsWith('#{')
+  }
+
   const internalLoad = async (file: string, rootFile: string) => {
     const result = await rebaseUrls(
       environment,
       file,
       rootFile,
       alias,
-      '$',
       resolvers.sass,
+      skipRebaseUrls,
     )
     if (result.contents) {
       return result.contents
@@ -2529,6 +2554,16 @@ const makeModernCompilerScssWorker = (
       sassOptions.url = pathToFileURL(options.filename)
       sassOptions.sourceMap = options.enableSourcemap
 
+      const skipRebaseUrls = (unquotedUrl: string, rawUrl: string) => {
+        const isQuoted = rawUrl[0] === '"' || rawUrl[0] === "'"
+        // matches `url($foo)`
+        if (!isQuoted && unquotedUrl[0] === '$') {
+          return true
+        }
+        // matches `url(#{foo})` and `url('#{foo}')`
+        return unquotedUrl.startsWith('#{')
+      }
+
       const internalImporter: Sass.Importer<'async'> = {
         async canonicalize(url, context) {
           const importer = context.containingUrl
@@ -2562,8 +2597,8 @@ const makeModernCompilerScssWorker = (
             fileURLToPath(canonicalUrl),
             options.filename,
             alias,
-            '$',
             resolvers.sass,
+            skipRebaseUrls,
           )
           const contents =
             result.contents ?? (await fsp.readFile(result.file, 'utf-8'))
@@ -2709,8 +2744,8 @@ async function rebaseUrls(
   file: string,
   rootFile: string,
   alias: Alias[],
-  variablePrefix: string,
   resolver: ResolveIdFn,
+  ignoreUrl?: (unquotedUrl: string, rawUrl: string) => boolean,
 ): Promise<{ file: string; contents?: string }> {
   file = path.resolve(file) // ensure os-specific flashes
   // in the same dir, no need to rebase
@@ -2733,20 +2768,22 @@ async function rebaseUrls(
   }
 
   let rebased
-  const rebaseFn = async (url: string) => {
-    if (url[0] === '/') return url
-    // ignore url's starting with variable
-    if (url.startsWith(variablePrefix)) return url
+  const rebaseFn = async (unquotedUrl: string, rawUrl: string) => {
+    if (ignoreUrl?.(unquotedUrl, rawUrl)) return false
+    if (unquotedUrl[0] === '/') return unquotedUrl
     // match alias, no need to rewrite
     for (const { find } of alias) {
       const matches =
-        typeof find === 'string' ? url.startsWith(find) : find.test(url)
+        typeof find === 'string'
+          ? unquotedUrl.startsWith(find)
+          : find.test(unquotedUrl)
       if (matches) {
-        return url
+        return unquotedUrl
       }
     }
     const absolute =
-      (await resolver(environment, url, file)) || path.resolve(fileDir, url)
+      (await resolver(environment, unquotedUrl, file)) ||
+      path.resolve(fileDir, unquotedUrl)
     const relative = path.relative(rootDir, absolute)
     return normalizePath(relative)
   }
@@ -2778,6 +2815,13 @@ const makeLessWorker = (
   alias: Alias[],
   maxWorkers: number | undefined,
 ) => {
+  const skipRebaseUrls = (unquotedUrl: string, _rawUrl: string) => {
+    // matches both
+    // - interpolation: `url('@{foo}')`
+    // - variable: `url(@foo)`
+    return unquotedUrl[0] === '@'
+  }
+
   const viteLessResolve = async (
     filename: string,
     dir: string,
@@ -2802,8 +2846,8 @@ const makeLessWorker = (
       resolved,
       rootFile,
       alias,
-      '@',
       resolvers.less,
+      skipRebaseUrls,
     )
     return {
       resolved,

playground/css/__tests__/sass-tests.ts

@@ -15,6 +15,15 @@ export const sassTest = () => {
     const atImport = await page.$('.sass-at-import')
     const atImportAlias = await page.$('.sass-at-import-alias')
     const urlStartsWithVariable = await page.$('.sass-url-starts-with-variable')
+    const urlStartsWithVariableInterpolation1 = await page.$(
+      '.sass-url-starts-with-interpolation1',
+    )
+    const urlStartsWithVariableInterpolation2 = await page.$(
+      '.sass-url-starts-with-interpolation2',
+    )
+    const urlStartsWithVariableConcat = await page.$(
+      '.sass-url-starts-with-variable-concat',
+    )
     const urlStartsWithFunctionCall = await page.$(
       '.sass-url-starts-with-function-call',
     )
@@ -32,6 +41,15 @@ export const sassTest = () => {
     expect(await getBg(urlStartsWithVariable)).toMatch(
       isBuild ? /ok-[-\w]+\.png/ : `${viteTestUrl}/ok.png`,
     )
+    expect(await getBg(urlStartsWithVariableInterpolation1)).toMatch(
+      isBuild ? /ok-[-\w]+\.png/ : `${viteTestUrl}/ok.png`,
+    )
+    expect(await getBg(urlStartsWithVariableInterpolation2)).toMatch(
+      isBuild ? /ok-[-\w]+\.png/ : `${viteTestUrl}/ok.png`,
+    )
+    expect(await getBg(urlStartsWithVariableConcat)).toMatch(
+      isBuild ? /ok-[-\w]+\.png/ : `${viteTestUrl}/ok.png`,
+    )
     expect(await getBg(urlStartsWithFunctionCall)).toMatch(
       isBuild ? /ok-[-\w]+\.png/ : `${viteTestUrl}/ok.png`,
     )

playground/css/__tests__/tests.ts

@@ -98,6 +98,9 @@ export const tests = (isLightningCSS: boolean) => {
     const atImportAlias = await page.$('.less-at-import-alias')
     const atImportUrlOmmer = await page.$('.less-at-import-url-ommer')
     const urlStartsWithVariable = await page.$('.less-url-starts-with-variable')
+    const urlStartsWithInterpolation = await page.$(
+      '.less-url-starts-with-interpolation',
+    )
 
     expect(await getColor(imported)).toBe('blue')
     expect(await getColor(atImport)).toBe('darkslateblue')
@@ -112,6 +115,9 @@ export const tests = (isLightningCSS: boolean) => {
     expect(await getBg(urlStartsWithVariable)).toMatch(
       isBuild ? /ok-[-\w]+\.png/ : `${viteTestUrl}/ok.png`,
     )
+    expect(await getBg(urlStartsWithInterpolation)).toMatch(
+      isBuild ? /ok-[-\w]+\.png/ : `${viteTestUrl}/ok.png`,
+    )
 
     if (isBuild) return
 

playground/css/index.html

@@ -35,6 +35,15 @@ <h1>CSS</h1>
   </p>
   <p class="sass-partial">@import from SASS _partial: This should be orchid</p>
   <p class="sass-url-starts-with-variable">url starts with variable</p>
+  <p class="sass-url-starts-with-interpolation1">
+    url starts with interpolation 1
+  </p>
+  <p class="sass-url-starts-with-interpolation2">
+    url starts with interpolation 2
+  </p>
+  <p class="sass-url-starts-with-variable-concat">
+    url starts with variable and contains concat
+  </p>
   <p class="sass-url-starts-with-function-call">
     url starts with function call
   </p>
@@ -62,6 +71,9 @@ <h1>CSS</h1>
     @import url() from Less: This should be darkorange
   </p>
   <p class="less-url-starts-with-variable">url starts with variable</p>
+  <p class="less-url-starts-with-interpolation">
+    url starts with interpolation
+  </p>
 
   <div class="form-box-data-uri">
     tests Less's `data-uri()` function with relative image paths

playground/css/nested/_index.scss

@@ -20,6 +20,23 @@ $var: '/ok.png';
   background-position: center;
 }
 
+.sass-url-starts-with-interpolation1 {
+  background: url(#{$var});
+  background-position: center;
+}
+
+.sass-url-starts-with-interpolation2 {
+  background: url('#{$var}');
+  background-position: center;
+}
+
+$var-c1: '/ok';
+$var-c2: '.png';
+.sass-url-starts-with-variable-concat {
+  background: url($var-c1 + $var-c2);
+  background-position: center;
+}
+
 $var2: '/OK.PNG';
 .sass-url-starts-with-function-call {
   background: url(string.to-lower-case($var2));

playground/css/nested/nested.less

@@ -10,6 +10,11 @@
 
 @var: '/ok.png';
 .less-url-starts-with-variable {
+  background: url(@var);
+  background-position: center;
+}
+
+.less-url-starts-with-interpolation {
   background: url('@{var}');
   background-position: center;
 }