Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(css): var in image-set #7921

Merged
merged 8 commits into from May 3, 2022
Merged

fix(css): var in image-set #7921

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
174ea11
fix(css): var in image-set
Apr 27, 2022
86522c7
fix(css): var in image-set
Apr 28, 2022
20b81b0
fix(css): var in image-set
Apr 29, 2022
d8b4a7a
fix(css): var in image-set
Apr 29, 2022
4951e7d
update
Apr 29, 2022
8e1d6ae
test: test for inline style in html
poyoho May 2, 2022
468b3cd
update
May 2, 2022
6a4b9c4
chore: update var regex
patak-dev May 2, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
23 changes: 23 additions & 0 deletions packages/playground/assets/__tests__/assets.spec.ts
View file
Open in desktop
Expand Up @@ -105,6 +105,29 @@ describe('css url() references', () => {
})
})

test('image-set with var', async () => {
const imageSet = await getBg('.css-image-set-with-var')
imageSet.split(', ').forEach((s) => {
expect(s).toMatch(assetMatch)
})
})

test('image-set with mix', async () => {
const imageSet = await getBg('.css-image-set-mix-url-var')
imageSet.split(', ').forEach((s) => {
expect(s).toMatch(assetMatch)
})
})

// not supported in browser now
// https://drafts.csswg.org/css-images-4/#image-set-notation
// test('image-set with multiple descriptor', async () => {
// const imageSet = await getBg('.css-image-set-multiple-descriptor')
// imageSet.split(', ').forEach((s) => {
// expect(s).toMatch(assetMatch)
// })
// })
patak-dev marked this conversation as resolved.
Show resolved Hide resolved

test('relative in @import', async () => {
expect(await getBg('.css-url-relative-at-imported')).toMatch(assetMatch)
})
Expand Down
23 changes: 23 additions & 0 deletions packages/playground/assets/css/css-url.css
View file
Open in desktop
Expand Up @@ -26,6 +26,29 @@
background-size: 10px;
}

.css-image-set-with-var {
--bg-img: url('../nested/asset.png');
background-image: -webkit-image-set(var(--bg-img) 1x, var(--bg-img) 2x);
background-size: 10px;
}

.css-image-set-mix-url-var {
--bg-img: url('../nested/asset.png');
background-image: -webkit-image-set(
var(--bg-img) 1x,
url('../nested/asset.png') 2x
);
background-size: 10px;
}

.css-image-set-multiple-descriptor {
background-image: -webkit-image-set(
'../nested/asset.png' type('image/png') 1x,
'../nested/asset.png' type('image/png') 2x
);
background-size: 10px;
}
Comment on lines +44 to +50
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this needs to test inline CSS in HTML because it doesn't have url( and also needs to compile, may have had a problem with inline CSS in HTML.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you share a failing example to add to the PR?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Lines 359 to 365 in 8e1d6ae

const inlineStyle = node.props.find(
(prop) =>
prop.name === 'style' &&
prop.type === NodeTypes.ATTRIBUTE &&
prop.value &&
prop.value.content.includes('url(') // only url(...) in css need to emit file
) as AttributeNode
in here.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! It is working though, are we good then?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because I no add the assertion. 😂

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@poyoho gotcha. Should the fix for this case be part of this PR, though? This wasn't working before even without var. I think we could merge this PR and then create a new PR or issue for inline style support, no?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I think this issue had two solutions. one is to remove the judge. other is add the judge. remove this judge may be better. But anyway it is also not part of this PR. (it seems to part of html instead of css).


.css-url-public {
background: url('/icon.png');
background-size: 10px;
Expand Down
28 changes: 28 additions & 0 deletions packages/playground/assets/index.html
View file
Open in desktop
Expand Up @@ -46,6 +46,34 @@ <h2>CSS url references</h2>
>CSS background with image-set() (relative)</span
>
</div>
<div class="css-image-set-with-var">
<span style="background: #fff">
CSS background image-set() (relative in var)
</span>
</div>
<div class="css-image-set-mix-url-var">
<span style="background: #fff">
CSS background image-set() (mix var and url)
</span>
</div>
<div class="css-image-set-multiple-descriptor">
<span style="background: #fff">
CSS background image-set() (with multiple descriptor)
</span>
</div>
<div
style="
background-image: -webkit-image-set(
'./nested/asset.png' type('image/png') 1x,
'./nested/asset.png' type('image/png') 2x
);
background-size: 10px;
"
>
<span style="background: #fff">
CSS background image-set() (with multiple descriptor)
</span>
</div>
<div class="css-url-relative-at-imported">
<span style="background: #fff"
>CSS background (relative from @imported file in different dir)</span
Expand Down
29 changes: 21 additions & 8 deletions packages/vite/src/node/plugins/css.ts
View file
Open in desktop
Expand Up @@ -102,6 +102,7 @@ const commonjsProxyRE = /\?commonjs-proxy/
const inlineRE = /(\?|&)inline\b/
const inlineCSSRE = /(\?|&)inline-css\b/
const usedRE = /(\?|&)used\b/
const varRE = /^var/i
patak-dev marked this conversation as resolved.
Show resolved Hide resolved
patak-dev marked this conversation as resolved.
Show resolved Hide resolved

const enum PreprocessLang {
less = 'less',
Expand Down Expand Up @@ -968,7 +969,7 @@ export const cssUrlRE =
export const cssDataUriRE =
/(?<=^|[^\w\-\u0080-\uffff])data-uri\(\s*('[^']+'|"[^"]+"|[^'")]+)\s*\)/
export const importCssRE = /@import ('[^']+\.css'|"[^"]+\.css"|[^'")]+\.css)/
const cssImageSetRE = /image-set\(([^)]+)\)/
const cssImageSetRE = /(?<=image-set\()((?:[\w\-]+\([^\)]*\)|[^)])*)(?=\))/

const UrlRewritePostcssPlugin: PostCSS.PluginCreator<{
replacer: CssUrlReplacer
Expand All @@ -989,7 +990,9 @@ const UrlRewritePostcssPlugin: PostCSS.PluginCreator<{
const importer = declaration.source?.input.file
return opts.replacer(rawUrl, importer)
}
const rewriterToUse = isCssUrl ? rewriteCssUrls : rewriteCssImageSet
const rewriterToUse = isCssImageSet
? rewriteCssImageSet
: rewriteCssUrls
promises.push(
rewriterToUse(declaration.value, replacerForDeclaration).then(
(url) => {
Expand Down Expand Up @@ -1042,11 +1045,15 @@ function rewriteCssImageSet(
replacer: CssUrlReplacer
): Promise<string> {
return asyncReplace(css, cssImageSetRE, async (match) => {
const [matched, rawUrl] = match
const url = await processSrcSet(rawUrl, ({ url }) =>
doUrlReplace(url, matched, replacer)
)
return `image-set(${url})`
const [, rawUrl] = match
const url = await processSrcSet(rawUrl, async ({ url }) => {
// the url maybe url(...)
if (cssUrlRE.test(url)) {
return await rewriteCssUrls(url, replacer)
}
return await doUrlReplace(url, url, replacer)
})
return url
})
}
async function doUrlReplace(
Expand All @@ -1061,7 +1068,13 @@ async function doUrlReplace(
wrap = first
rawUrl = rawUrl.slice(1, -1)
}
if (isExternalUrl(rawUrl) || isDataUrl(rawUrl) || rawUrl.startsWith('#')) {

if (
isExternalUrl(rawUrl) ||
isDataUrl(rawUrl) ||
rawUrl.startsWith('#') ||
varRE.test(rawUrl)
) {
return matched
}

Expand Down
13 changes: 8 additions & 5 deletions packages/vite/src/node/utils.ts
View file
Open in desktop
Expand Up @@ -545,18 +545,21 @@ interface ImageCandidate {
descriptor: string
}
const escapedSpaceCharacters = /( |\\t|\\n|\\f|\\r)+/g
const imageSetUrlRE = /^(?:[\w\-]+\(.*?\)|'.*?'|".*?"|\S*)/
export async function processSrcSet(
srcs: string,
replacer: (arg: ImageCandidate) => Promise<string>
): Promise<string> {
const imageCandidates: ImageCandidate[] = srcs
.split(',')
.map((s) => {
const [url, descriptor] = s
.replace(escapedSpaceCharacters, ' ')
.trim()
.split(' ', 2)
return { url, descriptor }
const src = s.replace(escapedSpaceCharacters, ' ').trim()
const [url] = imageSetUrlRE.exec(src) || []

return {
url,
descriptor: src?.slice(url.length).trim()
}
})
.filter(({ url }) => !!url)

Expand Down