fix(css): var in image-set by Bigfish8 · Pull Request #7921 · vitejs/vite

Bigfish8 · 2022-04-27T06:06:35Z

Description

fix #7874

Additional context

the case in #7874

input:

background-image: image-set(var(--hero--image-webp) 1x, var(--hero--image-jpg) 1x);

output:

background-image: image-set(url(var(--hero--image-webp) ) 1x, var(--hero--image-jpg) 1x);

step:

cssImageSetRE match the image-set(var(--hero--image-webp) (note: there only one ))
replace var(--hero--image-webp to url(var(--hero--image-webp)

it means that if the css is:
.some-class { --some-var: './a.png'; background-image: -webkit-image-set(var(--some-var) 1x, './b.png' 2x)
The cssImageSetRE will only match image-set(var(--some-var) and do not handle './b.png'.

But I think this is OK, because of the demo metioned in #7874. Url like './b.png' in image-set is invalid, only url('./b.png') is valid.

What is the purpose of this pull request?

Bug fix
New Feature
Documentation update
Other

Before submitting the PR, please make sure you do the following

Read the Contributing Guidelines.
Read the Pull Request Guidelines and follow the Commit Convention.
Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
Ideally, include relevant tests that fail without this PR but pass with it.

patak-cat · 2022-04-27T13:53:54Z

I think it would be better to do a review of the current flow. In MDN, there is an example without using url(), https://developer.mozilla.org/en-US/docs/Web/CSS/image/image-set

  background-image: image-set(
    "large-balloons.avif" type("image/avif"),
    "large-balloons.jpg" type("image/jpeg"));

Maybe the regex could be extended like: (?<=image-set\()(?:var\([^\)]*\)|[^)])*(?=\)) (and also including url(), type(), etc. And the order in https://github.com/vitejs/vite/pull/7921/files#diff-2cfbd4f4d8c32727cd8e1a561cffbde0b384a3ce0789340440e144f9d64c10f6R993 could be reversed:

const rewriterToUse = isCssUrl ? rewriteCssUrls : rewriteCssImageSet

to

const rewriterToUse = isCssUrl ? rewriteCssImageSet : rewriteCssUrls

So we stop processing image sets that have url() inside with the rewriteCssUrls util, and instead augment rewriteCssImageSet to properly handle url() when used.

patak-cat · 2022-04-28T10:01:01Z

packages/vite/src/node/utils.ts

        .trim()
-        .split(' ', 2)
-      return { url, descriptor }
+        .split(' ')


Should we split after ), ', " here? What happens if we have something like var( --my-var)?

Thanks for review!
Yes, the space inside var() or url() will cause some trouble.Maybe I can clean space inside it.

Thank you for pushing forward with a fix! ;)
Maybe a regex is better to match the first part? var(), url(), '...', "..." ?

Update.
According to spec, support 2 condition.

function call like var(), url(), linear-gradient()

url like 'test.png', "test.png"

packages/playground/assets/__tests__/assets.spec.ts

poyoho · 2022-05-02T11:31:33Z

packages/playground/assets/css/css-url.css

+.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;
+}


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.

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

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.

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

because I no add the assertion. 😂

@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?

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).

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

Bigfish8 · 2022-05-03T03:45:56Z

@patak-dev @poyoho The fail test seems not related, could you plz rerun it?

patak-cat · 2022-05-03T05:45:23Z

Thanks again @Bigfish8!

qnp · 2022-05-03T11:44:08Z

Thanks for the work 👍

fix(css): var in image-set

174ea11

fix(css): var in image-set

86522c7

patak-cat reviewed Apr 28, 2022

View reviewed changes

sapphi-red added feat: css p3-minor-bug An edge case that only affects very specific usage (priority) labels Apr 28, 2022

zhangyu96 added 3 commits April 29, 2022 11:33

fix(css): var in image-set

20b81b0

fix(css): var in image-set

d8b4a7a

update

4951e7d

patak-cat reviewed May 2, 2022

View reviewed changes

packages/playground/assets/__tests__/assets.spec.ts Show resolved Hide resolved

patak-cat previously approved these changes May 2, 2022

View reviewed changes

poyoho reviewed May 2, 2022

View reviewed changes

test: test for inline style in html

8e1d6ae

poyoho dismissed patak-cat’s stale review via 8e1d6ae May 2, 2022 11:58

update

468b3cd

poyoho reviewed May 2, 2022

View reviewed changes

packages/vite/src/node/plugins/css.ts Outdated Show resolved Hide resolved

patak-cat reviewed May 2, 2022

View reviewed changes

packages/vite/src/node/plugins/css.ts Outdated Show resolved Hide resolved

chore: update var regex

6a4b9c4

patak-cat approved these changes May 2, 2022

View reviewed changes

poyoho approved these changes May 3, 2022

View reviewed changes

patak-cat merged commit e96b908 into vitejs:main May 3, 2022

	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

Uh oh!

Conversation

Bigfish8 commented Apr 27, 2022

Description

Additional context

input:

output:

step:

What is the purpose of this pull request?

Before submitting the PR, please make sure you do the following

Uh oh!

patak-cat commented Apr 27, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Bigfish8 commented May 3, 2022

Uh oh!

patak-cat commented May 3, 2022

Uh oh!

qnp commented May 3, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants